Bug with multiple state-triggers

[Beiri22]

I just tried to use multiple state triggers as described with one decorator:

@state_trigger("binary_sensor.bad_bewegung=='on'","binary_sensor.wohnzimmer_bewegung=='on'", "person.rico == 'home'")

But, as long as the third trigger is included (which is always true, but should not trigger, because it does not change), the function triggers on every binary_sensors change, not only when changing to "on" (as stated)

binary_sensor.wohnzimmer_bewegung: {'trigger_type': 'state', 'value': 'on', 'old_value': 'off', 'context': Context(user_id=None, parent_id=None, id='466747cb06fdd2a162b1813d5e60dcce')}
binary_sensor.wohnzimmer_bewegung: {'trigger_type': 'state', 'value': 'off', 'old_value': 'on', 'context': Context(user_id=None, parent_id=None, id='24eeef4f530f9b24657ad7b4205d3c32')}

[craigbarratt]

This is correct behavior, even if unexpected.

@state_trigger combines all the different expressions into a single expression by using any (which logically "ors" the expressions together). The combined expression is evaluated each time any of the variables changes (it doesn't evaluate each expression separately just when those variables change). Since the expression aways evaluates to True (because of the last condition), the unexpected effect is it will trigger anytime any of the variables change.

There are various solutions:

• You could move the 3rd condition to @state_active, since it seems to be more an "enable" condition rather than a trigger.
• Since the third term seems to be more an "and" rather than an "or", you could rewrite the expression like this:

  @state_trigger(
      "binary_sensor.bad_bewegung=='on' and person.rico == 'home'",
      "binary_sensor.wohnzimmer_bewegung=='on' and person.rico == 'home'"
  )

  which is similar to, but not the same, as using @state_active (since this way would also trigger when person.rico changes to "home" provided either sensor is currently "on").
• You could also consider setting state_hold_false to 0, which makes the trigger condition edge triggered (ie, it only triggers when going from False to True). But in your case that won't help since the condition is always True

[Beiri22]

These are three separate triggers: Light should start when either the sensors detect motion or when I come home. I mean, if I'd like to evaluate all my triggers in one expression, I would write them as one expression. I thought that was the benefit of offering multiple parameters to state_trigger...

[craigbarratt]

Hmm, good point. Perhaps they should be implemented as separate triggers, although that would be a breaking change...

[Beiri22]

You could allow the use of multiple state-trigger-decorators for one function. This would be backwards compatible and very flexible. I don't know if python has any restrictions on that...(?)

[craigbarratt]

I just pushed a change 7171c4f that allows multiple state/time/event/mqtt trigger decorators per function. I agree that was the cleanest way to support multiple independent triggers, and it allows optional parameters like state_hold to be set independently in each @state_trigger.

Each @state_trigger continues to support multiple arguments that are or'ed together, which keeps it backward compatible.

[Beiri22]

That sounds good. Let me introduce a further idea. What do you think of the possibility to pass a specific payload to the trigger-definition that will be passed as parameter to the function. See the example:

@state_trigger("binary_sensor.bad_bewegung=='on'", data=240)
@state_trigger("binary_sensor.wohnzimmer_bewegung='on'", data=120)
@state_trigger("person.rico == 'home'", data=720)
def nightligh(data):
     # ...
     task.sleep(data)
      #...

In this case the payload would be the amount of seconds the nightlight should stay on; but as arbitrary object it would pose an easy way to pass trigger-specific data to the function. It would need to be implemented on every type of trigger, but in my eyes that could be a very useful feature.

[dlashua]

If this "payload" feature is going to happen, I think it would be best to either use ONE kwarg named the same as the function signature (i.e. call it payload in the trigger and payload in the function def line) OR to gather all kwargs that the trigger doesn't use and pass them back as the same kwarg name. In the case of the latter, if someone mistypes, for instance, state_false_holde, there would be no error/exception and it might be confusing (AppDaemon works like this and it causes confusion at times).

That being said, I find it just as clean to do this:

@state_trigger("binary_sensor.bad_bewegung=='on'")
def bad_bewegung_nightlight():
  nightlight(240)

@state_trigger("binary_sensor.wohnzimmer_bewegung='on'")
def wohnzimmer_bewegung_nightlight():
  nightlight(120)

@state_trigger("person.rico == 'home'")
def rico_nightlight():
  nightlight(720)

def nightlight(sleep_secs):
     # ...
     task.sleep(sleep_secs)
     #...

Alternately, if state_trigger is ever a real function and since all functions are already coroutines, this would be nice:

def nightlight(sleep_secs):
     # ...
     task.sleep(sleep_secs)
     #...

state_trigger("person.rico == 'home'", coro=nightlight(720))
#OR
state_trigger("person.rico == 'home'", func=nightlight, args=[720])

[Beiri22]

@dlashua: I first used the name payload; then found out, that mqtt-trigger uses this name already and switched to data. As you found out, I forgot to rename one occurrence. The solution with 3 trigger-functions and one parametrized main funnction is my current implementation. But I'd say, that a version with one function, multiple triggers and a payload per trigger would be more readable...

[craigbarratt]

The trigger function already gets called with several keyword arguments, including the entity name whose change caused the trigger to become True, and its prior and new values. That should allow it to figure out what caused the trigger.

That said, I agree it could be helpful to allow trigger-dependent arguments to be passed to the trigger function. Perhaps optional keyword arguments args and kwargs could be set for positional and keyword arguments (args would be set to a list and kwargs would be set to a dict)? A single hardcoded keyword argument would be easier, but less flexible. If there is a typo in the keyword argument there should be an error if the function's declaration doesn't expect that argument.

[dlashua]

Yes, this sounds ideal.

[Beiri22]

That sounds like a great way to increase the flexibility without any backwards compatibility issues...

[craigbarratt]

Implemented kwargs. I didn't do args since it seemed unnecessary.

[Beiri22]

I don't know, if we should check for naming collisions between kwargs and regular transmitted arguments of the trigger. If I would pass kwargs={"payload": "1"} for an mqtt_trigger which also creates a payload-kwarg -> will one be preferred or does it result in an error?

[craigbarratt]

The optional kwargs setting will override.

[Beiri22]

Ah yes, I just found it in the modified docs, commendable :-)

[Beiri22]

Question: Will you put these changes into a new release soon?

[craigbarratt]

Hopefully I'll do a new release in the next week or so.