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

Convert IKEA Starkvind fan speed attribute #3088

Merged
merged 2 commits into from
May 28, 2024

Conversation

freundTech
Copy link
Contributor

@freundTech freundTech commented Apr 4, 2024

Proposed change

IKEA Starkvind has two similar attributes: fan_mode and fan_speed.

fan_mode can take a value from 0-50 where 0 is off, 1 is auto and 10-50 are manual speed modes.
fan_speed can take a number from 10-50 or 0, where 0 is off and 10-50 is the fan speed. fan_speed is read-only.

If fan_mode is set to a manual mode that is not a multiple of 5 it is rounded to a multiple of 5, which can be seen by reading the fan_speed attribute. For this reason The current quirk scales that 10-50 range to a 2-10 range, eliminating the undefined and duplicate (rounded) values.

This patch applies the same downscaling to the fan_speed attribute, restoring the correspondence between fan_mode and fan_speed values.

Native values: Set fan_mode to 30 => fan_speed reads 30
Current state: Set fan_mode to 6 => fan_speed reads 30
With patch: Set fan_mode to 6 => fan_speed reads 6

Additional information

This will be followed up by a PR in home-assistant to eventually fix home-assistant/core#97440.
Because home-assistant currently only uses the fan_mode and not the fan_speed attribute this does not depend on the corresponding home-assistant PR. Only the other way around.

I also changed the < and > to <= and >= to improve understandability of the code by mentioning the limit values and replaced / with // to prevent introducing floating point values.

Checklist

  • The changes are tested and work correctly
  • pre-commit checks pass / the code has been formatted using Black
  • Tests have been added to verify that the new code works

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.94%. Comparing base (9ccb69a) to head (79de6c2).
Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3088      +/-   ##
==========================================
+ Coverage   87.90%   87.94%   +0.03%     
==========================================
  Files         300      300              
  Lines        9222     9222              
==========================================
+ Hits         8107     8110       +3     
+ Misses       1115     1112       -3     

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

@freundTech
Copy link
Contributor Author

Added a test to make codecov happy.

freundTech added a commit to freundTech/home-assistant-core that referenced this pull request Apr 4, 2024
…posed

Fixes home-assistant#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%.

This patch depends on zigpy/zha-device-handlers#3088 being merged and released.
freundTech added a commit to freundTech/home-assistant-core that referenced this pull request Apr 4, 2024
…posed

Fixes home-assistant#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%.

This patch depends on zigpy/zha-device-handlers#3088 being merged and released.
@freundTech
Copy link
Contributor Author

Alternatively it would also be possible to convert neither fan_mode, nor fan_speed and let the api consumer (Home Assistant in this case) deal with mapping that to meaningful settings.

@TheJulianJES TheJulianJES changed the title ikea: starkvind: Also convert fan_speed Convert IKEA Starkvind fan speed attribute Apr 7, 2024
@TheJulianJES TheJulianJES self-assigned this Apr 7, 2024
@TheJulianJES TheJulianJES added the enhancement Improve an existing quirk label Apr 7, 2024
@freundTech
Copy link
Contributor Author

Any update on the review?
@TheJulianJES

Copy link
Collaborator

@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.

Sorry for the long wait. The quirk part should be good-to-go IMO.
Also thanks for adding tests!

(In the future, the quirk should be updated to use new-style zigpy AttributeDefs though)

@TheJulianJES TheJulianJES merged commit 5434dfb into zigpy:dev May 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve an existing quirk
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IKEA Starkvind fan setting wrong
2 participants