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

Set lock status correctly for Schlage BE469 Z-Wave locks #18737

Merged
merged 2 commits into from Dec 7, 2018

Conversation

Projects
None yet
8 participants
@ahayworth
Copy link
Contributor

ahayworth commented Nov 27, 2018

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

@ahayworth ahayworth requested a review from home-assistant/z-wave as a code owner Nov 27, 2018

@wafflebot wafflebot bot added the in progress label Nov 27, 2018

Show resolved Hide resolved tests/components/lock/test_zwave.py Outdated
Show resolved Hide resolved homeassistant/components/lock/zwave.py Outdated
@dshokouhi

This comment has been minimized.

Copy link
Contributor

dshokouhi commented Nov 27, 2018

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

This comment has been minimized.

Copy link
Contributor

ahayworth commented Nov 27, 2018

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

This comment has been minimized.

Copy link
Member

balloob commented Dec 2, 2018

@adrum adrum referenced this pull request Dec 4, 2018

Merged

Added zwave lock state from alarm type workaround #18996

3 of 3 tasks complete
@ahayworth

This comment has been minimized.

Copy link
Contributor

ahayworth commented Dec 4, 2018

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

This comment has been minimized.

Copy link
Member

turbokongen commented Dec 4, 2018

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

This comment has been minimized.

Copy link

hoopsta1423 commented Dec 4, 2018

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

This comment has been minimized.

Copy link
Contributor

ahayworth commented Dec 4, 2018

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

This comment has been minimized.

Copy link

hoopsta1423 commented Dec 4, 2018

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

This comment has been minimized.

Copy link
Contributor

ahayworth commented Dec 4, 2018

@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

This comment has been minimized.

Copy link
Contributor

ahayworth commented Dec 4, 2018

@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

This comment has been minimized.

Copy link
Member

turbokongen commented Dec 4, 2018

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

@dshokouhi

This comment has been minimized.

Copy link
Contributor

dshokouhi commented Dec 4, 2018

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

This comment has been minimized.

Copy link

hoopsta1423 commented Dec 4, 2018

@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

This comment has been minimized.

Copy link
Contributor

dshokouhi commented Dec 6, 2018

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.

@turbokongen turbokongen merged commit 05586de into home-assistant:dev Dec 7, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.06%) to 92.938%
Details

@wafflebot wafflebot bot removed the in progress label Dec 7, 2018

mxworm added a commit to mxworm/home-assistant that referenced this pull request Dec 7, 2018

Merge branch 'dev' into current
* dev: (52 commits)
  Set lock status correctly for Schlage BE469 Z-Wave locks (home-assistant#18737)
  Upgrade Tibber lib (home-assistant#19098)
  Bump skybellpy version to fix api issue (home-assistant#19100)
  Automatically detect if ipv4/ipv6 is used for cert_expiry (home-assistant#18916)
  Upgrade pyatv to 0.3.12 (home-assistant#19085)
  Bump lakeside requirement to support more Eufy devices (home-assistant#19080)
  Set directv unavailable state when errors returned for longer then a minute (home-assistant#19014)
  Updated frontend to 20181207.0
  Force refresh Lovelace (home-assistant#19073)
  Upgrade aiolifx to 0.6.7 (home-assistant#19077)
  Fix missing colorTemperatureInKelvin from Alexa responses (home-assistant#19069)
  Add CM17A support (home-assistant#19041)
  Upgrade pylint to 2.2.2 (home-assistant#18750)
  Revert home-assistant#17745 (home-assistant#19064)
  Bumped version to 0.85.0.dev0
  Add support for more Tibber Pulse data (home-assistant#19033)
  Update locationsharinglib to 3.0.9 (home-assistant#19045)
  Implemented unique ID support for Fibaro hub integration (home-assistant#19055)
  Update pyhomematic to 0.1.53 (home-assistant#19056)
  Fix saving YAML as JSON with empty array (home-assistant#19057)
  ...
@ahayworth

This comment has been minimized.

Copy link
Contributor

ahayworth commented Dec 14, 2018

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

@marchingphoenix marchingphoenix added this to the 0.84.3 milestone Dec 14, 2018

balloob added a commit that referenced this pull request Dec 17, 2018

Set lock status correctly for Schlage BE469 Z-Wave locks (#18737)
* Set lock status correctly for Schlage BE469 Z-Wave locks

PR #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. #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.

* Fix linter

@balloob balloob referenced this pull request Dec 17, 2018

Merged

0.84.3 #19391

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