-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fix color temperature out of range #60
base: main
Are you sure you want to change the base?
Conversation
min and max colortemp per switch
Now fixes issue #29 too |
@claytonjn ready for review |
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. |
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. |
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! |
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. |
@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 :) |
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. |
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. |
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? |
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. |
Should I rebase for min max CT per switch as undocumented feature? |
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). |
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. |
No description provided.