-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add Support for RGBW/RGBWW Devices in Light Entity Configuration #449
Conversation
In entity.py, there is a method called has_config, which is implemented as follows:
This implementation always evaluates to True for BooleanSelector options, because False is not None. As a result, options like CONF_MUSIC_MODE are always considered True, even if they are explicitly set to False. While this may not directly affect lights, it is still incorrect behavior. In my current white light fix, I need to check whether CONF_COLOR_HAS_SINGLE_WHITE_CHANNEL is set, but has_config always returns True, which is fundamentally incorrect in this context. Should I modify the logic of has_config to also account for False values? Or should I simply use self._config.get(attr) directly? The latter might work, but it feels a bit inconsistent and inelegant to use multiple methods for checking configuration. Here is a proposed fix for the has_config method:
Does this have any impact or side effects, regressions, etc.? |
Thanks for the PR.
The As for white mode we can implement this without adding an option in config flow. if color_modes == {ColorMode.HS} and self.has_config(CONF_BRIGHTNESS):
return {ColorMode.HS, ColorMode.WHITE} in if self.is_white_mode and not self.has_config(CONF_COLOR_TEMP):
return ColorMode.WHITE Then |
@xZetsubou Thanks for your review! My initial idea was also to check for HS mode and brightness configuration, but I wasn’t sure if it would cause any regressions or break the behavior of other devices. I’m glad to see that it seems not to have any such issues, so I’ll adjust my PR to align with your suggestion. Avoiding the addition of another option is indeed a better approach. Also, thank you for clarifying how has_config works and its intended purpose! |
…ving single channel with existing logic and aligning with current architecture
…ving single channel with existing logic and aligning with current architecture
@xZetsubou The implementation is now limited to a single file with only a few changes. If you don’t mind, you can review it again. |
custom_components/localtuya/light.py
Outdated
if ( | ||
self.has_config(CONF_COLOR) | ||
and self.has_config(CONF_BRIGHTNESS) | ||
and not self.has_config(CONF_COLOR_TEMP) | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For cleaning up and not duplicates the check conditions above this line.
At this point if color_modes
doesn't have color temp but has HS
then we can check color_modes instead
if ( | |
self.has_config(CONF_COLOR) | |
and self.has_config(CONF_BRIGHTNESS) | |
and not self.has_config(CONF_COLOR_TEMP) | |
): | |
if color_modes == {ColorMode.HS} and self.has_config(CONF_BRIGHTNESS): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I don’t want to block the changes you’ve suggested, I’ll proceed with them. However, I’d like to point out that the suggested change introduces a common regression-prone pattern. I’ve encountered numerous regressions caused by this approach during my career.
The issue arises when someone refactors the code and changes the order of the if clauses. Relying on variables set by earlier if statements is an anti-pattern. This is why I explicitly check again when determining the ColorMode.White mode. It ensures the logic is clear and explicit for future developers who may work on this code.
With your approach, I’d first need to analyze when color_modes is only ColorMode.HS. Explicit checks avoid this ambiguity.
Having worked in the software industry for 15 years, I can confidently say this kind of regression is common because of such patterns. Additionally, the lack of tests in this project further increases the risk. Just wanted to note this for consideration.
I prefer the current implementation because it is explicit and doesn’t rely on the execution order of the if statements above. The suggested change (if color_modes == {ColorMode.HS} and self.has_config(CONF_BRIGHTNESS):) introduces a dependency on how color_modes is populated, which can break if the logic changes or conditions are reordered.
By directly checking CONF_COLOR, CONF_BRIGHTNESS, and CONF_COLOR_TEMP, the intent is clear and avoids ambiguity, making the code easier to understand and less prone to regressions. This is especially important in projects without tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well both ways is fine to be honest as long it works, Yes the order does matter with suggested set check yet I think it's fairly clear when is color_modes contains "One ColorMode item" I have seen this commonly used in HA built-in integrations and was cleaner to me, I used break-line to indicate that has_config checks is done, however
There is another suggestion as well since white cannot be existed with "color_temp" then we could use elif
color_temp
Now the question is why do we ensure that "CONF_COLOR" exists in order to add white mode? shouldn't check color_temp and brightness is enough?
e.g. for the whole property note that I made a little change to the last color_modes checks
@property
def supported_color_modes(self) -> set[ColorMode] | set[str] | None:
"""Flag supported color modes."""
color_modes: set[ColorMode] = set()
if self.has_config(CONF_COLOR_TEMP):
color_modes.add(ColorMode.COLOR_TEMP)
elif self.has_config(CONF_BRIGHTNESS):
color_modes.add(ColorMode.WHITE)
if self.has_config(CONF_COLOR):
color_modes.add(ColorMode.HS)
if not color_modes:
return {ColorMode.ONOFF}
elif color_modes == {ColorMode.WHITE}:
return {ColorMode.BRIGHTNESS}
return color_modes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this looks cleaner to me. Let me test it with all of my devices.
By the way, unrelated: When reconfiguring an entity and deleting, for example, the color mode, has_config still returns True. I assume this is because the key still exists in the dictionary, correct? Is this intended? To me, it seems like a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this a UI bug I haven't give it deep investigation to be honest but user_input
doesn't update empty fields if the field are empty it takes the latest value.
Sorry to bother you, but just for clarification: As far as I understand, both the For example, something like this: @property
def supported_color_modes(self) -> set[ColorMode] | set[str] | None:
"""Flag supported color modes."""
color_modes: set[ColorMode] = set()
if self.has_config(CONF_BRIGHTNESS):
if self.has_config(CONF_COLOR_TEMP):
color_modes.add(ColorMode.COLOR_TEMP)
else:
color_modes.add(ColorMode.WHITE)
if self.has_config(CONF_COLOR):
color_modes.add(ColorMode.HS)
if color_modes == {ColorMode.WHITE}:
return {ColorMode.BRIGHTNESS}
if not color_modes:
return {ColorMode.ONOFF}
return color_modes |
Not necessary I know from the code it seems like that but the bulb can be used with temperature only. I assume there is lights out there with only warm/cool without brightness config. So the last commit broke broke that. |
@xZetsubou Got it, gonna reverting that brightness part |
@xZetsubou The changes are now reverted to align with your suggestion. This should not break anything. What you mentioned about brightness basically implies that this light entity might need some refactoring in the future, right? I noticed |
Yes and it shouldn't break command, The device ignores invalid DPS unless the is part of the DP value similar to RF/IR remote command. this how it would looks like send a temp value only. you can see that mode is null because it not configured but it still works and adjust the temperature
probably yes, since it works I haven't give it much time yet there is still this pending haven't merged yet add_ble_support |
Problem
Currently, the light entity configuration in Home Assistant supports RGB+CCT devices (of course also others), where color temperature is controlled via minimum and maximum Kelvin values. However, some devices, such as the LSC Smart Connect Mood Light, do not support color temperature. Instead, these devices operate with two distinct color modes:
color
(managed viacolor_data
andbrightness
) andwhite
(managed solely viabrightness
).The Home Assistant UI does not currently provide a white channel mode for devices that support both RGB and a single white channel. This limitation stems from the fact that the light entity is not implemented correctly to handle such devices, making it impossible to configure them properly.
Solution
This pull request introduces an optional checkbox in the configuration flow, allowing users to specify if their device has a single white channel. When this option is selected, the light entity sets the
color_mode
towhite
and controls brightness directly, bypassing the need for minimum and maximum Kelvin values.Notes
Result