Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve roborock update handling #1685

Merged
merged 5 commits into from
Feb 3, 2023
Merged

Conversation

rytilahti
Copy link
Owner

Not all devices support all features, but we have currently no way of knowing what is supported. In order to allow embedding all supported information in the status container while avoiding making unnecessary I/O on subsequent queries, this introduces a small helper to do just that.

The initial status() call will call all defined devicestatus-returning methods to find out which information is acquired correctly, and skip the unsupported queries in the following update cycles.

This also embeds some more information (last clean details, mop dryer settings).

Ping @starkillerOG

Copy link
Contributor

@starkillerOG starkillerOG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good to me, just a small remark about overwriting the send method.

Another point:
The order in which status calls are made could be important, so we schould make sure that the order in which we add the methods is the same order in which the calls are made.
Furthermore in some cases we might want to return multiple status containers from a single method call, see https://github.com/rytilahti/python-miio/pull/1614/files

miio/integrations/vacuum/roborock/vacuum.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2023

Codecov Report

Merging #1685 (8d24738) into master (8d24738) will not change coverage.
The diff coverage is n/a.

❗ Current head 8d24738 differs from pull request most recent head 114f3e2. Consider uploading reports for the commit 114f3e2 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #1685   +/-   ##
=======================================
  Coverage   81.74%   81.74%           
=======================================
  Files         191      191           
  Lines       17937    17937           
  Branches     3845     3845           
=======================================
  Hits        14662    14662           
  Misses       2987     2987           
  Partials      288      288           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Not all devices support all features, but we have currently no way of knowing what is supported.
In order to allow embedding all supported information in the status container while avoiding making
unnecessary I/O on subsequent queries, this introduces a small helper to do just that.

The initial status() call will call all defined devicestatus-returning methods to find out which
information is acquired correctly, and skip the unsupported queries in the following update cycles.
@rytilahti
Copy link
Owner Author

rytilahti commented Feb 3, 2023

Hi @starkillerOG, I'll merge this now to unblock you. I changed the signature for embed to require a name (#1712), so that accessing the main status and other containers can be done in a nicer manner. Let me know if you encounter any issues!

@rytilahti rytilahti merged commit 7ad548a into master Feb 3, 2023
@rytilahti rytilahti deleted the feat/roborock/updatehelper branch February 3, 2023 18:01
@starkillerOG
Copy link
Contributor

@rytilahti thank you very much, I have used this PR to update the roborock multi map PR here: #1614

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants