Add new fan speed mapping for V2 Roborock Vacuum #20286

poma · 2019-01-21T09:33:00Z

Description:

Update fan speeds to reflect values that Mi Home app is sending to the vacuum. Tested on Roborock S50, this version correctly reflects values set by the app, and the app correctly highlights speeds set by this component.

To keep backwards compatibility, added a new boolean setting to yaml config - new_fan_speeds which defaults to the old fan speed mapping. Maybe someone can suggest shorter and more clear name for new variables.

Related issue (if applicable): fixes #17695
Link to an earlier attempt to fix this: #17090

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#8233

Example entry for `configuration.yaml` (if applicable):

vacuum:
- platform: xiaomi_miio
  host: <...>
  token: <...>
  new_fan_speeds: true

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.io

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

houndci-bot · 2019-01-21T09:33:15Z

homeassistant/components/vacuum/xiaomi_miio.py

                        if value == speed][0]
            return speed

    @property
    def fan_speed_list(self):
        """Get the list of available fan speed steps of the vacuum cleaner."""
-        return list(sorted(FAN_SPEEDS.keys(), key=lambda s: FAN_SPEEDS[s]))
+        return list(sorted(self._speed_map.keys(), key=lambda s: self._speed_map[s]))


line too long (85 > 79 characters)

houndci-bot · 2019-01-21T09:33:15Z

homeassistant/components/vacuum/xiaomi_miio.py

 PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
    vol.Required(CONF_HOST): cv.string,
    vol.Required(CONF_TOKEN): vol.All(str, vol.Length(min=32, max=32)),
    vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
+    vol.Optional(CONF_NEW_FAN_SPEEDS, default=DEFAULT_NEW_FAN_SPEEDS): cv.boolean,


line too long (82 > 79 characters)

frenck · 2019-01-21T11:32:27Z

⚠️ I was unable to find a documentation PR (matching this code change) in our documentation repository.

📚Please, update the documentation and open a PR for it (be sure to create a documentation PR against the next branch) and link 🔗 this PR and the new documentation PR by mentioning the link to the PR's in both openings posts/descriptions.

🏷 I am adding the docs-missing label until this has been resolved.

👍 Thanks (times a million!)

poma · 2019-01-21T11:49:08Z

@frenck fixed, home-assistant/home-assistant.io#8233

MartinHjelmare · 2019-01-23T03:49:37Z

Comment on the previous PR attempt:

These values are only valid to the 2nd generation model, the proper way to fix this is to change python-miio to report back the available fan speeds per model and use that information inside home-assistant.

poma · 2019-01-23T06:36:30Z

Relevant comment from the linked issue

But it is not an issue for the underlying library because it doesn't know about the speeds and it doesn't enumerate them, it just set them by integer value. HA is enumerating the speeds and using library to set them to certain integer value. That would be brand new feature for underlying library to know which type of vacuum it is communicating with and the PR would also need to change the API because now the underyling library would be doing the enumeration of speeds. And then we would also need to change HA.

So if we want to request to push functionality from HA to the underlying library it can take a really long time. The better approach is to solve this within HA. Auto detecting the vacuum version instead of setting it in config would be good though but I don't know a way to do that.

MartinHjelmare · 2019-01-23T07:56:31Z

Time isn't an argument for moving logic into home assistant. The library should preferably tell us, somehow, how many speed settings there are per device.

poma · 2019-01-23T07:58:51Z

But the logic is already in home assistant and it works incorrectly. By that logic we should remove the speed dropdown from home assistant and let user enter integer numbers until this is implemented in miio lib.

MartinHjelmare · 2019-01-23T08:06:08Z

No. The library should give us enough information, eg a version or a list of possible speeds, to know the correct interface to use.

poma · 2019-01-23T08:10:21Z

Maybe. But what currently happens is home assistant tries to guess the list of possible speeds and does this wrong. The existing functionality should be either fixed or removed. The current PR will at least allow user to choose the version to fix their UI.

MartinHjelmare · 2019-01-23T08:14:46Z

We should not add a config option for this.

poma · 2019-01-23T09:16:31Z

Well then, leaving this for someone with more spare time to fix

syssi · 2019-01-23T09:21:31Z

@poma I'm planning a python-miio release this weekend. Let's move your code there! :-) I will provide some support.

