gazoscalvertos / Hass-Custom-Alarm

Yet another take on a home assistant custom alarm
222 stars 96 forks source link

mqtt and custom alarm can create inconsistent state with retained messages #81

Closed adipose closed 5 years ago

adipose commented 5 years ago

Describe the bug If mqtt (via mqtt alarm control panel app, for example) is used to arm the alarm, and then the custom alarm interface is used to disarm the alarm, on the next restart of home assistant, it will rearm the alarm.

To Reproduce Steps to reproduce the behavior:

  1. Set up mqtt and mqtt alarm control panel app
  2. Arm alarm via control panel app
  3. Open home assistant and disarm using the keypad in the custom alarm interface
  4. Restart home assistant.
  5. Alarm will arm

Expected behavior Alarm should not arm since it was last disarmed

adipose commented 5 years ago

Checking the mqtt messages, I can see that a retained message is created on the command topic of "ARM_AWAY." After disarming the state topic is updated to "disarmed" but the command topic does not change. As a result, when resubscribing after a restart, the hca sees a "new" command message and arms the alarm again.

I updated the code to clear the message after receiving a new command. I will test when my wife isn't home to yell at me for tripping the alarm again. :-)

akasma74 commented 5 years ago

That's exactly what I'm saying. Tell me how exactly these mqtt commands are generated as I don't understand this bit at all:

  1. Set up mqtt and mqtt alarm control panel app
adipose commented 5 years ago

https://play.google.com/store/apps/details?id=com.thanksmister.iot.mqtt.alarmpanel&hl=en_US

It can be used via a tablet to send mqtt commands to home assistant alarms.

akasma74 commented 5 years ago

Well, now remove all retained messages and publish the very same message without a retain flag to the alarm panel to see if the bug is still there and report back.

adipose commented 5 years ago

I think that would work. In the app, however, there is no option to retain or not. You might call it a bug on the app, but the app may be assuming that for mqtt alarm it is 100% controlled by the app, so a retained message is no problem.

The problem comes because hca is hybrid, and can be updated via mqtt or otherwise.

akasma74 commented 5 years ago

Can you post here your mqtt log covering arm/disarm period?

adipose commented 5 years ago
MQTT v3.1/v3.1.1 Broker.
New connection from 1.2.3.4 on port 1883.
New client connected from 1.2.3.4 as phonepanel (c0, k60, u'mqtt').
No will message specified.
Sending CONNACK to phonepanel (0, 0)
Received SUBSCRIBE from phonepanel
        phonepanel/command (QoS 0)
phonepanel 0 phonepanel/command
        home/alarm (QoS 0)
phonepanel 0 home/alarm
Sending SUBACK to phonepanel
Sending PUBLISH to phonepanel (d0, q0, r1, m0, 'home/alarm', ... (disarmed))
New connection from 1.2.3.4 on port 1883.
New client connected from 1.2.3.4 as phonepanel (c0, k60, u'mqtt').
No will message specified.
Sending CONNACK to phonepanel (1, 0)
New connection from 4.3.2.2 on port 1883.
Received PUBLISH from phonepanel (d0, q1, r1, m5, 'home/alarm/set', ... ARM_HOME)
Sending PUBACK to phonepanel (Mid: 5)
New connection from homeassistant on port 1883.
New client connected from homeassistant as homeassistant (c1, k60, u'mqtt').
No will message specified.
Sending CONNACK to homeassistant (0, 0)
Received SUBSCRIBE from homeassistant
        home/alarm/set (QoS 0)
homeassistant 0 home/alarm/set
Sending SUBACK to homeassistant
Sending PUBLISH to homeassistant (d0, q0, r1, m0, 'home/alarm/set', ... ARM_HOME)
Received PUBLISH from homeassistant (d0, q0, r1, m0, 'home/alarm', ... (pending))
Sending PUBLISH to phonepanel (d0, q0, r0, m0, 'home/alarm', ... (pending))
Received PUBLISH from homeassistant (d0, q0, r1, m0, 'home/alarm', ... (pending))
Sending PUBLISH to phonepanel (d0, q0, r0, m0, 'home/alarm', ... (pending))
Received PUBLISH from homeassistant (d0, q0, r1, m0, 'home/alarm', ... (disarmed))
Sending PUBLISH to phonepanel (d0, q0, r0, m0, 'home/alarm', ... (disarmed))

