New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add workaround to use notification state for zwave lock state #17386

Merged
merged 3 commits into from Nov 6, 2018

Conversation

Projects
None yet
8 participants
@mtreinish
Contributor

mtreinish commented Oct 13, 2018

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:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@mtreinish mtreinish requested a review from home-assistant/z-wave as a code owner Oct 13, 2018

@wafflebot wafflebot bot added the in progress label Oct 13, 2018

mtreinish added a commit to mtreinish/home-assistant.io that referenced this pull request Oct 13, 2018

Add documentation for new zwave_lock configuration option
This commit adds documentation around setting the new
use_notification_state configuration option being added to
home-assistant in home-assistant/home-assistant#17386
@turbokongen

Looks valid to me. Thank you.

@dshokouhi

This comment has been minimized.

Contributor

dshokouhi commented Oct 14, 2018

I saw my issue mentioned here #11934 but this does not resolve it?

@balloob

This comment has been minimized.

Member

balloob commented Oct 15, 2018

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.

@mtreinish

This comment has been minimized.

Contributor

mtreinish commented Oct 15, 2018

@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.

@mtreinish

This comment has been minimized.

Contributor

mtreinish commented Oct 15, 2018

@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.

@balloob

This comment has been minimized.

Member

balloob commented Nov 1, 2018

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.

Add workaround to use notification state for zwave lock state
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.

@mtreinish mtreinish force-pushed the mtreinish:add-option-for-zwave-lock-access-control-status branch from 3caf239 to 8bf0bef Nov 4, 2018

@mtreinish mtreinish changed the title from Add config option to use notification state for zwave lock state to Add workaround to use notification state for zwave lock state Nov 4, 2018

@mtreinish

This comment has been minimized.

Contributor

mtreinish commented Nov 4, 2018

@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).

@adrum

This comment has been minimized.

adrum commented Nov 4, 2018

@mtreinish I have a Yale YRD220 that I believe suffers the same issue. How would I find the manufacturer id and device id?

@mtreinish

This comment has been minimized.

Contributor

mtreinish commented Nov 4, 2018

@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:

<Manufacturer id="90" name="Kwikset">
        <Product type="3" id="440" name="Unknown: type=0003, id=0440" />
</Manufacturer>

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.

@adrum

This comment has been minimized.

adrum commented Nov 4, 2018

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.

<Node id="36" name="" location="" basic="4" generic="64" specific="3" type="Secure Keypad Door Lock" listening="false" frequentListening="true" beaming="true" routing="true" max_baud_rate="40000" version="4" secured="true" query_stage="Complete">
    <Manufacturer id="109" name="Vision">
        <Product type="2" id="0" name="Unknown: type=0002, id=0000" />
    </Manufacturer>

My locks do suffer from the exact issue this PR fixes though.

@mtreinish

This comment has been minimized.

Contributor

mtreinish commented Nov 5, 2018

@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

This comment has been minimized.

@balloob

balloob Nov 5, 2018

Member

Why all these constants that are only used once ? We should just inline them at the one place where they are used.

This comment has been minimized.

@mtreinish

mtreinish Nov 5, 2018

Contributor

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.

This comment has been minimized.

@balloob

balloob Nov 5, 2018

Member

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

@balloob

balloob approved these changes Nov 6, 2018

@balloob balloob merged commit 087bffe into home-assistant:dev Nov 6, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 93.092%
Details

@wafflebot wafflebot bot removed the in progress label Nov 6, 2018

@mtreinish mtreinish deleted the mtreinish:add-option-for-zwave-lock-access-control-status branch Nov 6, 2018

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Add workaround to use notification state for zwave lock state (home-a…
…ssistant#17386)

* Add workaround to use notification state for zwave lock state

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.

* Add YRD220 as per adrum to workaround list

* Inline constants

ahayworth added a commit to ahayworth/home-assistant that referenced this pull request Nov 26, 2018

Set lock status correctly for Schlage BE469 Z-Wave Locks
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.

ahayworth added a commit to ahayworth/home-assistant that referenced this pull request Nov 26, 2018

Set lock status correctly for Schlage BE469 Z-Wave Locks
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.

ahayworth added a commit to ahayworth/home-assistant that referenced this pull request Nov 26, 2018

Set lock status correctly for Schlage BE469 Z-Wave Locks
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.

ahayworth added a commit to ahayworth/home-assistant that referenced this pull request Nov 27, 2018

Set lock status correctly for Schlage BE469 Z-Wave locks
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.

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

@Juggler00

This comment has been minimized.

Juggler00 commented Dec 4, 2018

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.

<Manufacturer id="109" name="Vision">
	<Product type="4" id="0" />
</Manufacturer>

I took a look through the databases and don't see any conflicts... could this also be added?

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.

<Node id="36" name="" location="" basic="4" generic="64" specific="3" type="Secure Keypad Door Lock" listening="false" frequentListening="true" beaming="true" routing="true" max_baud_rate="40000" version="4" secured="true" query_stage="Complete">
    <Manufacturer id="109" name="Vision">
        <Product type="2" id="0" name="Unknown: type=0002, id=0000" />
    </Manufacturer>

My locks do suffer from the exact issue this PR fixes though.

@adrum

This comment has been minimized.

adrum commented Dec 4, 2018

@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.

@Juggler00

This comment has been minimized.

Juggler00 commented Dec 4, 2018

Thanks... I will follow and test any updates you have!

@adrum adrum referenced this pull request Dec 4, 2018

Open

Added zwave lock state from alarm type workaround #18996

3 of 3 tasks complete

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Dec 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.