Bump Roborock to 17.0 adding device specific support and bugfixes #92547
Conversation
|
Hey there @humbertogontijo, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Lash-L
changed the title
Lash-L
changed the title
Lash-L
changed the title
Lash-L
changed the title
|
Hey @allenporter On the original PR you said you'd keep an eye out for Roborock PRs and review them if you had a chance, just wanted to make you aware of this one! If you don't have time, feel free to ignore this message and sorry for pinging! I have people on discord/forums/ issues asking for a lot more features that I'm unable to implement until I get this in ( as it is a major change) and people bringing up things that will be fixed by this, so I figured I'd see if you had the time. Thank you! |
There was a problem hiding this comment.
Great work @Lash-L 👍🏼 -- looks like a huge improvement overall (good code cleanup, good reliability improvement)
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
thanks for the feedback allen. One more thing I wanted to include with these changes but I couldn't figure out was translating the state_attribute for fan_speed. I have seen examples for climate, but as far as vacuum goes, I haven't seen anyone translate the fan speeds. I tried for a bit and tried a bunch of different things, but I am thinking maybe the state attributes for vacuums aren't supported for translation? It's not strictly needed, but I thought it would be a nice to have. Not a real hold up, was just curious if you had any idea. |
| await mqtt_client.async_disconnect() | ||
| except RoborockException as err: | ||
| _LOGGER.warning("Failed disconnecting from the mqtt server %s", err) | ||
| await asyncio.gather( |
There was a problem hiding this comment.
Should this still catch RoborockException here and below or is that now handled by the empty map check?
Aside: While not required, these types of failure cases may be tricky enough that you'd want to add tests for them if they aren't there already.
There was a problem hiding this comment.
It's handled by the empty map check, as disconnects don't matter if they fail, but I should probably add a warning to let the user know(even though there isn't much they can do)
However, I'm on vacation and won't be back until Wednesday, so I wouldn't be able to test it, so I'm not sure if it is worth adding a log for now. But your call, I can do it when I get back.
There was a problem hiding this comment.
Got it, eturn_exceptions=True. OK, yes, disconnects don't matter here.
| await mqtt_client.async_disconnect() | ||
| except RoborockException as err: | ||
| _LOGGER.warning("Failed disconnecting from the mqtt server %s", err) | ||
| await asyncio.gather( |
There was a problem hiding this comment.
Got it, eturn_exceptions=True. OK, yes, disconnects don't matter here.
Breaking change
Proposed change
This reworks how we look at devices so that it is a one coordinator/api per device. Each device also now has specifications to describe its fan codes/ mop codes/ any device specific options. Limits the code that repeats here.
changelog: https://github.com/humbertogontijo/python-roborock/blob/main/CHANGELOG.md
Changes: humbertogontijo/python-roborock@v0.8.3...v0.17.0
Summary:
note: conflicts with #92502 - but that is more of a hotfix and this is a solution for the new dependency change.
Type of change
Additional information
Checklist
black --fast 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: