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.

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

RGB Tradfri simple support #9703

Merged
merged 10 commits into from Oct 22, 2017
Merged

RGB Tradfri simple support #9703

merged 10 commits into from Oct 22, 2017

Conversation

matemaciek
Copy link
Contributor

@matemaciek matemaciek commented Oct 5, 2017

Description:

Simple support for RGB Tradfri lights. I intend to move detecting bulb type to pytradfri, so finaly this code will be much simpler. But let's allow people with RGB Tradfri to play with it now.

Checklist:

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies are only imported inside functions that use them (example).

from pytradfri.color import MIN_KELVIN_WS
return color_util.color_temperature_kelvin_to_mired(MIN_KELVIN_WS)
from pytradfri.color import MIN_KELVIN_WS, MIN_KELVIN
min_kelvin = MIN_KELVIN if "CWS" in self._light.device_info.model_number else MIN_KELVIN_WS

Choose a reason for hiding this comment

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

line too long (99 > 79 characters)

from pytradfri.color import MAX_KELVIN_WS
return color_util.color_temperature_kelvin_to_mired(MAX_KELVIN_WS)
from pytradfri.color import MAX_KELVIN_WS, MAX_KELVIN
max_kelvin = MAX_KELVIN if "CWS" in self._light.device_info.model_number else MAX_KELVIN_WS

Choose a reason for hiding this comment

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

line too long (99 > 79 characters)

self._features |= SUPPORT_RGB_COLOR

if self._light_data.hex_color is not None and self._light.device_info.manufacturer != IKEA:

Choose a reason for hiding this comment

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

line too long (99 > 79 characters)

@balloob
Copy link
Member

balloob commented Oct 5, 2017

This seems similar to #9698

This PR too I think we should wait till #7815 lands.

@matemaciek
Copy link
Contributor Author

Yay, #7815 has landed (-: Can't wait to try it out.
I didn't notice #9698, I think wes hould take what's best from both PRs. I like that #9698 fixes #9603. But it disables colour temprature for RGB, while they can support it.

@matemaciek
Copy link
Contributor Author

IMHO pytradfri should take care of cause of #9603. It should also detect bulb type and have different classes. I can take care of it (-: But as it will be bigger change, I wanted to share this one first.

spektren added a commit to spektren/home-assistant that referenced this pull request Oct 6, 2017
spektren added a commit to spektren/home-assistant that referenced this pull request Oct 6, 2017
@spektren
Copy link
Contributor

spektren commented Oct 6, 2017

pytradfri might be the right place to determine the features but...
what do you think of adding the changes in #9703 and #9698 for the async version of HA's tradfri (until the bigger changes in pytradfri are made)?

The improvements are noticable for the everybody user (UI bulb icons responding to color changes and color picker where necessary). Both changes seem to work fine with current dev:
https://github.com/spektren/home-assistant/tree/mydev54

@matemaciek
Copy link
Contributor Author

@spektren I'm totally for it (-: It would be best to have #9703 and #9698 released together. I've already included async version of tradfri in this PR, just need to inlude your fix for #9603.

@spektren spektren mentioned this pull request Oct 6, 2017
8 tasks
spektren added a commit to spektren/home-assistant that referenced this pull request Oct 6, 2017
spektren added a commit to spektren/home-assistant that referenced this pull request Oct 6, 2017
@fabaff fabaff changed the title [light.tradfri] RGB Tradfri simple support RGB Tradfri simple support Oct 13, 2017

@property
def max_mireds(self):
"""Return the warmest color_temp that this light supports."""
from pytradfri.color import MIN_KELVIN_WS
return color_util.color_temperature_kelvin_to_mired(MIN_KELVIN_WS)
return color_util.color_temperature_kelvin_to_mired(self._light_control.min_kelvin)

Choose a reason for hiding this comment

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

line too long (91 > 79 characters)

@@ -161,14 +161,12 @@ def __init__(self, light, api):
@property
def min_mireds(self):
"""Return the coldest color_temp that this light supports."""
from pytradfri.color import MAX_KELVIN_WS
return color_util.color_temperature_kelvin_to_mired(MAX_KELVIN_WS)
return color_util.color_temperature_kelvin_to_mired(self._light_control.max_kelvin)

Choose a reason for hiding this comment

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

line too long (91 > 79 characters)

self._rgb_color = color_util.rgb_hex_to_rgb_list(
self._light_data.hex_color)

self._rgb_color = color_util.rgb_hex_to_rgb_list(self._light_data.hex_color_inferred)

Choose a reason for hiding this comment

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

line too long (93 > 79 characters)

@ggravlingen
Copy link
Contributor

Trying this PR out. The color picker works like a charm. Great work @matemaciek!

image

@ggravlingen
Copy link
Contributor

Ping @lwis for merge. This PR runs on your pytradfri 3.0 (3.0.1 even.)

@lwis
Copy link
Member

lwis commented Oct 22, 2017

Shouldn't this be 3.0.2?

@ggravlingen
Copy link
Contributor

I forgot to make a release of 3.0.1 but pushed it as 3.0.1 to pypi. 3.0.2 is both a release and on pypi so yes, it might be better to use it?

@Danielhiversen Danielhiversen removed their request for review October 22, 2017 14:21
@matemaciek
Copy link
Contributor Author

OK, I've bumped pytradfri requirement.

@lwis lwis merged commit 193188b into home-assistant:dev Oct 22, 2017
@lwis
Copy link
Member

lwis commented Oct 22, 2017

Thanks! 🐊

@eddemans
Copy link

I'm on Home Assistant 0.56.2 (Hassio) and I can see only brightness and color temperature for my Tradfri RGB bulbs. Am I not on the latest version or is this not yet in the main line?

@lwis
Copy link
Member

lwis commented Oct 25, 2017

@eddemans this should be in the next release, but was backed out in dev due to some issues.

lwis pushed a commit that referenced this pull request Nov 2, 2017
@balloob balloob mentioned this pull request Nov 2, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Mar 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants