WIP: detect old attribute values

[dlashua]

This addresses #73 .

This may not be correct, but limited testing shows it works for me.

Old attribute values are available to state trigger as binary_sensor.test.old.attribute_name.

I've also added change to func_args to indicate that a change was detected.

@state_trigger takes kwarg only_on_change (default: True)

I will continue to update this PR as I test.

[dlashua]

This test code seems to do all the right things:

@time_trigger('startup')
def set_test_state():
    task.unique('set_test_state')
    while True:
        state.set('binary_sensor.test', 'on')
        task.sleep(15)
        state.set('binary_sensor.test', 'off')
        task.sleep(15)

@time_trigger('startup')
def set_test_attrib():
    task.unique('set_test_attrib')
    cnt = 0
    while True:
        cnt = cnt + 1
        if cnt > 10:
            cnt = 1
        state.set('binary_sensor.test', cnt=cnt)
        task.sleep(1)


@state_trigger('binary_sensor.test != "never_happens"')
def report_state_always(**data):
    log.info(data)

@state_trigger('binary_sensor.test.cnt != "never_happens"')
def report_attrib_always(**data):
    log.info(data)

@state_trigger('binary_sensor.test == "on" and binary_sensor.test.old == "off"')
def report_state_on_from_off():
    log.info('Is ON, was OFF')

@state_trigger('binary_sensor.test.cnt == 5 and binary_sensor.test.old.cnt == 4')
def report_cnt_5_from_4():
    log.info('CNT is 5, was 4')

@state_trigger('binary_sensor.test')
def report_state_by_eid(**data):
    log.info(data)

@state_trigger('binary_sensor.test.cnt')
def report_attrib_by_eid(**data):
    log.info(data)

[dlashua]

@craigbarratt I don't know why these two tests are failing. I don't even know that it's related or how to tell what went wrong. Can you shed some light. (my knowledge of python tests is limited)

[dlashua]

An issue of sorts.

This code...

@state_trigger('binary_sensor.test')
def report_state_by_eid(**data):
    log.info(data)

... before this PR meant to trigger on ANY change (state OR attribute) to binary_sensor.test. Now, with this PR, without adding only_on_change=False it only triggers if the actual STATE changes. This change will probably break some existing scripts.

My thinking is, if a user is doing a comparison to the state, they likely only care about a change in state in the majority of cases. If a user specifies an attribute (binary_sensor.test.some_attrib) then it is highly likely that they only care about a change in the attribute. But, an entity_id without an attribute and no comparison... they could mean that the only want to know about changes to state, or that they want to know about changes to the state or ANY attribute. In my own code, I've used it to mean both in different cases.

We need something like binary_sensor.test.*, except that's ugly, to mean any change to any attribute or state, but, ideally, still only if it CHANGED (unless only_on_change=False is provided, of course.).

Thoughts?

[dlashua]

Another option, which will create another possible collision in the binary_sensor.test namespace.

@state_trigger('binary_sensor.test') means trigger on any change to state OR any attribute. func_args will still just show binary_sensor.test and it's state for value and old_value.

@state_trigger('binary_sensor.test.some_attrib') means trigger on a change to that one attribute. func_args will show the attribute and the old and new values of the attribute.

@state_trigger('binary_sensor.test.state') means only trigger on a change to STATE. func_args will show binary_sensor.test as it does now.

So, we trigger on ANY change unless the user specifically indicates (via .state) that they only want to check the state.

Or... we add another kwarg of "state_only" or something to mean just the state.

[craigbarratt]

For some reason I thought that the state_changed event didn't have the new and old state attributes, but indeed it does. That's really good news.

I agree that @state_trigger shouldn't evaluate the expression if some unmentioned attribute changes.

I'd recommend we do the following. The @state_trigger expression is only evaluated if variables that are mentioned in the expression (whether its value domain.entity, attribute like domain.entity.attr, or virtual ones like domain.entity.old) have changed. That seems the most natural approach, and solves the current issue. Since we keep track of which state variables or attributes are mention in the trigger expression, the notify code in state.py could do the comparisons to see what has changed.

