Add Vacuum protocol for enforcing interface among vacuum integrations #1373
Conversation
| """Start vacuuming.""" | ||
|
|
||
| def pause(self): | ||
| """Pause vacuuming.""" |
There was a problem hiding this comment.
Some implementation does not contain pause method. It would be good to describe, what should happen, if the method is not supported. Raise an exception?
There was a problem hiding this comment.
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.
| """Return device status.""" | ||
|
|
||
| def set_fan_speed(self, int): | ||
| """Set fan speed.""" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 set_fan_speed_preset(x) that accepts "something" that is returned by the implementations' fan_speed_presets(). A feature flag & a separate set_fan_speed_percentage(int) could be introduced for devices that support arbitrary fan speeds.
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.
|
|
||
|
|
||
| @runtime_checkable | ||
| class Vacuum(Protocol): |
There was a problem hiding this comment.
Consider rename interface to IVacuum. It is useful to understand from the name, whether it is a class or interface.
| @runtime_checkable | ||
| class Vacuum(Protocol): | ||
| def start(self): | ||
| """Start vacuuming.""" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed. I just made this PR to show the idea, but your earlier proposal feels like a better solution than this.
rytilahti
left a comment
rytilahti
left a comment
There was a problem hiding this comment.
Thanks for the comments @2pirko, I added a couple of notes how I was thinking about this. In hindsight, I think that your proposal on using abstract base classes is a better approach, at least for the very basic common interface.
| """Start vacuuming.""" | ||
|
|
||
| def pause(self): | ||
| """Pause vacuuming.""" |
There was a problem hiding this comment.
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.
| """Return device status.""" | ||
|
|
||
| def set_fan_speed(self, int): | ||
| """Set fan speed.""" |
There was a problem hiding this comment.
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 set_fan_speed_preset(x) that accepts "something" that is returned by the implementations' fan_speed_presets(). A feature flag & a separate set_fan_speed_percentage(int) could be introduced for devices that support arbitrary fan speeds.
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.
| @runtime_checkable | ||
| class Vacuum(Protocol): | ||
| def start(self): | ||
| """Start vacuuming.""" |
There was a problem hiding this comment.
Agreed. I just made this PR to show the idea, but your earlier proposal feels like a better solution than this.
|
Obsoleted in favor of #1404 - it is better to have some common API that is enforced through the abstract base class. |
This is a first draft towards enforcing interfaces among vacuum integrations,
aimed to consolidate them to allow easier downstream support no matter implementation details.
Open questions: