Skip to content

Send XY color for non-osram hue bulbs#13665

Merged
emlove merged 3 commits intodevfrom
hue-color-fixes
Apr 6, 2018
Merged

Send XY color for non-osram hue bulbs#13665
emlove merged 3 commits intodevfrom
hue-color-fixes

Conversation

@emlove
Copy link
Copy Markdown
Contributor

@emlove emlove commented Apr 3, 2018

Description:

It seems that Philips bulbs don't display the correct colors when given a hue/sat command. This PR restores the previous behavior of sending an XY color command for non-osram bulbs.

Original behavior for reference:

https://github.com/home-assistant/home-assistant/pull/11288/files#diff-09fd3b3fbdd20cedcad9fcf474b4aeb7L275

Related issue (if applicable): fixes #13632 fixes #13614

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

@emlove emlove changed the title Send XY color for Philips hue bulbs Send XY color for non-osram hue bulbs Apr 3, 2018
command['sat'] = int(kwargs[ATTR_HS_COLOR][1] / 100 * 255)
if self.is_osram:
command['hue'] = int(kwargs[ATTR_HS_COLOR][0] / 360 * 65535)
command['sat'] = int(kwargs[ATTR_HS_COLOR][1] / 100 * 254)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

* 255?

command['hue'] = int(kwargs[ATTR_HS_COLOR][0] / 360 * 65535)
command['sat'] = int(kwargs[ATTR_HS_COLOR][1] / 100 * 254)
else:
# Philips bulbs seem don't seem to respond correctly to hue/sat
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typo, seem

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ Thanks for the catches!

@amelchio
Copy link
Copy Markdown
Contributor

amelchio commented Apr 5, 2018

It seems unfair to label Philips as being incorrect.

I think it is more a matter of the Home Assistant Hue platform historically treating RGB (and by extension HS) in an unusual way by not using the bulb gamut. We are currently treating HS in the normal way and this PR will then go back to the unusual way.

However, I am not against this change as it maintains compatibility with previous versions of Home Assistant.

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented Apr 5, 2018

That's totally fair. My main intent is to ensure there's a good explanation in the comment to save it from being removed in a refactor.

What if the comment read like this:

The Hue/Sat API doesn't address the full color space supported by Philips bulbs, so we're first converting to XY.

@amelchio
Copy link
Copy Markdown
Contributor

amelchio commented Apr 5, 2018

Hue/sat does support all possible bulb colors but they are shifted from xy and we don't know by how much because it differes for each light model. Maybe a reasonable comment would be:

We use our own conversion function for consistent xy->HS->xy translations.

However, I checked our xy conversion functions against Hue docs and I found a difference:

diff --git a/homeassistant/util/color.py b/homeassistant/util/color.py
--- a/homeassistant/util/color.py
+++ b/homeassistant/util/color.py
@@ -203,7 +203,7 @@ def color_RGB_to_xy_brightness(

     # Wide RGB D65 conversion formula
     X = R * 0.664511 + G * 0.154324 + B * 0.162028
-    Y = R * 0.313881 + G * 0.668433 + B * 0.047685
+    Y = R * 0.283881 + G * 0.668433 + B * 0.047685
     Z = R * 0.000088 + G * 0.072310 + B * 0.986039

     # Convert XYZ to xy
>>> color_xy_brightness_to_RGB(*color_RGB_to_xy_brightness(250, 250, 250))
(248, 255, 248)
>>> color_RGB_to_xy_brightness(*color_xy_brightness_to_RGB(0.5, 0.5, 128))
(0.486, 0.481, 128)

I think we should apply that diff to ensure that xy -> HS -> xy (as we end up with after this PR) actually does pass the original xy value to the bulb. That should then fix #13686 as well.

@emlove emlove merged commit 1a9727c into dev Apr 6, 2018
@emlove emlove deleted the hue-color-fixes branch April 6, 2018 00:17
@NaffanDroo
Copy link
Copy Markdown

Is this staying in dev for the next release cycle or will there be another MR for a hotfix? Not saying it's urgent as I'm just sitting back on a 0.65 version - just curious how these things fit into the updated release cycle.

@omriasta
Copy link
Copy Markdown
Contributor

omriasta commented Apr 7, 2018

Updated to the beta which should include this but the color in the UI is now worse. Changing the colors on the picker now change the bulb colors but they don't change the icon in the UI. The color is now "stuck" on the green color even when the bulbs are changing.

@amelchio
Copy link
Copy Markdown
Contributor

amelchio commented Apr 7, 2018

@NaffanDroo Another hotfix is unlikely. The idea of the updated release cycle is that people like you try out the beta (it's out now) and report issues so the .0 release will have less bugs.

@omriasta you should report new bugs in new issues, though that one sounds like #13589

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Apr 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed integration: hue small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hue RGB colors are broken in 0.66 Issues with Tradfri paired with Hue hub since on 0.66.0

6 participants