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

Add Adaptive lighting #14

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Add Adaptive lighting #14

wants to merge 25 commits into from

Conversation

Zer0x00
Copy link
Collaborator

@Zer0x00 Zer0x00 commented May 17, 2022

Funny story... I thought I had sent off the adaptive lighting implementation a long time ago. Only when I made an update, I realized that this is not the case.

So here I am making up for it 😄

  • added adaptive lighting
  • added brightness caching for changes between color modes
    • seperate flag added to mitigate flashing on low brightness values

@normen Could you test that nothing is broken so far? And could you also release an npm beta-update?

Thanks!

@normen
Copy link
Owner

normen commented May 17, 2022

Cool, thank you! I'll have to look deeper into this though.

From a rough overview the changed behavior in "stateChanged" can't really be right as we don't know about any state anyway after a reboot. And if we expect the hub to update the state before that then the check for "first run" should be elsewhere I suppose? I guess it's there to avoid boilerplate?

And as you TODO'd already, the added "previous_state" in "currentState" seems somehow backwards. I'll have to wrap my head around how the whole adaptive lighting thing appears to us in HomeKit first to make heads or tails of that though.

This might all be wrong as I didn't pull this into my dev environment yet, just wanted to give some quick feedback on all the work you did! Thanks again!

@Zer0x00
Copy link
Collaborator Author

Zer0x00 commented May 17, 2022

Normally HomeKit wouldn't do any updates before the bridge is fully started but with adaptive lighting this behaviour is changed, so therefore the "firstSuccessfulRunFinished" check is necessary. Adaptive Lighting fires a setColorTemperature() call as soon as the didFinishLaunching Event is called which causes a race condition. The position is irrelevant, yes I wanted to avoid boilerplate so checking for it in the setters was not an option for me. Maybe it would match better in applyDesignatedState()? Feel free to adjust it as you like :)

It could be that some of the conditions, especially the "previous_bulb_mode2" is now unnecessary since the brightness values between the bulb_modes "color" and "white" should now be in sync. I've implemented the brightness synchronisation after I wrote the "previous_bulb_mode2" part.. It was more of a code->check->code->check & fix every upcoming problem session instead of sitting down and making a plan on what to do so sorry for that but had to do it quick or it would've been an another year before I would have taken the time to do it 😆

I'll use this version as my daily driver from now on and I'll look for any problems. So far it works fine for me.
You're welcome and thank you for reviewing!

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.

2 participants