I've renamed some of the ids to make it clearer. But in this case I sent an arm command from the "phonepanel" (android phone with mqtt alarm control panel app). As it counts down, pending state is published from homeassistant (hca). Then I disarm from hca. Note that the retained message of "ARM_HOME". It will be read again when I restart home assistant.

adipose commented 5 years ago
            self._mqtt.async_publish(self._hass, self._command_topic, "", self._qos, True)

I've added this line to the script, so that when it receives a command topic, the command topic will be cleared immediately after. This will clean up any retained commands so they aren't processed again. I think it should be safe as it should be the only one listening for the command.

The fundamental disconnect here seems to be that the control panel thinks mqtt alarm is listening (in hass), and it (mqtt alarm control panel) is the controlling keypad. As a result it sends retained command messages and those messages will be available when hass reboots (to prevent a reboot losing alarm active). The problem comes when hca clears the alarm but leaves the retained command, which will be active after next restart.

HCA does not need the retained messages if persistence is enabled. But if it is disabled, my fix would cause the alarm state to be lost. But a retained "arm" message is a bit of a hack itself for keeping state through reboots.

akasma74 commented 5 years ago

Well, it's not for me to decide, but apparently the root of your problem is that you use that "phonepanel" to control this alarm panel. I did not see it as "recommended to use" in Readme or elsewhere. The fact it can send MQTT commands doesn't make it the right tool for the job. And you can tweak the alarm panel code to find workarounds, but who knows what else hides in that app and will be introduce to it in future as it has no relation to this project.

My opinion is there is no bug on this side as everything works using mqtt commands properly (i.e without retain flag). If you prefer to use that "phone panel" interface, you do it on your own risk and there is no guarantee.

p.s If it helps, I use that WallPanel on my wall-mounted tablet and have no issues, but I don't have MQTT enabled (don't know any reason to have it enabled)

adipose commented 5 years ago

Yes, I tend to agree with your assessment. And I've asked that other dev for some help. The reason he is retaining the state is because traditional hass doesn't store the alarm state for the mqtt alarm. So he retains it via the message. I've asked to have that be an optional parameter.

However, since it is a popular way to control hass alarms, and hca behaves similar to the "official" hass mqtt alarm, it would be good if hca mitigated that behavior as I did. But the bug is not in hca.

akasma74 commented 5 years ago

So basically that optional parameter can be on either side, here or on the app. Therefore it's more like feature request, isn't it? ;)

I personally feel against using tools in a way they are not designed to be used as it might create issues that are very difficult to debug (as we can see here). That's why I wouldn't follow the trend and behave like the "official" mqtt alarm (there are many design flaws I hope they will fix eventually, no need to support every single one). It's enough to note that difference to potential users ;)

adipose commented 5 years ago

Agreed. It makes the most sense as an optional setting on the Android app. But for historical info, it's good to know that if you switch from the mqtt alarm to the hca, this behavior is different--not wrong, but different and incompatible with the android app.

The reason I use that app is it is much more controllable than the hca interface. I don't provide the full hass interface to my touch screens as it's too powerful. I just have a hadashboard and the alarm control panel.

adipose commented 5 years ago

The option exists in the Alarm Panel app now, which solves the issue. However, I would still argue the custom alarm, to be most robust, should update the mqtt command topic to reflect what it has done (in my fix, I cleared it for worry of a command topic recursion). Since hca only sees the retained message every time it restarts, it is a very weird behavior--it believes it's unlocked but there is a "pending" command out there that will be processed on restart.

Bottom line is hca is behaving as a topic reader when it comes to the commands, but a topic writer only when it comes to the state.