home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
72.76k stars 30.47k forks source link

No way to set the speed steps from the mqtt fan configuration. #48802

Closed finity69x2 closed 3 years ago

finity69x2 commented 3 years ago

The problem

When creating an MQTT Fan using the new percentage controls for speed there is no way to set the "percentage_step" from the yaml configuration.

it reverts to the default step of 1 instead of the desired step of 33.333333336 for a normal three speed fan.

What is version of Home Assistant Core has the issue?

core-2021.4.0

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant Container

Integration causing the issue

MQTT Fan

Link to integration documentation on our website

https://www.home-assistant.io/integrations/fan.mqtt/

Example YAML snippet

- platform: mqtt
    name: "Master Bedroom Fan"
    command_topic: "cmnd/iFan03-2/FanSpeed"
    state_topic: "stat/iFan03-2/RESULT"
    state_value_template: >
      {% if value_json.FanSpeed is defined %}
        {% if value_json.FanSpeed == 0 -%}off{%- elif value_json.FanSpeed > 0 -%}on{%- endif %}
      {% else %}
        {% if states.fan.master_bedroom_fan.state == 'off' -%}off{%- elif states.fan.master_bedroom_fan.state == 'on' -%}on{%- endif %}
      {% endif %}
    availability_topic: tele/iFan03-2/LWT
    payload_off: "off"
    payload_on: "on"
    payload_available: Online
    payload_not_available: Offline
    percentage_command_template: >
      {% if value == 33 %}
        1
      {% elif value == 67 %}
        2
      {% elif value == 100 %}
        3
      {% else %}
        0 
      {% endif %}
    percentage_command_topic: "cmnd/iFan03-2/FanSpeed"
    percentage_state_topic: "stat/iFan03-2/RESULT"
    percentage_value_template: >
      {% if value_json.FanSpeed == 1 %}
        33
      {% elif value_json.FanSpeed == 2 %}
        67
      {% elif value_json.FanSpeed == 3 %}
        100
      {% else %}
        0
      {% endif %}

Anything in the logs that might be useful for us?

No response

this integration should contain a speed_count as the template fan does so we can set the expected percentage steps.

I can override the "percentage_step" attribute in the customize section but I don't think that should be needed when the option to make it work is already included in another fan configuration. If it was added here it would maintain consistency.

probot-home-assistant[bot] commented 3 years ago

Hey there @emontnemery, mind taking a look at this issue as its been labeled with an integration (mqtt) you are listed as a codeowner for? Thanks! (message by CodeOwnersMention)

emontnemery commented 3 years ago

@jbouwh, @bdraco Can you comment on what is the expected behavior here?

jbouwh commented 3 years ago

The speed_step =100 / speed_count and is calculated for you and cannot be set through the configuration. If you use percentage, you will have 100 steps of 1. When you use preset_modes, the amount of steps depends on the speeds you have configured (mind that only non speeds like off and auto are not counted). If you still use the legacy speeds, you will have max three speeds (low, medium and high).

Non speeds are: _NOT_SPEED_OFF = "off" _NOT_SPEED_ON = "on" _NOT_SPEED_AUTO = "auto" _NOT_SPEED_SMART = "smart" _NOT_SPEED_INTERVAL = "interval" _NOT_SPEED_IDLE = "idle" _NOT_SPEED_FAVORITE = "favorite" _NOT_SPEED_SLEEP = "sleep" _NOT_SPEED_SILENT = "silent"

If you want 3 speeds through percentage, you could also use a use speed_range_min: 1, speed_range_max: 3. This will convert 100% to 3 and 0% to 0. Do not include 0 in the speed_range since this is not a speed. You will not need a command_template for that since the integration is doing the conversion for you.

speed_step will remain 100 when using percentage_topic. You will need preset_mode_topic and preset_modes to get a larger speed_step that will depend on the amount of preset_modes (non speeds excluded).

finity69x2 commented 3 years ago

OK,

there is a sort of discrepancy between the information here and in the documentation for the MQTT fan, the template fan and in one of the forum posts by bdraco in which it was said to not use speeds as a preset mode:

https://community.home-assistant.io/t/need-help-with-fan-template-with-the-new-options/287904/18

Which is also in the documentation for the template fan:

https://www.home-assistant.io/integrations/fan.template/#preset_modes

but not in the docs for the mqtt fan:

https://www.home-assistant.io/integrations/fan.mqtt/#preset_modes

But it seems that you are saying that it is expected to use preset_modes of low, medium, high for a regular three speed fan and to not use percentages? And it should work? And I assume that will also work with voice assistants to set the speed to those named preset speed modes (IOW, "set the fan to low" will work)?

I just tried your suggestion and it does seem to work to set the correct fan speeds but the at the speed 1 setting of the slider (34 not 33 as I would have thought) the fan speed changes to low but the slider immediately returns to the 0 position. It doesn't retain the setting. but 67 and 100 both do. and if I set the slider 35 it actually stays there and sets the correct speed (1).

The interface for the slider is clunky when using percentages. I seriously doubt that this is the best user interface because I doubt there is any fan out there that uses 100 different speeds. the vast majority of fans will use a few (or at most several) discrete speed percentages. and if there is a fan with 100 individual speeds then let the default be 100 as it is now.

And if your fan only has a set number of speeds (most do) using percent speeds with no configurable slider detents the slider can stay at any number of percentage points between 0 and 100 so it is totally useless to give an indication of the actual fan speed. And to make it worse the "percentage:" attribute stays at "null" the entire time so you can't even use the attribute to get the current speed.

Is there some reason to allow a speed count setting option in the template fan but not in the mqtt fan to get that same functionality between the two platforms?

