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

feat: service call adaptation #628

Merged

Conversation

protyposis
Copy link
Collaborator

@protyposis protyposis commented Jul 5, 2023

Adapt light.turn_on service calls, that initially turn on a light, directly (called proactive adaptation in code) by injecting brightness and color into the service call data payload. This makes lights immediately turn on with the correct adaptation settings. Resolves #594.

Traditionally, AL follows a reactive adaptation approach, which reacts to state change events. This works fine in most cases, but can result in a noticeable adaptation delay when turning on a light (due to events not being instant, and due to various light hardware quirks). Example: Previously, a light might first turn on at 100% brightness before AL kicks in and adapts it down to 1%. Now it directly turns on at 1% brightness.

This only affects service calls which turn on lights that are off, i.e., the initial turn-on. Also, this first version only adapts service calls that target a single light entity. Everything else continues to work as usual. This feature requires access to an internal/protected hass property and automatically falls back to the reactive adaptation mode in case it becomes unavailable in future HA versions (and requires adjustments in AL). This also makes certain workarounds obsolete, e.g., using adaptive_lighting.apply to turn on lights, or explicitly applying AL state to light service calls (#585 (comment)).

Update:

  • Adapt light.toggle service calls iff they turn a light on.
  • Prefer transition times specified in the service call over the default initial_transition.

@basnijholt
Copy link
Owner

@protyposis, I just had 2 babies and cannot find the time to properly review this.

If you like, I can add you as a maintainer of this repo. Then you can merge this and release if you are comfortable. LMK

I am approving without detailed look at this. Otherwise idea is great though!

@protyposis
Copy link
Collaborator Author

Thanks, I accepted the invitation :) Is my assumption correct that releases are created manually in this repo, like this?

  1. bump version in manifest.json
  2. create version tag
  3. create GitHub release and write release notes

@basnijholt
Copy link
Owner

@protyposis, that’s 100% correct! 😄

@protyposis protyposis merged commit ed80bd7 into basnijholt:main Jul 19, 2023
@protyposis protyposis deleted the feature/service-call-adaptation branch July 19, 2023 07:10
@ScottG489
Copy link

I haven't taken a look at the code in depth yet, but if I'm understanding this implementation correctly, it's a pretty huge deal.

It sounds like you're able to hook into service calls and modify them before sending them off again? This would be absolutely massive and something I've been interested in doing for a while, not just in the lighting space. This is useful to generally augment behavior of service calls without having to write a wrapper script (as seen in that comment you linked) and call that instead.

Would something along these lines also allow you to set up "hooks" for actions you could take before a service runs?

Most importantly, do you think this would be a candidate for an architecture proposal?

Alternatively, if core doesn't want to go in that direction architecturally, perhaps this general functionality could be extracted into its own custom_component?

Thoughts @protyposis?

@protyposis
Copy link
Collaborator Author

It sounds like you're able to hook into service calls and modify them before sending them off again?

Exactly✌️

Would something along these lines also allow you to set up "hooks" for actions you could take before a service runs?

Definitely. Currently, it allows registering a single async "interceptor" function, which defers service execution until it resolves. It could be easily extended with a flexible hooking interface.

Most importantly, do you think this would be a candidate for an architecture proposal?

Yes. The current implementation has a few disadvantages that a proper implementation in core could fix:

  • It accesses protected hass internals and could break with a future update (hence the implemented fallback).
  • Service call events are fired before service calls are processed (HA architecture decision), thus also before the interceptor applies changes. So they always carry the original data (which is luckily not an issue for AL itself, and could even be argued as a feature).
  • Service call data is processed by HA (filtered, modified, restructured) before the interceptor is called, thus AL currently cannot access the original data and needs to reprocess after applying changes. With the current approach, it's not possible to hook in earlier to avoid this.
  • If the service is re-registered by any integration, the interceptor gets lost. This can be detected and interceptor reinstalled (not implemented in this PR), but a clean implementation would avoid it altogether. If another integration used this approach as well, they would compete for the interceptor, and if both would implement the detection/recovery, they could end up in an endless loop and tear down HA.

I'd like to start that discussion once we collected enough evidence (through positive user feedback) that this approach has the potential to be a valuable core HA feature.

@basnijholt
Copy link
Owner

basnijholt commented Jul 20, 2023

Thanks so much for all the awesome work!

I got some time to try it out but unfortunately it doesn't seem to work in my setup (https://github.com/basnijholt/home-assistant-config/blob/master/includes/adaptive_lighting.yaml).

I added some logging statements here:

if entity_id not in self.adaptive_switch.lights:
return

And see:
entity_id not in self.adaptive_switch.lights self.adaptive_switch.lights=[] entity_id='light.sphere_tv'

I believe this is because there is only a single TurnOnOffListener instance which multiple AdaptiveSwitches use. However, now TurnOnOffListener.adaptive_switch gets a single switch assigned. Which means it might get the switch that the light is not in.

Instead it might be an idea to use find_switch_for_lights:

https://github.com/basnijholt/adaptive-lighting/blob/1cedb3fbc7672f2de7043b08ebd38af8f698ee64/custom_components/adaptive_lighting/switch.py#L308C5-L332

to identify the switch?

Any thoughts?

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.

Adapting service calls directly (instead of reacting to service call events)
3 participants