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

roborock: auto empty dustbin support #1188

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

craigcabrey
Copy link
Contributor

@craigcabrey craigcabrey commented Nov 28, 2021

addresses #1107

This is just an initial pass at this, probably more needs to be done. Will also look at the home assistant component.

Example CLI run:

(miio-runtime) [craigcabrey@alto python-miio]$ python3 miio/cli.py vacuum --ip 192.168.10.222 --token XXXX dust_collection_mode
Running command dust_collection_mode
DustCollectionMode.Smart

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @craigcabrey!

As we want to have some sort of feature flags at some point to allow downstreams to decide which features are supported, could you please add a guard if clause to check for the models supporting this feature prior to calling any of the methods? You could raise a VacuumException("Device does not support dustbin emptying") or something similar when trying to run these commands on a non-supported devices.

That would be only the s7 and s7+ for the time being, I suppose?

miio/integrations/vacuum/roborock/vacuum.py Outdated Show resolved Hide resolved
@craigcabrey
Copy link
Contributor Author

As we want to have some sort of feature flags at some point to allow downstreams to decide which features are supported, could you please add a guard if clause to check for the models supporting this feature prior to calling any of the methods? You could raise a VacuumException("Device does not support dustbin emptying") or something similar when trying to run these commands on a non-supported devices.

Yea, that works. It's a little ugly but over time with a feature flag framework we could clean it up.

That would be only the s7 and s7+ for the time being, I suppose?

There is no S7+, just the S7. S7+ is a bundle of the S7 with the auto empty bin.

@rytilahti
Copy link
Owner

rytilahti commented Nov 29, 2021

Yea, that works. It's a little ugly but over time with a feature flag framework we could clean it up.

Indeed, I have some code around locally, but I haven't yet figured out what's the best approach to make it generic among all supported devices. But this will do for now, it's fairly simple to replace that with something more generic when the time comes.

There is no S7+, just the S7. S7+ is a bundle of the S7 with the auto empty bin.

Ah, thanks for the clarification. This looks good to go as soon as the checks pass 👍 Please run pre-commit run -a or tox -e lint locally to fix it.

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

This is ready to be merged as soon as the checks pass 👍

@rytilahti rytilahti added this to the 0.5.12 milestone Jul 6, 2022
@rytilahti rytilahti merged commit 74b6b42 into rytilahti:master Jul 7, 2022
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.

2 participants