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

Fix xiaomi_miio vacuum fan speed list #32821

Closed

Conversation

springstan
Copy link
Member

@springstan springstan commented Mar 14, 2020

Breaking change

The fan speed settings have been changed to reflect a change in the latest xiaomii_mio vacuum version (3.5.7_002008). Please update your device accordingly to be able to set the fan speed setting of your vacuum correctly.

Proposed change

Alter some of the vacuum fan speed numbers because they have been changed in the newest version 3.5.7_002008. See #32723 for more information.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

vacuum:
  - platform: xiaomi_miio
    host: 192.168.1.2
    token: YOUR_TOKEN

Additional information

Checklist

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link

Hey there @rytilahti, @syssi, mind taking a look at this pull request as its been labeled with a integration (xiaomi_miio) you are listed as a codeowner for? Thanks!

@JurajNyiri
Copy link

I think this is a breaking change, users with older firmware on their vacuum will need to update to the newest fw since the old fw still uses the old values.

This change is not related to app version, app uses old values on old firmware and new values on a new one. Maybe we could do the same?

@frenck
Copy link
Member

frenck commented Mar 14, 2020

Yeah, this is a breaking change, @springstan. Could you adjust the PR to have a breaking change section?

Let's also tag it for the 0.107 release

@springstan
Copy link
Member Author

Of course I can add a breaking change paragraph, so should we advise users to update to the latest version? What if they do not have it available?

@springstan springstan added this to the 0.107.0 milestone Mar 14, 2020
@JurajNyiri
Copy link

There are also a lot of users who are using valetudo fw, which requires rooted fw and as far as I know this new version has not been rooted yet.

I think the best thing to do here is to detect fw version and if it is one of the old ones use old values, else use new.

https://vacuumz.info/ has a list of fw both for gen1 and gen2.

@springstan
Copy link
Member Author

I think the best thing to do here is to detect fw version and if it is one of the old ones use old values, else use new.

That would be ideal, I have found the following while looking through python-miio:

https://github.com/rytilahti/python-miio/blob/9a7c6c456a0a2b66f9331cf7481f0d140f5b3135/miio/device.py#L77-L82

Not sure if we can ask this with a vacuum device though.
@syssi @rytilahti what do you think?

@rytilahti
Copy link
Member

I don't like the idea of breaking this for users who are unwilling to upgrade their firmware (due to using valetudo or otherwise). On top of that, this change really needs to be done upstream in python-miio, which should report the available speeds for a given device instead of hardcoding these to homeassistant. This will also allow us to support viomi vacuums in the future (see #27268).

Re version detection: vacuums without cloud access do not respond correctly to the info query, so using that information without override possibility would break the integration for everyone not willing to update their vacuums and/or letting them to the internet.

@springstan
Copy link
Member Author

I don't like the idea of breaking this for users who are unwilling to upgrade their firmware (due to using valetudo or otherwise). On top of that, this change really needs to be done upstream in python-miio, which should report the available speeds for a given device instead of hardcoding these to homeassistant. This will also allow us to support viomi vacuums in the future (see #27268).

If this should be done upstream I will close this issue for now. Should we add an alert to https://alerts.home-assistant.io/ to notify users which have updated their vacuums to the latest version?

rytilahti added a commit to rytilahti/python-miio that referenced this pull request Mar 15, 2020
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
rytilahti added a commit to rytilahti/home-assistant that referenced this pull request Mar 15, 2020
This needs input from Xiaomi vacuum owners to verify that it does not break anything.
I have personally tested this on rockrobo v1 (old mapping).

Related issues/PRs:
home-assistant#32821
home-assistant#31268
home-assistant#27268

This is a WIP as it requires a new upstream release.
The PR is rytilahti/python-miio#643
@rytilahti
Copy link
Member

I created a PR to fix this, but it will obviously take some time to propagate to homeassistant. I don't know when alerts is supposed to be used, but if this is such a case (which I doubt, though), notifying the users to avoid upgrading until the fix is available would be fine by me.

@springstan
Copy link
Member Author

Yeah I am doubting the usefulness of creating an alert now too since this is not really a permanent issue that cannot be solved.

Closing this PR in favour of the above mentioned one.

@springstan springstan closed this Mar 16, 2020
@springstan springstan removed this from the 0.107.0 milestone Mar 16, 2020
@lock lock bot locked and limited conversation to collaborators Mar 17, 2020
@springstan springstan deleted the fix-fan-speeds-xiaomii branch April 7, 2020 14:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xiaomi_miio: vacuum.set_fan_speed not working on fw: 3.5.7_002008+
6 participants