Fire numeric_state action when first state change matches criteria #10125

amelchio · 2017-10-24T21:59:48Z

Description:

This changes the numeric_state trigger to potentially fire on its first state change, even if the criteria were already met when the automation was created. That makes these two automations work in the same way:

  - alias: 'Numeric state'
    trigger:
      platform: numeric_state
      entity_id: sensor.ram_used
      below: 11000

  - alias: 'Template'
    trigger:
      platform: template
      value_template: "{{ states.sensor.ram_used.state | float < 11000 }}"

The current situation is that template will fire even without crossing the threshold but numeric_state will not. This inconsistency has caused severe confusion, like seen in this forum post (with links to several more).

I know this change is (once again) a bit controversial; you can easily argue that it should only fire when the threshold is actually passed. However, I think it is handy when developing automations to have it fire on startup and I definitely think the above two triggers should work in the same way.

There is a deeper problem that I do not know how to fix: when setting up sensors, the automations are not yet running. So, confusingly, things will not fire on startup but rather on the first change.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

Local tests with tox run successfully.
Tests have been added to verify that the new code works.

pvizeli · 2017-10-24T22:26:44Z

There is a deeper problem that I do not know how to fix: when setting up sensors, the automations are not yet running. So, confusingly, things will not fire on startup but rather on the first change.

That is not a bug, that is a feature. We do setup automation as last platform. We don't know if a sensor is initial with a old state or new state or have they change between last startup. In passt, we was every start wired while we can't know the exactly direction. That is the reason why we change this on 0.39. Maybe not all people love it while he don't understand how the automation work and then he need only set startup as trigger to trigger a automation after startup and the condition need set the correct state of home to run the correct and needed action and not the trigger should represent the state to run the action.

The current situation is that template will fire even without crossing the threshold but numeric_state will not. This inconsistency has caused severe confusion

Yes that is a bug.

pvizeli

Look good

pvizeli · 2017-10-24T22:32:54Z

homeassistant/components/automation/numeric_state.py


    if value_template is not None:
        value_template.hass = hass

    @callback
    def check_numeric_state(entity, from_s, to_s):
-        """Return True if they should trigger."""
-        if to_s is None:


Don't remove this or we trigger it if we remove a state from state machine

I removed this because the test seems to happen anyway in condition.async_numeric_state.

You can believe me, it's needed. I add to my task list, that I will add a test for this case. But yeah it is rare.

Pascal is correct. Needs to be added back.

I trust you but I am really curious, can you explain why this async_numeric_state check is not sufficient?

Safe memory and is optimize. We need not create a function frame to stack if we already know that we not need do this. The dict that follow eat in this case only memor and process in wors case a memory leak on heap

Hmm, I am not sure that I agree with an optimization argument. This is optimizing the unlikely case while making the common case a little bit slower with two identical tests. Extra code also takes up memory :)

Never mind, I will add it back no problem. For sure, there are code style and robustness cases that can be made.

Thanks.

Oh interesting, I wasn't aware there was another check in the condition. I still think it's good to just be explicit to people reading the trigger code what is going on . Thanks for putting it back 👍

amelchio · 2017-10-24T22:41:47Z

I am happy that we agree the two automations should work identically, that was my main beef with this.

Personally, I do not feel strongly about which way is best but I think it is excessive that you have to specify the same conditions twice if you also want it to fire during startup. This is speaking from a usability point of view, I do understand that it is a nice clean architecture :)

pvizeli · 2017-10-25T06:37:37Z

Ready to merge after my commend is addressed

amelchio · 2017-10-25T06:43:05Z

All right, I woke up with the insight that this consistency issue of course also applies to other triggers such as state and sun so it is now clear to me that it is template that should be changed around. I can make a PR for that.

So I am a bit confused that you say this PR can be merged. I have to change the firing back to work like before, right (i.e. not modify any tests)?

I do think the changes in this PR are also a cleanup so I will be happy to fix it up and have it merged.

pvizeli · 2017-10-25T06:50:48Z

Now I'm not sure what the main idea or behave is.
This case, I will a second eyes from @balloob

The code is okay if we have right.

EDIT:

State is okay:

Sun trigger looks also okay. So It's only a problem of numeric_state.

EDIT: Remove text that could be missanderstand in context of that PR

balloob · 2017-10-25T07:27:20Z

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.

amelchio · 2017-10-25T07:43:49Z

Wow, so I was right from the start and now I convinced myself to be wrong? Well I will just address the comment then :)

pvizeli · 2017-10-25T09:01:03Z

That was not for the change request. I mean the handling while we had a test to explicit protect this

aaronwolen · 2017-10-25T12:23:44Z

