Lights that support "color_temp" mode break "brightness" changes

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

73.51k stars 30.71k forks source link

Lights that support "color_temp" mode break "brightness" changes #110440

Open fredck opened 8 months ago

fredck commented 8 months ago

The problem

If a light support color_temp mode, it is not possible to set the brightness of the light only, without setting its color temperature first.

For such lights, if you use the UI to change it's brightness level, the brightness attribute in the light state will stay null. Calling the turn_on service passing just the brightness will have the same effect.

What version of Home Assistant Core has the issue?

core-2024.2.1

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Light

Link to integration documentation on our website

https://www.home-assistant.io/integrations/light/

Diagnostics information

No response

Example YAML snippet

# This happens with physical lights but it can be easily reproduced with a template:
light:
  - platform: template
    lights:
      test_light:
        friendly_name: "Test Light"
        turn_on:
        turn_off:
        set_level:
        set_rgb:
        set_temperature:

Anything in the logs that might be useful for us?

No response

Additional information

No response

home-assistant[bot] commented 8 months ago

Hey there @home-assistant/core, mind taking a look at this issue as it has been labeled with an integration (light) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `light` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign light` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

light documentation light source _{^{(message by IssueLinks)}}

fredck commented 8 months ago

Correction. This seem to happen when the light supports also some color feature (set_rgb in my example).

fredck commented 8 months ago

Other than templates, the very same issue happens with real world lights. I am able to confirm the issue with a RGB+CCT led strip controlled by https://www.zigbee2mqtt.io/devices/TS0505B_1.html#tuya-ts0505b_1

This happens when the controller is fresh-installed in HA. After that, just open the entity dialog and change its brightness level freely. You'll notice that, although the light reflect the change, the UI loses the brigtness information.

This is a screenshot of the dialog after changing the level:

Instead of "On" it should show "30%", which is the level I set it to.

And this is the resulting light state for the above:

min_color_temp_kelvin: 2000
max_color_temp_kelvin: 6535
min_mireds: 153
max_mireds: 500
effect_list:
  - blink
  - breathe
  - okay
  - channel_change
  - finish_effect
  - stop_effect
supported_color_modes:
  - color_temp
  - xy
effect: null
color_mode: unknown
brightness: null
color_temp_kelvin: null
color_temp: null
hs_color: null
rgb_color: null
xy_color: null
friendly_name: Kitchen Wall Shelf
supported_features: 44

fredck commented 8 months ago

For now I was able to figure out this much, when it comes to the template integration code:

The "light" platform, by default, sets the color_mode to ColorMode.UNKNOWN if the light supports more than one color mode:

https://github.com/home-assistant/core/blob/d004011d415707ce85cf657c024ff248d7bc2caf/homeassistant/components/template/light.py#L247-L248

Theoretically this is ok, but for some reason it doesn't set color_mode if just setting brightness in async_turn_on:

https://github.com/home-assistant/core/blob/d004011d415707ce85cf657c024ff248d7bc2caf/homeassistant/components/template/light.py#L442-L447

Finally, since color_mode stays set to ColorMode.UNKNOWN, the light entity decides to set the brightness attribute to None, even if it's being set, which causes the issue:

https://github.com/home-assistant/core/blob/d004011d415707ce85cf657c024ff248d7bc2caf/homeassistant/components/light/__init__.py#L1200-L1204

The above explains the problem within the Template integration but, as said previously, this also affects real lights. I believe a broader analysis must be made.

This is a serious issue since it is really breaking the use of some types of lights (basically, those that don't implement color_mode properly, light the Group integration).

fredck commented 8 months ago

The above explains the problem within the Template integration but, as said previously, this also affects real lights. I believe a broader analysis must be made.

Ok, I've figured that the MQTT integration doesn't set color_mode when setting just brightness, just like the Template integration:

https://github.com/home-assistant/core/blob/d004011d415707ce85cf657c024ff248d7bc2caf/homeassistant/components/mqtt/light/schema_json.py#L598-L609

So either both the Template and MQTT integrations have similar bugs or there is a bigger issue when it comes to color_modes.

emontnemery commented 8 months ago

@fredck we don't want to make guesses in the light entity about what state the light is in, because if we do that, the light's state can't reliably be restored in a scene.

I don't think the template example illustrates something that should be fixed, because you intentionally configure the template light to an ambiguous state.

Since the physical Tuya light has an adjustable color temperature, it's impossible to turn on the light without the light having a defined color temperature, I guess the problem is that we don't know the color temperature? I'd say this a problem either in zigbee2mqtt or in the Tuya light causing the state of the light to be partially unknown.

What is the actual behavior of the Tuya light, does it remember the last set color temperature and restore to that without reporting it, or does it revert to some default color temperature? I'd suggest to discuss on the zigbee2mqtt issue tracker if the behavior of the reported state of the light can be improved.

FWIW, I have a Tuya dimmer which in Home Assistant gets a a brightness of -1 or -2 when operated via push buttons which is very annoying.

fredck commented 8 months ago

@emontnemery, thanks for the comments.

I don't think the template example illustrates something that should be fixed, because you intentionally configure the template light to an ambiguous state.

That was intentional but not by case. It reflects the exact case I have with the Tuya (Miboxer) controllers since they are RGB+CCT, so they support color (xy) and temperature.

Since that's a template for a "possible" light, shouldn't it work? Shouldn't it be possible for the light state to have brightness set but color_temp, color_temp_kelvin and xy_color null?

Btw, as a result of the research I've made so far, I've opened #111327 as well, which may be one of the causes of this issue.

emontnemery commented 8 months ago

Since that's a template for a "possible" light, shouldn't it work? Shouldn't it be possible for the light state to have brightness set but color_temp, color_temp_kelvin and xy_color null?

No, because the state of the light is then ambiguous and saving its state in a scene and then restoring it will fail.

fredck commented 8 months ago

Since the physical Tuya light has an adjustable color temperature, it's impossible to turn on the light without the light having a defined color temperature, I guess the problem is that we don't know the color temperature? I'd say this a problem either in zigbee2mqtt or in the Tuya light causing the state of the light to be partially unknown.

What is the actual behavior of the Tuya light, does it remember the last set color temperature and restore to that without reporting it, or does it revert to some default color temperature? I'd suggest to discuss on the zigbee2mqtt issue tracker if the behavior of the reported state of the light can be improved.

I see that, for some of my lights, Z2M reports color_mode and color_temp and for those it all works right. For the problematic ones, Z2M doesn't report these properties.

My guess is that, for the problematic lights, I never set their temperature and therefore they go with the factory default and they don't see a reason to report the color information back to Z2M. If that's the case, I don't see a chance of Tuya changing this any time soon.

fredck commented 8 months ago

Since that's a template for a "possible" light, shouldn't it work? Shouldn't it be possible for the light state to have brightness set but color_temp, color_temp_kelvin and xy_color null?

No, because the state of the light is then ambiguous and saving its state in a scene and then restoring it will fail.

I mean, such state means "I don't care about the color, just set the brightness and let the rest be handled by the light". Of course, ideally, ha would know everything about that light but knowing that much makes anyway the lights usable. And that's a (unhandled) real world situation, in fact.

fredck commented 8 months ago

I have the impression that "brightness" and "color" should be separate features. Just like the "on/off" state.

For comparison, ha allows a light to be set to "on", even if no information about its color is available. But it doesn't allow it for "brightness", which is, as well, a property separate from color.

I know I'm talking about important definitions here that may be hard to change at this stage. But there is a chance that what I'm saying makes sense. And since this is indeed causing issues, it's worth talking, I suppose.

emontnemery commented 8 months ago

I mean, such state means "I don't care about the color, just set the brightness and let the rest be handled by the light". Of course, ideally, ha would know everything about that light but knowing that much makes anyway the lights usable. And that's a (unhandled) real world situation, in fact.

No, that's not the case. The state of the light is expected to correctly describe the light's state including brightness, color and possibly an active effect.

fredck commented 8 months ago

@emontnemery, thank you for the follow-up so far.

I understand that decisions were taken in the past and they have to be defended now. I understand that I’m not a core developer and from the start, I have a weaker (or non-existing) decision position. I ask you to just take this message in the “maybe there is something to improve here” way.

Properties of light

Among the physical properties of light, the following are the ones that home devices can control:

On/Off: technically not a physical property of light but it’s the property that says “it is or it is not”
Brightness: determines how much light is emitted. It can be quantified in terms of “luminance” or “illuminance”
Color: determines the frequency of the light wave (wavelength)

These properties can be/are controlled independently without interference with one another. There is a relationship between them in the physical perception of light (brightness can change the perception of color and vice versa) but devices don’t control this perception directly, keeping these properties statically separate.

Effects

Effects are features available in devices. They use the above light properties in a programmed sequential way. They may have some level of control by inputting brightness and color information since their programs control all three properties of light independently.

Color Modes

There are several methods to quantify color which are used in light devices:

RGB, HS, XY
White Level: controls the white “portion" of the color.
Color Temperature: manipulates color specifically targeting the spectrum to mimic the effect of changing the color temperature

A light device may allow for controlling color using more than one of the above methods, independently.

Home Assistant

The ability to control all the above features of a light is of course limited by the light itself (which is a combination of a light source and its controlling device). Home Assistant needs to have enough information about a light though to be able to represent the light in the UI, with compatible features enabled, and to properly control such features. Programmatically, this information is provided by a Light Entity and the data representing this information comes in the form of the “entity state”.

The problem we’re trying to solve is related to constraints and intentions connected to the data defined in light entity states.

(Ok, that's the end of the lecture :D)

Proposal

State that lights have the following independent features:
- On/Off
- Brightness
- Color
- Effect
Although On/Off and Brightness have nothing to do with Color, keep using supported_color_modes to specify the features of the light:
- ColorMode.ONOFF: is optional to be included. It is implicitly supported by all lights, for obvious reasons.
- ColorMode.BRIGHTNESS: indicates that the light supports brightness. It is optional and implicitly supported by lights that support any of the other color modes.
- Other color modes currently available: included if the light supports color. It can support more than one mode (e.g., xy and temp)
Use color_mode to identify what HA exactly knows about the state of the light:
- None: the light is off. Brightness, color and effect information is None
- ColorMode.ONOFF: the only information available is that the light is “on”
- ColorMode.BRIGHTNESS: the light is on and the only additional information available is brightness
- Other color modes: the light is on and there is color information available.
  - Use a priority order if more than one color information is available for a light that supports more than one of the (real) color modes
Effects are controlled by the effect property itself, in the way it is right now. They don’t change color_mode per se. Instead, the turn_on service has to explicitly send effect + brightness and/or color information if it wants the effect to be changed by those features. Then it’s those features that will define the proper value for color_mode, as described in the above point 3. For example, a turn_on with just the effect set, will end up having color_mode set to ColorMode.ONOFF.

The above will help avoid the guesswork that is done today, in a non-ambiguous way. There is no reason for a color_mode to be set to a mode that doesn’t have data in the entity state.

The above will also fix the “data loss” that happens today if a device or an integration doesn’t provide complete information about the state of the light. There is no reason for the brightness information to be lost just because the color information is not available.

Ofc, ideally integrations should provide the complete state of their lights. But since that's not the way the real world works, this proposal may help ha to be bulletproof agains incomplete or low quality implementations.

But more importantly, I hope it makes sense and raise the quality of ha as a whole.

Thoughts?

emontnemery commented 8 months ago

@fredck As already mentioned multiple times in this issues, requiring that a light's state is complete is a conscious choice, to make sure saving and then restoring the state of a light is predictable.

What you're suggesting is to allow a light which color can be controlled to only report it's brightness or only report if it's turned on or off. This reverts back to the situation before introducing color modes, where saving the state of a light in a scene had very unpredictable results when the scene is activated.

Again, is there a reason why your problem can't be solved in Zigbee2MQTT?

fredck commented 8 months ago

@emontnemery, I've taken into account your repeated mentions of scenes. My focus wasn't on that initially because, despite its desirability, the current implementation doesn't magically solves all problems. Devices in HA are, regrettably, prone to bugs and likely will remain so. Our best course of action is to refine their integrations while ensuring the core remains stable.

In my opinion, the correct approach to enforcing the requirements in the state information that you mentioned includes:

Clear documentation (warnings) regarding the requirements (and maybe their impact in scenes)
Logging warnings for an extended period (a few years), explicitly stating that devices failing to provide complete state information will cease to function past the deadline
Transitioning warnings into errors when the deadline arrives, setting such devices as unavailable

Ofc, we can also go easier and stay on warnings forever.

Meanwhile core code works in a predictable and stable way.

I still believe my proposal aligns with the above intent. Additionally, in my suggested implementation, HA itself would be the responsible to set color_mode based on available entity state data, not the integrations.

The discussion also needs to be initiated on Z2M and anywhere else where we see "bad state" behavior. However, this doesn't change the necessity for core improvements to reduce dependence on the quality of third-party code. Expecting flawless operation from the numerous coordinators, integrations, and hundreds of devices out there is overly optimistic (or too dictatorial?)

I think my previous comments may have lacked clarity, leading to misunderstandings. When I expressed a desire to "set the brightness without concern for colors and the rest," I was referring to the end-user perspective. While we do expect integrations to provide complete state information, the reality often falls short. Therefore, preserving brightness information - which aligns with user expectations - represents a meaningful achievement.

Aside from my attempt to propose enhancements for handling light state in core (aiming to making it more robust), I wish to point out a bug in the current implementation relevant to the original issue reported.

I use templates to mimic the behavior of lights with limited state reporting, creating two lights: one supporting both color and temperature, and the other only color:

light:
  - platform: template
    lights:
      # Color + Temperature
      test_light_color_temp:
        friendly_name: "Test Light Color Temp"
        turn_on:
        turn_off:
        set_level:
        set_rgb:
        set_temperature:

      # Color Only
      test_light_color_only:
        friendly_name: "Test Light Color Only"
        turn_on:
        turn_off:
        set_level:
        set_rgb:
        set_temperature:

Invoke the turn_on service for both lights, specifying only the brightness:

service: light.turn_on
target:
  entity_id: light.test_light_color_temp
data:
  brightness: 100

service: light.turn_on
target:
  entity_id: light.test_light_color_only
data:
  brightness: 100

Check the state attributes of the lights using the developer tools:

Color + Temperature light:

min_color_temp_kelvin: 2000
max_color_temp_kelvin: 6535
min_mireds: 153
max_mireds: 500

# This light supports more than one color mode
supported_color_modes:
  - color_temp
  - rgb

# As a result, color_mode is `unknown`
color_mode: unknown

# Brightness set by the user has been erroneously cleared by the core light component
brightness: null

# Since color/temp has never been set, there is no where to take this information from so the following are all null
color_temp_kelvin: null
color_temp: null
hs_color: null
rgb_color: null
xy_color: null

friendly_name: Test Light Color Temp
supported_features: 0

Color Only light:

# This light supports only one color mode
supported_color_modes:
  - rgb

# Color_mode is pointlessly set to rgb despite the lack of color information
color_mode: rgb

# Brightness information is retained though, as expected
brightness: 100

# Since color/temp has never been set, there is no where to take this information from so the following are all null
hs_color: null
rgb_color: null
xy_color: null

friendly_name: Test Light Color Only
supported_features: 0

An evident issue is that there is not reason for the brightness information to have been lost in the first case but not in the second one. Brightness has nothing to do with the fact that the color mode of the light is unknown, especially considering that both possible color modes support brightness. If nothing else will be taken in consideration from this issue, I hope that at least this bug can have some attention. WDYT?

Additionally, in the second case, what’s the point of having a color_mode value if we don’t have information about the color?

I’ve included already several proposals in this issue which I still believe would clear out and raise the quality of the current core implementation and more prominently influence the development of integrations and raise their quality. Parts of the proposal got scattered around different comments though. I could certainly work to summarize all in a single piece. If there would be any sign of interest on that, just let me know.

Thanks!

emontnemery commented 8 months ago

I'm sorry, but your understanding of the issue is not correct.

You keep using template light in optimistic mode as an example of how things work. However, a template light in optimistic mode is not a good proxy of an MQTT light controlled by zigbee2mqtt. Optimistic mode in Home Assistant lingo means we assume an entity's state is the same as the user last set it to.

The problem you see with a template light in optimistic mode which supports more than one color mode, is that the color mode is initiated to UNKNOWN here: https://github.com/home-assistant/core/blob/5527abad30eb67395af23c09bec16d2e9028cf35/homeassistant/components/template/light.py#L248

In optimistic mode, the template light will parrot back whatever it was last set to. Until a color or a color temperature has been set, the color mode will be stuck at UNKNOWN.

An MQTT light controlled by zigbee2mqtt is not optimistic; it's state is actively controlled by zigbee2mqtt, and I believe zigbee2mqtt actively pushes the last known state when Home Assistant Core is (re)started.

There may be an actual problem, either in the Home Assistant mqtt integration or in zigbee2mqtt, but what you linked to here is irrelevant for zigbee2mqtt because zigbee2mqtt lights do not work in optimistic mode.

Let's circle back to your actual problem, which is that a certain Tuya light connected to zigbee2mqtt does not have the expected state.

To get anywhere with that, please share a diagnostic dump, when the light is in the unexpected state, i.e. you expect its brightness to show but it doesn't:

fredck commented 8 months ago

Ok, let's forget about the rest. I just wanted to clarify one thing.

Why do we have the following?

https://github.com/home-assistant/core/blob/d004011d415707ce85cf657c024ff248d7bc2caf/homeassistant/components/light/__init__.py#L1200-L1204

PS: I'll provide the diagnostics in another comment

fredck commented 8 months ago

Here it is the diagnostics file right after calling turn_on with brightness set to 100.

mqtt-2bbc91138b6da3baa7888ae8f73a19dd-Kitchen Wall Shelf-919597079447b1e17b915e090d039e58.json

emontnemery commented 8 months ago

About why we block the brightness when we don't know the light's color mode:

The idea is that we can't trust the light's properties when we the light doesn't know what mode it is in. This is mostly an issue for lights which support both color and color temperature though, before we introduced color modes, such lights often set both a color and a conflicting color_temperature and it was not clear which one was correct.

This may not be an issue with the brightness property though, and it could indeed make sense to not block the brightness if the light does support brightness but does not know which mode it is currently in.

About the Tuya light:

According to the zigbee2mqtt documentation, it's possible to read the color temperature and color from the light: https://www.zigbee2mqtt.io/devices/TS0505B_1.html

However, the state update from the light only includes the brightness, but not the color mode or the color temperature. I would expect zigbee2mqtt to ask the light for it's state if the state is not known. I think you should report this on the zigbee2mqtt bug tracker. I'm happy to join the discussion there if you link the issue here.

This is the last received state update:

                {
                  "payload": "{\"brightness\":100,\"color_power_on_behavior\":null,\"do_not_disturb\":null,\"linkquality\":18,\"state\":\"ON\"}",
                  "qos": 0,
                  "retain": 0,
                  "time": "2024-03-04T09:13:12.004624+00:00",
                  "topic": "zigbee2mqtt_ground_floor/Kitchen Wall Shelf"
                }

fredck commented 8 months ago

I think you should report this on the zigbee2mqtt bug tracker

I've opened https://github.com/Koenkk/zigbee2mqtt/issues/21672.

Just like here, I've started a broader issue discussion since I don't want to just "solve my problem" but hopefully drive the software to a better overall shape.

@emontnemery, please feel invited to chime in there with any further information or to correct me if I'm wrong ;)

Erbengi commented 7 months ago

This also breaks a lovelace light widget:

https://github.com/home-assistant/core/assets/31409742/9742987e-0912-4fba-95b5-d044a20ee791

ph30n1x commented 4 months ago

I think the root cause for a number of these light issues is the function filter_supported_color_modes. That specific function removes ColorMode.BRIGHTNESS from the list provided to it. The Tuya integration has been modified to use the function and by removing ColorMode.BRIGHTNESS it breaks the functionality for Tuya lights that have separate RGB and CCT LEDs. A couple of issues have been raised 115056 & 116326, and so far ignored. It appears a lot of changes are being made to integrations that are moved into Core without justification which break functionality for real world devices!

DozoG commented 3 months ago

I am sorry to ask, but is there something a simple mqtt user can do to get brightness to work?

In the logs I am getting:

2024-08-10 13:49:22.686 WARNING (MainThread) [homeassistant.components.light] light.office_light_my_mqtt_light (<class 'homeassistant.components.mqtt.light.schema_basic.MqttLight'>) set to unsupported color mode onoff, expected one of {<ColorMode.BRIGHTNESS: 'brightness'>}, this will stop working in Home Assistant Core 2025.3, please create a bug report at https://github.com/home-assistant/core/issues?q=is%3Aopen+is%3Aissue+label%3A%22integration%3A+mqtt%22

Its a light that I have auto-discovered through a /config topic (from an Arduino device) The Broker gets the command and the status for both the switch and the brightness correctly (MQTT explorer)

As far as I can tell, Homeassistant gets the mqttt messages correctly too.

I then check the entity in the Developer tools. I can sett the brightness from there. (the set state button) Then, as soon as touch the control on the dashboard, the brightness is set to null and the color_mode is set to onoff To be more specific: Touching the switch: I can toggle the state between on and off the color_mode attribute toggles between onoff and null the brightness sets to null Touching the slider: The color_mode remains in onoff and the brightness jumps to and remains null

mqtt_light

Do I need to add an atrribute to the light when I define my device in the Arduino code? Or are homeassistant developers still bug-fixing ?

Edit: Oh, sorry. In the picture that I uploaded, I set the color_mode to brightness by hand. It is onoff by default.

issue-triage-workflows[bot] commented 6 hours ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.