-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Architectural review of vacuum entity #29
Comments
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:
|
What about the state? Should we also touch that? I'm thinking something along the lines of: 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. Currently when you call |
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" ? |
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. |
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). |
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. |
@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:
If the state @rytilahti proposed Do we need a 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" |
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. :) |
I don't like the word "STOPPED", because it is ambiguous. I think a |
Yeah I think STATE_IDLE would be better |
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. |
I like this proposal. However, let's look to |
Yep, I just confirmed that media_player has However, Deebot can handle discrete play and pause, so I wouldn't need |
Not really sure, as I only have access to the Roborock, that one seems to have discreet play and pause commands. |
I just checked every single vacuum component we currently have, and every one of them is actually having to internally convert the So this means that we should absolutely adopt your proposal of separating it to discrete services, and probably remove the |
Regarding the states I would suggest to add an additional |
You are right, I totally forgot that state (I edited my proposal). Also I think we should remove the combined |
@cnrd since there was no more feedback to the proposal, do you want to create a PR for it? |
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 |
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 #29 (comment) 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? |
@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! |
No that's about it. |
* Added entity_vacuum as agreed in home-assistant/architecture#29 * Better states overview
I can control my Roomba with my voice. And I don't need iRobot's shitty service to do it anymore. Dope. Thanks @cnrd! |
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:
That was accomplished by observing the status changes:
This changes remove 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 |
@marciogranzotto the status attribute has been restored just recently: home-assistant/core#16366 |
NOTE: My current proposal can be found here: #29 (comment)
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_START
andvacuum.SERVICE_PAUSE
.The text was updated successfully, but these errors were encountered: