Support xy_color for all light platforms by amelchio · Pull Request #6727 · home-assistant/core

amelchio · 2017-03-20T21:48:18Z

Description:

I wanted to add xy_color support for LIFX, but moving the conversion to core seems like a preferable approach?

The xy_color support is needed for light profiles and the flux switch and it is also used in several online examples. Thus, it makes sense to have it available globally.

I have tried to update the hue.py file but I have no way of testing that it still works!

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): The documentation already gives the impression that xy_color is supported everywhere.

Checklist:

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully.

This moves the OSRAM conversion to core and makes it available for all lights that do not support xy_color.

mention-bot · 2017-03-20T21:48:19Z

@amelchio, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @michaelarnauts to be potential reviewers.

houndci-bot · 2017-03-20T21:48:25Z

homeassistant/components/light/hue.py

-        return SUPPORT_HUE.get(self.info.get('type'), SUPPORT_HUE_EXTENDED)
+        features = SUPPORT_HUE.get(self.info.get('type'), SUPPORT_HUE_EXTENDED)
+        if self.info.get('manufacturername') == "OSRAM":
+            return features & ~SUPPORT_XY_COLOR;


statement ends with a semicolon

houndci-bot · 2017-03-20T23:20:19Z

tests/testing_config/custom_components/light/test.py


 DEVICES = []

+class MockLightDevice(MockToggleDevice):


expected 2 blank lines, found 1

houndci-bot · 2017-03-20T23:20:19Z

tests/testing_config/custom_components/light/test.py

 Call init before using it in your tests to ensure clean test data.
 """
 from homeassistant.const import STATE_ON, STATE_OFF
+from homeassistant.components.light import ( SUPPORT_XY_COLOR )


whitespace after '('
whitespace before ')'

balloob

This will require a test to be added for the demo light.

balloob · 2017-03-21T03:54:19Z

homeassistant/components/light/__init__.py

+            # Convert xy_color if unsupported
+            if xy_color is not None:
+                support_xy = bool(light.supported_features & SUPPORT_XY_COLOR)
+                brightness = params.get(ATTR_BRIGHTNESS, light.brightness)


We should not use light.brightness as a fallback, it can be completely different and not give us the right results.

I actually thought this to be an improvement over the OSRAM case where current brightness was used always, not just as a fallback. Apart from lag due to polling, I do not understand how it could be wrong, can you elaborate?

Most importantly: what can I use for the current brightness when an xy_color is set without a brightness?

We should only convert it if an xy color and a brightness is given. Using the light one is pretty random. All of a sudden we're combining actual state with a state that we want to achieve.

Exactly! It's quite feasible to want to change the color of a group of lights but maintain their individual brightness. Philips Hue also lets us set just the XY color and it will maintain current brightness so I feel the emulated XY should support that as well. The flux switch has disable_brightness_adjust that uses this.

Granted, it may be wrong sometimes, but not doing anything without a provided brightness will be wrong all the time.

If we don't do anything, the user will know it did something wrong and fix it. If we set the wrong value, the user thinks we did something wrong.

I think that if the user wants to set an XY color and does not provide a brightness, it will still work for lights that support XY colors. It's just that we can't do a conversion and so can't automatically fix it, I think that is totally ok.

amelchio · 2017-03-21T06:11:26Z

I change the subject to [WIP] until I have the time to get a test case added.

The demo light does not SUPPORT_XY_COLOR, so remove the code that nevertheless did handle xy_color. There is already a test that expects RGB state after setting an XY color. This removal of code makes the demo light work more like a real light by making that RGB value come from the new conversion in turn_on rather than from the similar conversion in state_attributes.

amelchio · 2017-03-31T19:29:41Z

This is a bit weird, but the test already expected a non-XY supporting light to accept XY colors and then return RGB values.

So the new test is to remove that supporting code and observe that the RGB test still passes.

This way, the demo light works more like a real RGB-based light and with this PR it still accepts XY colors as input.

balloob · 2017-03-31T19:42:50Z

homeassistant/components/light/__init__.py

+                brightness = params.get(ATTR_BRIGHTNESS, light.brightness)
+
+                if not support_xy and brightness is not None:
+                    rgb = color_util.color_xy_brightness_to_RGB(


I think that it makes more sense to do this conversion outside of the for loop for lights ?

It does, if you really want to enforce a single user-provided brightness value. The current code uses a per-light brightness and thus cannot be moved out of the loop.

balloob · 2017-03-31T19:43:11Z

homeassistant/components/light/__init__.py

+                    rgb = color_util.color_xy_brightness_to_RGB(
+                        *xy_color,
+                        ibrightness=brightness)
+                    params[ATTR_RGB_COLOR] = rgb


We should only overwrite RGB when it has not already been set.

houndci-bot · 2017-04-06T21:25:17Z

tests/components/light/test_init.py


        self.assertEqual(
-            {light.ATTR_XY_COLOR: (.4, .6), light.ATTR_BRIGHTNESS: 100},
+            {light.ATTR_XY_COLOR: (.4, .6), light.ATTR_BRIGHTNESS: 100, light.ATTR_RGB_COLOR: (147, 181, 0)},


line too long (109 > 79 characters)

amelchio · 2017-04-06T21:46:31Z

I have changed this as requested though I think it is a shame that we do not get feature parity (such as the Flux disable_brightness_adjust support).

Just for my education, can you give an example where light.brightness will be wrong?

balloob · 2017-04-12T04:36:21Z

homeassistant/components/light/__init__.py

+        brightness = params.get(ATTR_BRIGHTNESS, None)
+
+        # Convert xy_color/brightness for lights that do not support xy
+        if xy_color is not None and brightness is not None:


We should only convert it if the light does not support XY but does support RGB.

I added commits so it now works like that. It is a bit ugly because we can have mixed lights.

balloob · 2017-04-12T04:39:43Z

Using light.brightness from the active state feels weird. With taking the brightness we will never be able to represent the color that was passed in.

If I call a service to turn two lights red, I think that it will feel weird if one light be pink and the other red. Although we try our best, it's not what the user expects. So by not working, the user will make sure it also passes the right RGB color into the service.

amelchio · 2017-04-14T09:28:35Z

Sorry, but I am not convinced.

When the user does not specify a brightness, keeping it at the current setting is exactly what he will expect. He is not going to file error reports, he will just think “So that took care of the color. Now, on to the brightness”.

Because color and brightness are separate things. One can see that they are separate because they are separate widgets everywhere you select a color, indeed also in the HA UI. Forcing the two together in the service call is what’s weird and it will make a number of useful operations impossible.

Finally, to keep shared examples working and to keep the documentation simple, I think it is important that HA – as much as possible – presents a unified interface for all platforms. If avoiding this alleged red/pink confusion is truly the most user friendly option surely we should be kind enough to also deny Hue owners the right to set an xy_color without brightness? Imagine doing such a service call on a light group that has both a Hue and a LIFX bulb ... only one of them will react.

(Note, though I disagree, I did update the code as you requested)

amelchio · 2017-04-14T09:28:58Z

On a more technical note, I can only test with LIFX and I am actually not sure that using light.brightness will currently work for all platforms. I see that as an implementation detail that can be fixed, not something that should dictate whether the feature is desirable or not.

balloob · 2017-04-17T00:22:57Z

We can still allow users to set a XY color on Hue because in that case Hue will take the responsibility of doing the right thing. Maybe we should invite a tie breaker to this discussion, anyone else who wants to join in, @armills maybe?

balloob

The current change set is ok and can be merged 🐬 I'll wait with merging until we decide if we want to use light brightness.

amelchio · 2017-04-17T04:16:46Z

I guess that sums up my confusion: why is the red/pink thing "weird" if HA does it, but "the right thing" when Hue does it.

(with red/pink, I assume we are talking about a dim/bright version of the same color, as described by the xy_color coordinate)

Good idea with a third opinion and I am relieved that the patch set is okay now, thanks 👍

emlove · 2017-04-17T12:52:42Z

OK, here are my thoughts: First of all, I'm OK using the current brightness state if necessary. I think the red/pink issue is a bit of a confusion. If we think in terms of HSV colorspace, red vs pink would be high saturation vs low saturation. We're talking about changing the value (brightness) here, so it would just be bright red vs dim red. Especially in the context of light bulbs the perception is that these are the same color.

I don't think that we should be doing this conversion at the component level, though. Like @balloob said, we really want this to pass through to platform level to reduce core complexity. Plus, after this change lights that don't flag SUPPORT_XY_COLOR will start responding to xy requests, which I don't think is a good idea. Also consider that if we pass this down to the LIFX platform, we could instead map xy to the LIFX hue/saturation, and then just not send brightness. LIFX can retain it's brightness internally, so we're not calculating a command based on the hass stored state.

amelchio · 2017-04-17T13:21:29Z

Thanks for your input @armills. Even though I worked disproportionately hard to get this small change in shape, I am happy to toss it and move the functionality down to LIFX. Especially if it means that I get to do things the way I want to :-)

I thought it would be nice to get xy_color support for all platforms, in particular because other features depend on it. But it is entirely likely that I was naive. So @balloob, just tell me if I should change course.

(Unfortunately, the low-level LIFX LAN protocol does require brightness when setting a color. The cloud API that you link to is probably caching state the same way that I propose to do it. Also, our xy->hsv converter util currently goes xy->rgb->hsv so there is no easy way to avoid the rgb step)

emlove · 2017-04-17T13:37:17Z

Yeah, I've done my fair share of throwing out work myself. :) In the end we want to make sure we're doing what's best long-term.

It's unfortunate to about the LAN API issue. I do think it's the best to allow each platform to handle it on its own, since each platform API could have similar features that could be taken advantage of. Just my opinion though.

It might be worth considering a conversion of color_xy_brightness_to_hsv to color_xy_to_hs. The current code should basically always give back the same value as the input brightness, but is first being converted to RGB. IMO it would be better to have the function more specifically map xy to hs, since it's easy to pass through the value if you want it, but in the context of IoT, it's common to only be interested in the chromacity. Again, depends on @balloob's opinion. This is currently the only usage, and it's possible this could take advantage of not defining brightness there, since it's just passing through the old state.

amelchio · 2017-04-17T16:29:25Z

I agree, let's close this and I will implement it just for LIFX.

balloob · 2017-04-20T05:08:30Z

Alright, thanks for staying positive while we figure out the right thing 👍

balloob · 2017-04-20T05:09:34Z

(and just so you know, for the LIFX integration you're in charge 👍 )

Support xy_color for all light platforms

604793c

This moves the OSRAM conversion to core and makes it available for all lights that do not support xy_color.

homeassistant added cla-signed integration: light platform: light.hue labels Mar 20, 2017

balloobbot added platform and removed cla-signed integration: light platform: light.hue labels Mar 20, 2017

houndci-bot reviewed Mar 20, 2017

View reviewed changes

amelchio added 2 commits March 21, 2017 00:13

Fix lint

1665128

Adapt test

55791a1

homeassistant added the cla-signed label Mar 20, 2017

houndci-bot reviewed Mar 20, 2017

View reviewed changes

Fix more lint

075571e

amelchio changed the title ~~[WIP] Support xy_color for all light platforms~~ Support xy_color for all light platforms Mar 20, 2017

balloob requested changes Mar 21, 2017

View reviewed changes

amelchio changed the title ~~Support xy_color for all light platforms~~ [WIP] Support xy_color for all light platforms Mar 21, 2017

homeassistant added the cla-signed label Mar 31, 2017

amelchio changed the title ~~[WIP] Support xy_color for all light platforms~~ Support xy_color for all light platforms Mar 31, 2017

balloob reviewed Mar 31, 2017

View reviewed changes

Do not overwrite user-provided rgb_color

395225e

homeassistant added the cla-signed label Mar 31, 2017

amelchio added 2 commits April 6, 2017 22:32

Only convert xy->rgb when a new brightness is supplied

e36fd63

Add xy->rgb converted values to test

c723e09

houndci-bot reviewed Apr 6, 2017

View reviewed changes

Wrap long line

373f950

balloob requested changes Apr 12, 2017

View reviewed changes

amelchio added 2 commits April 12, 2017 21:45

Only provide xy->rgb converted value to relevant lights

5e5c667

Revert "Add xy->rgb converted values to test"

5c52ee9

balloob approved these changes Apr 17, 2017

View reviewed changes

balloob closed this Apr 20, 2017

home-assistant locked and limited conversation to collaborators Aug 12, 2017

Uh oh!

Conversation

amelchio commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

mention-bot commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amelchio commented Mar 21, 2017

Uh oh!

amelchio commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amelchio commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Apr 12, 2017

Uh oh!

amelchio commented Apr 14, 2017

Uh oh!

amelchio commented Apr 14, 2017

Uh oh!

balloob commented Apr 17, 2017

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

amelchio commented Apr 17, 2017

Uh oh!

emlove commented Apr 17, 2017

Uh oh!

amelchio commented Apr 17, 2017

Uh oh!

emlove commented Apr 17, 2017

Uh oh!

amelchio commented Apr 17, 2017

Uh oh!

balloob commented Apr 20, 2017

Uh oh!

balloob commented Apr 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

amelchio commented Mar 20, 2017 •

edited

Loading