home-assistant / architecture

Repo to discuss Home Assistant architecture
313 stars 100 forks source link

Boolean triggers on startup #456

Closed amelchio closed 3 years ago

amelchio commented 3 years ago

Context

Inspired by https://github.com/home-assistant/core/issues/40331 I would like to discuss the startup behaviour of numeric_state and template triggers. These two triggers are a bit special because they do not directly represent events but rather a Boolean value. They trigger when the Boolean value turns true.

So what happens if the value is already true at startup? Nothing currently happens at startup because automations are disabled at startup. However, once the first relevant state change happens, the trigger is evaluated. If it is still true, then it fires.

This is the example from the issue.

- alias: 'Switch on lights at night'
  trigger:
    - platform: numeric_state
      entity_id: sun.sun
      value_template: "{{ state.attributes.elevation }}"
      below: 3.0
  action:
    - service: switch.turn_on
      entity_id: group.plugs_auto

If Home Assistant is restarted at night, this will turn on the lights maybe half an hour later (because sun.sun rarely updates during the night). That is generally unexpected and undesired.

Here is another example that will fire the first time light brightness is changed after startup (if the light is on during the restart);

- alias: 'Log office light turning on'
  trigger:
    - platform: template
      value_template: '{{ states("light.office") == "on" }}'
  action:
    - service: system_log.write
      data:
        message: 'The office light is now on'

After that first firing, brightness changes will never again affect the template.

Proposal

I propose that these two triggers should only fire when going from false to true.

They should not fire if already true at startup. They should not fire if going from true to unavailable and back to true.