poma · 2019-01-23T09:24:30Z

First we will need some way to automatically figure out which mapping to use for a given vacuum. It could depend on vacuum model, firmware version, or even version of the Mi Home app that user installed on their phone, I don't know.

syssi · 2019-01-23T09:30:59Z

Got it. Let's start with your device. ;-) Which model and firmware version do you use?

poma · 2019-01-23T09:35:04Z

Roborock S50, firmware 3.3.9_001710

upd: AFAIK the new mappings should apply to gen 2 vacuum models. Also I've notice that sometimes fan speeds can still use older speed mappings, so when mapping from int to string it should check all mapping versions.

syssi · 2019-01-23T10:15:09Z

This is a first draft: rytilahti/python-miio#468

robbiet480 · 2019-03-25T01:16:17Z

@syssi @poma Checking in on this + Can you fix the merge conflict?

andrewsayre · 2019-04-27T00:54:15Z

This PR has gone stale. Please update the code and open a new PR if you wish to continue with this contribution.

Add new fan speed mapping for V2 Roborock Vacuum

58ab84b

homeassistant added the cla-signed label Jan 21, 2019

ghost added in progress and removed cla-signed labels Jan 21, 2019

homeassistant added platform: vacuum.xiaomi_miio small-pr PRs with less than 30 lines. labels Jan 21, 2019

houndci-bot reviewed Jan 21, 2019

View reviewed changes

frenck added the docs-missing label Jan 21, 2019

poma mentioned this pull request Jan 21, 2019

update vacuum.xiaomi_miio.markdown home-assistant/home-assistant.io#8233

Closed

2 tasks

frenck removed the docs-missing label Jan 21, 2019

Fix indentation

203190f

homeassistant added the cla-signed label Jan 23, 2019

robbiet480 added integration: xiaomi_miio and removed platform: vacuum.xiaomi_miio labels Mar 20, 2019

andrewsayre closed this Apr 27, 2019

rytilahti mentioned this pull request Jun 21, 2019

Fix fan speeds to match roborock.vacuum.s5 v3.3.9_001768 firmware #24676

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new fan speed mapping for V2 Roborock Vacuum #20286

Add new fan speed mapping for V2 Roborock Vacuum #20286

poma commented Jan 21, 2019 •

edited

Loading

houndci-bot Jan 21, 2019

houndci-bot Jan 21, 2019

frenck commented Jan 21, 2019

poma commented Jan 21, 2019

MartinHjelmare commented Jan 23, 2019

poma commented Jan 23, 2019 •

edited

Loading

MartinHjelmare commented Jan 23, 2019

poma commented Jan 23, 2019

MartinHjelmare commented Jan 23, 2019

poma commented Jan 23, 2019

MartinHjelmare commented Jan 23, 2019

poma commented Jan 23, 2019

syssi commented Jan 23, 2019

poma commented Jan 23, 2019

syssi commented Jan 23, 2019

poma commented Jan 23, 2019 •

edited

Loading

syssi commented Jan 23, 2019

robbiet480 commented Mar 25, 2019

andrewsayre commented Apr 27, 2019

Add new fan speed mapping for V2 Roborock Vacuum #20286

Add new fan speed mapping for V2 Roborock Vacuum #20286

Conversation

poma commented Jan 21, 2019 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

houndci-bot Jan 21, 2019

Choose a reason for hiding this comment

houndci-bot Jan 21, 2019

Choose a reason for hiding this comment

frenck commented Jan 21, 2019

poma commented Jan 21, 2019

MartinHjelmare commented Jan 23, 2019

poma commented Jan 23, 2019 • edited Loading

MartinHjelmare commented Jan 23, 2019

poma commented Jan 23, 2019

MartinHjelmare commented Jan 23, 2019

poma commented Jan 23, 2019

MartinHjelmare commented Jan 23, 2019

poma commented Jan 23, 2019

syssi commented Jan 23, 2019

poma commented Jan 23, 2019

syssi commented Jan 23, 2019

poma commented Jan 23, 2019 • edited Loading

syssi commented Jan 23, 2019

robbiet480 commented Mar 25, 2019

andrewsayre commented Apr 27, 2019

poma commented Jan 21, 2019 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

poma commented Jan 23, 2019 •

edited

Loading

poma commented Jan 23, 2019 •

edited

Loading