Koenkk / zigbee2mqtt

Zigbee 🐝 to MQTT bridge 🌉, get rid of your proprietary Zigbee bridges 🔨
https://www.zigbee2mqtt.io
GNU General Public License v3.0
12.15k stars 1.68k forks source link

What is the correct way to utilise auto-discovered buttons? #959

Closed mihalski closed 5 years ago

mihalski commented 5 years ago

The example shows the use of MQTT triggers but shouldn't that be redundant for auto-discovered devices?

Previously this format of automation worked for me but sometime recently that has changed.

- alias: Family Room TV Subtitles
  trigger:
    platform: state
    entity_id: sensor.family_room_button
  condition:
      condition: state
      entity_id: sensor.family_room_button
      state: 'long'
  action:
    service: remote.send_command
    entity_id: remote.harmony_hub
    data:
      command:
        - Subtitle
      device: 38539492

Now all button automations triggered by state trigger twice. I considered using to: in state but this fails when the same button presses are triggered as previously.

Am I doing something wrong? Has Home Assistant changed something about the way it handles these automations?

I am using Home Assistant 0.86.3 and zigbee2mqtt 1.0.1.

Koenkk commented 5 years ago

I indeed would like to get rid of the current MQTT triggers and just use the corresponding HA entities. Could you try playing with force_update and expire_after (https://www.home-assistant.io/components/sensor.mqtt/). This should fix the issue with: I considered using to: in state but this fails when the same button presses are triggered as previously.

mihalski commented 5 years ago

That would require shifting to manually specified MQTT topic based triggers no? The interesting thing is that this affects the Xiaomi buttons but no the motion or door/window sensors. I don't know if this is because they are sensors instead of binary_sensors but that's the only difference I can see.

Koenkk commented 5 years ago

@mihalski the problem with the buttons is that the state doesn't change (single -> single)

mihalski commented 5 years ago

In my testing that is only an issue when you don't use a condition that checks the state and instead use to: 'single' under the state trigger. When your trigger is the state a (single -> single) "change" triggers a check of the condition and it works as intended.

Problem I am having is that the condition triggers TWICE from a single press (and single MQTT publish).

screen shot 2019-01-30 at 4 56 27 pm

subzero79 commented 5 years ago

This is correct, i don't know why this is happening. Yesterday my whole network went down so i had to reflash and repair all my devices. After upgrading the zigbee2mqtt to the latest dev commit i realised the click automatons in my double and single wireless wall xiaomi switches (first gen, no long or double hw click) weren't working.

I've recently implemented a nodered flow to have single, double and triple click on those wall switches by using the ha click sensors pushed by zigbee2mqtt discovery. When i started debugging in the state:change node i can see two msg's per click, even if the mqtt sub shows one line topic-payload. At the beginning i thought this was an error in nodered, but connecting to the ha websocket stream api also showed two events per click .

ata: {"event_type": "state_changed", "data": {"entity_id": "sensor.button_desktop_8072_click", "old_state": {"entity_id": "sensor.button_desktop_8072_click", "state": "single", "attributes": {"battery": 100, "voltage": 3062, "linkquality": 76, "click": "single", "friendly_name": "button_desktop_8072_click", "icon": "mdi:toggle-switch"}, "last_changed": "2019-01-30T10:10:50.266755+00:00", "last_updated": "2019-01-30T10:10:50.266755+00:00", "context": {"id": "343c917569c44e10a0cbb5e27fe0dbd6", "user_id": null}}, "new_state": {"entity_id": "sensor.button_desktop_8072_click", "state": "single", "attributes": {"battery": 100, "voltage": 3062, "linkquality": 76, "click": "single", "friendly_name": "button_desktop_8072_click", "icon": "mdi:toggle-switch"}, "last_changed": "2019-01-30T10:10:53.342483+00:00", "last_updated": "2019-01-30T10:10:53.342483+00:00", "context": {"id": "458451d2237b4590b2cf7b41f9fa3acb", "user_id": null}}}, "origin": "LOCAL", "time_fired": "2019-01-30T10:10:53.342522+00:00", "context": {"id": "458451d2237b4590b2cf7b41f9fa3acb", "user_id": null}}

data: {"event_type": "state_changed", "data": {"entity_id": "sensor.button_desktop_8072_click", "old_state": {"entity_id": "sensor.button_desktop_8072_click", "state": "single", "attributes": {"battery": 100, "voltage": 3062, "linkquality": 76, "click": "single", "friendly_name": "button_desktop_8072_click", "icon": "mdi:toggle-switch"}, "last_changed": "2019-01-30T10:10:53.342483+00:00", "last_updated": "2019-01-30T10:10:53.342483+00:00", "context": {"id": "458451d2237b4590b2cf7b41f9fa3acb", "user_id": null}}, "new_state": {"entity_id": "sensor.button_desktop_8072_click", "state": "single", "attributes": {"battery": 100, "voltage": 3062, "linkquality": 76, "click": "single", "friendly_name": "button_desktop_8072_click", "icon": "mdi:toggle-switch"}, "last_changed": "2019-01-30T10:10:53.344018+00:00", "last_updated": "2019-01-30T10:10:53.344018+00:00", "context": {"id": "788961d7d46d456b9efe180e0ce79f7b", "user_id": null}}}, "origin": "LOCAL", "time_fired": "2019-01-30T10:10:53.344044+00:00", "context": {"id": "788961d7d46d456b9efe180e0ce79f7b", "user_id": null}}

Looks like something changed in ha mqtt

subzero79 commented 5 years ago

The problem here is the json_attributes_topic being the same as the state topic. HA docs says should be different, just deleting the directive causes the sensors go back into to normal behaviour.

Koenkk commented 5 years ago

I'm wondering if the automation of the OP is correct at all. Isn't this also triggered when e.g. a new battery percentage is being send?

mihalski commented 5 years ago

To be honest I don't know as that has never happened since I started using zigbee2mqtt. The battery levels remain at 100. And I don't get triggers when there is a regular update that does not include a click.

subzero79 commented 5 years ago

This commit https://github.com/Koenkk/zigbee2mqtt/commit/6912f8108d034b5233b00e834180a7cdf66b339c

should be reverted IMO until is properly implemented

Koenkk commented 5 years ago

@emontnemery can you confirm this?

Koenkk commented 5 years ago

@subzero79 refactoring json_attributes_topic to a different topic will only partially solve this problem. The above automation is also trigger on a zigbee2mqtt restart because the sensor goes from unavailable to online again.

emontnemery commented 5 years ago

@Koenkk any update of a device, including updating attributes, will cause automation to be evaluated. Hence, to not have issues, the automation condition must check state change. I hope this answers the question..

OPs automation trigger is not correct, there should be a from and to state in the trigger to not trigger on attribute change. https://www.home-assistant.io/docs/automation/trigger/#state-trigger

Koenkk commented 5 years ago

@emontnemery thanks for clarifying.

I've submitted a PR to Home Assistant (https://github.com/home-assistant/home-assistant/pull/20716). Once this is accepted we can use the following syntax to respond to button clicks.

- alias: Resond to button click
  trigger:
    platform: state
    entity_id: sensor.my_switch_click
    to: 'single'
  action:
    entity_id: light.my_bulb_light
    service: light.toggle
emontnemery commented 5 years ago

The trigger might still not be correct, it might fire if there is for example a change of battery level within the 0.4 s. I think you need to specify a from: state also. I'm not totally sure about this though. Maybe you can test with a long expire_after.

This was wrong, see below.

emontnemery commented 5 years ago

To be clear, to make sure you can have non ambiguous automation triggers, I think it's better to fire a state update when the button is no longer pressed (if possible).

subzero79 commented 5 years ago

Yes, partially but the second case is likely not to happen as much as normally pressing the button, and according how users have their automations, lights will go on, then off on single click when it shouldn’t. As for now (same as when the project started) solution is to use the mqtt topic.

h4nc commented 5 years ago

The trigger might still not be correct, it might fire if there is for example a change of battery level within the 0.4 s. I think you need to specify a from: state also. I'm not totally sure about this though. Maybe you can test with a long expire_after.

I‘m also not sure about this. But I think setting a certain from state will cause other issues. It could come from several states. E.g. the Aqara double switch has 9 different states. So this would mean setting up 9 different triggers, right? Could come from every of those to „left“. This would make the code confusing and long.

And if you set up 9 of those automations than you will have to add those lines to every automation. So 9 triggers per automation. Would make the code incredibly and unnecessarily long.

emontnemery commented 5 years ago

@h4nc I double checked the code, specifying to: in the state trigger is enough, sorry for giving wrong information. https://github.com/home-assistant/home-assistant/blob/1fe67fb1b0ceb7859be8192bfe9a29fca3f6eb30/homeassistant/components/automation/state.py#L54-L57

mihalski commented 5 years ago

Is some sort of solution going to be possible so that an automation is only triggered once as it had been before support for json_attributes_topic was added? And I mean by using state rather than mqtt.

Koenkk commented 5 years ago

@mihalski probably yes, I'm going to try @emontnemery solution which he mentioned here: https://github.com/home-assistant/home-assistant/pull/20716#issuecomment-460525994.

This should make an automation in the form of

- alias: Resond to button click
  trigger:
    platform: state
    entity_id: sensor.my_switch_click
    to: 'single'
  action:
    entity_id: light.my_bulb_light
    service: light.toggle

possible. Until then, use the MQTT trigger.

mihalski commented 5 years ago

Interesting discussion. Will the solution still cause a double trigger from an automation such as this?

- alias: Resond to button click
  trigger:
    platform: state
    entity_id: sensor.my_switch_click
  condition:
      condition: state
      entity_id: sensor.my_switch_click
      state: 'single'
  action:
    entity_id: light.my_bulb_light
    service: light.toggle

Admittedly just having to: and no condition: is much cleaner but this solution doesn't care what the previous state was, it just checks the current state. If we can't have the cleaner solution I'd settle for the next best thing.

emontnemery commented 5 years ago

@mihalski the to state trigger will only trigger on change of the sensor state, and not trigger if there is an attribute change.

h4nc commented 5 years ago

But this wouldn’t trigger if you use your button to toggle something, right?

Because in this case it would „change“ from single to single. I think this won’t trigger the automation again.

Koenkk commented 5 years ago

@h4nc that's why zigbee2mqtt needs to send a {"click": "idle"} after a {"click": "single"}. (like @emontnemery mentioned here: https://github.com/home-assistant/home-assistant/pull/20716#issuecomment-460525994

mihalski commented 5 years ago

So technically it SEEMS like it would be best to trigger idle instantly after whatever click was made? Home Assistant wouldn't miss the initial trigger would it?

emontnemery commented 5 years ago

@mihalski right, Hass will act on each state change regardless on if they happen back to back or with some delay

Koenkk commented 5 years ago

Latest dev branch allows automations in the form of:

- alias: Resond to button click
  trigger:
    platform: state
    entity_id: sensor.my_switch_click
    to: 'single'
  action:
    entity_id: light.my_bulb_light
    service: light.toggle

thanks @emontnemery for helping with this!

h4nc commented 5 years ago

Thanks!

Could we inform each other in this thread when this is released in the master version too?

I for myself use hassio, so it while take a little.

However basically there is no difference between this solutuon and the mqtt solution, right? It's just a cosmetical issue no?

Koenkk commented 5 years ago

@h4nc yes, it's just cosmetical, should be available for hassio under -edge within an hour.

h4nc commented 5 years ago

Ah, I thought it's something that must be changed in home assistant too. Didn't get that this can be fixed in zigbee2mqtt only.

Thanks @Koenkk, you brought up one of the most useful projects on github (in my opinion).

Edit: After thinking about it again, it makes sense that this is solved at the zigbee side (change state to idle).

BudBundi commented 5 years ago

With the last image the state change is not recognized every time in HomeAssistant, over the MQTT topic no problems

h4nc commented 5 years ago

I also have the newest image.

I changed my automations to the state change and tried them several times.

For me it felt like it works faster or more reliable now. I However, I didn’t notice any problems with it yet.

Koenkk commented 5 years ago

@BudBundi did you also try after https://github.com/Koenkk/zigbee2mqtt/commit/f4bf504642cf7b8a3c71e8fc60e874a0310d9ffe ?

BudBundi commented 5 years ago

@BudBundi did you also try after f4bf504 ?

Yes, with the Docker Image from this morning it always triggers if I press one remote and then another remote. If I press the same remote twice with a half second or a second in between it triggers 1 of 3 times correct, on the MQTT topic every time, so I will stay at MQTT.

HA and Mosquitto in Docker on the NAS, zigbee2mqtt in Docker on a Raspberry Pi 3B+ (the NAS does not recognize the CC stick) all the latest development images.

Koenkk commented 5 years ago

@BudBundi I'm experiencing the same.

The transition seems to be timing critical, meaning that on slower system (e.g. Pi's) it will not always trigger, even with a timeout of 300ms. Even on my Intel nuc it's not 100% reliable.

My conclusion here is that:

h4nc commented 5 years ago

I didn't notice issues. However, if the conclusio is that mqtt is the best way, I agree to remove those _click sensors as they will confuse people and make them use it wrong.

mihalski commented 5 years ago

I would be very sad to lose this functionality and have to go back to explicitly specifying MQTT triggers. Doing so runs counter to the recent initiative of auto-discovery and integration which has helped improve zigbee2mqtt use with Home Assistant immensely. Is there really no way to prevent the state trigger from firing twice?

Koenkk commented 5 years ago

@mihalski no, using the MQTT trigger seems to be the best solution. Also see the responses from balloob (HA founder) here: https://github.com/home-assistant/home-assistant/pull/20716#issuecomment-460367572

mihalski commented 5 years ago

That depends on your definition of "best". If it is "most pragmatic" as I believe balloob tends to default to then yes, otherwise no as it completely negates all the advantages of auto-discovery and integration and reduces the Xiaomi buttons to third class citizens.

The only way an MQTT trigger could be redeemable would be if it could be called using entity_id instead:

- alias: Resond to button click
  trigger:
    platform: mqtt
    entity_id: sensor.my_switch_click
  condition:
      condition: state
      entity_id: sensor.my_switch_click
      state: 'single'
  action:
    entity_id: light.my_bulb_light
    service: light.toggle

It is the requirement to know the MQTT topic that is completely antithetical to the concept of auto-discovery and integration.

P.S. I don't mean any offence and am very grateful for the work that you have done. I'm just trying to make a case for something I believe in.

Koenkk commented 5 years ago

@mihalski I completely agree, and also would prefer to use the home assistant entity_id. What do you think about the following solution?

Support trigger via entity_id, in the form of this, with a limitation that it can only trigger 1 time in 1-2 seconds (because expire_after: 1).

Users wo need more triggers per second can use the MQTT trigger.

To me this seems the best achievable result.

Koenkk commented 5 years ago

The entity can also be used to trigger the automation now. With a side note that it can only trigger 1 time per 1 or 2 seconds.

Updated documentation: https://github.com/Koenkk/zigbee2mqtt.io/blob/dev/integration/home_assistant.md#responding-to-button-clicks

emontnemery commented 5 years ago

@Koenkk There should be no reason why the button can't be pressed more often than every 1-2 seconds. Did you see this comment: https://github.com/home-assistant/home-assistant/pull/20716#issuecomment-460525994?

Koenkk commented 5 years ago

@emontnemery this didn't work, I modified the code that after a single immediately a idle was send. This was not reliable, somehow Home Assistant missed a lot of state changes (even when we put a delay of 300 ms between single to idle). See https://github.com/Koenkk/zigbee2mqtt/issues/959#issuecomment-461013801 and https://github.com/Koenkk/zigbee2mqtt/issues/959#issuecomment-461585947

emontnemery commented 5 years ago

@Koenkk OK, got it! That might be a bug in HA, I'll look into it.

Edit: The root cause why it didn't work is that the state is not copied here: https://github.com/home-assistant/home-assistant/blob/f61f6504954a69edeebf571de4f826cd9c4f72d9/homeassistant/helpers/entity.py#L318-L321, so when the async_update_ha_state is called, fast state transitions are missed.

emontnemery commented 5 years ago

Anyhow, since the recommendation is to use HA Events, HA MQTT discovery could be extended to allow automatic defining of events. Would that be useful?

In that case, instead of doing:

automation:
  - alias: Respond to button clicks
    trigger:
      platform: mqtt
      topic: 'zigbee2mqtt/<FRIENDLY_NAME>'
    condition:
      condition: template
      value_template: '{{ "single" == trigger.payload_json.click }}'
    action:
      entity_id: light.bedroom
      service: light.toggle

The automation would be something like:

automation:
  - alias: Respond to button clicks
    trigger:
      platform: event
      event_type: mqtt_event
      event_data:
        id: zigbee2mqtt_<FRIENDLY_NAME>
        event: single
    action:
      entity_id: light.bedroom
      service: light.toggle
mihalski commented 5 years ago

That sounds like a good alternative to state changes. Maybe make the id just for ease of use?

emontnemery commented 5 years ago

@mihalski But will it be easier to use than just hard coding the mqtt topic as in the first example?

h4nc commented 5 years ago

I can’t see the advantage of this, what difference does it make?

Do we want to avoid the condition?

Koenkk commented 5 years ago

@emontnemery MQTT events would be awesome!!!

mihalski commented 5 years ago

Easier to reference family_room_button (as in entity_id sensor.family_room_button) than zigbee2mqtt_family_room_button... zigbee2mqtt seems more like an implementation issue rather than something you should have to worry about during writing automation scripts.

At least that's my humble opinion.

P.S. I actually can't get long click working with the state to method (everything else works), so I wouldn't mind trying an alternative. Plus using events might bypass the delay requirement?