Add new Roborock Integration

[Lash-L]

Breaking change

The current Roborock integration which is just an alias for Xiaomi Miio will no longer be an alias, but instead will be this integration. I'm not sure what the best way to handle this is. In my opinion, a dedicated Roborock integration should take priority over an alias integration. But I'm not sure how that will affect current users

Proposed change

This is a conversion of the popular hacs integration: https://github.com/humbertogontijo/homeassistant-roborock. It aims to allow users to be able to use the Roborock app and the home assistant integration, rather than having to use the Xiaomi app. The roborock app gives a significantly better use experience than the Xiaomi app for Roborock users. The aim is also to provide better support for Roborock specific devices.

Because this is a HACS conversion, the original project was very large. I tried to remove things that would not be needed for an initial PR. Some files are still rather larger as it was difficult to determine what was worth removing to keep the PR small and what was functionality that it didn't make sense to remove( A large portion of this PR is from test on vacuum.py. I could potentially remove them to keep the PR smaller, but I wasn't sure if it made sense)

Here are the features that exist that were removed for this PR and will be added in future PRS once this is merged in:

• Email/Password login
• Map support
• Binary Sensors
• Sensors
• Option Flow
• Manual Vacuum control
• go to x y location
• mopping
• services

This integration uses the following pypi package: https://github.com/humbertogontijo/python-roborock

Type of change

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

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/26520

Checklist

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

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

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

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

• [x] The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• [x] New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• [x] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• [x] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

• [x] I have reviewed two other open pull requests in this repository.

[home-assistant[bot]]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

[Lash-L]

Please make PRs small and limit them to the smallest usable set. For example, remove the additional services, those can be added later.

See also:

https://developers.home-assistant.io/docs/review-process/#creating-the-perfect-pr

Understood, I initially was unsure if I should have kept services in, but I removed them + mopping + tests that used either of those.

hopefully this is more manageable now.

[Lash-L]

Thanks for the feedback Allen. I handled some of the more simple tasks, and I'll spend some time tomorrow taking care of the more difficult tasks, i.e. pypi and config flow rework and giving the code another pass to try to find anything unused.

[Lash-L]

Thanks for the second pass Allen! I've gone through and made the changes, I'll make a note of pydantic and potentially add it down the line

[humbertogontijo]

@allenporter what do you mean by actually implementing async correctly? The library have everything asynchronous just to not block the main thread waiting for the mqtt response. The api uses mqtt, but the device do not publish status unless a mqtt message is sent. So its polling unfortunately

[allenporter]

@allenporter what do you mean by actually implementing async correctly? The library have everything asynchronous just to not block the main thread waiting for the mqtt response. The api uses mqtt, but the device do not publish status unless a mqtt message is sent. So its polling unfortunately

OK i may be misunderstanding. These async_connect and async_disonnect methods appear to just call the sync methods example on the mqtt client.

[humbertogontijo]

@allenporter what do you mean by actually implementing async correctly? The library have everything asynchronous just to not block the main thread waiting for the mqtt response. The api uses mqtt, but the device do not publish status unless a mqtt message is sent. So its polling unfortunately

OK i may be misunderstanding. These async_connect and async_disonnect methods appear to just call the sync methods example on the mqtt client.

Yes, Paho mqtt uses callbacks for both connection and disconnection, so async_connect and async_disconnect methods will call the sync method, and await for the callback to be called

[allenporter]

@allenporter what do you mean by actually implementing async correctly? The library have everything asynchronous just to not block the main thread waiting for the mqtt response. The api uses mqtt, but the device do not publish status unless a mqtt message is sent. So its polling unfortunately

OK i may be misunderstanding. These async_connect and async_disonnect methods appear to just call the sync methods example on the mqtt client.

Yes, Paho mqtt uses callbacks for both connection and disconnection, so async_connect and async_disconnect methods will call the sync method, and await for the callback to be called

OK thanks. So the sync methods don't actually do any I/O and you have to call loop() to do the IO? Interesting API! :)

Thanks for confirming.

[Lash-L]

@allenporter Yeah, I have some local changes in which I am changing it back to polling.

Which comments in particular? I thought I resolved them all and I just did a quick look through (I could have easily missed things) and it looked like I fixed them. I made all of the changes to config flow and removed all the unneeded assertions, reworked the config flow tests, and all of the other comments you made. If there is anything you notice that I didn't resolve, just unresolve the comment and I'll take a look - I'm sorry about that.

Edit: Don't want to double post and spam you - I'm currently updating to the newest version of our package so that this can be an offline polling integration. I will push these changes later today.

[Lash-L]

Thanks for addressing all the previous feedback. (You got everything I previously asked for, thanks!)

I think this is the last round and otherwise this looks good to go with just a few small items. I'm very excited to use this myself as i was turned off by the existing integration since its too complicated to setup and this appears much easier. 👍🏼

Awesome! I'll take a look at these and bundle them with the changes I'm working on.

I'm not sure if you saw my edit above - but I was planning to update this to be a local polling integration from the start. So I bumped up the version.

It still uses an online connection to get your account information and then all of your devices and the local key and the ip of the devices, but then it communicates entirely through local connections. I have a feeling you will have to give a little bit more feedback once I put that out.

If you think it would be better - I could hold off on making the change to make it local and instead just get this initial version out with it being online only.

[allenporter]

Thanks for addressing all the previous feedback. (You got everything I previously asked for, thanks!) I think this is the last round and otherwise this looks good to go with just a few small items. I'm very excited to use this myself as i was turned off by the existing integration since its too complicated to setup and this appears much easier. 👍🏼

