Fix ESPHome color temperature precision for light entities #91424

danielkent-net · 2023-04-14T14:44:24Z

Proposed change

This PR changes the behavior of ESPHome light entities when setting color temperature. Currently, the color temperature can only be set in whole-number mireds, which cannot map to the full range of Kelvin color temperature values.

Since ESPHome entities both transmit and store color temperature in floating point mireds, we can use this property to convert this value to and from Kelvin more accurately, and avoid many of the issues that result from rounding the value.

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

Additional information

This PR fixes or closes issue: fixes Loss of color temperature precision for ESPHome Light devices #91392 fixes ESPHome RGBCCT causes repeated "Connection error occurred: division by zero" disconnections #89846
This PR is related to esphome issue 2186

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
I have followed the perfect PR recommendations
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:

Documentation added/updated for www.home-assistant.io

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.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

In components/esphome/light.py: - Add ADDR_COLOR_TEMP_KELVIN to list of parsed attributes - Import mired to kelvin conversion util code - If ATTR_COLOR_TEMP_KELVIN is set in service call, calculate target mired value in floating point. Fall back to ATTR_COLOR_TEMP if _KELVIN not available - Add {min,max}_color_temp_kelvin properties, computed from floating point mired,

home-assistant · 2023-04-14T14:44:29Z

Hi @danielkent-net

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!

home-assistant · 2023-04-14T14:44:34Z

Hey there @OttoWinter, @jesserockz, mind taking a look at this pull request as it has been labeled with an integration (esphome) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of esphome can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign esphome Removes the current integration label and assignees on the pull request, add the integration domain after the command.

danielkent-net · 2023-04-14T15:07:32Z

Based on the current changes, I can verify that floating point mireds are now being sent to ESPHome light devices when changing them via the color temperature slider in the Web UI:

However, I am still seeing some issues, which is why I am keeping this PR as a draft for now:

The Kelvin value shown in the Web UI usually changes slightly between when it's first clicked and after it's sent to the ESPHome device.
The color temperature still cannot be set to the absolute upper bound using the slider; in my current test setup with a configured 3000K to 6000K range, I can only set the maximum to 5999K. It's better than it was before, but still annoying.

danielkent-net · 2023-04-14T16:51:28Z

As a related sidenote, the documentation for the Home Assistant light entity seems a bit outdated; it still mentions color_temp and kelvin properties in the service call, while in the code these properties are marked as deprecated and to favor the color_temp_kelvin property instead. I can open a new issue on the home-assistant.io repo for that one.

danielkent-net · 2023-04-18T17:33:33Z

Finally figured out the remaining issue; I needed to override the color_temp_kelvin property, and the parent function looks for a property that doesn't exist, so it used the built-in mired to Kelvin conversion that loses precision. With the latest change, this precision is maintained all the way to the UI.

There is one remaining issue, but it's out of scope of this PR: the function color_temperature_mired_to_kelvin in util/color.py:619 uses a floor function. This causes any values with fractional mired values to be off by one if the fractional part is greater than 0.5. I think it would be more appropriate to round, but changing that might break other integrations.

bdraco · 2023-05-05T21:59:24Z

There is one remaining issue, but it's out of scope of this PR: the function color_temperature_mired_to_kelvin in util/color.py:619 uses a floor function. This causes any values with fractional mired values to be off by one if the fractional part is greater than 0.5. I think it would be more appropriate to round, but changing that might break other integrations.

You could open another PR for this

bdraco · 2023-05-05T22:03:08Z

Testing this

before

bdraco · 2023-05-05T22:03:43Z

After

Check for color temp support before setting Kelvin color attributes Co-authored-by: J. Nick Koston <nick@koston.org>

danielkent-net · 2023-06-22T03:24:40Z

I noticed the main implementation of mired_to_kelvin didn't have a zero division check so I didn't add one to _mired_to_kelvin() initially. Should we choose a sane default instead? The default range in HA light entities is 2000-6500K, so we could do something like:

return round(1000000/mired_temperature) if mired_temperature > 0 else 6500

bdraco · 2023-06-22T06:49:55Z

I noticed the main implementation of mired_to_kelvin didn't have a zero division check so I didn't add one to _mired_to_kelvin() initially. Should we choose a sane default instead? The default range in HA light entities is 2000-6500K, so we could do something like:

return round(1000000/mired_temperature) if mired_temperature > 0 else 6500

Since we don't know if it's the minimum or maximum value its effectively GIGO at this point and it's probably best to return the original value (0) so we don't have to figure out later where the data was mutated on us if there is a bug.

bdraco · 2023-06-22T09:09:27Z

#95029 will add some basic tests for this platform

Won't be able to test today as I won't get home till tomorrow now as my original flight canceled and I have a few stops now

…zero

homeassistant/components/esphome/light.py

bdraco · 2023-06-22T15:04:57Z

Managed to pickup a different flight so might be able to test later but at worst I'll be able to test tomorrow. Still plenty of time to get this merged before beta on Wednesday

bdraco · 2023-06-22T19:29:12Z

One of the new tests will likely need to be adjusted for the behavior change

sorry about the conflicts

bdraco · 2023-06-22T19:37:08Z

One of the new tests will likely need to be adjusted for the behavior change

sorry about the conflicts

danielkent-net · 2023-06-22T20:23:35Z

Unfortunately it looks like the functionality that automatically derives the min/max mireds from Kelvin around components/light/__init__.py:928 still uses the floor function, and that's well out of the esphome light scope.

The only way I can think to get around this is to override the capability_attributes function, first calling the superclass function to get the base dictionary, then if Kelvin properties exist repopulate the ATTR_{MIN,MAX}_MIREDS properties with our "correct" implementation. Something like:

   @property
    @esphome_state_property
    def capability_attributes(self) -> dict[str, Any]:
        """Return capability attributes.
           This function takes the base class attributes and corrects any
           incorrect calculation of the color temperature bounds in mireds."""
        data = super(LightEntity, this).capability_attributes()
        if ATTR_MIN_COLOR_TEMP_KELVIN in data:
            data[ATTR_MAX_MIREDS] = _kelvin_to_mireds(data[ATTR_MIN_COLOR_TEMP_KELVIN])
        if ATTR_MAX_COLOR_TEMP_KELVIN in data:
            data[ATTR_MIN_MIREDS] = _kelvin_to_mireds(data[ATTR_MAX_COLOR_TEMP_KELVIN])
        return data

IMO it's an inelegant solution, but until the base class implementation is fixed, we're going to still see off-by-one errors.

bdraco · 2023-06-22T20:49:39Z

Let's adjust the test here and get this merged (assuming testing is ok)

Than the light class can be adjusted and we can rework this again

bdraco · 2023-06-23T12:45:29Z

Tested, everything looks good.

bdraco · 2023-06-23T12:57:31Z

I just realized the test is wrong since esphome sends the full float and the test doesn't.

I'll fix the test and merge this

bdraco · 2023-06-23T12:58:21Z

Thanks @danielkent-net

home-assistant bot added bugfix cla-needed integration: esphome small-pr PRs with less than 30 lines. labels Apr 14, 2023

home-assistant bot assigned OttoWinter and jesserockz Apr 14, 2023

home-assistant bot added the Quality Scale: No score label Apr 14, 2023

home-assistant bot added cla-recheck cla-signed and removed cla-recheck cla-needed labels Apr 14, 2023

danielkent-net and others added 4 commits April 14, 2023 16:43

Merge branch 'dev' into esphome_color_temp_precision

89e0aaa

Merge branch 'dev' into esphome_color_temp_precision

3f2a252

Merge branch 'dev' into esphome_color_temp_precision

c333924

Define color_temp_kelvin for ESPHome lights

9da6b3f

danielkent-net marked this pull request as ready for review April 18, 2023 17:25

