Fix notifications being reversed when coming from external lock state changes (Unlock for lock, etc) #492
Conversation
This fix discards state changes without a user_id in handle_lock_state_change. The reasoning is simple - handle_lock_state_change is fired WAY too early during the zwave lock notification process and sees very wrong values for the other elements of zwave lock state, at least with my schlage lock testing. (This is because zwave is actually a whole negotiation - with many different things changing and notifications flying back and forth from the lock to HA and back again, and the state change on the lock entity in HA is one of the very earliest in the sequence at least with my Schlage lock). Anyway, the key problem for the notification code is that the state change fires WAY before the zwave _notification_ that the lock has changed state, which is a much more reliable method of getting audit information about the changing state of the lock. Due to this out of sequence behaviour, handle_lock_state_change would fire well before handle_zwave_js_lock_event and the former would make very wrong assumptions based on the transient state the lock is in at the time that event is fired. This change is simple: if the lock state change event _has_ a user ID it has come from HA and NOT zwave, and thus we can act on it (I can't seem to get zwave to send a _notification_ back if we programmatically change the lock state from HA, which is annoying), but if it _doesn't_ have a user ID, it is an event that is part of an external change in state. As such, it's firing WAY too early in this circumstance, and should just be ignored - wait for the real zwave notification event instead.
cpw
changed the title
| return | ||
|
|
||
| # Ignore events without a user_id - they're from zwave - let handle_zwave_js_lock_event handle it | ||
| if event.context.as_dict()["user_id"] is None: |
There was a problem hiding this comment.
Shouldn't this be event.data.get("user_id") ?
There was a problem hiding this comment.
The user_id isn't part of the data member of the event? As far as I can tell, it's only part of the context, which is a separate member.
There was a problem hiding this comment.
| if event.context.as_dict()["user_id"] is None: | |
| if event.context.get("user_id") is None: |
then we can do this instead.
There was a problem hiding this comment.
I don't believe we can - as far as I can tell, the context object does not implement a Dict interface directly. Which is why I did as_dict() to get a dict out of it. As far as I can tell, it implements basically NO API at all. https://github.com/home-assistant/core/blob/7eb0f2993f25deba1fba9e4dcbcc8d13cf574482/homeassistant/core.py#L1243 - perhaps directly fetching the member would work?
| return | ||
|
|
||
| # Ignore events without a user_id - they're from zwave - let handle_zwave_js_lock_event handle it | ||
| if event.context.as_dict()["user_id"] is None: |
There was a problem hiding this comment.
| if event.context.as_dict()["user_id"] is None: | |
| if event.context.get("user_id") is None: |
then we can do this instead.
|
Having run this code for a few days for myself, there's an issue that probably need thinking about. Autolocks don't quite work properly - the autolock has no user_id so the event gets dropped based on this code. A few moments later, you get a "sync" event instead where keymaster resynchronizes its state because the lock did lock, KM just didn't get the event because this filtered it out. Perhaps, adding a pending "we're locking" flag in the Coordinator object might work, from the timer trigger. Overall, this is just a cursed code path - the fact that you don't get ZW notifications when you trigger the lock event locally, and you can't filter based on REMOTE/LOCAL because the zwave code doesn't set the REMOTE event state for any of it's events is mildly frustrating. |
2 participants
Symptom
You have this issue if you notice that lock events from the lock are processed as their opposite in keymaster - you get Door unlock notifications when you lock the door, for example.
Breaking change
It should not be a breaking change unless the lock doesn't send any zwave notification messages when it is operated, which probably means it's not going to work very well with this system anyway (and is probably pretty broken in HA in general in that case).
Proposed change
This fix discards state changes without a user_id in handle_lock_state_change. The reasoning is simple - handle_lock_state_change is fired WAY too early during the zwave lock notification process and sees very wrong values for the other elements of zwave lock state, at least with my schlage lock testing. (This is because zwave is actually a whole negotiation - with many different things changing and notifications flying back and forth from the lock to HA and back again, and the state change on the lock entity in HA is one of the very earliest in the sequence at least with my Schlage lock).
Anyway, the key problem for the notification code is that the state change fires WAY before the zwave notification that the lock has changed state, which is a much more reliable method of getting audit information about the changing state of the lock, such as what happened and who caused it. Due to this out of sequence behaviour, handle_lock_state_change would fire well before handle_zwave_js_lock_event and the former would make very wrong assumptions based on the transient state the lock is in at the time that event is fired (usually, it would emit a notification event based on the prior state of the lock - so a Lock event from the lock would fire a notification as an Unlock event, because none of the sensors containing the lock state information had been updated except the lock entity itself.
This change is simple: if the lock state change event has a user ID it has come from HA and NOT zwave, and thus we can act on it (I can't seem to get zwave to send a notification back if we programmatically change the lock state from HA, which is annoying), but if it doesn't have a user ID, it is an event that is part of an external change in state. As such, it's firing WAY too early in this circumstance, and should just be ignored - wait for the real zwave notification event instead.
Something else to note - events have a whole LOCAL/REMOTE thing, but zwave events are ALL local all the time, so you can't reliably use that to detect if the event came from the lock or from a user in HA. (That is probably a bug in the zwave stuff - I would expect an event triggered from a zwave action outside HA to carry the remote flag, but ho hum).
Type of change
Additional information
Thing to note: This issue slightly caused because I configured the Alarm Level/User Code and Alarm Type/Access Control sensors for my locks. It is also worth noting that it seems that zwave locks are pretty weird in general in HA as far as I can tell. There might be better ways to handle this situation - but, at least for me, locally, this fix has completely fixed notifications that I care about from my locks.