Full disclosure: I actually implemented the current behavior for numeric_state (in https://github.com/home-assistant/core/pull/10125). I was in doubt about how to handle it back then and I now think I did it the wrong way around (even though everybody agreed with me).

Consequences

The main potential benefit is that lights and other stuff would no longer toggle for every restart.

This is a breaking change but I struggle to think of examples where the current behavior is desirable (please help). One situation is testing though: when implementing the above automation it is nice that one does not have to wait for the next sunset to see that it works.

If one really wants the current behavior it can be implemented with a homeassistant start trigger combined with a condition.

CC: @aaronwolen @Tom7320 @stefanroelofs (from the mentioned issues)

frenck commented 3 years ago

I honestly think we should change the behavior

I honestly think that that is a dangerous assumption and totally depends on the use case of the boolean in this case. The behavior now, might not always be the one wished for, however, it is consistent. (Or I'm totally misunderstanding this one).

I rather see a change in handling restored values as a whole (getting rid of the whole changing values on startup for any state as much as possible, opening possible doors for restoring last_changed).

amelchio commented 3 years ago

I am not sure which assumption you are talking about since your quote does not seem to appear anywhere?

You are suggesting that the current behavior is useful in some scenarios. Can you give a couple of examples?

You imply that this proposal is not consistent but I think it is. Can you elaborate?

The comment about restored state seems out of scope; neither of my examples relate to state restoration and there are no value changes at startup.

frenck commented 3 years ago

Sorry, I'm explaining myself badly. And failed at quoting as well (which didn't help).

For example; I write all my automation in such a way they ensure everything is in the state it should be. This means I want things to trigger to ensure the result is as I want it to be.

For example: 'Switch on lights at night'

If for whatever reason, this automation is missed (because e.g., my instance was offline during the sun transition or maybe I changed the automation). I want things to trigger; to guarantee the end states of things.

However, as you wrote:

So what happens if the value is already true at startup? Nothing currently happens at startup because automations are disabled at startup. However, once the first relevant state change happens, the trigger is evaluated.

The problem here is the startup and restore. States are restored and thus, maybe not changing. And when they do, automations are not running yet, so they miss the change.

IMHO, the only proper way around this: If you want fire on startup (like in my situation above), you'll need to listen for the Home Assistant start event.

Additionally:

If it is still true, then it fires.

That is weird, if it was true and still true, it should not fire at all. Same for if false, and still false (the latter in these cases isn't relevant of course).

From that perspective:

I propose that these two triggers should only fire when going from false to true.

Is not a design thing, but a bug. And to resolve it, I don't think we should monitor for a false->true transition. The problem I think relies in the initial state it stores and compares against when state change happens. This means we need to evaluate the template, before attaching/tracking template result changes.

This is the only way to know if it actually changed. As far as I am concerned, the first tracked change that triggers, cannot simply be disregarded (as it might be intentional, as the sun might actually have changed 30 min later).

(PS: for the restored stuff, yeah offtopic, but I do think handling that differently would deal with most of this stuff).

amelchio commented 3 years ago

I think we actually agree about the thing I tried to describe in this issue. Just to be sure, let me repeat the trigger:

  trigger:
    - platform: numeric_state
      entity_id: sun.sun
      value_template: "{{ state.attributes.elevation }}"
      below: 3.0

Do you agree that this should not trigger on HA restart if the sun is currently down (like, maybe elevation is -20)?

It currently does trigger; I am fine with calling that a bug but it is implemented on purpose (https://github.com/home-assistant/core/pull/10125#issuecomment-339239872):

I think that the first state change to come in for a numeric trigger should trigger the automation. We should not wait for a state to be first below the threshold.

I think the rationale is that the sun might have set just as we restarted and thus we try to catch that. But as you said, there is no generic way to handle that, one has to manually catch the start event and set things properly.

Tom7320 commented 3 years ago

I must admit that I don't understand the technical details. But from a user's perspective the current behavior is not logical and not consistent at all. Maybe users think different from developers. I use a lot of sun triggers in my automations to control lights and shutters. And I expect the sun automation mentioned above to trigger only once a day. Namely, if the sun moves from above 3° to below 3°.

I run HA container. As soon as watchtower updates this container automatically all of a sudden all my sun automations trigger in the middle of the night which is definitely not desired and expected behavior....

I try to avoid this by adding an auxiliary construct like this:

condition:
    condition: numeric_state
    entity_id: sensor.uptime
    above: 10

But this does not help in all cases since it sometimes takes 30 minutes for the trigger to fire.

I'm not sure if I made my point clear. But maybe you understand a little what happens from a user's (not a developer's) perspective.

Thx a lot and have a nice evening!

frenck commented 3 years ago

Do you agree that this should not trigger on HA restart if the sun is currently down (like, maybe elevation is -20)?

Yes. I 100% agree on that. It isn't changing. It already is.

The difference to me, relies in the factual: Was it this way before the automations started? If so, we should not trigger it. As nothing changed; we are now triggering additional false positives.

With reference to the PR you gave, I now fully understand the creation of an architectural issue vs an issue/bug report. Nevertheless, from my personal perspective, it is a bug as I cannot imaging this being a deliberate implementation/idea.

Edit; to sum up. I support changing the behavior. (With the exception that we should determine the template result before tracking state changes, instead of just tracking false->true. However, that is an implementation detail).

dgomes commented 3 years ago

The difference to me, relies in the factual: Was it this way before the automations started?

This where I fell into the "restore state" rabbit hole. We don't always have a state to restore from, and for instance a template sensor, should it even restore ?

stefanroelofs commented 3 years ago

In addition to triggering on HA restart, this automation will also trigger when reloading automations:

automation:
  alias: "Exterior Lighting on when dark outside"
  trigger:
    platform: numeric_state
    entity_id: sun.sun
    attribute: elevation
    # Can be a positive or negative number
    below: -4.0

So when you reload your automations late at night, the lights will turn on, which I think is undesirable. So I support changing the current implementation.

And to give some context, one of the reasons it works the way that it works now is this automation:

- alias: 'Monitor CPU temperature'
   trigger:
     platform: numeric_state
     entity_id: sensor.cpu_temperature
     above: 200
   action:
     - service: notify.slack
       data:
         message: Your Raspberry Pi is melting.

The user wanted a notification if the trigger is true on HA start. So if the current behaviour is changed, which I approve of, this will no longer trigger on HA start. The user will have to rewrite his automation this way:

- alias: 'Monitor CPU temperature'
  trigger:
    - platform: homeassistant
      event: start
    - platform: event
      event_type: automation_reloaded
    - platform: numeric_state
      entity_id: sensor.cpu_temperature
      above: 200
  condition:
    - condition: numeric_state
      entity_id: sensor.cpu_temperature
      above: 200
  action:
    - service: notify.slack
      data:
        message: Your Raspberry Pi is melting.
frenck commented 3 years ago

In addition to triggering on HA restart, this automation will also trigger when reloading automations

That is expected, as it is the same thing.

frenck commented 3 years ago

This where I fell into the "restore state" rabbit hole. We don't always have a state to restore from, and for instance a template sensor, should it even restore ?

That is why I initially started rambling about state restore. However, I do agree with @amelchio that that is out of scope for this issue (although I can see jump have been made because of it).

Nevertheless, the automation engine starts later (after startup has been completed). So we can evaluate the template at that point in time, before hooking up the change/event/state/template tracking logic of the automation.

Does that cover all cases? No; simply because there is no guarantee an integration has finished loading before the Home Assistant start is fired. However, that is another edge case, which we most likely are not going to solve (as the only solutions are to block full startup until all integrations have reported in).

dgomes commented 3 years ago

we don't want to lock Home Assistant start until all integration have finished loading, but maybe we can stall automations/templates that require information from an entity belonging to an entity that has not finish loading yet...

frenck commented 3 years ago

@dgomes With templates, we simply can't. That information isn't available and requires running it.

balloob commented 3 years ago

I agree that both sun and template should store the current value when the trigger is attached, and then ignore the next time it's triggered, because it's not an actual trigger.

amelchio commented 3 years ago

Sweet. I can work on reversing this but it will probably be a week or two before I am done.

stefanroelofs commented 3 years ago

I'm not really sure, but could this issue be related? https://github.com/home-assistant/core/issues/39192

It is about a time of day sensor that should trigger on sunset/sundown, but somehow stops updating.

Spectre5 commented 3 years ago

Any movement on this @amelchio?

amelchio commented 3 years ago

Well, I got it working for numeric_state: https://github.com/amelchio/home-assistant/commit/21e9328d8379028a4d1580f0b449ec4e93554e33

However, I hit a bit of a roadblock. The core condition helpers return False for undecided conditions, for example if an entity is unavailable. We must distinguish this false condition from a condition actually evaluating to false or else we won't be able to ignore the first kind and trigger on the last kind.

In the commit above, I solved this by returning None in the helper when the state is undecided. I think this works but it is quite ugly and would not be accepted in review.

So a larger rework of the condition helpers is probably needed and I am afraid that I do not currently have the time/motivation to undertake that. Or maybe there is a simpler solution that I am missing?

Spectre5 commented 3 years ago

I would think that using None for an undecided state makes perfect sense, since it isn't know if it is True or False. Why would that not get accepted? I don't have much familiarity with the code base, so it's certainly possible (likely!) that I'm just overlooking things.

amelchio commented 3 years ago

Thank you for the feedback @Spectre5. Even if technically correct, ternary Booleans are just weird.

I have found a way that I think is cleaner, namely to raise an exception if it is not possible to evaluate a condition. Example implementation: https://github.com/amelchio/home-assistant/commit/fe3d529cce338c366e04e76bbb626eb627257d96

As can be seen in that commit, a challenge with this approach is that the exception will have to be caught in a number of places. I actually think that this could be an improvement since we are then able to do something appropriate when the condition is undecided.

However, I am not sure how this proposal would be received since it is quite hard to add a central exception at this time (i.e. it probably needs to be caught in more places but I don't know where).

Spectre5 commented 3 years ago

So what would be the next step? I guess you'd need a PR to get some core developers input on whether they would consider this appropriate. How easy would it be to help test this on my HA instance (on a raspberry pi)? Which branch of yours is this on?

amelchio commented 3 years ago

I now submitted a PR proposing the addition of exceptions for errors during evaluation of conditions: home-assistant/core#45923

Adminiuga commented 3 years ago

I agree that transition from state.restored and state.state == 'unavailable' should not trigger the automation, but I'm not so sure about the behavior during the normal operation, e.g. when the underlying sensor transitions from unavailable to the state satisfiing the condition? At least we should call out the agreed behavior in the documentation