danielkent-net requested review from OttoWinter and jesserockz as code owners April 18, 2023 17:25

danielkent-net added 2 commits April 18, 2023 13:45

Merge branch 'dev' into esphome_color_temp_precision

cc51b3c

Merge branch 'dev' into esphome_color_temp_precision

5c7d331

Merge branch 'dev' into esphome_color_temp_precision

f761781

Update homeassistant/components/esphome/light.py

a3b34bf

Check for color temp support before setting Kelvin color attributes Co-authored-by: J. Nick Koston <nick@koston.org>

Merge branch 'dev' into esphome_color_temp_precision

66f91f6

danielkent-net and others added 2 commits June 22, 2023 08:57

Merge branch 'dev' into esphome_color_temp_precision

b602cdd

Add sanity check for _mired_to_kelvin for esphome to avoid divide by …

8fd1187

…zero

bdraco reviewed Jun 22, 2023

View reviewed changes

homeassistant/components/esphome/light.py Outdated Show resolved Hide resolved

bdraco approved these changes Jun 22, 2023

View reviewed changes

danielkent-net and others added 2 commits June 22, 2023 11:08

Merge branch 'dev' into esphome_color_temp_precision

21dfac5

Refactor _mired_to_kelvin to match HA convention

6aa6f7c

bdraco approved these changes Jun 22, 2023

View reviewed changes

Merge branch 'dev' into esphome_color_temp_precision

e614764

Merge branch 'dev' into esphome_color_temp_precision

862316e

Merge branch 'dev' into esphome_color_temp_precision

5e04795

fix tests

a943b1a

bdraco approved these changes Jun 23, 2023

View reviewed changes

Merge branch 'dev' into esphome_color_temp_precision

41f332c

bdraco added has-tests and removed Testing required labels Jun 23, 2023

bdraco merged commit 983ff10 into home-assistant:dev Jun 23, 2023

github-actions bot locked and limited conversation to collaborators Jun 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ESPHome color temperature precision for light entities #91424

Fix ESPHome color temperature precision for light entities #91424

danielkent-net commented Apr 14, 2023 •

edited by bdraco

Loading

home-assistant bot commented Apr 14, 2023

home-assistant bot commented Apr 14, 2023

danielkent-net commented Apr 14, 2023

danielkent-net commented Apr 14, 2023

danielkent-net commented Apr 18, 2023

bdraco commented May 5, 2023

bdraco commented May 5, 2023

bdraco commented May 5, 2023

danielkent-net commented Jun 22, 2023

bdraco commented Jun 22, 2023

bdraco commented Jun 22, 2023

bdraco commented Jun 22, 2023

bdraco commented Jun 22, 2023

bdraco commented Jun 22, 2023

danielkent-net commented Jun 22, 2023

bdraco commented Jun 22, 2023

bdraco commented Jun 23, 2023

bdraco commented Jun 23, 2023

bdraco commented Jun 23, 2023

Fix ESPHome color temperature precision for light entities #91424

Fix ESPHome color temperature precision for light entities #91424

Conversation

danielkent-net commented Apr 14, 2023 • edited by bdraco Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Apr 14, 2023

home-assistant bot commented Apr 14, 2023

danielkent-net commented Apr 14, 2023

danielkent-net commented Apr 14, 2023

danielkent-net commented Apr 18, 2023

bdraco commented May 5, 2023

bdraco commented May 5, 2023

bdraco commented May 5, 2023

danielkent-net commented Jun 22, 2023

bdraco commented Jun 22, 2023

bdraco commented Jun 22, 2023

bdraco commented Jun 22, 2023

bdraco commented Jun 22, 2023

bdraco commented Jun 22, 2023

danielkent-net commented Jun 22, 2023

bdraco commented Jun 22, 2023

bdraco commented Jun 23, 2023

bdraco commented Jun 23, 2023

bdraco commented Jun 23, 2023

danielkent-net commented Apr 14, 2023 •

edited by bdraco

Loading