hass-circadian_lighting icon indicating copy to clipboard operation
hass-circadian_lighting copied to clipboard

Fix color temperature out of range

Open mouth4war opened this issue 4 years ago • 14 comments

mouth4war avatar Dec 26 '19 13:12 mouth4war

Now fixes issue #29 too

mouth4war avatar Dec 26 '19 17:12 mouth4war

@claytonjn ready for review

mouth4war avatar Dec 27 '19 17:12 mouth4war

I will review ASAP. I still have not heard a good reason for configuring colortemp at the switch level, however. If I do merge this I will likely not publish the availability of those options - my belief is still that this goes against the core mission of Circadian Lighting and that using these options will likely nullify all benefits of the integration unless the user is extremely careful.

EDIT: All that being said, I do truly appreciate the PR.

claytonjn avatar Dec 27 '19 19:12 claytonjn

Please at least allow the merge for initial transition. I have an automation that used to turn on lights at sunset over a period of 60 seconds. Since I turned on circadian lighting this transition is now instant. I could write an automation to disable the CL light, wait and then turn it back on but if this was a feature it would be much easier.

broyuken avatar Jan 10 '20 02:01 broyuken

I'll definitely merge initial transition, and I'm still doing my best to get time to review the full PR. Super busy right now but I haven't forgotten!

claytonjn avatar Jan 10 '20 03:01 claytonjn

I have a reason I would like the min and max color temp at the switch level.

I have some bulbs that bottom out at a ct 2000, others at 2200. Current min temp setting is 2200 to keep the bulbs from looking different.

Visually, the difference between 2200 and 2000 is so small I only notice if the bulbs are in the same light fixture. So, in the same lamp for example, where they are side by side.

For those fixtures, I'd like to have the min at 2200, while the remaining lights set the min to 2000.

Not sure if that case is really worth the extra code and potential other problems users could encounter by a bad configuration could cause.

gderber avatar Feb 14 '20 00:02 gderber

@claytonjn I would prefer to replace the config parameters for min/max CT with a check on each light's min_mireds and max_mireds. This will fix the errors that come up when CL tries to set a CT outside of the light's range.

Though, I don't know how to access each light's min and max mireds. Would appreciate some help :)

mouth4war avatar Feb 19 '20 13:02 mouth4war

Visually, the difference between 2200 and 2000 is so small I only notice if the bulbs are in the same light fixture. So, in the same lamp for example, where they are side by side.

If that's the case, what's the point of having some transition to 2200 and some to 2000? One thing to keep in mind is that with different min colortemps the lights will be different all day. It's not the case that they would be in sync until hitting 2200 and then those with a 2000 min will continue transitioning longer, rather they will start 200 apart, converge at max colortemp, and immediately start diverging again.

claytonjn avatar Feb 19 '20 18:02 claytonjn

@claytonjn I would prefer to replace the config parameters for min/max CT with a check on each light's min_mireds and max_mireds. This will fix the errors that come up when CL tries to set a CT outside of the light's range.

Though, I don't know how to access each light's min and max mireds. Would appreciate some help :)

Typically this can be accessed from the attributes of the light entity. I've thought about it and actually started implementing it at one point, but ultimately changed my mind. The reality is that I still remain unconvinced that there is a reason to allow for different temperature settings for different lights.

The only real argument is that different models of lights may be more or less accurate with their representation of a color temperature and allowing configuration at the switch level would allow some level of "calibration". Color temperature meters cost between $250-$1,500+ and unless someone is serious enough about this to buy a meter how will anyone know which lights need calibration and by how much? If someone does have a color temperature meter I would think a better solution would be to update the integration for each particular light so that the colors are accurate regardless of being adjusted by CL, voice, frontend, etc.

My current feeling is still that allowing for adjusting color temperature per-switch goes against the core purpose of this integration. That being the case, I would rather adjustments fail with an error so that the user knows to adjust min_colortemp. Otherwise, every time CL is initiated it would have to check each configured light to see what its minimum and maximum are and then set the min/max to the lowest denominator. Alternatively, if it was decided that the min/max should be automatically set per-light that would require querying those values for each light, every time an adjustment is made. That all sounds like a LOT of unnecessary overhead that can be easily resolved from the user manually setting up the component correctly one time. Also, some lights have minimum colortemp of well under 2000 which is far below what most users would want; users won't necessarily want to use the full range of each light.

claytonjn avatar Feb 19 '20 18:02 claytonjn

Hmm can we not store the light's CT max/min values once during initialization since they don't change? If not, we can make the error handling better instead of the light throwing exceptions. Maybe a warning after catching the exception?

mouth4war avatar Feb 20 '20 09:02 mouth4war

Oops. Separated PR for initial transition so this one can focus on fixing the color temperature out of range exceptions.

Scope: Add exception handling or prevention - check light min/max mired during initialization and warn if out of range. Optional: limit changes to CT within light's range.

mouth4war avatar Feb 20 '20 09:02 mouth4war

Should I rebase for min max CT per switch as undocumented feature?

mouth4war avatar Feb 25 '20 17:02 mouth4war

How do you envision that not being documented? Currently min/max is set at the platform level so by moving it to the switch level it would have to be documented and would be a breaking change (which I'm still not convinced is the best option but am am still mulling over).

claytonjn avatar Feb 25 '20 19:02 claytonjn

No breaking change. I hate those! :)

Maybe just default these min/max CT per switch to the same values as for the CL component config if not specified in switch config? That way nothing breaks and behaviour is the same as before if switch min/max CT isnt used.

The most elegant way IMHO is to read the min/max values for each light and use those during adjustment.

Let me know what you decide.

mouth4war avatar Feb 26 '20 17:02 mouth4war