Set lock status correctly for Schlage BE469 Z-Wave locks

[ahayworth]

PR #17386 attempted to improve the state of z-wave lock tracking for
some problematic models. However, it operated under flawed
assumptions. Namely, that we can always trust self.values to have
fresh data, and that the Schlage BE469 sends alarm reports after every
lock event. We can't trust self.values, and the Schlage is very
broken. But popular, so we should fix it! :)

When we receive a notification from the driver about a state change,
we call update_properties - but we can (and do!) have stale
properties left over from previous updates. #17386 really works best
if you start from a clean slate each time. However, update_properties
is called on every value update, and we don't get a reason why.
Moreover, values that weren't just refreshed are not removed. So blindly
looking at something like self.values.access_control when deciding to
apply a workaround is not going to always be correct - it may or may not
be, depending on what happened in the past.

For the sad case of the BE469, here are the Z-Wave events that happen
under various circumstances:

RF Lock / Unlock:

• Send: door lock command set
• Receive: door lock report
• Send: door lock command get
• Receive: door lock report

Manual lock / Unlock:

• Receive: alarm
• Send: door lock command get
• Receive: door lock report

Keypad lock / Unlock:

• Receive: alarm
• Send: door lock command get
• Receive: door lock report

Thus, this PR introduces yet another work around - we track the current
and last z-wave command that the driver saw, and make assumptions based
on the sequence of events. This seems to be the most reliable way to go - simply asking the driver to refresh various states doesn't clear out
alarms the way you would expect; this model doesn't support the access
control logging commands; and trying to manually clear out alarm state
when calling RF lock/unlock was tricky.

The lock state, when the z-wave network restarts, may look out of sync
for a few minutes. However, after the full network restart is complete,
everything looks good in my testing.

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.

[dshokouhi]

hmm could this possibly fix #11934 ? The code seems like it does since those are the alarm_type I was expecting to get updated.

[ahayworth]

hmm could this possibly fix #11934 ? The code seems like it does since those are the alarm_type I was expecting to get updated.

@dshokouhi Yes and no - the alarm_type doesn't get updated because the BE469 doesn't actually send a zwave alarm/notification command in that case. It makes sense in some respects - why would the lock need to notify the controller about an action that the controller initiated?

This PR updates state and attributes correctly, but doesn't update the underlying alarm type (and thus, the sensor.front_door_lock_alarm_type in your issue). I had mucked around with modifying the actual z-wave alarm states, but results were discouraging. If this were to be merged, you'd need to re-write your automation to something different, like:

• trigger on lock state, and use a template condition for the attributes
• trigger on template, and use your template to assert the desired attributes

...but in a sense, yes, it would fix the problem. 😄

[balloob]

CC @home-assistant/z-wave

[ahayworth]

If there's anything I can do to help out with the review process, let me know. The whole problem and solution can be a little confusing, for sure. :)

[turbokongen]

I'm sorry for the late reply. Life happens.... So what I have been thinking, seeing all the workarounds being made for different lock types and makes:
Would it be better to watch all the locks' value, and if any of them changes, compare to the types available, and set the state based on the changed value, No matter which one changed?
Just a thought

[hoopsta1423]

After manually engaging the lock, the lock status updates appropriately in HA. However, in order to unlock, I need to call lock.unlock twice before the door unlocks. (the same is true going the other way as well)

Any thoughts on why that is?

[ahayworth]

After manually engaging the lock, the lock status updates appropriately in HA. However, in order to unlock, I need to call lock.unlock twice before the door unlocks. (the same is true going the other way as well)

Any thoughts on why that is?

@hoopsta1423 Unless you're using this PR as a custom component, we should probably talk about that on the forums or on an issue. 😄

[hoopsta1423]

After manually engaging the lock, the lock status updates appropriately in HA. However, in order to unlock, I need to call lock.unlock twice before the door unlocks. (the same is true going the other way as well)
Any thoughts on why that is?

@hoopsta1423 Unless you're using this PR as a custom component, we should probably talk about that on the forums or on an issue.

Yup, this PR is a custom component on my HA install. Was having issues after updating to 0.83 and saw your PR so I tried it out. Working so far except for that little quirk

[ahayworth]

@turbokongen

I'm sorry for the late reply. Life happens....

No need to apologize! 😄

So what I have been thinking, seeing all the workarounds being made for different lock types and makes:
Would it be better to watch all the locks' value, and if any of them changes, compare to the types available, and set the state based on the changed value, No matter which one changed?
Just a thought

It's an interesting idea, but:

1. I think for it to be effective, we'd essentially have to track a whole dict of values we care about between messages. We'd also need to ensure we're only updating when we get a lock status or alarm message (ignoring all else that might pop up, like a battery report or something, in between).

2. I worry that we might still need to care about certain models of locks. If we were to implement such a method, we'd still need to know whether or not a certain type of update makes sense to use as a value for a particular model. Some models need just self.values.primary no matter what, some need self.values.access_control, some need self.values.alarm_type, and some use inane combinations of the three.

I do like the idea though - I think it would be a good refactor to the code, and make it a lot more clean to track the current message <-> state mappings, and to track any in the future. I would vote we merge this and #18996 as bug fixes, and work on the refactor. There are a number of reports around the BE468/469 specifically that'd be nice to fix (forums, discord, here, etc). I'd also happily volunteer to do the refactor over the next week or so.

[ahayworth]

@hoopsta1423 - I was able to reproduce it, but it’s not actually a bug. If you manually unlock, then immediately lock, the z-wave command to lock actually times out. A manual unlock actually triggers a flurry of messages on the z-wave network, and since its low-bandwidth, that takes a few seconds to all complete. This PR doesn’t alter that behavior at all.

I got the correct behavior by manually unlocking, counting to 30, then locking via the web UI. It’s slow, but that’s z-wave for ya.

[turbokongen]

Then let's merge this and aim for the refactor later. 👍

[dshokouhi]

Should we tag this for 0.83.4 since its a bug fix? Seems to pretty important since it deals with a security device and its state/control.

[hoopsta1423]

@hoopsta1423 - I was able to reproduce it, but it’s not actually a bug. If you manually unlock, then immediately lock, the z-wave command to lock actually times out. A manual unlock actually triggers a flurry of messages on the z-wave network, and since its low-bandwidth, that takes a few seconds to all work. This PR doesn’t alter that behavior at all.

I got the correct behavior by manually unlocking, counting to 30, then locking via the web UI. It’s slow, but that’s z-wave for ya.

Yup, you're right. I just never noticed this before. thanks!

[dshokouhi]

Just wanted to comment that this PR does indeed fix the issue with Schalge after 0.83.x update, we should get this merged quickly.

[ahayworth]

@turbokongen hey, quick question: is there any chance of this getting into the next 0.84 point release, if we have another before the holidays? There’s a fair amount of people experiencing problems and it’d be a nice Christmas gift for them to have it fixed. 😀😀