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.

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

Complying to #65 - reading parameters of different bulbs types #67

Merged
merged 8 commits into from
Oct 19, 2017
Merged

Complying to #65 - reading parameters of different bulbs types #67

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
db5065c
Test data for different lights added
matemaciek Oct 14, 2017
ab535cf
Test data for different lights added
matemaciek Oct 14, 2017
2a3ee98
Calculating missing hex values
matemaciek Oct 14, 2017
28f990f
Lets comply to flake
matemaciek Oct 14, 2017
2ffa1f2
Add _raw properties
matemaciek Oct 15, 2017
df25c06
Let's flake again
matemaciek Oct 15, 2017
902e0dd
LightControl can_set_* properties
matemaciek Oct 15, 2017
483b54c
Rename properties according to review
matemaciek Oct 16, 2017
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
33 changes: 33 additions & 0 deletions pytradfri/color.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,3 +132,36 @@ def prepare(val):

x, y = xyz2xyY(*rgb2xyzA(r, g, b))
return {X: x, Y: y}


# Converted to Python from Obj-C, original source from:
# http://www.developers.meethue.com/documentation/color-conversions-rgb-xy
# pylint: disable=invalid-sequence-index
def xy_brightness_to_rgb(vX: float, vY: float, ibrightness: int):
"""Convert from XYZ to RGB."""
brightness = ibrightness / 255.
if brightness == 0:
return (0, 0, 0)
Y = brightness
if vY == 0:
vY += 0.00000000001
X = (Y / vY) * vX
Z = (Y / vY) * (1 - vX - vY)
# Convert to RGB using Wide RGB D65 conversion.
r = X * 1.656492 - Y * 0.354851 - Z * 0.255038
g = -X * 0.707196 + Y * 1.655397 + Z * 0.036152
b = X * 0.051713 - Y * 0.121364 + Z * 1.011530
# Apply reverse gamma correction.
r, g, b = map(
lambda x: (12.92 * x) if (x <= 0.0031308) else
((1.0 + 0.055) * pow(x, (1.0 / 2.4)) - 0.055),
[r, g, b]
)
# Bring all negative components to zero.
r, g, b = map(lambda x: max(0, x), [r, g, b])
# If one component is greater than 1, weight components by that value.
max_component = max(r, g, b)
if max_component > 1:
r, g, b = map(lambda x: x / max_component, [r, g, b])
ir, ig, ib = map(lambda x: int(x * 255), [r, g, b])
return (ir, ig, ib)
30 changes: 24 additions & 6 deletions pytradfri/device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
ATTR_TRANSITION_TIME
)
from .color import can_kelvin_to_xy, kelvin_to_xyY, xyY_to_kelvin, rgb_to_xyY,\
COLORS
xy_brightness_to_rgb, COLORS
from .resource import ApiResource


Expand Down Expand Up @@ -230,12 +230,27 @@ def dimmer(self):

@property
def hex_color(self):
return self.raw.get(ATTR_LIGHT_COLOR)
raw_color = self.raw.get(ATTR_LIGHT_COLOR)
if raw_color is not None and len(raw_color) == 6:
return raw_color
(x, y) = self.xy_color
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if I'm 100% happy with this.

This property used to be a direct 1:1 mapping with the underlying data from the API. I think that it should stay like that. If we want to offer smarter things on top, maybe we can offer it as a utility function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true for all hex_color, xy_color and kelvin_color. Maybe each of them need to have two flavours - "raw" and "best guess"? Maybe boolean parameter? Than it can default to one of flavours - which one you'd think would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, they're properties, so it conflicts with parametrizing them. So two sets of properties maybe? [hex|xy|kelvin]_color_[raw|guess]? Or [hex|xy|kelvin]_color[_best_guess], or even [hex|xy|kelvin]_color[_raw]. I also don't have a good suffix for this new property - guess, best_guess, estimate? Neither captures exact behaviour, which is "If I have a value from API, return it, else estimate from other values from API".

Copy link
Member

Choose a reason for hiding this comment

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

Are you not talking about something along the line of an inferred value?
https://en.wikipedia.org/wiki/Inference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, inference is the word, thanks! So how do you find a set of properties: [hex|xy|kelvin]_color_[raw|inferred]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then we have inconsistency in kelvin_color, which is definitely inferred. And this change would be breaking because homeassistant checks bulb features by verifing if hex_color makes sense? So maybe it would be good to introduce breaking change, together with can_set_, to introduce more order? I'll then add can_set_ to this PR soon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Breaking with Home Assistant is fine as HASS uses pinned requirements. If we messed up with kelvin_color, let's fix it.

Copy link
Contributor Author

@matemaciek matemaciek Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

OK, so which convention should we follow:

  1. [hex|xy]_color[_raw] and kelvin_color
  2. [hex|xy]_color[_inferred] and kelvin_color_inferred
  3. [hex|xy]_color_[raw|inferred] and kelvin_color_inferred

? (My order from most to least preferred: 1, 3, 2)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't follow your convention list. This is what I would prefer:

  • I want, by default, property names to reflect what is returned from the API. xy_color comes directly from the returned dictionary. If we don't do this, the user can no longer trust if some data is there or that we're trying to come up with an answer, which might not fit their use case
  • I want the property name to reflect if we're doing magic to make it work. So add _inferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, so what's you're describing is I guess 2) [hex|xy]_color[_inferred] and kelvin_color_inferred. which means 5 properties:

  • hex_color,
  • xy_color,
  • hex_color_inferred,
  • xy_color_inferred,
  • kelvin_color_inferred.
    I'll change properties name that way.


def scale(val):
return float(val)/65535
return '{0:02x}{1:02x}{2:02x}'.format(
*xy_brightness_to_rgb(scale(x), scale(y), self.dimmer)
)

