My shot at 'Vacuum: Provide human readable fan speeds (rytilahti#468)' #523
Conversation
| vac.set_fan_speed(speed) | ||
| else: | ||
| click.echo("Current fan speed: %s" % vac.fan_speed()) | ||
|
|
||
| @cli.command() |
miio/vacuum.py
Outdated
| ) | ||
| def set_fan_speed(self, speed: int): | ||
| def set_fan_speed(self, speed: [int,FanSpeed]): |
| Turbo = 77 | ||
| Max = 90 | ||
|
|
||
| @enum.unique |
miio/vacuum.py
Outdated
|
|
||
|
|
||
| FanSpeed = enum.Enum('FanSpeed', | ||
| [ |
There was a problem hiding this comment.
continuation line under-indented for visual indent
|
seems there is no interest... |
There was a problem hiding this comment.
Every PR is very much appreciated and I'm sorry that it took so long time to answer to you. As explained elsewhere, I do not currently have access to my test device, so I'm hesitant to merge things that can break something for existing users.
If you are still willing to work on this, I'm sure it'd be greatly appreciated by those who are having a problem with the current implementation, so I left some commentary behind..
| ('Mop', 'mop'), | ||
| ] | ||
| # comment if only levels above are wanted... | ||
| + [("Level_%d" % x, x) for x in range(0, 101)] |
There was a problem hiding this comment.
I'm not sure sure if there should be specific values for separate levels, I'd prefer to have just a "Custom" or "Other" and let the user of the library to handle the case however it wants to.
| @@ -46,6 +79,32 @@ def __init__(self, ip: str, token: str = None, start_id: int = 0, | |||
| debug: int = 0) -> None: | |||
| super().__init__(ip, token, start_id, debug) | |||
| self.manual_seqnum = -1 | |||
| self._model = self.info().model | |||
| self._init_fan_speeds() | |||
There was a problem hiding this comment.
Until now the library does not do any I/O operations in constructors, so I think this should be deferred until it's really necessary. One way would be to have an internal _fanspeedMap function/property, which queries and stores the data during the first fetch for later uses.
| ) | ||
| def set_fan_speed(self, speed: int): | ||
| def set_fan_speed(self, speed: [int, FanSpeed]): |
There was a problem hiding this comment.
This could be an Union, but I think it is better to keep it as integer; or is there some specific need why both should be accepted?
| return self.send("get_custom_mode")[0] | ||
| speed = self.send("get_custom_mode")[0] | ||
| try: | ||
| if hasattr(self, '_fanspeedMap'): |
There was a problem hiding this comment.
This should be already defined when it gets this far, so no need for checking, right?
| return speed | ||
|
|
||
| @command() | ||
| def fan_speed_list(self): |
| if speed: | ||
| click.echo("Setting fan speed to %s" % speed) | ||
| try: |
There was a problem hiding this comment.
I think it makes sense first to check for the preset name, and then fall back to int conversion. Furthermore, if the conversion is not possible, and the exception should be captured and reported back.
This returns a dictionary {name, value} of the available fan speeds,
the main driver being the need to simplify homeassistant integrations for different devices.
For viomi vacuums this is currently straightforward and hard-coded, but we do special handling
for rockrobo devices (based on info() responses):
* If the query succeeds, newer firmware versions (3.5.7+) of v1 are handled as a special case,
otherwise the new-style [100-105] mapping is returned.
* If the query fails, we fall back to rockrobo v1's percentage-based mapping.
This happens, e.g., when the vacuum has no cloud connectivity.
Related to #523
Related downstream issues
home-assistant/core#32821
home-assistant/core#31268
home-assistant/core#27268
* Add fan_speed_presets() for querying available fan speeds
This returns a dictionary {name, value} of the available fan speeds,
the main driver being the need to simplify homeassistant integrations for different devices.
For viomi vacuums this is currently straightforward and hard-coded, but we do special handling
for rockrobo devices (based on info() responses):
* If the query succeeds, newer firmware versions (3.5.7+) of v1 are handled as a special case,
otherwise the new-style [100-105] mapping is returned.
* If the query fails, we fall back to rockrobo v1's percentage-based mapping.
This happens, e.g., when the vacuum has no cloud connectivity.
Related to #523
Related downstream issues
home-assistant/core#32821
home-assistant/core#31268
home-assistant/core#27268
* Fix method naming..
* small pre-merge cleanups
Since #468 seems to be stalled for some time I took the freedom to give it a try.
It supports both enums and integer levels.
The issue with V2 levels is that those 100 > levels < 105 get converted to percentage level on device so when queried later they return a different value.