Thought I'd weigh in as the severely confused user. I strongly agree with @balloob and @amelchio's original position: the initial state should trigger the automation.

If I setup something like:

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

My assumption is I will be notified whenever the CPU temperature is above 200° — period. I'd hate to discover my Pi had turned into a puddle of plastic and silicon just because it was already running hot when HASS was started.

pvizeli · 2017-10-25T13:06:22Z

Yeah, that is correct and that is not the point @aaronwolen

pvizeli · 2017-10-25T13:09:11Z

sorry @amelchio that is childish, If done while I had other 80 PRs to review and no time for that shit

amelchio · 2017-10-25T13:22:13Z

@pvizeli I have no idea what childish shit you talk about. I apologize for any perceived rudeness and can assure you that it is only due to failed communication.

@aaronwolen Likewise, I hope you didn’t take my description the wrong way. It was meant to convey that the implementation is currently confusing, not that anybody is a confused person in general :)

aaronwolen · 2017-10-25T13:31:02Z

@amelchio, not at all! It was indeed my confusion regarding numeric_state's behavior that instigated this conversation. I really appreciate your feedback in the forums and willingness to follow-up with a PR.

pvizeli · 2017-10-25T14:01:06Z

Travis is not realated to this changes

pvizeli · 2017-10-25T17:50:12Z

@amelchio I'm sorry. The comment was to hard and over the target. It also not conform to our https://home-assistant.io/code_of_conduct/. In this case I apologize me.

amelchio · 2017-10-25T20:05:52Z

@pvizeli Thank you, that is much appreciated.

Fire numeric_state action when first state change matches criteria

707036c

amelchio requested a review from a team as a code owner October 24, 2017 21:59

homeassistant added platform: automation.numeric_state cla-signed labels Oct 24, 2017

Remove lint

a84956a

pvizeli requested changes Oct 24, 2017

View reviewed changes

emlove added Hacktoberfest and removed Hacktoberfest labels Oct 25, 2017

Update numeric_state.py

2926d0f

homeassistant added the cla-signed label Oct 25, 2017

pvizeli approved these changes Oct 25, 2017

View reviewed changes

pvizeli merged commit 61ccbb5 into home-assistant:dev Oct 25, 2017

balloob mentioned this pull request Nov 2, 2017

0.57 #10293

Merged

aronsky mentioned this pull request Nov 28, 2017

numeric_state broken #10836

Closed

home-assistant locked and limited conversation to collaborators Mar 2, 2018

ghost added integration: numeric_state and removed platform: automation.numeric_state labels Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fire numeric_state action when first state change matches criteria #10125

Fire numeric_state action when first state change matches criteria #10125

amelchio commented Oct 24, 2017

pvizeli commented Oct 24, 2017 •

edited

Loading

pvizeli left a comment

pvizeli Oct 24, 2017

amelchio Oct 24, 2017

pvizeli Oct 25, 2017

balloob Oct 25, 2017

amelchio Oct 25, 2017

pvizeli Oct 25, 2017

amelchio Oct 25, 2017

balloob Oct 25, 2017

amelchio commented Oct 24, 2017

pvizeli commented Oct 25, 2017

amelchio commented Oct 25, 2017

pvizeli commented Oct 25, 2017 •

edited

Loading

balloob commented Oct 25, 2017

amelchio commented Oct 25, 2017

pvizeli commented Oct 25, 2017

aaronwolen commented Oct 25, 2017

pvizeli commented Oct 25, 2017

pvizeli commented Oct 25, 2017

amelchio commented Oct 25, 2017

aaronwolen commented Oct 25, 2017

pvizeli commented Oct 25, 2017

pvizeli commented Oct 25, 2017

amelchio commented Oct 25, 2017

Fire numeric_state action when first state change matches criteria #10125

Fire numeric_state action when first state change matches criteria #10125

Conversation

amelchio commented Oct 24, 2017

Description:

Checklist:

pvizeli commented Oct 24, 2017 • edited Loading

pvizeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amelchio commented Oct 24, 2017

pvizeli commented Oct 25, 2017

amelchio commented Oct 25, 2017

pvizeli commented Oct 25, 2017 • edited Loading

balloob commented Oct 25, 2017

amelchio commented Oct 25, 2017

pvizeli commented Oct 25, 2017

aaronwolen commented Oct 25, 2017

pvizeli commented Oct 25, 2017

pvizeli commented Oct 25, 2017

amelchio commented Oct 25, 2017

aaronwolen commented Oct 25, 2017

pvizeli commented Oct 25, 2017

pvizeli commented Oct 25, 2017

amelchio commented Oct 25, 2017

pvizeli commented Oct 24, 2017 •

edited

Loading

pvizeli commented Oct 25, 2017 •

edited

Loading