Add valve platform support to google_assistant#106139
Conversation
home-assistant
Bot
added
cla-signed
core
has-tests
integration: google_assistant
new-feature
labels
|
Hey there @home-assistant/cloud, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
jbouwh
force-pushed
the
jbouwh-google-assistant-valve
branch
from
cdaeff3 to
19d1d14
Compare
| COVER_VALVE_STATES[domain]["closing"], | ||
| COVER_VALVE_STATES[domain]["opening"], | ||
| ) | ||
| or domain == valve.DOMAIN |
There was a problem hiding this comment.
We want the stop function to be always available since we can not read determine from, the state if stop should be available,
There was a problem hiding this comment.
It will ensure that the Stop button is shown, not the Start button. See also the comment below.
| if domain == valve.DOMAIN: | ||
| return {"isRunning": True} |
There was a problem hiding this comment.
We return True, so the state is always running and the Stop function will be always available (if supported)
There was a problem hiding this comment.
Hm, why don't we rely on CLOSING + OPENING states like we do for cover?
There was a problem hiding this comment.
The reason is that not all valves support that, but they can support the stop function. Which should be always available and not depend on the reported state.
Futher the Start button, that will pop up instead will not make any sense, hence I made this button to always show Stop.
To avoid this:
There was a problem hiding this comment.
I think that should be a UX fix. Replace Start button for open/close depending on the state. And isRunning uses the same logic as the cover.
There was a problem hiding this comment.
This is not something we can easily customize. It is how the start stop treat works. The integration can decide if a stop command should be processed.
There was a problem hiding this comment.
BTW. the cover implentation ideed has the same issue:
There was a problem hiding this comment.
I suggest I open a separate PR to fix this for cover as well.
There was a problem hiding this comment.
Another suggestion is to use Start to toggle the valve (or cover) #106378
rokam
left a comment
rokam
left a comment
There was a problem hiding this comment.
Looks good, only one item called my attention.
emontnemery
left a comment
emontnemery
left a comment
There was a problem hiding this comment.
Looks great, just some minor comments and a question about the opening / closing states.
| if domain == valve.DOMAIN: | ||
| return {"isRunning": True} |
There was a problem hiding this comment.
Hm, why don't we rely on CLOSING + OPENING states like we do for cover?
| COVER_VALVE_STATES[domain]["closing"], | ||
| COVER_VALVE_STATES[domain]["opening"], | ||
| ) | ||
| or domain == valve.DOMAIN |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
jbouwh
dismissed
emontnemery’s stale review
As discussed on discord, will open a PR to adjust the behavior for cover as well
* Add valve platform to google_assistant * Use constant for domains set
* Add valve platform to google_assistant * Use constant for domains set
4 participants
Proposed change
Add valve platform to google_assistant
Binary valve example screenshot
stopfeature:Position reporting valve example
stopfeature:Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: