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

Separate fan speeds into percentages and presets modes #45407

Merged
merged 83 commits into from
Jan 27, 2021
Merged

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Jan 21, 2021

Continuation of @Jc2k 's work in #39204

Architecture issue: home-assistant/architecture#127

WTH: https://community.home-assistant.io/t/wth-homekit-controller-support-for-4-fan-speeds/221305

Breaking change

For users

The fan entity model has changed to split named speeds into percentages in the range from 0 (off)-100 and preset modes.

Why change?

This change allowed us to expand the number of supported speeds to accommodate additional fan models in Home Assistant. (#45407 (comment))

We had 3 fan speeds and that worked great as long as the fan had no more than 3 speeds

Over time we received a number of requests to add fans with 4, 5, or even more speeds and preset modes. This put us in the difficult position of having to reject this support because the underlying fan model didn’t support it.

Percentages were chosen because they can represent up to 100 speeds which should accommodate all fans. Additionally it’s a lot easier to ask your voice voice assistant to set the fan to 20% then remember that medium-low is actually low-medium or have to learn and remember how to say the speeds for every fan model you want to control.

Moving forward

Calls in automations and scripts to fan.set_speed should be replaced:

  • Calls that set a speed should use fan.set_percentage.
  • Calls that set a preset mode should use fan.set_preset_mode.

The following speeds existed in core integration and will now be automatically identified as preset modes: auto, smart, interval, idle, and favorite

Both of the new calls are backwards compatible.

Calls in automations and scripts to fan.turn_on that use the speed attribute should be switched to use the percentage or preset_mode attribute once the underly integration has been updated to support it.

All core fans have been updated to ensure that calls to the fan.turn_on service
map percentage or preset_mode to speed for backwards compatibility.

For developers

The core issues we are solving is:

  • non portable interface to speed for voice assistants
  • limited about of speeds
  • presets and speeds mixed together in speeds

Custom fan integrations should add the @fan_compat decorator to
async_turn_on or turn_on function to ensure backwards compatibility:

Example:

from homeassistant.components.fan import fan_compat

...

@fan_compat
async def async_turn_on(self, speed: Optional[str] = None, percentage: Optional[int] = None, preset_mode: Optional[str] = None, **kwargs: Any) -> None:

The function signature for async_turn_on is now:

async def async_turn_on(self, speed: str = None, percentage: int = None, preset_mode: Optional[str] = None, **kwargs) -> None:

The function signature for turn_on is now:

def turn_on(self, speed: str = None, percentage: int = None, preset_mode: Optional[str] = None, **kwargs) -> None:

Developers that are creating new integrations should review the updated fan entity model documentation at https://developers.home-assistant.io/docs/core/entity/fan

Proposed change

At its heart this PR adds support for a new method on fans, async_set_percentage (and its sync counterpart).

The goal is to provide a gradual transition and allow the existing interface to continue working by detecting when async_set_percentage and set_percentage have been overridden and provide a backwards compatible interface.

If a device supports async_set_percentage then it doesn't need to implement async_set_speed - HA provides a mapping function that converts medium into a percentage.

If a device does not support async_set_percentage then a mapping function will translate calls to that method into reasonable async_set_speed calls.

The speed attribute will always be available - using a mapping function if only a percentage is available.

A new percentage attribute is added and will also always be available - again using a mapping function where needed - turning low into 33% etc.

The mapping function is dynamic and tries to accommodates fans that don't follow the spec properly (e.g. they have added extra speeds) using a similar mechanism to the one in homekit.

The PR ports demo fans to the new API to test it out.

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 configuration.yaml

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:

@bdraco bdraco changed the title Fan speed percentage WIP: WTH: Support more than 3 fan speeds Jan 21, 2021
@bdraco bdraco changed the title WIP: WTH: Support more than 3 fan speeds WIP: WTH: Convert fan speeds to use percentages Jan 22, 2021
@bdraco bdraco changed the title WIP: WTH: Convert fan speeds to use percentages WTH: Convert fan speeds to use percentages Jan 22, 2021
@bdraco bdraco added the WTH Issues & PRs generated from the "Month of What the Heck?" label Jan 22, 2021
@bdraco bdraco marked this pull request as ready for review January 22, 2021 04:23
@bdraco bdraco requested a review from Jc2k January 22, 2021 04:25
@Jc2k
Copy link
Member

Jc2k commented Jan 22, 2021

I've only had a cursory look through so far but I like what I see. I am a big fan of the gradual transition approach.

What is the breaking change at this stage?

@bdraco
Copy link
Member Author

bdraco commented Jan 22, 2021

I've only had a cursory look through so far but I like what I see. I am a big fan of the gradual transition approach.

What is the breaking change at this stage?

Custom integration will have to update their async_turn_on to be able to handle percentage otherwise it will be ignored when a user calls the fan.turn_on service with a percentage

Note that speed will still work with fan.turn_on as it did before so it only affects users who switch over and have a custom fan integration that hasn't updated their code.

@Jc2k
Copy link
Member

Jc2k commented Jan 22, 2021

Ah ok - that's not too onerous I don't think.

@bdraco
Copy link
Member Author

bdraco commented Jan 22, 2021

Looks like coverage is needed for backcompat

https://github.com/home-assistant/core/pull/45407/checks?check_run_id=1746793288

@bdraco
Copy link
Member Author

bdraco commented Jan 22, 2021

It might be better to make a '@fan_speed_compat' and '@fan_percentage_compat' decorator instead. Might need 4 of them though for async vs sync so not sure how much it saves but at least it's more dry

@bdraco
Copy link
Member Author

bdraco commented Jan 22, 2021

Will pick this back up tomorrow, but it's at least ready to get feedback

homeassistant/components/fan/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/fan/__init__.py Show resolved Hide resolved
homeassistant/components/fan/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/fan/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/fan/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/fan/__init__.py Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Jan 22, 2021

I'm hoping to clean this up a bit to use a @speed_compat and @percentage_compat decorator to make this easier for devs who are transitioning their fans and make this a bit more DRY

Will probably rework the shared comment to something like

    #
    # The fan entity model has changed to use percentages.
    #
    # The @percentage_compat decorator will ensure the speed argument is set
    # when a percentage is passed in. When this entity is updated to use the
    # new model with `speed` and `set_speed` removed, switch the decorator to
    # @speed_compat to ensure the percentage argument will be filled for
    # places that still pass in speed instead of percentage.
    #
    @percentage_compat

@bdraco
Copy link
Member Author

bdraco commented Jan 22, 2021

Managed to get this to 100% diff hit 👍

@bdraco bdraco changed the title WTH: Convert fan speeds to use percentages Convert fan speeds to use percentages Jan 22, 2021
@MartinHjelmare
Copy link
Member

Do we need to limit the mqtt fan platform allowed speed values to make it compatible and have some valid speeds?

@bdraco
Copy link
Member Author

bdraco commented Jan 22, 2021

Do we need to limit the mqtt fan platform allowed speed values to make it compatible and have some valid speeds?

I think we should remove the ability for mqtt to set a non-allowed speed value or one not in the speed_list. Right now its accepting an arbitrary string to async_set_speed

I opened a separate PR for this since we should document the breaking change in the release notes for MQTT in case someone is actively using this #45445

I'm not sure anyone actually does this in production (well except that one guy who does) since it looks like it was only added in #33584 to ensure there was full coverage since the code supported passing it.

@bdraco
Copy link
Member Author

bdraco commented Jan 22, 2021

It looks like zha has a fan speed of on

ValueError: 'on' is not in list

@bdraco
Copy link
Member Author

bdraco commented Jan 22, 2021

# Additional speeds in zigbee's ZCL
# Spec is unclear as to what this value means. On King Of Fans HBUniversal
# receiver, this means Very High.
SPEED_ON = "on"

apparently SPEED_ON means very high

@bdraco
Copy link
Member Author

bdraco commented Jan 25, 2021

I pulled out Jc2k's original work to update homekit_controller from #39204 and updated it in #45547 to verify I didn't break anything in the 1:1 case.

@bdraco
Copy link
Member Author

bdraco commented Jan 25, 2021

I've done all the manual testing I can think to do with this and ran though the code line by line again.

@bdraco
Copy link
Member Author

bdraco commented Jan 26, 2021

Did another run though. Can't find anything else that needs to be adjusted.

@bdraco
Copy link
Member Author

bdraco commented Jan 26, 2021

I've converted 14 out of the 26 fans in core in separate PRs.

I was able to make all 14 of them conform to the spec without having to the ability to set some speeds.

@Jc2k
Copy link
Member

Jc2k commented Jan 27, 2021

This is looking really good. My +1 doesn't mean much outside of homekit_controller, but you have it. I can't think of a better way we could handle the transition AND cope with the non compliant fans.

@bdraco
Copy link
Member Author

bdraco commented Jan 27, 2021

I've got HomeKit updated in #45549

Alexa and Google both support the percentage model and they looks quite strait-forward to update

Alexa percentage controller

Google supportsFanSpeedPercent

https://developers.google.com/assistant/smarthome/traits/fanspeed

@MartinHjelmare
Copy link
Member

I'll look at this again after the beta cut.

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Changes look good!

@balloob
Copy link
Member

balloob commented Jan 27, 2021

This should not be merged until we cut the beta.

homeassistant/components/demo/fan.py Outdated Show resolved Hide resolved
homeassistant/components/demo/fan.py Outdated Show resolved Hide resolved
bdraco and others added 2 commits January 27, 2021 08:21
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
@bdraco
Copy link
Member Author

bdraco commented Jan 27, 2021

Final testing looks good 👍

@bdraco bdraco merged commit 068d1b5 into dev Jan 27, 2021
@bdraco bdraco deleted the fan_speed_percentage branch January 27, 2021 23:44
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change cla-signed core integration: fan WTH Issues & PRs generated from the "Month of What the Heck?"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants