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

Add Support for RGBW/RGBWW Devices in Light Entity Configuration #449

Merged
merged 10 commits into from
Jan 6, 2025

Conversation

geforcefan
Copy link

@geforcefan geforcefan commented Jan 3, 2025

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 via color_data and brightness) and white (managed solely via brightness).

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 to white and controls brightness directly, bypassing the need for minimum and maximum Kelvin values.

Notes

  • This implementation is a proposed solution. I am not deeply familiar with the Home Assistant codebase, so there might be alternative or more appropriate patterns to achieve this functionality. Nonetheless, this approach aligns with the device's behavior and fills the gap in supporting single white channel configurations.

Result

Screenshot

@geforcefan geforcefan marked this pull request as draft January 3, 2025 13:03
@geforcefan
Copy link
Author

geforcefan commented Jan 3, 2025

@xZetsubou,

In entity.py, there is a method called has_config, which is implemented as follows:

value = self._config.get(attr, "-1")
return value is not None and value != "-1"

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:

def has_config(self, attr) -> bool:
    """Return if a config parameter has a valid value."""
    value = self._config.get(attr, "-1")
    return value is not None and value is not False and value != "-1"

Does this have any impact or side effects, regressions, etc.?

@geforcefan geforcefan marked this pull request as ready for review January 3, 2025 16:56
@xZetsubou
Copy link
Owner

Thanks for the PR.

In entity.py, there is a method called has_config, which is implemented as follows:

The has_config method tells if the device config data contains this config and not tell you the value of that config so False actually is valid option since it has the config attr and has "False" value. if you want to check the value use "dict.get" method e.g. self._config.get(attr)


As for white mode we can implement this without adding an option in config flow.
in supported_color_modes property we add white check:

        if color_modes == {ColorMode.HS} and self.has_config(CONF_BRIGHTNESS):
            return {ColorMode.HS, ColorMode.WHITE}

in color_mode :

        if self.is_white_mode and not self.has_config(CONF_COLOR_TEMP):
            return ColorMode.WHITE

Then turn_on can be left as you implemented it to support white mode.

custom_components/localtuya/light.py Outdated Show resolved Hide resolved
custom_components/localtuya/manifest.json Outdated Show resolved Hide resolved
custom_components/localtuya/light.py Outdated Show resolved Hide resolved
custom_components/localtuya/const.py Outdated Show resolved Hide resolved
custom_components/localtuya/entity.py Outdated Show resolved Hide resolved
custom_components/localtuya/light.py Outdated Show resolved Hide resolved
custom_components/localtuya/translations/en.json Outdated Show resolved Hide resolved
custom_components/localtuya/translations/it.json Outdated Show resolved Hide resolved
custom_components/localtuya/translations/pl.json Outdated Show resolved Hide resolved
custom_components/localtuya/translations/pt-BR.json Outdated Show resolved Hide resolved
@geforcefan
Copy link
Author

@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
@geforcefan
Copy link
Author

@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.

Comment on lines 304 to 308
if (
self.has_config(CONF_COLOR)
and self.has_config(CONF_BRIGHTNESS)
and not self.has_config(CONF_COLOR_TEMP)
):
Copy link
Owner

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

Suggested change
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):

Copy link
Author

@geforcefan geforcefan Jan 4, 2025

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.

Copy link
Owner

@xZetsubou xZetsubou Jan 4, 2025

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

Copy link
Author

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.

Copy link
Owner

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.

@geforcefan
Copy link
Author

geforcefan commented Jan 5, 2025

@xZetsubou

Sorry to bother you, but just for clarification: As far as I understand, both the ColorMode.WHITE and ColorMode.COLOR_TEMP modes require the brightness configuration. Shouldn’t the check for self.has_config(CONF_BRIGHTNESS) be a prerequisite for determining either ColorMode.WHITE or ColorMode.COLOR_TEMP?

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

@xZetsubou
Copy link
Owner

both the ColorMode.WHITE and ColorMode.COLOR_TEMP modes require the brightness configuration

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.

@geforcefan
Copy link
Author

@xZetsubou Got it, gonna reverting that brightness part

@geforcefan
Copy link
Author

geforcefan commented Jan 6, 2025

@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 states[self._config.get(CONF_BRIGHTNESS)] = brightness is used in multiple places. Does calling this line break the code or execution when CONF_BRIGHTNESS is not set, or is the state just ignored in that case?

@xZetsubou
Copy link
Owner

states[self._config.get(CONF_BRIGHTNESS)] = brightness is used in multiple places

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

{"protocol":5,"t":1736168457,"data":{"dps":{"null":"white","23":986}}}

light entity might need some refactoring in the future, right?

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

@xZetsubou xZetsubou merged commit 3e5e940 into xZetsubou:master Jan 6, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants