Skip to content

Percentage and preset mode support for MQTT fan#47944

Merged
emontnemery merged 29 commits intohome-assistant:devfrom
jbouwh:percentage-and-preset-mode-support-for-mqtt-fan
Mar 26, 2021
Merged

Percentage and preset mode support for MQTT fan#47944
emontnemery merged 29 commits intohome-assistant:devfrom
jbouwh:percentage-and-preset-mode-support-for-mqtt-fan

Conversation

@jbouwh
Copy link
Copy Markdown
Contributor

@jbouwh jbouwh commented Mar 15, 2021

Breaking change

The fan entity model has been changed. This impacts the way the MQTT Fan supports speeds and the following configuration options are deprecated and will become invalid in core-2021.7.0: speed_command_topic, speed_state_topic, speed_value_template, speeds, payload_off_speed, payload_low_speed, payload_medium_speed, payload_high_speed.

Proposed change

The existing speeds mechanism that supports the three speeds 'low', 'medium' and 'high' will be replaced with a percentage based speed capability that can be operated using a slider. Using preset modes it is possible to supply a ordered list of speeds supported. The legacy speeds will keep working for backwards compatibility.

New settings are introduced to support controlling the speed using percentage or preset modes.

Settings for percentage based speed control:

Setting Abbreviation Type Default Description
percentage_command_topic pct_cmd_t string(optional) None The MQTT topic to publish commands to change the fan speed state based on a percentage.
percentage_state_topic pct_stat_t string (optional) None The MQTT topic subscribed to receive fan speed based on percentage.
percentage_value_template pct_val_tpl string(optional) None Defines a template to extract a value from fan percentage speed.
speed_range_min spd_rng_min int(optional) 1 The minimum of numeric output range (off not included).
speed_range_max spd_rng_max int(optional) 100 The maximum of numeric output range (representing 100%).

Settings for preset mode based speed control:

Setting Abbreviation Type Default Description
preset_mode_command_topic  pr_mode_cmd_t string (optional) None The MQTT topic to publish commands to change the preset mode.
preset_mode_state_topic pr_mode_stat_t string (optional) None The MQTT topic subscribed to receive preset mode state updates.
preset_mode_value_template  pr_mode_val_tpl string (optional) None Defines a template to extract a value from the preset_mode payload.
preset_modes pr_modes list (optional) [] List of preset modes this fan is capable of running at. Common examples include  auto, smart, whoosh, eco and breeze.

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

Example entry for configuration.yaml:

# Example using percentage based speeds with preset modes configuration.yaml
fan:
  - platform: mqtt
    name: "Bedroom Fan"
    state_topic: "bedroom_fan/on/state"
    command_topic: "bedroom_fan/on/set"
    oscillation_state_topic: "bedroom_fan/oscillation/state"
    oscillation_command_topic: "bedroom_fan/oscillation/set"
    percentage_state_topic: "bedroom_fan/speed/percentage_state"
    percentage_command_topic: "bedroom_fan/speed/percentage"
    preset_mode_state_topic: "bedroom_fan/speed/preset_mode_state"
    preset_mode_command_topic: "bedroom_fan/speed/preset_mode"
    preset_modes:
       -  "auto"
       -  "smart"
       -  "whoosh"
       -  "eco"
       -  "breeze"
    qos: 0
    payload_on: "true"
    payload_off: "false"
    payload_oscillation_on: "true"
    payload_oscillation_off: "false"
    speed_range_min: 1
    speed_range_max: 100

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.
  • 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:

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 15, 2021

I might need some help with updating documentation and defining additional test.

@probot-home-assistant
Copy link
Copy Markdown

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with an integration (homeassistant) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@emontnemery
Copy link
Copy Markdown
Contributor

emontnemery commented Mar 16, 2021

Some tests are failing, I think it should be possible to make this a non-breaking change.
We can mark the old speed related settings as deprecated, including removing them from the documentation, scheduled for removal 2021.10 or something like that.

The new code needs to be tested. The tests are in tests/components/mqtt/test_fan.py.
Follow the guide here https://developers.home-assistant.io/docs/development_testing for how to get the environment ready for testing then you can do:

  • pytest tests/components/mqtt/test_fan.py - to test only MQTT fan
  • pytest --cov=homeassistant/components/mqtt --cov-report html -- tests/components/mqtt/test_fan.py - to test only MQTT fan and keep track of test coverage of the new code

Comment or ping me on discord if you need some more pointers to get started with the tests.

PS
Can you accept the CLA please?

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 16, 2021

Some tests are failing, I think it should be possible to make this a non-breaking change.
We can mark the old speed related settings as deprecated, including removing them from the documentation, scheduled for removal 2021.10 or something like that.

The new code needs to be tested. The tests are in tests/components/mqtt/test_fan.py.
Follow the guide here https://developers.home-assistant.io/docs/development_testing for how to get the environment ready for testing then you can do:

  • pytest tests/components/mqtt/test_fan.py - to test only MQTT fan
  • pytest --cov=homeassistant/components/mqtt --cov-report html -- tests/components/mqtt/test_fan.py - to test only MQTT fan and keep track of test coverage of the new code

Comment or ping me on discord if you need some more pointers to get started with the tests.

PS
Can you accept the CLA please?

I am still working on the tests to complete them and fixing some code. There is good progress in this.
I will have a look at the CLA signing soon.

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 16, 2021

I have completed and fixed the tests. Where can I sign the CLA, cannot find a link. I will do a PR for the fixes asap.

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @jbouwh,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@jbouwh jbouwh marked this pull request as ready for review March 16, 2021 21:51
@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 16, 2021

Looks like still I have to clean op some _LOGGER code. Further tests seem ok. Can some one advise me on this?

Can see now not all new code is covered with tests. Perhaps there a simple way check this locally this in the dev container.

@emontnemery
Copy link
Copy Markdown
Contributor

Run this to check coverage locally:
pytest --cov=homeassistant/components/mqtt --cov-report html -- tests/components/mqtt/test_fan.py

@emontnemery
Copy link
Copy Markdown
Contributor

@jbouwh You don't allow combining preset modes with percentage although that is allowed by the entity model: https://developers.home-assistant.io/docs/core/entity/fan/
Can you explain why you make them exclusive?

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 17, 2021

There are still a few tests needed as you can see, but I will try to fix those today. The only thing I wonder this might be a bug in the core fan integration is the calculation from numeric ranges to percentages to percentages and back.

from homeassistant.util.percentage import int_states_in_range, ranged_value_to_percentage, percentage_to_ranged_value

SPEED_RANGE = (1, 255)  # off is not included

percentage = ranged_value_to_percentage(SPEED_RANGE, 127)

value_in_range = math.ceil(percentage_to_ranged_value(SPEED_RANGE, 50))

If the base is 1 (whiteout off), the functions work fine. But when you select a higher range bottom.
0% should give an answer of bottom range -1 and the ceil should be the top of the range. Because 0% is still off a higher bottom than 1, all values below the bottom should result to 0%.

The code is not working correct when e.g. the SPEED_RANGE = (80, 1023)
0% will still result in 0, but 100% does not return 1023 but 944 (1023-80+1).
If we would add 80 bot not the bottom of the range (1) then the top wil be 1023
0 % should still result to a value of 0, but 1% should start at 80.

@emontnemery
Copy link
Copy Markdown
Contributor

@bdraco can you have a look at #47944 (comment), is it a bug in the base component?

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 17, 2021

@jbouwh You don't allow combining preset modes with percentage although that is allowed by the entity model: https://developers.home-assistant.io/docs/core/entity/fan/
Can you explain why you make them exclusive?

What is not allowed is combining a percentage_command_topic with preset_mode_command_topic. I will have a look if this can be combined and what will be de consequence of that.

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 17, 2021

I have fixed the constraint not being able to combine preset modes with percentages. De speed step will be combined with speeds or preset modes if they are combined. I will update the documentation in this PR. The problem with the base component still exists but will only have impact if speed_range_min > 1. Most integrations wil have set this value to 1.

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 18, 2021

Documentation update PR (home-assistant/home-assistant.io#17048)

Copy link
Copy Markdown
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Looks good overall 👍
Please fix the comments.

Comment thread homeassistant/components/mqtt/fan.py Outdated
Comment thread homeassistant/components/mqtt/fan.py Outdated
@jbouwh jbouwh requested a review from emontnemery March 18, 2021 12:48
jbouwh and others added 4 commits March 25, 2021 10:01
Comment speeds for mqtt fan are deprecated not needed here

Co-authored-by: Erik Montnemery <erik@montnemery.com>
Comment speeds for mqtt fan are deprecated not needed here

Co-authored-by: Erik Montnemery <erik@montnemery.com>
Comment speeds for mqtt fan are deprecated not needed here

Co-authored-by: Erik Montnemery <erik@montnemery.com>
Comment speeds for mqtt fan are deprecated not needed here

Co-authored-by: Erik Montnemery <erik@montnemery.com>
@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 25, 2021

Looking great. Quick question, how would tuya fans work with this that use tuyasend4 to send speed commands? For example, my current tuya/tasmota fan (that has 12 speeds) uses this to set the speed

payload_low_speed: '2,3' payload_medium_speed: '2,7' payload_high_speed: '2,11'
Would you be able to use "percentage_value_template" and have a template similar to this?
{% if speed | string == "1" %}
'2,0'

Or would there need to be a separate thing like "percentage_command_payload_template"
Thanks for your work.

You can use percentage_value_template on states. For a fan with 12 speeds you can use:
"speed_range_min": 1, a "speed_range_max":12

This will output 0 for 0% and 12 for 100%.
If you want to use templating for your output I would advise to use automation rules.

@joshua-pe
Copy link
Copy Markdown

joshua-pe commented Mar 25, 2021

If you want to use templating for your output I would advise to use automation rules.

Thanks for your response.

Are you saying the best way of sending the mqtt speed commands is not directly though yaml and is to make a seperate automation for setting the fan speed? I didn't necessarily want to use templates, I just thought that would be the easiest way of converting a speed of 1 to a tuya send of '2,1'

Sorry for the questions, I am just trying to prepare for this awesome change.

@jbouwh jbouwh requested a review from emontnemery March 25, 2021 15:05
@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 25, 2021

If you want to use templating for your output I would advise to use automation rules.

Thanks for your response.

Are you saying the best way of sending the mqtt speed commands is not directly though yaml and is to make a seperate automation for setting the fan speed? I didn't necessarily want to use templates, I just thought that would be the easiest way of converting a speed of 1 to a tuya send of '2,1'

Sorry for the questions, I am just trying to prepare for this awesome change.

No problem,

There might be a good reason to implement a preset_mode_command_template and percentage_mode_command_template like the fan_mode_command_template in mqtt climate. It seems not to difficult to implement this. For now it will not be part of this PR.

@emontnemery
Copy link
Copy Markdown
Contributor

Let's add support for templates in a separate PR, this one is big enough as it is 👍

Comment thread homeassistant/components/mqtt/abbreviations.py Outdated
Comment thread homeassistant/components/mqtt/fan.py Outdated
Comment thread homeassistant/components/mqtt/fan.py Outdated
Comment thread homeassistant/components/mqtt/fan.py Outdated
Comment thread homeassistant/components/mqtt/fan.py Outdated
jbouwh and others added 5 commits March 25, 2021 19:54
Co-authored-by: Erik Montnemery <erik@montnemery.com>
Co-authored-by: Erik Montnemery <erik@montnemery.com>
Co-authored-by: Erik Montnemery <erik@montnemery.com>
Co-authored-by: Erik Montnemery <erik@montnemery.com>
Co-authored-by: Erik Montnemery <erik@montnemery.com>
@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 25, 2021

Documentation PR home-assistant/home-assistant.io#17048

Comment thread homeassistant/components/mqtt/fan.py
@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Mar 25, 2021

I will save with black and commit the changes 👍

@emontnemery
Copy link
Copy Markdown
Contributor

Thanks, @jbouwh, this is a great first PR! 🎉

@emontnemery emontnemery merged commit 5b17aaf into home-assistant:dev Mar 26, 2021
@home-assistant home-assistant deleted a comment from homeassistant Mar 26, 2021
@home-assistant home-assistant deleted a comment from homeassistant Mar 26, 2021
@home-assistant home-assistant deleted a comment from homeassistant Mar 26, 2021
@joshua-pe
Copy link
Copy Markdown

it seems not to difficult to implement this. For now it will not be part of this PR.

Awesome, and congrats on the awesome pull request. If/when this happens could you link the pull request here?

Thanks for you work!

@jbouwh jbouwh deleted the percentage-and-preset-mode-support-for-mqtt-fan branch March 26, 2021 07:54
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 27, 2021
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.

5 participants