Architectural review of vacuum entity about architecture

[cnrd]

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.

[crnd]

@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!

[rytilahti]

@marciogranzotto the status attribute has been restored just recently: home-assistant/core#16366

[balloob]

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:

• cleaning: true/false
• charging: true/false
• docked: true/false

[cnrd]

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]

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]

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]

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]

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]

@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_ERROR

STATE_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]

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:

1. User pressed pause/stop on the UI
2. User pressed pause/stop on a remote for the robot
3. Vacuum was stuck, user fixed it but did not push the button to resume cleaning

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]

I don't like the word "STOPPED", because it is ambiguous. I think a STATE_IDLE could work?

[cnrd]

Yeah I think STATE_IDLE would be better

[balloob]

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]

This would be my proposal then:

States

STATE_CLEANING
STATE_DOCKED
STATE_PAUSED
STATE_IDLE
STATE_RETURNING
STATE_ERROR

Splitting play / paused

Again I still think that we should split these into two different calls:

SERVICE_PLAY
SERVICE_PAUSE

I 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]

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]

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]

Not really sure, as I only have access to the Roborock, that one seems to have discreet play and pause commands.

[OverloadUT]

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]

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.

[dthulke]

@cnrd since there was no more feedback to the proposal, do you want to create a PR for it?

[arbreng]

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]

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?

[balloob]

No that's about it.

[arbreng]

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]

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:

• Use remote control to go forward
• Turn on spot clean
• Go back to the base when it's finished.

That was accomplished by observing the status changes:

• Manual Mode -> Spot Cleaning
• Spot Cleaning -> Idle

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