I'd prefer the any-change trigger @state_trigger("domain.entity") to mean only any change to the value, not attributes. While that would be a breaking change, I doubt many people are depending on that triggering on attribute changes right now. But I could be convinced otherwise - perhaps it could be a kwargs flag that defaults to the old behavior as you've done in #76. We could also support @state_trigger("domain.entity.attr") to mean any change to that attribute.

That's leaves a couple of open questions:

• how to specify an any-change trigger. It could be a kwarg option as just mentioned, some virtual attribute or a different decorator. I haven't settled on the preferred method here - maybe the kwarg approach is best.
• we now have an issue that a trigger could involve multiple variables changes if there are multiple attributes that change and the trigger expression mentions them. What should we set the function arguments var_name, value and old_value to in that case?
• do we support old attributes in the trigger expression, of the form domain.entity.old.attr? I'd say yes.

[craigbarratt]

One way to generalize and address the 2nd bullet (what to pass as value to the trigger function) is to make state variables evaluate to class instances that do the right thing, rather than fake them as being strings, but attributes still work.

This addresses a comment you made in #30:

    log.info(hasattr(input_boolean.test_1,'editable'))  # False
    log.info(input_boolean.test_1.editable)  #True

If we support that, it would be backwards compatible (the instance on its own would magically evaluate to a string), but would actually be an instance that provides attributes etc.

