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

[mqtt.homeassistant] Fix multi-speed fans #17813

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Nov 27, 2024

The original implementation for a fan with speeds was written without an actual device, and just guessing from Home Assistant's limited documentation. Now I actually have a 3-speed fan, and I can see how it's expecting to behave. In short, "percentage" is actually "speed", and it just defaults to a range of 0-100, but if it advertises as say 0-3, it actually wants 0-3, not 0, 33, 66, and 100.

  • fix step math so that the state description represents the step scaled to 0-100%
  • introduce a format override to PercentageValue - Home Assistant expects integers in several places. for a true percentage, most devices were accepting a floating point just fine, but if there's a device (like a 3-speed fan) that has a command template that translates the speeds to some other string value, and uses a lookup to do that, it's important that the value we send to that template is an integer
  • fix that the value template for fan state is state_value_template, not value_template like almost everything else

 * fix step math so that the state description represents the step
   scaled to 0-100%
 * introduce a format override to PercentageValue - Home Assistant
   expects integers in several places. for a true percentage, most
   devices were accepting a floating point just fine, but if there's
   a device (like a 3-speed fan) that has a command template that
   translates the speeds to some other string value, and uses a
   lookup to do that, it's important that the value we send to that
   template is an integer
 * fix that the value template for fan state is state_value_template,
   not value_template like almost everything else

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer ccutrer requested a review from antroids as a code owner November 27, 2024 23:38
@ccutrer
Copy link
Contributor Author

ccutrer commented Nov 28, 2024

@openhab/core-maintainers: is there any guidance on the preferred way to model a multi-speed fan (with discrete speeds between 0 and some number less than 100 - usually, between 2 and 10)? I see four options:

  • As a Dimmer. This has the advantage of being able to accept ON/OFF commands, with ON restoring to the previous speed. A state description could theoretically provide a step value to make any sliders behave better (though I seem to recall that at least one of the UIs don't handle a step value on a slider very well). This is how my ZWave fan controllers present themselves, but I think that's just how ZWave handles it (multi-level switch control command class), and less a conscious decision on the part of the openHAB ZWave binding, or zwave-js. It's also how HomeKit handles Fan accessories (you can set min/max on it to limit to discrete speeds, but it gets weird in the UI sometimes, and Siri's spoken interpretation pretty much assumes percentages). There can also be weirdness with a Dimmer - my ZWave fan controllers are 4 speed, but because ZWave presents it as 0-99 (and 100 is return-to-previous-value), speed 1 is 24.24%, speed 2 is 49.49%, speed 3 is 74.75%, and speed 4 is 100%. This PR essentially implements this option. For myself, I like the discrete numbers, and have rules to maintain number items calculated from the dimmer items, and use the number item for my UIs.
  • As a Number. 0 is off, and speeds map directly - 1, 2, 3 etc. This could be harder to expose to some UIs, especially HomeKit. You also have no way to send a command turning it on returning to whatever speed it previously was.
  • As a Number and a Switch. This duplicates things slightly, but gives you an ON command. This is how [wiz] Initial contribution #17681 that I'm working on currently models it for WiZ devices (as it's a direct mapping from the underlying API). This also matches most closely with how Home Assistant's discovery information for MQTT models it. Though I get the impression Home Assistant itself maps it to something more closely resembling a Dimmer. It is a little unclear if the Number should go to 0 when the fan is off, or if it can be used to always show the what the speed would return to when it gets turned back on.
  • As a Number and a Dimmer. This would give the most flexibility, but with the most duplication of information. Use the Dimmer when you need something that deals better with percentages. Use the Number when you want something that better deals with discrete speeds.

The latter two have a little bit of precedent when dealing with a light bulb that supports dimming (or color) and color temperature. My experience with those things is it can be very inconsistent if the color temperature channel goes to NULL or UNDEF when the light is OFF, or if it stays at its previous value (and what it would return to if it turns back on). It's also inconsistent if sending a command to the color temperature channel will turn the bulb on if it's currently off.

In a slightly different way, there is precedent with color temperature to model as a Dimmer and a Number.

So... thoughts?

All of these questions could probably use some explanation and reasoning in the FAQ at https://www.openhab.org/docs/developer/addons/faq.html#modelling-channels.

@ccutrer ccutrer requested a review from lsiepel December 3, 2024 17:30
@ccutrer
Copy link
Contributor Author

ccutrer commented Dec 3, 2024

@lsiepel : regardless of any responses to my above questions, I think this PR is ready for review - it maintains the current modeling, while fixing issues with some fans.

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

Created a new issue to not loose your question.

@lsiepel lsiepel merged commit 156e691 into openhab:main Dec 3, 2024
5 checks passed
@lsiepel lsiepel added the enhancement An enhancement or new feature for an existing add-on label Dec 3, 2024
@lsiepel lsiepel changed the title [mqtt.homeassistant] fix multi-speed fans [mqtt.homeassistant] Fix multi-speed fans Dec 3, 2024
@lsiepel lsiepel added this to the 4.3 milestone Dec 3, 2024
@ccutrer ccutrer deleted the mqtt-homeassistant-3-speed-fan branch December 3, 2024 18:48
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Dec 16, 2024
* fix step math so that the state description represents the step
   scaled to 0-100%

Signed-off-by: Cody Cutrer <cody@cutrer.us>
cipianpascu pushed a commit to cipianpascu/openhab-addons that referenced this pull request Jan 2, 2025
* fix step math so that the state description represents the step
   scaled to 0-100%

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants