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

Fix color temperature out of range #60

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mouth4war
Copy link
Contributor

@mouth4war mouth4war commented Dec 26, 2019

No description provided.

@mouth4war
Copy link
Contributor Author

Now fixes issue #29 too

@mouth4war mouth4war changed the title CONF_INITIAL_TRANSITION Add initial transition, min colortemp and max colortemp to switch Dec 26, 2019
@mouth4war
Copy link
Contributor Author

@claytonjn ready for review

@claytonjn
Copy link
Owner

claytonjn commented Dec 27, 2019

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.

@broyuken
Copy link

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.

@claytonjn
Copy link
Owner

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!

@gderber
Copy link

gderber commented Feb 14, 2020

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.

@mouth4war
Copy link
Contributor Author

mouth4war commented Feb 19, 2020

@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 :)

@claytonjn
Copy link
Owner

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
Copy link
Owner

@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.

@mouth4war
Copy link
Contributor Author

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 mouth4war closed this Feb 20, 2020
@mouth4war mouth4war changed the title Add initial transition, min colortemp and max colortemp to switch Fix color temperature exceptions Feb 20, 2020
@mouth4war mouth4war changed the title Fix color temperature exceptions Fix color temperature out of range Feb 20, 2020
@mouth4war
Copy link
Contributor Author

mouth4war commented Feb 20, 2020

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 mouth4war reopened this Feb 20, 2020
@mouth4war
Copy link
Contributor Author

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

@claytonjn
Copy link
Owner

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).

@mouth4war
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants