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 Starkvind fan capabilities and modes being incorrectly exposed #87

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

freundTech
Copy link
Contributor

@freundTech freundTech commented Jul 16, 2024

Breaking change (for end-users in HA)

The IKEA Starkvind fan modes have been updated. Automations that used to set the fan to 10% speed to switch it to "auto mode" should now be updated to set the preset to "auto."

In auto mode, the actual fan speed is now properly displayed.

For more information, see the PR in the ZHA repo: #87

Proposed change

Fixes home-assistant/core#97440.
Previously, Starkvind exposed 10 speed settings and no modes, where 10% corresponded to auto mode and 20%-100% corresponded to fixed speeds.

This patch correctly exposes auto mode as a mode. It also adds support for showing the actual fan speed while auto mode is enabled.

Starkvind supports 9 fan speeds. Because 9 doesn't neatly fit into 100% I cheated a bit and divided the 100% into 10% increments, where trying to set the fan to 10% sets it to 20% instead. I believe that this gives the overall better user experience compared to having 11.11% increments. This also keeps the previous fixed speeds in HA, instead of changing them all.
The 5 speed modes present on the physical interface of the device correspond to HA speed settings 20%, 40%, 60% and 100%.

Additional information

This depends on home-assistant/core#122033 to work. That PR is no longer needed.
Previous PR: home-assistant/core#114854

@freundTech freundTech changed the title zha: ikea: starkvind: Fix capabilities and modes being incorrectly exposed fan: starkvind: Fix capabilities and modes being incorrectly exposed Jul 16, 2024
@puddly
Copy link
Contributor

puddly commented Jul 16, 2024

You can fix most of the CI errors by running pre-commit:

pip install pre-commit
pre-commit install
pre-commit run --all-files

Ignoring the fan speed attribute values, just so that I understand correctly, the fan supports auto and then five unnamed speeds that you've mapped to 20-100%?

@freundTech
Copy link
Contributor Author

freundTech commented Jul 16, 2024

You can fix most of the CI errors by running pre-commit

Done. Both pytest and pre-commit should now pass.

the fan supports auto and then five unnamed speeds that you've mapped to 20-100%?

The fan supports 9 speeds, of which only 5 are exposed on the physical interface. The other four speeds are halfway between those speeds and used in auto mode, as well as exposed in the official app.

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.80%. Comparing base (c516144) to head (b363b7f).
Report is 8 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev      #87   +/-   ##
=======================================
  Coverage   95.79%   95.80%           
=======================================
  Files          61       61           
  Lines        9324     9340   +16     
=======================================
+ Hits         8932     8948   +16     
  Misses        392      392           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@freundTech
Copy link
Contributor Author

Improved test coverage, cleaned up some code and addressed one of the review comments.

@freundTech freundTech force-pushed the fix-starkvind branch 2 times, most recently from d73ab32 to 76ef28e Compare July 30, 2024 15:27
@freundTech

This comment was marked as resolved.

Fixes home-assistant/core#97440
Previously starkvind exposed 10 speed settings and no modes, where 10% corresponded to auto mode
and 20%-100% corresponded to fixed speeds.

This patch correctly exposes auto mode as a mode. It also adds support for showing the actual
fan speed while auto mode is enabled.
Starkvind supports 9 fan speeds. Because 9 doesn't neatly fit into 100% I cheated a bit and divided
the 100% into 10% increments, where trying to set the fan to 10% sets it to 20% instead. I believe
that this gives the overall better user experience compared to having 11.11% increments.
The 5 speed modes present on the physical interface of the device correspond to HA speed settings 20%,
40%, 60% and 100%.
@freundTech
Copy link
Contributor Author

Rebased again because of the conflict caused by #125

@freundTech
Copy link
Contributor Author

Do you think it will be possible to get this into HA 2024.9 now that the zha rework has landed in 2024.8?

@TheJulianJES TheJulianJES changed the title fan: starkvind: Fix capabilities and modes being incorrectly exposed Fix Starkvind fan capabilities and modes being incorrectly exposed Aug 22, 2024
@TheJulianJES TheJulianJES added the breaking change PR is a breaking change label Aug 22, 2024
@TheJulianJES
Copy link
Contributor

Testing the PR right now. The behavior and code of the the PR seems good to me.

where trying to set the fan to 10% sets it to 20% instead. I believe that this gives the overall better user experience compared to having 11.11% increments

This behavior is slightly weird, I could see a bug report coming along the lines of "I can't set my STARKVIND to 10% anymore". But on the other hand, I also don't like 11.11% increments..
Changing the increments might also be more of a breaking change than not doing that. So, let's keep it like this for now.
Eh, and 10% meant "auto" before, so, should be fine.

Still, for anyone using the Starkvind in automations, this PR could be a breaking change, as setting it to 10% no longer sets it to auto mode. Maybe, this should be mentioned in the "breaking changes" section when ZHA is bumped in HA.
Automations just have to be updated to the proper "auto" fan mode now.

Lastly, I wonder if we can/should add a translation key for the "auto" fan mode on the HA side after this:
image

zha/application/platforms/fan/__init__.py Outdated Show resolved Hide resolved
zha/application/platforms/fan/__init__.py Outdated Show resolved Hide resolved
zha/zigbee/cluster_handlers/manufacturerspecific.py Outdated Show resolved Hide resolved
@TheJulianJES
Copy link
Contributor

TheJulianJES commented Aug 22, 2024

I've added a "breaking change" text in the PR description. This should be copy-pasted by whoever bumps the lib in HA.
Also applied the small suggestions made above.

Copy link
Contributor

@TheJulianJES TheJulianJES left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@TheJulianJES TheJulianJES merged commit 1bbeafb into zigpy:dev Aug 27, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change PR is a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IKEA Starkvind fan setting wrong
4 participants