and as an FYI I can create a custom attribute to set the "percentage_step:" attribute in the mqtt fan created with my config above to "33.333333333333336" and it works as expected with the stops being at 33, 67 and 100 with nothing allowed in between and the "percentage:" attribute reflects the correct speed setting (33, 67, 100). But that seems like an unnecessary work around if you can include the speed count option in the config.

for completeness here is the recent config that I am describing the results of above:

  - platform: mqtt
    name: "Master Bedroom Fan"
    command_topic: "cmnd/iFan03-2/FanSpeed"
    state_topic: "stat/iFan03-2/RESULT"
    state_value_template: >
      {% if value_json.FanSpeed is defined %}
        {% if value_json.FanSpeed == 0 -%}off{%- elif value_json.FanSpeed > 0 -%}on{%- endif %}
      {% else %}
        {% if states.fan.master_bedroom_fan.state == 'off' -%}off{%- elif states.fan.master_bedroom_fan.state == 'on' -%}on{%- endif %}
      {% endif %}
    availability_topic: tele/iFan03-2/LWT
    payload_off: "off"
    payload_on: "on"
    payload_available: Online
    payload_not_available: Offline
    percentage_command_topic: "cmnd/iFan03-2/FanSpeed"
    percentage_state_topic: "stat/iFan03-2/RESULT"
    speed_range_min: 1
    speed_range_max: 3
jbouwh commented 3 years ago

This is as it was intended to work. I will have a look at this consistency issue you are addressing and see if I can improve this. It should not be that difficult to introduce a different speed step then 1 when using percentage. Some fans like mine use PWM control. That could result in a speed range of 1-255 or 1-1023. I agree with ranges 1-50 or smaller a higher speed step could be used to enforce the correct setting in the GUI.

The percentage slider step should adopt to preset modes or legacy speeds if no percentage topic or legacy speeds have been configured. If percentage command topic is configured speed step should be something like 100 / (speed_range_max - speed_range_min). Would this lead to a better consistency?

emontnemery commented 3 years ago

In the "converting speeds" section here: https://developers.home-assistant.io/docs/core/entity/fan, there's an example of a fan with three named fan speeds. Do we want to make that an option for MQTT fan?

jbouwh commented 3 years ago

In the "converting speeds" section here: https://developers.home-assistant.io/docs/core/entity/fan, there's an example of a fan with three named fan speeds. Do we want to make that an option for MQTT fan?

I thing the only thing to do is updating one code line and add some tests. This will fix percentage_step. I can do a PR if you like. I think an option should not be added.

jbouwh commented 3 years ago

I thing the only thing to do is updating one code line and add some tests.

The only thing is that the number of speeds should be limited to 100. Adding an for speed_count (as override) option could be something for a next release since it affects documentation.

emontnemery commented 3 years ago

I thing the only thing to do is updating one code line

What change do you suggest?

jbouwh commented 3 years ago

in fan.py from line 305:

        if not self._speeds_list or self._feature_percentage:
            speed_count = self._speed_range[1] - self._speed_range[0] + 1
            self._speed_count = speed_count if speed_count <= 100 else 100
        else:
            self._speed_count = len(self._speeds_list)

previous code was:

        if not self._speeds_list or self._feature_percentage:
            self._speed_count = 100
        else:
            self._speed_count = len(self._speeds_list)
emontnemery commented 3 years ago

Oh, I see! That seems reasonable, and there's probably no need for an override?

bdraco commented 3 years ago

@jbouwh we actually have a helper for that


from homeassistant.util.percentage import int_states_in_range

        if self._feature_percentage:
            self._speed_count = min(int_states_in_range(self._speed_range), 100)
        else:
            self._speed_count = len(self._legacy_speeds_list_no_off) or 100
bdraco commented 3 years ago

Suggested cleanups

https://github.com/bdraco/home-assistant/commit/a6bf09eff9cb03739dc9d31479396c00ec7d5356

I'll check back tomorrow. I need to go to bed as its late late here and I have a busy work day tomorrow

finity69x2 commented 3 years ago

Would this lead to a better consistency?

it would be better in the case of the UI reflecting the actual speeds (I think...) and it will fix the speed detents.

However, I'm not sure that the "speed_range_max" and "speed_range_min" options are easily understood to mean what you are trying to accomplish.

When I first saw them I assumed they were going to set the actual end limits of the slider. IOW, if the min was 5 and the max was 15 then the slider would have endpoints of 5 to 15 instead of 0 to 100.

And to add to that confusion if the they are expected to be used for speed step conversions then I'm trying to see why there would be a need to have a minimum in the first place. Setting the min to 50 and the max to 53 would (should?) have the exact same result as having the min set to 1 and the max set to 3. So can't see the usefulness of having a min setting.

if you eliminate those two options and introduce the "speed_count" option it would result in the expectation that the slider would still have the 0 to 100 range but it would be broken up in into the 100/speed_count steps. Just as the template fan does now.

That would be even better consistency between the two platforms IMO.

as far as the named speeds I don't necessarily think it would be important to add those as long as it can be verified that using speed names in the preset_modes would allow it to work with similar functionality to the legacy speed names. I'm still unclear on that.

jbouwh commented 3 years ago

When I first saw them I assumed they were going to set the actual end limits of the slider. IOW, if the min was 5 and the max was 15 then the slider would have endpoints of 5 to 15 instead of 0 to 100.

This is something @bdraco actually created in the base class. If your fan has PWM speeds (0-255) and you want the speed to to range from from 80 to 200 in (speed range 80 - 200) the gui you will see 0 to 100% scaling leading to 80 to 200 as output. In most cases spraad_range_min will be 1. I am preparing a PR, and are still testing.