Awesome! I'll take a look at these and bundle them with the changes I'm working on.

I'm not sure if you saw my edit above - but I was planning to update this to be a local polling integration from the start. So I bumped up the version.

Oh I missed that.

It still uses an online connection to get your account information and then all of your devices and the local key and the ip of the devices, but then it communicates entirely through local connections. I have a feeling you will have to give a little bit more feedback once I put that out.

If you think it would be better - I could hold off on making the change to make it local and instead just get this initial version out with it being online only.

Up to you, i don't mind either way. I think the important thing to worry about here would just be the config flow details and if those are saying the same, then i'm fine with either approach.

[Lash-L]

edit edit: You can ignore this for now and just wait for my implementation - I think I figured it out.

Up to you, i don't mind either way. I think the important thing to worry about here would just be the config flow details and if those are saying the same, then i'm fine with either approach.

Okay I'm going to add local then for this release.

Here's my logic - if you have a chance let me know if I'm doing something that breaks standards.

• config flow would store the Network information that is needed for each device that you have
• In init, we attempt to use the cloud connected mqttclient to update network information/ get any new devices
• If a new devices is found - are we allowed to add the information to the ConfigEntry? Or are we not supposed to change the config entry in init?
• If the mqtt client can't get the networking information, we just use the stored local information.

My thought with this approach is that someone could block their roborock from reaching outside their home network and everything would still work correctly. Then if they got another Roborock as long as it isn't blocked for the first time you open up Homeassistant, it would be added, and then you could block it.

If I just handled everything within the init, then I wouldn't be able to store(unless I am able to add to config entry?) , and then the roborock would have to be reachable on the internet everytime you started up HomeAssistant

Edit: Maybe for an initial approach it just makes sense to have it communicate locally but not support it being blocked completely from the outside?

What are your thoughts?

Edit edit: I found hass.config_entries.async_update_entry so I am using that in init and not modifying config flow at all

[allenporter]

Edit: Maybe for an initial approach it just makes sense to have it communicate locally but not support it being blocked completely from the outside?

What are your thoughts?

Edit edit: I found hass.config_entries.async_update_entry so I am using that in init and not modifying config flow at all

Given this is the initial integration PR, i would suggest to keep this as simple as possible, which to me means either:

• get the list of devices from the cloud on init and use that
• get the list of devices from the cloud in the config flow and use that

The other cases seem like they are optimizations or improvements that can follow in future smaller PRs. (If you go with the first approach, its improving reliability by using falling back to stored configuration when the cloud is not available. If you go with the second approach, its improving device discovery, automatically discovering and setting up new devices.). This way we can focus on getting something initially checked in. This will also allow having reviews in parallel for future platforms with discovery improvements.

[Lash-L]

The other cases seem like they are optimizations or improvements that can follow in future smaller PRs. (If you go with the first approach, its improving reliability by using falling back to stored configuration when the cloud is not available. If you go with the second approach, its improving device discovery, automatically discovering and setting up new devices.). This way we can focus on getting something initially checked in. This will also allow having reviews in parallel for future platforms with discovery improvements.

Understood! For now I'll just get it in init then. and I will add the extra functionality in a future PR.

I've pushed the changes, I assume I'll need some changes, so just let me know whatever you think you should. There is one bug with the fan_speed we are working on, but other than that I have tested it a bit with some pretty good success.

If we can't figure out the fan_speed bug, I'm going to switch this back to being online polling as the bug does not exist there.

[Lash-L]

Fixed issues with the tests and the fan speed bug.

[Lash-L]

did you cancel the codecov update Allen or did it just get cancelled based on time?

I was gonna update branch to get it to pass, but I guess that doesn't really matter?

Edit: ignore this - it looks fixed

[Lash-L]

I realized there was a potential problem where if you timed out getting the networking information or the networking wasn't available, you could be left with 0 devices and technically the integration would still set up. I believe a ConfigEntryNotReady is more appropriate.

[Lash-L]

Hey allen, sorry to make another push, but I went through and fixed typing in the base package using mypy - so this iversion bump should theoretically make us slightly less likely to get errors so I wanted to include it.

Let me know if there is anything else you need me to do. I won't push anymore unless this pipeline fails.

[Lash-L]

Sounds good - I'll add strict typing down the line. I ran it now just to see, and there are a few things I have to change in the package first, so I'll add it to my to-do list

[allenporter]

This says "Merging can be performed automatically once the requested changes are addressed." so it may need re-review by the original requestor? Can't tell.

[Lash-L]

This says "Merging can be performed automatically once the requested changes are addressed." so it may need re-review by the original requestor? Can't tell.

Ah yeah - probably because frenck requested changes to begin with but doesn't have a corresponding accepting of said changes.

I know I'm not supposed to ping, but should we just wait? Will he end up seeing this PR?

[allenporter]

@frenck the changes you requested (splitting into smaller focused pr) was addressed. I've approved otherwise, but the checks would ike your re-review.

[Lash-L]

Let me know if you need anything from me - I'd love to get it included in the 2023.05 release/ potentially even get a few other small additional features included in the initial release through small PRS, but I completely understand if either one of those things is not possible.

[allenporter]

I chatted w/ the core members and learned how to dismiss the blocking requested changes since you addressed them a long time ago and it was a trivial requested change. We should be good here now, and can address anything else in follow up PRs.

[Lash-L]

Amazing! Thanks a ton allen!