@property
def xy_color(self):
return (self.raw.get(ATTR_LIGHT_COLOR_X),
self.raw.get(ATTR_LIGHT_COLOR_Y))
current_x = self.raw.get(ATTR_LIGHT_COLOR_X)
current_y = self.raw.get(ATTR_LIGHT_COLOR_Y)
if current_x is not None and current_y is not None:
return (self.raw.get(ATTR_LIGHT_COLOR_X),
self.raw.get(ATTR_LIGHT_COLOR_Y))
# White Tradfri has no x and y, but spec provide 2700K value
xy = kelvin_to_xyY(2700)
return (xy[ATTR_LIGHT_COLOR_X], xy[ATTR_LIGHT_COLOR_Y])

@property
def kelvin_color(self):
Expand All @@ -246,6 +261,8 @@ def kelvin_color(self):
# Only return a kelvin value if it is inside the range that the
# kelvin->xyY function supports
return kelvin if can_kelvin_to_xy(kelvin) else None
# White Tradfri has no x and y, but spec provide 2700K value
return 2700

@property
def raw(self):
Expand All @@ -259,6 +276,7 @@ def __repr__(self):
"state: {}, " \
"dimmer: {}, "\
"hex_color: {}, " \
"xy_color: {}" \
"xy_color: {}, " \
"kelvin_color: {}" \
">".format(self.index, self.device.name, state, self.dimmer,
self.hex_color, self.xy_color)
self.hex_color, self.xy_color, self.kelvin_color)
190 changes: 190 additions & 0 deletions tests/test_light.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
from pytradfri.device import Device

LIGHT_W = {
'3': {
'0': 'IKEA of Sweden',
'1': 'TRADFRI bulb E27 opal 1000lm',
'2': '',
'3': '1.2.214',
'6': 1
},
'3311': [
{
'5850': 1,
'5851': 96,
'9003': 0
}
],
'5750': 2,
'9001': 'Light W name',
'9002': 1494695583,
'9003': 65545,
'9019': 1,
'9020': 1507969307,
'9054': 0
}

LIGHT_WS = {
'3': {
'0': 'IKEA of Sweden',
'1': 'TRADFRI bulb E27 WS opal 980lm',
'2': '',
'3': '1.2.217',
'6': 1
},
'3311': [
{
'5706': 'f1e0b5',
'5707': 0,
'5708': 0,
'5709': 30138,
'5710': 26909,
'5711': 0,
'5850': 1,
'5851': 157,
'9003': 0
}
],
'5750': 2,
'9001': 'Light WS name',
'9002': 1491149680,
'9003': 65537,
'9019': 1,
'9020': 1507970265,
'9054': 0
}

LIGHT_WS_CUSTOM_COLOR = {
'3': {
'6': 1,
'0': 'IKEA of Sweden',
'1': 'TRADFRI bulb E27 WS opal 980lm',
'2': '',
'3': '1.2.217'
},
'3311': [
{
'5706': '0',
'5707': 0,
'5708': 0,
'5709': 32228,
'5710': 27203,
'5711': 0,
'5850': 1,
'5851': 157,
'9003': 0
}
],
'5750': 2,
'9001': 'Light WS name',
'9002': 1491149680,
'9003': 65537,
'9019': 1,
'9020': 1507986461,
'9054': 0
}


LIGHT_CWS = {
'3': {
'6': 1,
'0': 'IKEA of Sweden',
'1': 'TRADFRI bulb E27 CWS opal 600lm',
'2': '',
'3': '1.3.002'
},
'3311': [
{
'5706': 'd9337c',
'5707': 0,
'5708': 0,
'5709': 32768,
'5710': 15729,
'5711': 0,
'5850': 1,
'5851': 254,
'9003': 0
}
],
'5750': 2,
'9001': 'Light CWS name',
'9002': 1506114735,
'9003': 65544,
'9019': 1,
'9020': 1507970551,
'9054': 0
}

LIGHT_CWS_CUSTOM_COLOR = {
'3': {
'0': 'IKEA of Sweden',
'1': 'TRADFRI bulb E27 CWS opal 600lm',
'2': '',
'3': '1.3.002',
'6': 1
},
'3311': [
{
'5706': '0',
'5707': 0,
'5708': 0,
'5709': 23327,
'5710': 33940,
'5711': 0,
'5850': 1,
'5851': 254,
'9003': 0
}
],
'5750': 2,
'9001': 'Light CWS name',
'9002': 1506114735,
'9003': 65544,
'9019': 1,
'9020': 1507970551,
'9054': 0,
}


def light(raw):
return Device(raw).light_control.lights[0]


def test_white_bulb():
bulb = light(LIGHT_W)

assert bulb.hex_color == 'c19b57'
assert bulb.xy_color == (30101, 26913)
assert bulb.kelvin_color == 2700


def test_spectrum_bulb():
bulb = light(LIGHT_WS)

assert bulb.hex_color == 'f1e0b5'
assert bulb.xy_color == (30138, 26909)
assert bulb.kelvin_color == 2697


def test_spectrum_bulb_custom_color():
bulb = light(LIGHT_WS_CUSTOM_COLOR)

assert bulb.hex_color == 'f9bc5a'
assert bulb.xy_color == (32228, 27203)
assert bulb.kelvin_color == 2325


def test_color_bulb():
bulb = light(LIGHT_CWS)

assert bulb.hex_color == 'd9337c'
assert bulb.xy_color == (32768, 15729)
assert bulb.kelvin_color == 4866


def test_color_bulb_custom_color():
bulb = light(LIGHT_CWS_CUSTOM_COLOR)

assert bulb.hex_color == 'cdff67'
assert bulb.xy_color == (23327, 33940)
assert bulb.kelvin_color == 5046