Add Braava support to iRobot Roomba component

[shenxn]

Breaking change

Proposed change

Add Braava jet m6 support to the iRobot Braava integration. More specifically, the lid, tank, and pad states and spray amount settings are added as attributes and the wet mopping behavior can be set through "fan speed".

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

The configuration is identical to the original integration.

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request: #13060

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

[homeassistant]

Hi @shenxn,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[probot-home-assistant]

Hey there @pschmitt, mind taking a look at this pull request as its been labeled with a integration (roomba) you are listed as a codeowner for? Thanks!

[shenxn]

By the way, here is a screenshot of the settings on the app if you need a better sense of what those settings actually are.

[bdraco]

@shenxn I'll look at this one once #33302 is completed

[bdraco]

@shenxn The one ahead of this just merged, would you please rebase

https://developers.home-assistant.io/docs/development_catching_up/

[freekode]

@bdraco don't you think that Braava should have it own component?
the merge breaks single responsibility here, there are a lot of if's, there are lot of stuff which will be None when braava connected and otherwise when roomba connected.
IMHO braava integration should be made in new component, a lot of common things can be moved to abstract level

[bdraco]

@bdraco don't you think that Braava should have it own component?
the merge breaks single responsibility here, there are a lot of if's, there are lot of stuff which will be None when braava connected and otherwise when roomba connected.
IMHO braava integration should be made in new component, a lot of common things can be moved to abstract level

It is using the same library so it should be the same integration. It makes sense to make the Braava device a new class though.

[freekode]

@bdraco don't you think that Braava should have it own component?
the merge breaks single responsibility here, there are a lot of if's, there are lot of stuff which will be None when braava connected and otherwise when roomba connected.
IMHO braava integration should be made in new component, a lot of common things can be moved to abstract level

It is using the same library so it should be the same integration. It makes sense to make the Braava device a new class though.

yeah, makes sense

[shenxn]

@bdraco I'll rebase and try to create a new class. It should be finished in a few days.

[shenxn]

@bdraco I just wanna double check. Since now we wait for data in the async_setup_entry of the platform, data should be already available in the initialization of RoombaVacuum class so it is safe to move the capability check from async_update to __init__ or async_setup_entry, right? If that is the case, it will be much easier to create a separate class for Roomba.

[bdraco]

Should be good to do in async_setup_entry. We shouldn't do any side effects in __init__

[bdraco]

Ideally there is a base class for the shared things between roomba and braava and then a separate class for each only where they differ

[shenxn]

@bdraco I've re-based and re-implemented the code. Since I only have a Braava Jet and don't have any Roomba robots, I'll need someone else to test the code.

[bdraco]

@shenxn Good news. Everything still appears to be working with my Roomba and Braava (and I can now see much more detail!)

There are a few things to cleanup in the PR that I noted. The most important being the part that should be upstreamed into the pypi package. Thank you for opening a PR for that.

Optional: Having everything in async_update is a legacy issue but it would be nice to be cleaned up if you feel up to it.

[freekode]

on mobile app you can click return to base
then last command will be shown in coming data from irobot.
so maybe braava requires different command name

[bdraco]

After additional testing its clear its not the return to base command being the issue. All commands are unreliable. Sometimes they get executed sometimes not

[bdraco]

2020-04-18 00:05:34 DEBUG (SyncWorker_17) [roomba.roomba] Send command: dock
2020-04-18 00:05:34 DEBUG (SyncWorker_17) [roomba.roomba] Publishing Roomba Command : {"command": "dock", "time": 1587168334, "initiator": "localApp"}
2020-04-18 00:05:34 DEBUG (Thread-6) [roomba.roomba] Received Roomba Data 192.168.210.68: $aws/things/<SNIP>/shadow/update, b'{"state":{"reported":{"lastCommand": {"command": "dock", "time": 1587168334, "initiator": "localApp"}}}}'
2020-04-18 00:05:34 DEBUG (Thread-6) [custom_components.roomba.irobot_base] Got new state from the vacuum: {'state': OrderedDict([('reported', OrderedDict([('lastCommand', OrderedDict([('command', 'dock'), ('time', 1587168334), ('initiator', 'localApp')]))]))])}

[bdraco]

After some testing it turns out that in order for roomba or the braava to accept the dock command it needs to be paused. This makes sense as this is how it works with in the app

[freekode]

yeah, just wanted to write about, found such behaviour long time ago, thought it was bug in HA

[bdraco]

Not sure which path its taking but its either not sending pause or sending stop which causes the roomba or braava to

• with no pause: ignore the dock command
• with stop: start saving maps and ignore the dock command

[bdraco]

@freekode https://github.com/cseelye/pyirobot/blob/master/pyirobot/__init__.py#L296
Looks like someone else documented that as well

[bdraco]

I was able to get this working reliably with the below naive implementation.

    async def async_turn_off(self, **kwargs):
        """Turn the vacuum off and return to home."""
        await self.async_return_to_base()

    async def async_return_to_base(self, **kwargs):
        """Set the vacuum cleaner to return to the dock."""
        if self.is_on:
           await self.async_pause()
           for _ in range(0, 10):
               if self.status == "Stopped":
                   break
               await asyncio.sleep(1)
        await self.hass.async_add_executor_job(self.vacuum.send_command, "dock")

[shenxn]

@bdraco I noticed that Home Assistant introduced StateVacuumDevice and separated start and pause commands in PR #15573 and #15751. Then we'll have very nice start, pause, stop and return_to_base commands instead of the very confusing turn_on and turn_off. I think that will make the whole thing make more sense.

[shenxn]

So I tried to implement the StateVacuumDevice and it seems that the roombapy package reports Stopped state when I pause my robot. I'm looking into the state machine implementation (update_state_machine()) but it's kinda messy and the comments seem to be outdated. Should I just ignore the state machine and use the cleanMissionStatus directly? I think the state machine is a little bit unnecessary here.

[shenxn]

I switched to cleanMissionStatus and it now works great on my Braava Jet.

[shenxn]

I refactored the code a little bit and removed some duplicated code in binary_sensor, sensor and vacuum.

[bdraco]

I'm retesting with my roomba and braava now.

[bdraco]

Everything works reliably now. Nice job @shenxn

I'll look over the code in a bit.

[bdraco]

@shenxn

Thank you for your contribution thus far! 🎖 Since this is a significant contribution, we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

[bdraco]

Nice job on this. Much easier to see what is going on now.

[MartinHjelmare]

Please address the comments in a new PR. Thanks!