Then value and old_value could be the trigger state variables (no matter whether the trigger happened due to an attribute change or a value change). Inside the trigger function things like value.attr or old_value.attr would do the right thing, and would be based on the actual trigger state change, and not be subject to a race condition if the current attributes are fetched (which might have changed since the trigger), which is what happens now (and old attributes aren't currently available at all).

Then the @state_trigger function would be passed domain.entity and domain.entity.old set the real state objects, with their exact value and attributes at and before the trigger, rather than just the strings currently with attribute accesses falling back to fetching current values, which is prone to race conditions.

[dlashua]

As this PR is now, it works as you describe, minus the "state variables are class instances" bit. A state trigger for binary_sensor.test will only trigger if the value changes. A state trigger for binary_sensor.test.cnt will only trigger if that attribute changes. And the func_args have var_name set to the state variables or the attribute variable based on the trigger, with value and old_value being set as expected.

Making "state variables class instances" gets into the tricky parts of pyscript code I don't fully understand yet (the Ast bits). So I'm not quite qualified to do that if that's the direction you want to take.

[craigbarratt]

I'm also not yet sure how to make "state variables class instances". It's quite tricky and I need to think about it some more. If it's possible I think that's the way to go, although it's secondary to the bigger issue of only checking the state trigger when a referenced value changes. It does make the value and old_value arguments work in a more elegant way, and would simplify the implementation.

The PR definitely got me thinking about how to do this. Currently, however, it potentially calls State.update() multiple times - once if the value changes, and once for each attribute that has changed, independent of whether those are used in the @state_trigger. It actually needs to call it just once, so that the trigger function is only evaluated once. The logic about whether a referenced state variable or attribute has changed (ie, the ones mentioned in the @state_trigger) needs to move to the notify logic in state.py.

Let me think about the "state variables class instances" some more.

[dlashua]

Ah ha. My test code was too narrow to see if the trigger function was being called more than once.

I'll see if I can make these changes. Thank you!

[dlashua]

This PR essentially makes entity attributes act like entity states. Effectively, it issues its own version of a state_changed event for every state AND attribute. I think having a state change for every attribute is desirable. In fact, I wish Home Assistant did that already on its own. It would certainly elimate the tons of "feature requests" they've gotten for things that don't support attributes in the same way they support states.

Native Home Assistant Automation TEMPLATE Triggers don't have this issue because they approach the problem differently. If I write a Native Automation with two separate (NON TEMPLATE) triggers -- one for binary_sensor.test and one for binary_sensor.test.cnt -- it WILL trigger twice, by design.

However, if I instead use a single HASS Template Trigger in my automation, it would only fire once. This is because the HASS Automation keeps track of the True/False state of the Template Trigger, and only triggers if it changes from False to True. Future state_changed events that STILL evaluate to True do not cause a subsequent firing of the automation. With this in place, it doesn't matter if the changes come in one state_changed event or two, because the True/False evaluation of the Template will still only change once.

Personally, I PREFER the way pyscript works. I want it to trigger every time there is a change (not just when the False to True line is crossed). I can decide in my function if I want to do anything about that change. I can even keep track of False/True if I want, and only take action when it changes from False to True.

Something like a kwarg of "only_once=True" on state_trigger could invoke this False to True trigger logic if you wanted that to be in pyscript. Users can already do this themselves in code, but it wouldn't hurt to do it for them. I think it's common enough in general that it'd be nice to have, but not common enough for me personally that it's any trouble to just do it myself when I need it.

As it stands now, a state_trigger is, essentially, this:

@state_trigger('binary_sensor.test == "on" and int(sensor.cnt) > 5')

- trigger:
    platform: state
    entity_id:
      - binary_sensor.test
      - sensor.cnt
  condition:
    platform: template
    value_template: "{{ is_state('binary_sensor.test', 'on') and states('sensor.cnt')|int > 5 }}"

Though I haven't tested it, I imagine the above Home Assistant Automation would also trigger twice if the changes came close enough together.

For me, pyscript has the perfect blend of functionality and terseness of expression. The things I do most often are the most terse to write, the things I need less often are a bit more verbose but still possible. And having the trigger fire twice isn't a problem since I can account for it in my trigger function.

All of this to say, I think the trigger function running twice in race conditions, while not desirable, is okay. And it's happening now anyway without this PR so, while this PR doesn't fix that problem, it also doesn't make it much worse. If someone NEEDS to have just ONE trigger for any given entity this syntax is available:

@state_trigger('binary_sensor.test', only_on_change=False)

[dlashua]

We can pass all of "old_state" and "new_state" from state_changed into the func_args. This would give users the ability to see what the old attribute value was in their code by doing:

@state_trigger('binary_sensor.test')
def a(**data):
  log.info(data['old_state'].attributes['cnt'])

"value" and "old_value" in func_args becomes redundant because the data is available in "old_state" and "new_state" as well.

But this would resolve the issue of not knowing what the old attribute value was inside the trigger function. This isn't the issue mentioned in #73, but is still a problem pyscript users have had trouble navigating.

[dlashua]

I can make the call to State.update() only happen once per state_changed event, but State.update() breaks that into several queue.put()s anyway, so the end result would still be, potentially, multiple trigger function fires. And with a single call to State.update() we lose the ability to customize func_args for the values of the attribute.

If we go back to only notifying on state variables (domain.entity), we'd get back to only one trigger function fire per state_changed event. And users could still still test for binary_sensor.test.old.cnt in the state_trigger because we could pass those variables in to State.update(). And a check before the trigger function fired could make sure that SOMETHING changed (state or attribute) but we'd still see trigger function fires when an attribute changed when the user only cares about the state.

If we want to limit trigger function fires to only when the thing we care about actually changes value, then the most straight forward solution seems to be to notify on each attribute.

[dlashua]

Perhaps we can call State.update() only once. We include all of old_state and new_state in the notification. Then trigger.py can compare them all, determine what changed, look at the variables in state_trigger and build func_args on its own based on what the user cares about. However, if I have @state_trigger('int(sensor.test) > 5 and sensor.test.cnt > 5 and sensor.test.other > 5') and all three values change in the same state_changed event, which one would we trigger on?

Perhaps we always trigger with var_name = 'sensor.test', even if it was an attribute that changed?

We could include a dict like this in func_vars:

changes:
  sensor.test:
    old: 4
    new: 5
  sensor.test.cnt:
    old: 3
    new: 7
  sensor.test.other:
    old: 4
    new: 15

And we only trigger the function if they mentioned that specific thing in state_trigger. So a change to sensor.test.cnt with state_trigger('sensor.test') would not call the function.

[dlashua]

replaced by #82