Disable lazy discover in xiaomi_miio

[mrwogu]

Proposed change

This PR adds a fix to Xiaomi Miio integration which disable lazy_discover mode, which is supported by python-miio.
This changes the default behavior as it defaulted previously to enabled. If this change breaks user installations during the beta, we should revert it back to enabled.
Disabling this mode resolves network connection or timeout issues on some Xiaomi devices.

Currently, python-miio supports this for all devices (except RoborockVacuum). For that, we're waiting for python-miio 0.6 release with merged PR rytilahti/python-miio#1606.
For now, I left the TODO in the code.

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)
• 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 Timeout fetching data in Xiaomi Miio #59215 Xiaomi Miio and Smartmi Fan 2 (zhimi.fan.za3) not responding intermittently #67926
• This PR is related to issue:
• Link to documentation pull request:

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.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

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

[home-assistant]

Hi mrwogu

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!

[home-assistant]

Hey there @rytilahti, @syssi, @starkillerOG, mind taking a look at this pull request as it has been labeled with an integration (xiaomi_miio) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of xiaomi_miio can trigger bot actions by commenting:

• @home-assistant close Closes the issue.
• @home-assistant rename Awesome new title Change the title of the issue.
• @home-assistant reopen Reopen the issue.
• @home-assistant unassign xiaomi_miio Removes the current integration label and assignees on the issue, add the integration domain after the command.

[rytilahti]

I'm fine with this, but I'm wondering if we should default to False at least for the beta? If we receive bug reports about issues, we can always revert it back to True during the beta phase.

My reasoning is that I cannot anymore remember why the lazy discovery was even added to the library, but what it does is, IIRC, is to perform the handshake only 1) if it wasn't ever done before or 2) after an error with communicating the device occurred. If it works fine when it's false, I could remove the setting altogether (or default to False) in the future python-miio release.

I'll do some testing with my few devices using False, and report back my findings soonish.

[mrwogu]

I've fixed code with your review comments, and added new commit with change default lazy_discover to False.
I'm ok with that change for beta release.
I assumed a non-invasive approach, because not everyone had a problem with the timeout. This is really a non-deterministic error.

Thanks

[rytilahti]

Great, I modified the description and added second-opinion-wanted to get also someone else to chime in if the approach is viable. Please fix the config flow tests to make the CI pass again.

[rytilahti]

Just for the reference, I did some digging to find out that the lazy discovery was added in rytilahti/python-miio#215 and I noted that some other impl. (likely aholstenson's miio lib) performed the handshaking only every 180 or 200 seconds.

I am still perplexed why not having the handshake is causing the issues with the integration given a handshake should be done automatically when encountering an error, but I start to feel that instead of adding a setting we should just default the lazy_handshake to False.

For 0.6 series of python-miio, I think it the option will go away in favor of "handshake interval" setting that will default to 120s, if the experiences from beta look promising.

[starkillerOG]

@rytilahti I think sending the handshake every 120 seconds is indeed better than doing it always.
When updating the status of a device like roborock, multiple commands are send sequentially for one call. In those cases otherwise a handshake will happen in between each command, which is a probablly a bit of a waste of IO.

[rytilahti]

@starkillerOG you are right, I added a todo to rytilahti/python-miio#1114 to replace the current lazy discovery in favor of "time since last handshake" option. If some devices require this for every request, it can be configured to 0.

The beta cut off is on upcoming wednesday, hopefully we can get this PR merged for that.

[mrwogu]

@rytilahti what I'm supposed to do now? Remove setting and just pass False as argument to lazy_discover?

[rytilahti]

@mrwogu yeah, I think it would be the best option for the time being, and other devs agreed on discord that introducing low-level configuration options is not a way to go.

I cannot test this currently against the devices in my possession, but we are going to need more input to verify that it doesn't break the integration against other devices. Do you have other devices besides the air purifier to check that they keep working?

@starkillerOG would you have some time to test the change against the devices you have?

[mrwogu]

@rytilahti done.

Do you have other devices besides the air purifier to check that they keep working?

Yes, I have several purifiers, a humidifier, a fan and a Roborock.
My PR made the problems disappear. Unfortunately, my PR won't fix Roborock, and here it is also a nuisance.
I have to use the python-miio version I built myself.
That's why I'm quietly hoping that maybe it would be possible to release a python-miio hotfix with a corrected constructor.

[rytilahti]

Yes, I have several purifiers, a humidifier, a fan and a Roborock.

Okay, great!

My PR made the problems disappear. Unfortunately, my PR won't fix Roborock, and here it is also a nuisance.
That's why I'm quietly hoping that maybe it would be possible to release a python-miio hotfix with a corrected constructor.

So, my personal preference–given the current ongoing refactoring of the lib & the integration–would be disabling it also for the vacuum by setting the property just like dev._protocol.lazy_discover = False.

I created rytilahti/python-miio#1606 to fix the constructor signature, but I don't have any free time to create a separate maintenance branch for 0.5 series and prepare releases, etc. The 0.6 push is much more important for future-proofing the library, and that's where I prioritize my personal efforts at the moment.

[starkillerOG]

I can indeed test with my roborock and aqara gateways, but the current code does not touch the vacuums or gateways, so there is now no point in testing for me.

[frenck]

@rytilahti What are the next steps needed to move this PR forward?

[rytilahti]

So, my personal preference–given the current ongoing refactoring of the lib & the integration–would be disabling it also for the vacuum by setting the property just like dev._protocol.lazy_discover = False.

@frenck Basically doing this & getting this merged for broader testing would be my preference. If it causes more trouble than it fixes, we can easily revert/flip the setting.

@starkillerOG if you don't mind testing my suggestion on your local setup (for the vacuum and the gateway), it would be great :-)

[Maeur1]

Friendly ping on this PR (if its still relevant for #59215). I have this with one of my fans, it constantly is timing out and would be happy to test this fix out if guided on how to run this locally?

[rytilahti]

@Maeur1 I usually just use the gh command (from github cli) to checkout PRs, so testing this locally would involve the steps to get the source from git, executing gh pr checkout 82601 and following the regular homeassistant installation instructions.

[rytilahti]

I think we should just merge this, and revert if it causes more troubles. In the best case this at least alleviates the issue encountered by so many. Thanks for the PR @mrwogu, let's hope this fixes the issue for the time being! 🥇