Skip to content

Address inconsistent behavior on flux_led component #14713

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.

Sign up for GitHub

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

Merged
amelchio merged 7 commits into from
Jun 26, 2018
Merged

Address inconsistent behavior on flux_led component #14713

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
28221ae
Address inconsistent behavior between different controllers.
oblogic7 May 31, 2018
5877753
Add white mode for Flux LED
oblogic7 Jun 1, 2018
4cd29fb
Merge remote-tracking branch 'upstream/dev' into flux_bug_fix_2.1
oblogic7 Jun 18, 2018
0ca1c40
Call _bulb.turnOn() after bulb properties have been set to prevent im…
oblogic7 Jun 19, 2018
0cedda0
Only use existing brightness if rgb is None to prevent unexpected rec…
oblogic7 Jun 19, 2018
2cd2265
Remove blank line
oblogic7 Jun 20, 2018
d3800ee
Undo change so current brightness is used in all cases.
oblogic7 Jun 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 28 additions & 8 deletions homeassistant/components/light/flux_led.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
MODE_RGB = 'rgb'
MODE_RGBW = 'rgbw'

# This mode enables white value to be controlled by brightness.
# RGB value is ignored when this mode is specified.
MODE_WHITE = 'w'

# List of supported effects which aren't already declared in LIGHT
EFFECT_RED_FADE = 'red_fade'
EFFECT_GREEN_FADE = 'green_fade'
Expand Down Expand Up @@ -84,7 +88,7 @@
DEVICE_SCHEMA = vol.Schema({
vol.Optional(CONF_NAME): cv.string,
vol.Optional(ATTR_MODE, default=MODE_RGBW):
vol.All(cv.string, vol.In([MODE_RGBW, MODE_RGB])),
vol.All(cv.string, vol.In([MODE_RGBW, MODE_RGB, MODE_WHITE])),
vol.Optional(CONF_PROTOCOL):
vol.All(cv.string, vol.In(['ledenet'])),
})
Expand Down Expand Up @@ -181,6 +185,9 @@ def is_on(self):
@property
def brightness(self):
"""Return the brightness of this light between 0..255."""
if self._mode == MODE_WHITE:
return self.white_value

return self._bulb.brightness

@property
Expand All @@ -191,9 +198,12 @@ def hs_color(self):
@property
def supported_features(self):
"""Flag supported features."""
if self._mode is MODE_RGBW:
if self._mode == MODE_RGBW:
return SUPPORT_FLUX_LED | SUPPORT_WHITE_VALUE

if self._mode == MODE_WHITE:
return SUPPORT_BRIGHTNESS

return SUPPORT_FLUX_LED

@property
Expand All @@ -208,9 +218,6 @@ def effect_list(self):

def turn_on(self, **kwargs):
"""Turn the specified or all lights on."""
if not self.is_on:
self._bulb.turnOn()

hs_color = kwargs.get(ATTR_HS_COLOR)

if hs_color:
Expand Down Expand Up @@ -247,10 +254,23 @@ def turn_on(self, **kwargs):
if rgb is None:
rgb = self._bulb.getRgb()

self._bulb.setRgb(*tuple(rgb), brightness=brightness)
if white is None and self._mode == MODE_RGBW:
white = self.white_value

if white is not None:
self._bulb.setWarmWhite255(white)
# handle W only mode (use brightness instead of white value)
if self._mode == MODE_WHITE:
self._bulb.setRgbw(0, 0, 0, w=brightness)

# handle RGBW mode
elif self._mode == MODE_RGBW:
self._bulb.setRgbw(*tuple(rgb), w=white, brightness=brightness)

# handle RGB mode
else:
self._bulb.setRgb(*tuple(rgb), brightness=brightness)
Copy link
Copy Markdown
Contributor

@pezinek pezinek Jun 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you confirm that calling turn_on(rgb=[5,0,0], brighness=None) will really set the RGB color to [5,0,0] and not to [255,0,0] or anything else deformed by the previous brightness settings you re-use on line 254 ?

Copy link
Copy Markdown
Contributor Author

@oblogic7 oblogic7 Jun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally had a chance to test this and I found some inconsistent behavior. However, it appears that it is not with the handling of the values by the flux component, but some issue with the way the values are passed into turn_on.

I used the following payload to test:

{
  "entity_id": "light.test_led_1",
  "rgb_color": [0,5,0]
}

rgb_color is being converted to hs_color before being passed into the turn_on method. The flux component then uses color_util.color_hs_to_RGB to convert into a tuple of rgb values. This conversion results in 0,255,0 for the payload above instead of the expected 0,5,0. This is the source of the inconsistent behavior that I'm experiencing.

The flux library will only recalculate rgb values if a numeric value is passed for brightness. If None is passed, then it does not recalculate. I am making a change so that existing brightness value is only passed if rgb is None. This should prevent unexpected recalculation of the brightness when brightness is not included on the payload.

I'm not familiar enough with HA internals to know where to fix the hs_color bug mentioned above.

Copy link
Copy Markdown
Contributor

@pezinek pezinek Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oblogic7: Thanks for investigating furhter !
It seems that this breakage that changes rgb_color [0,5,0] to [0,255,0] has been introduced in PR #11288.
@armills: any clues/suggestions how to fix that ?

Copy link
Copy Markdown
Contributor Author

@oblogic7 oblogic7 Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After digging through the discussion on #11288, I see that @amelchio calls out this bug specifically.

https://github.com/home-assistant/home-assistant/pull/11288/files/1ee6a9f786c0508a7079aa2eaf282e03b2b83c9d#r175284144

I'm still checking out the changes, but I don't think the fix will be implemented on components unless there have been updates since that PR was merged. Maybe a new method has been added that I haven't found yet? If not, it looks to me like it will require more changes to the conversion methods.

Copy link
Copy Markdown
Contributor

@amelchio amelchio Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Popping in because I was tagged. I did not read all of these comments and I don't have flux_led hardware. Still, if you have specific questions about how things should work, I might be able to help.

Copy link
Copy Markdown
Contributor Author

@oblogic7 oblogic7 Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amelchio Are you aware of a fix or work around for the issue that you point out here: https://github.com/home-assistant/home-assistant/pull/11288/files/1ee6a9f786c0508a7079aa2eaf282e03b2b83c9d#r175284144

If not, do you know if it will be a component level change or if it will be more involved?

Copy link
Copy Markdown
Contributor

@amelchio amelchio Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After #11288, the intention is that rgb_color no longer changes the brightness. So turn_on(rgb=[5,0,0], brighness=None) and turn_on(rgb=[255,0,0], brighness=None) should act in the same way: set the light to red with the current brightness level.

This must be implemented in each light platform, like flux_led.py in this case, and

       if brightness is None:
            brightness = self.brightness

seems like a reasonable way to achive that.

Copy link
Copy Markdown
Contributor Author

@oblogic7 oblogic7 Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. That is how it was implemented. I will roll back the last change so that the current brightness is used. Thanks!


if not self.is_on:
self._bulb.turnOn()
Copy link
Copy Markdown
Contributor

@amelchio amelchio Jun 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that self._bulb.getRgb() works even when the bulb is not turned on?

Copy link
Copy Markdown
Contributor Author

@oblogic7 oblogic7 Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does.


def turn_off(self, **kwargs):
"""Turn the specified or all lights off."""
Expand Down