basnijholt / adaptive-lighting

Adaptive Lighting custom component for Home Assistant
https://basnijholt.github.io/adaptive-lighting/
Apache License 2.0
1.81k stars 134 forks source link

Adapting service calls directly (instead of reacting to service call events) #594

Closed protyposis closed 1 year ago

protyposis commented 1 year ago

When a light is turned on, AL reacts to an event produced by the light.turn_on service call and calls the service again with the adjusted brightness and color (twice with separate_turn_on_commands). Are there specific reasons you went with this approach vs. intercepting and adjusting the initial service call?

I am currently experimenting with this approach (it works in my setup) and wondering if I am missing something prohibitive or if it makes sense to continue the investigation. I think this could reduce race conditions and smoothen turn-on transitions.

th3w1zard1 commented 1 year ago

IIRC someone submitted the code for separate_turn_on_commands into a PR and it was lightly refactored and implemented as is.

th3w1zard1 commented 1 year ago

When a light is turned on, AL reacts to an event produced by the light.turn_on service call and calls the service again with the adjusted brightness and color (twice with separate_turn_on_commands).

I'm not sure about this specifically though. AL only checks light.turn_on to see if it needs to set the light as manually controlled, but that doesn't seem to be what you're asking. If a light is already on and light.turn_on is called again outside of AL, the light is set as manually controlled (when adaptive_lighting.take_over_control is set, otherwise light.turn_on events are ignored). Otherwise AL doesn't do anything with the specific light.turn_on events that HA logs.

th3w1zard1 commented 1 year ago

I am currently experimenting with this approach (it works in my setup) and wondering if I am missing something prohibitive or if it makes sense to continue the investigation. I think this could reduce race conditions and smoothen turn-on transitions.

Anything you're implementing probably is another way to smooth over the problems with your lights and should not impact the main design too significantly. Hope this helps.

protyposis commented 1 year ago

AL only checks light.turn_on to see if it needs to set the light as manually controlled, but that doesn't seem to be what you're asking.

More context:

Turning on a light that is off needs a light.turn_on service call (for reference, I call this the initial service call) which is then processed by HA integrations and other layers to physically turn the light on. If all goes well, HA fires at least two events related to this call: the EVENT_CALL_SERVICE, and EVENT_STATE_CHANGED. AL monitors both for different reasons, and it specifically uses EVENT_STATE_CHANGED (via async_track_state_change_event) to determine whether a light turned on.

Now, if AL detects that a light turned on, it reacts to it by issuing an additional light.turn_on service call to adjust the brightness and color temperature. Due to the async nature of this approach, the adaptation happens with an unavoidable delay, i.e. a short time after the light was turned on. This results in two issues:

It seems to me that this can be avoided if AL proactively intercepts and modifies the initial service call before it is processed by the responsible HA integration, instead of reacting to delayed events. (I think it is possible, but it requires access to a protected property in hass.services.)

Hence, my question:

Is anybody aware of specific reasons why AL doesn't handle the initial service call directly, or, why it's not an option to consider this in the future?

(I deliberately omitted separate_turn_on_commands here. In an actual implementation, it still needs to be considered and increases complexity, but it's irrelevant to the question.)

basnijholt commented 1 year ago

In principle this seems like a great idea. However, I do think Home Assistant might not be designed to write over existing other service_calls. However, if you got it to work without a too insane hack, we might try it?!

protyposis commented 1 year ago

Great! The hack is actually very simple, but due to its nature it could of course fail in a future HA release (or require adjustments). I'll make it an opt-in feature with a fallback to the conventional way in case a HA update breaks it.

benwainwright commented 1 year ago

I have motion sensors setup. My main issue with AL is that when a motion sensor triggers the lights

This kind of breaks the experience for me. If I'm understanding this thread correctly, would your solution eliminate the first step entirely - ie - motion sensor triggers would result in the light being turned on at the correct temperature/brightness in the first instance?

If that's the case, then I'd really love to see this working

protyposis commented 1 year ago

Yes, it eliminates exactly that first step. I'm testing a prototype implementation since a few days and it works great. It really improves the experience :)

benwainwright commented 1 year ago

Don't suppose its at a stage where I could try it?

StSaens commented 1 year ago

but due to its nature it could of course fail in a future HA release (or require adjustments).

Wouldn't it be better to see if it's possible to implement in HA core a native way to support this (=intercepting/rerouting specific service calls) ? I understand wanting to implement a fall-back, but home automation should be as stable as possible and that's only achievement by staying close to stock.

protyposis commented 1 year ago

Of course, but why would they add it at this stage? Typically, this is easier to argument by first creating a use-case and demonstrating the value of such a feature, for which a workaround is a practical first step. Unless you know a better way and can help us to get this in there?

benwainwright commented 1 year ago

@protyposis don't suppose you missed the above, but I'd really love to help you test this if possible?

protyposis commented 1 year ago

A prototype is available at https://github.com/protyposis/adaptive-lighting/tree/feature/service-call-adaptation

protyposis commented 1 year ago

I started a discussion on implementing this as a HA core API at https://github.com/home-assistant/architecture/discussions/956