Add workaround to use notification state for zwave lock state#17386
Conversation
homeassistant
added
cla-signed
integration: zwave
platform: lock.zwave
small-pr
ghost
added
the
This commit adds documentation around setting the new use_notification_state configuration option being added to home-assistant in home-assistant/core#17386
turbokongen
left a comment
turbokongen
left a comment
There was a problem hiding this comment.
Looks valid to me. Thank you.
|
I saw my issue mentioned here #11934 but this does not resolve it? |
|
We should not have this be done as a user config. Instead we should have it behave like a workaround, where we whitelist devices that need this to function properly. |
|
@balloob I thought about doing it as a workaround (based on what was in workaround.py) but based on the bugs in the commit message this seemed to effect a lot of different model locks and I didn't have all the model information available for all of them. I also felt like we'd constantly be playing whack a mole trying to figure out exactly which models need this. So I thought it would be better to just let the user set this. That being said there is already a lock model workaround doing this in lock/zwave.py (Polycontrol Danalock v2 BTZE) that has a hardcoded workaround doing this already. So if you think it'll be better to do it as a hard coded list of workaround devices I can re-spin this pr to do that instead of a config option. Although I don't think any of the bugs that I referenced in the commit message list the manufacturer and product ids for the locks that need this, so I'll have to try and look that up in the zwave alliance device db. |
|
@dshokouhi it wasn't clear to me whether this would fix that particular bug. There was a lot of varied discussion in it and part of that discussion was the issue this fixes (the is_locked state for a lock doesn't get updated on every type of lock notification). But, at least the initial bug description seemed to indicate your issue was more that you're not getting the notification (or the alarm type) set properly for keypad event, so I just referenced #11934 in the message and didn't mark this as fixing it. |
|
Yes, we should put workarounds in our code for each device. It's not good to outsource known issues to users in the form of a config variable. That's not a good user experience. |
There are several zwave lock models out there which do not seem to update the lock state on non-rf events (see home-assistant#11934 home-assistant#14632 home-assistant#14534 for examples) including kwikset smartkey zwave plus locks (which I own). In these cases it seems that the notifications for non-rf events the access_control value is updated but not the primary value for the lock state, which is what is used to set the is_locked property. To properly have the lock state accurate for all types of notifications on these models we need to use the access_control field. This commit adds a workaround for the 4 models reported to exhibit this behavior so that home-assistant will reliably set the lock state for all device notifications.
mtreinish
force-pushed
the
add-option-for-zwave-lock-access-control-status
branch
from
3caf239 to
8bf0bef
Compare
mtreinish
changed the title
|
@balloob ok, I've updated the PR to use a workaround instead of a config flag. I am only able to confirm the device id for the kwikset lock though (as that's the only one I currently own). I used the zwave device db to get the ids for the other 3 devices people reported issues with (and found 3 device ids for one of them). |
|
@mtreinish I have a Yale YRD220 that I believe suffers the same issue. How would I find the manufacturer id and device id? |
|
@adrum you can find it in a couple places. The easiest is probably in the zwcfg file (which is what I used for my kwikset locks) and find the node in that file for your lock. The information will be there, for example on my locks it was: Where the manufacturer id is 0x0090 and the device id is 0x0440. That being said looking at what I found here: https://www.cd-jackson.com/index.php/zwave/zwave-device-database/zwave-device-list/devicesummary/321 you might already be covered by the patch (since it doesn't check device type, just id). It might be worth trying to see if this fixes the issue for you as is. |
|
I'm afraid I might have found an additional issue. Here is my zwcfg entry. It's not even showing up as a Yale. I tried looking in openzwave and this repo for existing issues like this, but could not find anything. My locks do suffer from the exact issue this PR fixes though. |
|
@adrum hmm, that is weird. I did some searching and found that there are vision branded door locks: http://www.visionsecurity.com.tw/index.php?option=product&lang=en&task=showlist&id=320&index=8 but it doesn't really look anything like the YRD220. Looking at the device db again it doesn't overlap with your entry either:
(based on that it looks like they're the oem for monoprice zwave locks) Anyway I'll push a commit on to this pr adding an entry for your lock and leave a comment that the specific combination of manufacturer id and device id were reported by you as needing this patch (fwiw this kind of situation was why I thought having a config flag would be useful). |
| WORKAROUND_DEVICE_STATE = 'state' | ||
| # Kwikset 914TRL ZW500 | ||
| KWIKSET = 0x0090 | ||
| ZW500_914TRL = 0x440 |
There was a problem hiding this comment.
Why all these constants that are only used once ? We should just inline them at the one place where they are used.
There was a problem hiding this comment.
I was just mirroring the pattern that was already in there for the Polycom workaround (and in other zwave components that have workaround sections) But, if it's that much of a problem I can change it to inline the ids in the device mapping dict.
There was a problem hiding this comment.
Well we only ever use the constant once, the code would get more readable if we just inline it. I would be open to accept another PR to fix this for the others too
Some Z-Wave locks do not send a DOOR_LOCK_OPERATION_REPORT after a user manually manipulates a lock (via key, deadbolt, keypad, etc). Instead, these locks only send a DOOR_LOCK_OPERATION_REPORT in response to Z-Wave commands such as DOOR_LOCK_OPERATION_GET or DOOR_LOCK_OPERATION_SET. Based on my reading of the Z-Wave specification, it could be considered correct. In fact, OpenZWave seems to know about this, and queues a DOOR_LOCK_OPERATION_GET immediately after a DOOR_LOCK_OPERATION_SET: https://github.com/OpenZWave/open-zwave/blob/master/config/schlage/BE469.xml#L141 We can see that in action here on my own network with this lock: 2018-11-25 19:44:52.153 Info, Node019, Value::Set - COMMAND_CLASS_DOOR_LOCK - Locked - 0 - 1 - False 2018-11-25 19:44:52.153 Info, Node019, Value_Lock::Set - Requesting lock to be Unlocked 2018-11-25 19:44:52.153 Detail, Node019, Queuing (Send) DoorLockCmd_Set (Node=19): 0x01, 0x0a, 0x00, 0x13, 0x13, 0x03, 0x62, 0x01, 0x00, 0x25, 0x7b, 0xcb 2018-11-25 19:44:52.153 Detail, Node019, Queuing (Send) DoorLockCmd_Get (Node=19): 0x01, 0x09, 0x00, 0x13, 0x13, 0x02, 0x62, 0x02, 0x25, 0x7c, 0xcd 2018-11-25 19:44:53.718 Detail, Node019, Decrypted Packet: 0x00, 0x62, 0x03, 0x00, 0x00, 0x02, 0xfe, 0xfe 2018-11-25 19:44:53.718 Info, Node019, Received DoorLock report: DoorLock is Unsecure Unfortunately, when we call `update_properties()` for a Z-Wave lock, we don't really know what property has changed, or what message the Z-Wave stack received, or even what it plans to do next. So we potentially construct a state for the device that isn't actually valid. In fact, the right thing to do here is _nothing_ - ideally the component would wait until the DOOR_LOCK_OPERATION_GET completed, and _then_ update state. In home-assistant#17386, we began setting the lock state for certain models of locks based solely on the Alarm status. This is a good workaround for many models of locks, but it also depends on a lock *ALWAYS* sending an Alarm after *any* operation. That is unfortunately not the case for at least the Schlage BE469. In fact, we get an Alarm notification for every operation _aside_ from RF locks/unlocks. Long story short, this PR introduces yet another work around state - this time one based on clearing alarm status. When a user calls lock() or unlock(), we explicitly set any alarm_type or alarm_level to None. For a normal lock, this should be fine - we can't always assert that an alarm state will be present anyways. Moreover, if a user is manipulating a lock via RF we could assert that they are okay with clearing any alarm statuses - they're working with the device. But for the BE469, we can look for an enabled workaround flag, and a cleared alarm, and then decide that the value from a received door lock update is in fact correct, and set flags appropriately. The PR sets more things manually than I'd really like - it seems that this model really does send _just_ the bare minimum, and we can't rely on other things like access_control being set when an RF event happens.
PR home-assistant#17386 attempted to improve the state of z-wave lock tracking for some problematic models. However, it operated under a 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. :) 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. home-assistant#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.
|
I've upgraded to HA 0.83.3. It seems I have a similar issue, in that my "Yale" YRD210 is identified as a Vision. I took a look through the databases and don't see any conflicts... could this also be added?
|
|
@Juggler00 It turns out this PR did not seem to fix my issues for my YRD220. I have a patch on my install that reads the state from the lock's alarm type property. This has been working reliably for me for a couple weeks now. I will try to get another PR going here soon. |
|
Thanks... I will follow and test any updates you have! |
8 participants
Description:
There are several zwave lock models out there which do not seem to
update the lock state on non-rf events (see #11934 #14632 #14534 for
examples) including kwikset smartkey zwave plus locks (which I own).
In these cases it seems that the notifications for non-rf events the
access_control value is updated but not the primary value for the
lock state, which is what is used to set the is_locked property. To
properly have the lock state accurate for all types of notifications
on these models we need to use the access_control field. This commit
adds a workaround for the 4 models reported to exhibit this behavior
so that home-assistant will reliably set the lock state for all
device notifications.
Related issue (if applicable): fixes #14632
Checklist:
tox. Your PR cannot be merged unless tests passIf the code does not interact with devices: