Closed cnrd closed 6 years ago
balloob
commented
6 years ago We should not make changes to our backend just to match some other system like Google Assistant. We can still easily implement with a single service by only calling the service if we need to based on the current state of a vacuum.
I think that we can split the state into different attributes:
cnrd
commented
6 years ago What about the state? Should we also touch that?
I'm thinking something along the lines of: If the device is currently moving: On if the device is NOT moving: Off
Or are platforms free to report the state as what they see fit?
EDIT: Also to address your concern about changing the backend to support Google Assistant. Maybe I should have worded the part about Start/Pause differently:
Currently when you call vacuum.SERVICE_START_PAUSE it will only react when the status is different: Say I want to pause the vacuum but the current status already reports it as paused because of a delay in updating the status therefor not pausing the vacuum. I still think that it would be more graceful to try to pause, if that is what the user wish to do, even if the current status reports something different. So I still think that splitting it into two different services makes sense.
balloob
commented
6 years ago No, platforms are not allowed to pick their own. We should decide as a component what we want to represent as state. Otherwise we'll end up with all the different stuff in Alexa/Google Assistant or whatever integration is built on top of Home Assistant.
Do we need to be able to distinguish when a device is "docking" ?
OverloadUT
commented
6 years ago So, in my efforts to add Deebot vacuum support to Hass, I have found myself having to work out the following situations:
On the happy path, the vacuum can be CLEANING, STOPPED (aka PAUSED), CHARGING (synonymous with DOCKED for Deebot, and RETURNING.
It's that last one that I would definitely like to see better supported at the vacuum level. This is a state that the vacuum can be in for quite some time before it actually finds its base, and in the current vacuum component it's not clear how to represent this state.
I don't think differentiating between CHARGING and DOCKED is necessary at the state level, because the battery_level attribute is already there to check for a full charge. Unless there are vacuums that can be docked, not fully charged, yet still not charging for some reason? Seems unlikely.
As for STUCK, that one is tricky. The Deebot can have a few different error states, and it can have more than one at a time. STUCK is different than BIN_MISSING, as well as a few others. And of course OFFLINE happens when the vacuum is stuck long enough to turn off entirely. However, to keep things simple, I do think a simple STUCK state could be useful.
cnrd
commented
6 years ago The xiaomi vacuum also reports charging when docked so yeah I think that charging should be equal to docked, as I can't see any case where the robot would be charging but not docked.
Yes I think having a "docking" state would also be a good idea, the xiaomi currently report this as: "Returning Home".
I'm not sure which kinds of error states makes sense, as @OverloadUT writes there can be multiple different error states. What about a state called just "error" with the specific error as an attribute?
Also I think we should have both off and paused, at least the xiaomi one have both states: When starting a clean from off: It will restart a new cleanup; "Starting cleanup" while starting from paused will resume the previous cleanup; "Resuming cleanup".
Also I'm not sure if we need to standardize the fan_speed modes or if this can be an arbitrary list of modes? The xiaomi miio platform supports the following fan_speed modes: Quiet, Balanced, Turbo, and Max, I'm guessing that other platforms support different fan_speed modes. I do however think that we should differentiate between fan_speed and modes such as thoroughness (I can't come up with other operating modes that differ from fan_speed right now).
OverloadUT
commented
6 years ago For Deebot, there are a lot of different cleaning modes that the vacuum can be in: spot, single room, auto (most common), edge. Some (all?) of these are already supported in the base component. The Deebot can also have "normal" and "high" fan speeds, and I believe each of those modes has a default of the two it uses depending on the mode.
That reminds me of a more significant issue I had implementing Deebot: issuing a fan speed order to Deebot can only be done in the context of a cleaning mode ("Start Auto cleaning, High speed") so Hass's default Fan Speed adjustment caused bad behavior: Either I had to tell Hass that Deebot doesn't support fan speeds and ignore the feature, or we have to be okay with the user experience being that the user selects a fan speed (while the vacuum is docked) which causes it to immediately start cleaning. I don't like the idea that the same service call ("Set motor speed") could cause cleaning to begin in some vacuums and not in others. I can't immediately think of how to solve that.
cnrd
commented
6 years ago @OverloadUT a bit OT, but: Would it be possible for you to just save the Fan Speed for the next time that the user calls a start cleaning cycle?
My proposal for STATEs would be the following:
STATE_CLEANING
STATE_DOCKED
STATE_RETURNING
STATE_ERRORSTATE_CLEANING is pretty self-explanatory.
STATE_DOCKED would cover both charging and docked but done charging, as I can't come up with any case where a vacuum would be charging but not docked.
STATE_RETURNING would be used when the vacuum is done with it's cleaning cycle, but not yet docked, I think that most (all) vacuums does report this state?
If the state STATE_ERROR is encountered we could have an attribute ATTR_ERROR that reports the error encountered, such that the user could for example send a push notification if STATE_ERROR is encountered reporting the specific error from ATTR_ERROR.
@rytilahti proposed STATE_STOPPED or STATE_IDLE I'm however not completely sure when this should be used and what it is supposed to show? Would this be used when the vacuum is not in it's dock and therefor not charging? Or would it also be used if it is done charging but still in the dock?
Do we need a STATE_DISCONNECTED in case the vacuum goes offline? Or is this already handled elsewhere?
I also want to reiterate the proposal of splitting up START and PAUSE, @rytilahti put it well in the discord chat: "I tend to agree with "pausing even when paused", especially on devices that are polled"
OverloadUT
commented
6 years ago We definitely need something like paused/stopped/idle. It's what would be used when the vacuum is stopped but not in its dock. This can happen all the time:
In fact right now I turned around and saw my vacuum just sitting there idle, because I left it out to remind myself to empty its bin. :)
balloob
commented
6 years ago I don't like the word "STOPPED", because it is ambiguous. I think a STATE_IDLE could work?
cnrd
commented
6 years ago Yeah I think STATE_IDLE would be better
balloob
commented
6 years ago Once we have agreed on the proper architecture, we should make sure to document it like we do for other types: https://developers.home-assistant.io/docs/en/entity_switch.html and then open a PR to change the design in HASS.
cnrd
commented
6 years ago This would be my proposal then:
STATE_CLEANING
STATE_DOCKED
STATE_PAUSED
STATE_IDLE
STATE_RETURNING
STATE_ERRORAgain I still think that we should split these into two different calls:
SERVICE_PLAY
SERVICE_PAUSEI know that I already added this citation once, but I think that it explains the situation really good:
I tend to agree with "pausing even when paused", especially on devices that are polled
as most vacuums are either local or cloud poll.
This would require changing the frontend to call the new services, either as two different UI buttons or combining both calls into one button. I think that splitting them into two different UI buttons would be optimal.
I know that this is a really bad photoshop, but the UI would look something like this:
OverloadUT
commented
6 years ago I like this proposal.
However, let's look to media_player for how it handles discrete play/pause commands. That's certainly got to be something that it handles, because some players only have a play+pause button, and others have separate buttons.
OverloadUT
commented
6 years ago Yep, I just confirmed that media_player has media_play, media_pause, and media_play_pause
However, Deebot can handle discrete play and pause, so I wouldn't need play_pause. Do any of the vacuums that you deal with only function in a play_pause manner? Perhaps I am overengineering for a situation that doesn't exist in vacuums
cnrd
commented
6 years ago Not really sure, as I only have access to the Roborock, that one seems to have discreet play and pause commands.
OverloadUT
commented
6 years ago I just checked every single vacuum component we currently have, and every one of them is actually having to internally convert the start_pause service call in to either a start or pause command to the vacuum. Not a single one actually uses a vacuum's native "toggle" or "start_pause" command.
So this means that we should absolutely adopt your proposal of separating it to discrete services, and probably remove the start_pause service entirely (or keep it for backwards compatibility, but change the UI to no longer use it?)
dthulke
commented
6 years ago Regarding the states I would suggest to add an additional STATE_PAUSED (indicating that there is a cleaning session which can be resumed). At least the Dyson and Xiaomi components support this state and it may be useful in some automations.
cnrd
commented
6 years ago You are right, I totally forgot that state (I edited my proposal). Also I think we should remove the combined start_pause and make that a breaking change.
dthulke
commented
6 years ago @cnrd since there was no more feedback to the proposal, do you want to create a PR for it?
arbreng
commented
6 years ago Hi,
I started looking into adding support for Roomba to Google Assistant when I stumbled upon this issue.
@crnd I'd love to know where you're at on implementing this and help out wherever I can
cnrd
commented
6 years ago I'm pretty sure the first thing we need to do is fill out this: https://github.com/home-assistant/developers.home-assistant/edit/master/docs/entity_climate.md
You can see other examples here: https://developers.home-assistant.io/docs/en/entity_binary_sensor.html I'm pretty sure @balloob sent me one for the Climate component, but I can't seem to find that one.
We also need to fix the vacuum component: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/vacuum/__init__.py and any platforms that already exist.
All of this of course should follow what we agreed upon in https://github.com/home-assistant/architecture/issues/29#issuecomment-392861377
I'm sorry for not having done this, but I have had exams and a small kid in the house also dosen't help :-)
@balloob anything else that I have forgotten?
crnd
commented
6 years ago @arbreng I haven't really looked into this at all and I believe I'll completely leave this to you guys. You'll figure it out!
balloob
commented
6 years ago No that's about it.
arbreng
commented
6 years ago I can control my Roomba with my voice. And I don't need iRobot's shitty service to do it anymore. Dope.
Thanks @cnrd!
marciogranzotto
commented
6 years ago Ok, I have a few problems with this.
I used to have a automation that heavily depended on states that are no longer available with this change.
My scenario is the following: I want my vacuum to clean the laundry (where the base is) in a schedule that I defined on Node-RED. To do it, it has to do 3 things:
That was accomplished by observing the status changes:
This changes remove Manual Mode and return me unknown.
I think that if you guys want to change the architecture, that's ok, but you are removing useful information that can be helpful for more people
rytilahti
commented
6 years ago @marciogranzotto the status attribute has been restored just recently: https://github.com/home-assistant/home-assistant/pull/16366
NOTE: My current proposal can be found here: https://github.com/home-assistant/architecture/issues/29#issuecomment-392861377
I am in the process of implementing vacuum cleaners in the Google Assistant component, however this is currently not possible as the different vacuum components use different ways of reporting state and supported fan speeds (mode).
I think we should start a discussion of which states and modes the vacuums should be able to use.
I only own the Roborock vacuum which uses the Xiaomi Miio platform, so that is the platform I'm most familiar with.
State
As an example of different ways of state reporting, we can take a look at the Xiaomi miio platform and the dyson platform:
The xiaomi miio platform uses an attribute called status, while the dyson platform uses the actual state of the entity.
I propose that we add the supported states to the base vacuum component, these are the states I have been able to observe:
I'm not completely sure which states we should support, as different manufactures report different things.
Modes
Currently different vacuums seems to be able to be set in different modes depending on supported features. The Roborock only supports setting different fan modes, but some vacuums may support setting a thoroughness state. Some of these modes may also be better served as toggles, at least if we look at how to present a vacuum device according to Google such as the quiet mode of the Xiaomi Miio platform.
Start/Paused
Currently the implementation of Start/Paused is a single service call (
vacuum.SERVICE_START_PAUSE), however if we want to support the StartStop trait of Google Assistant, I think that we should split this in to two different service calls:vacuum.SERVICE_STARTandvacuum.SERVICE_PAUSE.