-
-
Notifications
You must be signed in to change notification settings - Fork 595
Add Vacuum protocol for enforcing interface among vacuum integrations #1373
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| # flake8: noqa | ||
| from .vacuum import Vacuum |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| from typing import Dict, Protocol, runtime_checkable | ||
|
|
||
| from miio import DeviceStatus | ||
|
|
||
| FanspeedPresets = Dict[str, int] | ||
|
|
||
|
|
||
| @runtime_checkable | ||
| class Vacuum(Protocol): | ||
| def start(self): | ||
| """Start vacuuming.""" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally it is good to specify more details about behavior of the interface methods, format of the arguments and returning value. This helps to keep implementations more consistent.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I just made this PR to show the idea, but your earlier proposal feels like a better solution than this. |
||
|
|
||
| def pause(self): | ||
| """Pause vacuuming.""" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some implementation does not contain
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On roborock, calling pause differs from stop in sense that resuming from a pause will continue using the existing internal map if the cleaning is resumed, in contrast to starting a completely new cleaning round. |
||
|
|
||
| def stop(self): | ||
| """Stop vacuuming.""" | ||
|
|
||
| def home(self): | ||
| """Return back to base.""" | ||
|
|
||
| def find(self): | ||
| """Request vacuum to play a sound to reveal its location.""" | ||
|
|
||
| def status(self) -> DeviceStatus: | ||
| """Return device status.""" | ||
|
|
||
| def set_fan_speed(self, int): | ||
| """Set fan speed.""" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to describe argument. Is argument in range 0-100 (percent)? Or can the argument be only one of the "presets"? What the method do if the argument does not match?
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this was just based on the current roborock implementation, but neither viomi nor g1vacuum seem to support percentage-based fan speed setting. The solution would be to have a I think this is more clearly visible on some fan/air(de)humidifer implementations where the "levels" or "presets" are more common than continuous values between 0 and 100. |
||
|
|
||
| def fan_speed_presets(self) -> FanspeedPresets: | ||
| """Return available fan speed presets.""" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider rename interface to IVacuum. It is useful to understand from the name, whether it is a class or interface.