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 fan speed regression for some xiaomi fans #78406

Merged
merged 4 commits into from
Sep 18, 2022
Merged

Fix fan speed regression for some xiaomi fans #78406

merged 4 commits into from
Sep 18, 2022

Conversation

peteh
Copy link
Contributor

@peteh peteh commented Sep 13, 2022

Proposed change

In my original PR (#77626) I assumed all miot based fans would have the problem that the speed level is 3-4 values only instead of percentage. However, this holds only true for the 1c fan. So my previous PR fixed the 1c fan but broke the others.

This PR only handles the 1c fan as an exception and reverts to the previous behavior for the other fans.

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)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

This pull request needs to be manually signed off by @home-assistant/core before it can get merged.
(message by ReviewEnforcer)

@probot-home-assistant
Copy link

Hey there @rytilahti, @syssi, @starkillerOG, @bieniu, mind taking a look at this pull request as it has been labeled with an integration (xiaomi_miio) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@peteh peteh changed the title FIX: only dfan 1c needs levels to % mapping Only Xiaomi dfan 1c needs levels to % mapping, revert speed behavior for other fans Sep 13, 2022
Copy link
Member

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice cleanup, too! 👍 Let's wait for someone with a test device to verify that it's working, but I'm marking this already for the next patch release.

@rytilahti rytilahti added this to the 2022.9.3 milestone Sep 13, 2022
@rytilahti rytilahti changed the title Only Xiaomi dfan 1c needs levels to % mapping, revert speed behavior for other fans Fix fan speed regression for some xiaomi fans Sep 13, 2022
@balloob balloob modified the milestones: 2022.9.3, 2022.9.4 Sep 14, 2022
@frenck frenck modified the milestones: 2022.9.4, 2022.9.5 Sep 14, 2022
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@peteh
Copy link
Contributor Author

peteh commented Sep 15, 2022

Quick question, how do we make sure that somebody tests it? I only have the 1c fan and already verified the behavior but cannot test for other fan types. Technically the code is reverted for the other fans to the known-to-be-working state, but I cannot verify this.

@rytilahti
Copy link
Member

Alas, there is no easy way for that, besides having enough unit tests. In this case, considering the current level of code coverage, I'd argue against adding more tests if only done for a single device. I'm slowly trying to move all relevant information to python-miio, which will make it the only ground truth for device specifics, but it's a slow process as I don't have test devices either.

@peteh considering you know how to do development work, could you please consider checking out rytilahti/python-miio#1495 and adding the necessary metadata decorators to python-miio to help remove hardcoding? There is a link to my homeassistant fork that makes use of the library-provided information to make testing a breeze :-)

@balloob balloob merged commit 1f7c90b into home-assistant:dev Sep 18, 2022
balloob pushed a commit that referenced this pull request Sep 18, 2022
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
@balloob balloob mentioned this pull request Sep 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2022
@peteh peteh deleted the bugfix/fan1cspeed branch September 19, 2022 18:15
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.

Smartmi Standing Fan 3 (zhimi.fan.za5) wrong intensity.
9 participants