home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
71.15k stars 29.82k forks source link

numeric_state trigger "below" acting like "below or equal" #7170

Closed tedstriker closed 7 years ago

tedstriker commented 7 years ago

HA: 0.42.4 Python: 3.4.2 Raspberry Pi, Jessie (Debian 8.0) automation/ numeric_state trigger

Description of problem: The sensor sensor.people_counter has the state value of 1 and when there's movement sensed, the notification action is executed. I had a look at the numeric_state test cases and 10 is below 10 is not being tested yet, but I don't know if this would be a necessary test at all.

Expected: It is expected that the below constraint is behaving like < and not like <=

Problem-relevant configuration.yaml entries and steps to reproduce:

# Notify if there's movement and nobody's at home
alias: "notify_movement_empty_home"
trigger:
  - platform: state
    entity_id: binary_sensor.movement
    state: 'on'
condition: and
  conditions:
  - condition: numeric_state
    entity_id: sensor.people_counter
    below: 1
action:
  - service: notify.pushover
    data:
      title: 'Warning'
      message: 'Movement in empty house'
  1. have binary sensor to indicate movement
  2. have a threshold calculating sensor (in this case it was a template sensor)
  3. use the given automation
  4. see it triggered even when the sensor people_counter has a value of 1
  5. the action is not executed if the value is 2
point-4ward commented 7 years ago

According to the docs this is default behavior, so if you want < 10 you should put 9 as the argument.

I think if this is changed now it will affect too many people's automations that rely on the current model.

tedstriker commented 7 years ago

The docs actually say its just below and/or above a certain value. At least the one I found: https://home-assistant.io/docs/automation/trigger/ (look for numeric state). Depending on what value range one is trying to catch, it is or is not desirable to make it behave like a "below or equal". If I want to catch a range of e.g. [16-32] I probably would want to include 16 and 32. But if I want to trigger on values less than 16 or greater than 32 it is likely that 16 and 32 are excluded. To have the ultimate degree of freedom, we actually need both. Not only <= and >= but also < and > below: < above: > and, to make a proposal for the most flexible way I can think of right now, equal could be an additional comparator, which is quite self explanatory. Combine them if necessary. equal: =

it could go like this:

# Notify if temperature is out of range
alias: "notify_temp_out_of_range"
trigger:
  platform: event
  event_type: STATE_CHANGED
  entity_id: sensor.temp
condition:
  condition: numeric_state
  entity_id: sensor.temp
  above: 32
  equal: 32
  equal: 16
  below: 16
action:
  service: notify.pushover
  data:
    title: 'Warning'
    message: 'Temp is out of range!!'
point-4ward commented 7 years ago

https://home-assistant.io/docs/scripts/conditions/

That's where it is explained in the docs, somebody in the know will have to give an opinion on the additional argument you have suggested.

tedstriker commented 7 years ago

You are right. I looked at the trigger docs instead of the condition ones - obviously they differ. I'd really appreciate if someone could give a thought or two.

terba commented 7 years ago

I came here to file the same issue. I think it should be corrected, below means below, above means above. Configuration of such a software should be intuitive, looking up every primitive keyword in the docs is a nonsense. It would be even better to rename keywords to random strings, so everybody has to check the docs for everything, but they will not create faulty automations :)

I understand that it "will affect too many people's automations that rely on the current model.", but this is a bug. If it gets not corrected, it will make a lot of confusion in the future for a lot of people. I think the best solution would be to include the fix in a major release and mention it in the release notes.

point-4ward commented 7 years ago

@balloob - when this is integrated in to a release can it be marked as a breaking change please?

Reason I ask:

I currently have a number of automations using the haveibeenpwned sensor, the alert is set to go off when the sensor reading is above: 1 which is compliant with the current format. This will need changing to above: 0 for the new format, and if I don't change it my email addresses could be compromised without the alert sounding, however if I change it now I will get false alerts.

I suspect that a number of people will be in this boat for one reason or another. I appreciate it's not an actual breaking change, but it is important that people know to change this number if it is critical to their automations/scripts/etc, and I only know about it because I commented in this thread, many other users won't.