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

Remove global limit on white light temperature #7206

Merged
merged 3 commits into from Apr 29, 2017

Conversation

amelchio
Copy link
Contributor

@amelchio amelchio commented Apr 21, 2017

Description:

Here are the supported temperatures of some popular bulbs:

Philips Hue: 2000K-6500K (the current 500-154 mired range)
LIFX Color 1000: 2500K-9000K
IKEA TRÅDFRI: 2200K, 2700K, 4000K

Obviously, Home Assistant cannot enforce a global limit and work properly
with all of these bulbs. So just remove the limit and leave it up to each
platform to work it out.

This commit updates the existing users and adds a clamp to Hue (where the
limit appears to have originated). It does not attempt to update other
platforms that might need extra handling of the larger range that is now
possible.

The UI also has a hardcoded range for the temperature and it would probably be good to scale it to the supported range of the current bulb. Unfortunately, that range is generally unknown.

@cgrin @armills what do you think of this?

Related issue (if applicable): fixes #6606

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

Here are the supported temperatures of some popular bulbs:

 Philips Hue: 2000K-6500K (the current 500-154 mired range)
 LIFX Color 1000: 2500K-9000K
 IKEA TRÅDFRI: 2200K, 2700K, 4000K

Obviously, Home Assistant cannot enforce a global limit and work properly
with all of these bulbs. So just remove the limit and leave it up to each
platform to work it out.

This commit updates the existing users and adds a clamp to Hue (where the
limit appears to have originated). It does not attempt to update other
platforms that might need extra handling of the larger range that is now
possible.
@mention-bot
Copy link

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

@cgrin
Copy link

cgrin commented Apr 21, 2017

This should solve the issue of LIFX commands with color temps above 6500K being ignored, and looks like it sets Hue bulbs to 6500K in the event a command is sent with a higher temp. Given that, the changes I made in home-assistant/frontend#243 should work to update the temp slider ranges in the UI.

@emlove
Copy link
Contributor

emlove commented Apr 21, 2017

I don't think any change to this logic makes sense unless there is a plan in place for the frontend. I think the easiest step forward at the moment is to broaden the hass range to support all platforms, (so 2000K-9000K), and update the frontend respectively. (So use @cgrin's PR) Individual platforms can then apply their own clamps where required.

@amelchio
Copy link
Contributor Author

That could also work, though I think it is odd to enforce a range that is wrong in every single case. In that case I would rather not enforce anything (just like color_name will accept every string).

How about a kelvin_range property on each light? The frontend would use that to scale the slider. To get started, the light component could default it to the current range.

@balloob
Copy link
Member

balloob commented Apr 22, 2017

I prefer platforms to get properties min_mireds and max_mireds that are exposed as attributes so that we don't have to guess or assume things in the frontend.

@amelchio amelchio changed the title Remove global limit on white light temperature [WIP] Remove global limit on white light temperature Apr 22, 2017
@amelchio
Copy link
Contributor Author

I will add the attributes that @balloob suggests. Hopefully someone else can do the frontend.

As you can have a group of lights with different ranges, I think my initial commit still makes sense. It is just impossible for the core to do the check.

"""Return the warmest color_temp that this light supports."""
# The 3 LIFX "White" products supported a limited temperature range
# https://lan.developer.lifx.com/docs/lifx-products
if self.product in [ 10, 11, 18 ]:

Choose a reason for hiding this comment

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

whitespace after '['
whitespace before ']'

"""Return the coldest color_temp that this light supports."""
# The 3 LIFX "White" products supported a limited temperature range
# https://lan.developer.lifx.com/docs/lifx-products
if self.product in [ 10, 11, 18 ]:

Choose a reason for hiding this comment

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

whitespace after '['
whitespace before ']'

@amelchio amelchio force-pushed the remove-global-light-temp-range branch from 753abd1 to 37c2298 Compare April 23, 2017 14:01
@amelchio amelchio changed the title [WIP] Remove global limit on white light temperature Remove global limit on white light temperature Apr 23, 2017
@amelchio
Copy link
Contributor Author

I added global and LIFX min_mireds and max_mireds now. I do not feel comfortable updating the platforms where I don't have any hardware to test.

@balloob
Copy link
Member

balloob commented Apr 29, 2017

Bueno! 🐬

@balloob balloob merged commit 64a7be6 into home-assistant:dev Apr 29, 2017
@balloob balloob mentioned this pull request May 5, 2017
@ajma
Copy link

ajma commented Jul 6, 2017

Can we update the documentation on how to use min_mireds and max_mireds too? I can do it if you give me an example

@amelchio
Copy link
Contributor Author

amelchio commented Jul 6, 2017

@ajma There is nothing to update, these are internal values like for example supported_features that each light platform provides to tell the UI about their capabilities.

@home-assistant home-assistant locked and limited conversation to collaborators Oct 20, 2017
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.

Home Assistant using invalid min color temp
9 participants