Introduce support for color temperature#25503
Conversation
probot-home-assistant
Bot
added
integration: flux_led
small-pr
|
Please fix the linter errors. |
amelchio
left a comment
amelchio
left a comment
There was a problem hiding this comment.
So I think it is awesome to get support for the second white channel (when available) but the PR seems quite rushed and honestly, I don't think it should have been merged in the current state. The PR is so determined to solve one use case involving Google Home that it forgets about all other situations.
I have added some comments but there are also a few things missing:
- The
color_tempproperty thatSUPPORT_COLOR_TEMPimplies is not implemented - Properties
min_miredsandmax_miredsare also not implemented. - There is an overlap with
SUPPORT_WHITE_VALUEbut it is not clear what it is. - I would expect the introduction of a call to
getRgbww()to report thew2brightness. - (Probably, a new
MODE_RGBWWmode is needed to make everything click.)
Sorry if I got some points wrong; I don't have these bulbs so I go only by the code.
|
|
||
| SUPPORT_FLUX_LED = (SUPPORT_BRIGHTNESS | SUPPORT_EFFECT | | ||
| SUPPORT_COLOR) | ||
| SUPPORT_COLOR | SUPPORT_COLOR_TEMP) |
There was a problem hiding this comment.
This is wrong as now we are advertising support for color temperature with RGB (not just RGBW).
| if color_temp is not None: | ||
| if brightness is None: | ||
| brightness = self.brightness | ||
| if color_temp == COLOR_TEMP_WARM_WHITE: |
There was a problem hiding this comment.
I don't like this magic value. Using the frontend slider will give erratic behavior when this value is hit.
If only two temperatures are available I think it is better to decide a cutoff (like 3500 kelvin) and make all temperatures above/below the same.
But I will be surprised if only two temperatures are available since mixing w and w2 should give the full range of temperatures and that seems like a nice bullet on a feature list.
There was a problem hiding this comment.
You cannot mix w and w2 see #23704:
1. In order to activate warm white state, all values *but* `w` should be `None`, while `w` can be between 0 and 255. If any other parameter is not `None`, the call will not result in *any action*
2. In order to activate cool white state, all values *but* `w2` should be `None`, while `w2` can be between 0 and 255. If any other parameter is not `None`, the call will not result in *any action*
There was a problem hiding this comment.
Okay, maybe there is some product differentiation or maybe you found a bug. There is so much different flux_led hardware, I still don't think this is true for it all.
For example, this commit suggests white mixing is possible.
| self._bulb.setRgbw(w2=brightness) | ||
| else: | ||
| self._bulb.setRgbw( | ||
| *color_util.color_temperature_to_rgb(color_temp)) |
There was a problem hiding this comment.
Don't do this conversion, it gives poor results and setting RGB is not what ATTR_COLOR_TEMP is for.
There was a problem hiding this comment.
Hmmm, what can be the alternative?
There was a problem hiding this comment.
Always set some white light even if it is the wrong temperature.
|
No worries, we can reopen the issue, and continue with improving this PR @amelchio |
|
My bad. I was doing a quick sweep through PRs prior to cutting the beta. |
|
It's not critical. It certainly doesn't break things, so it is fine to keep it in there. But we do need to add more changes |
|
Hmmm, as far as I can tell, this actually breaks the white support of Google Home for RGB and RGBW lights because it will now always use the second white LED which these products do not have. It also adds a color temperature slider that works poorly or not at all (again, AFAICT). Thinking about it more, I suggest reverting this while working on the amendments. |
This reverts commit e1d884a.
|
I submitted a revert, also reverting some other bad changes that were recently made to flux_led :-/. Let's do further discussions in a new PR or in #23704 which I have now reopened. |
* Revert Black * Revert "Introduce support for color temperature (#25503)" This reverts commit e1d884a. * Revert "Fix flux_led only-white controllers (#22210)" This reverts commit 4813818. * Revert "Fix MagicHome LEDs with flux_led component (#20733)" This reverts commit 1444a68. * Re-Black * Use mode detection for scanned bulbs
5 participants
Description:
flux_led component does not properly support all available modes that Flux Led Lights can operate in. In particular: warm-white mode (when controlled from a smart assistants Alexa/Google Home) and cool-white mode (cannot be set from Homeassistant at all).
Related issue (if applicable): fixes #23704
Checklist:
tox. Your PR cannot be merged unless tests passIf the code does not interact with devices: