Skip to content
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

feat: rework how endpoints and lifeline associations are handled #2997

Merged
merged 9 commits into from
Jul 13, 2021

Conversation

AlCalzone
Copy link
Member

@AlCalzone AlCalzone commented Jul 10, 2021

This PR solves a long-standing issue with devices that misuse endpoints and don't correctly use multi channel associations (at least according to how I understand the terrible specification around these). See #2286 for a detailed explanation.

We expect that this will be breaking for some devices that need one or more of the below compat flags from now on. If that is the case, please open issues and include a driver log of a re-interview.
At the very least, value IDs will change from endpoints 1+ to 0 for several devices.

Here's a breakdown of the changes:

Ignore unnecessary endpoints instead of the root device

Until now we used to hide the root device's application CC values if the same value was existing on an endpoint. It has been found that many devices mis-use endpoints when it does not actually make sense and don't correctly report their values. If all endpoints have different device classes, they are most likely unnecessary and will be ignored, because all the functionality will also be exposed on the root device. Those devices that only report via the root device should now work out of the box.

For the other devices that actually need their endpoints despite them being different, a new compat flag was added to restore the current behavior:

"preserveEndpoints": "*", // to preserve all endpoints and hide the root values instead
"preserveEndpoints": [2, 3], // to preserve endpoints 2 and 3, but ignore endpoint 1.

For example, there are some thermostats which expose an external sensor with battery through a seemingly unnecessary endpoint, but it needs to be preserved

If some endpoints share a device class, like multi-channel switches, the current behavior of hiding the root device's in favor of the endpoints' values will stay in place.

Define which endpoint to map root reports to

Because the specs stated that the root device should mirror at least endpoint 1 (and possibly others), we used to map reports that came through the root device to the first supporting endpoint. This behavior has changed a couple of times and has never been perfect for everyone. With this PR, device configs need to opt-in instead to turn on the mapping and specify the target endpoint. Otherwise, reports to the root device get silently ignored if the root's values are hidden.

To do so, a new compat flag was added:

"mapRootReportsToEndpoint": 1

This also needs to be used for some devices, e.g. dual channel switches, that partially report un-encapsulated through the root device.

Prefer node associations on each endpoint over multi channel associations

Many manufacturers seem to get multi channel associations "wrong". We now prefer setting up node associations on each endpoint instead if possible. The default strategy for each endpoint is the following:

  1. Try a node association on the current endpoint/root
  2. If Association CC is not supported, try assigning a node association with the Multi Channel Association CC
  3. If that did not work, fall back to a multi channel association (target endpoint 0)
  4. If that did not work either, the endpoint index is > 0 and the node supports Z-Wave+:
    Fall back to a multi channel association (target endpoint 0) on the root, if it doesn't have one yet.

Incorrect lifeline associations like the ones set up by OZW with target endpoint 1 will automatically get cleaned up during the process.

This strategy can be controlled with the multiChannel flag in the association config. true forces a multi channel association (if supported) and does not fall back to node associations. false forces a node association (if supported) and does not fall back to a node association.

fixes: #2286
fixes: #2257
fixes: #1989
fixes: #2714
fixes: #1860
fixes: #2910

@AlCalzone AlCalzone force-pushed the no-unnecessary-endpoints branch 2 times, most recently from 0177d63 to 1fc224a Compare July 10, 2021 15:02
@AlCalzone AlCalzone added the breaking This is a breaking change label Jul 10, 2021
@zwave-js-assistant
Copy link

🚧 It seems like this PR has lint errors 🚧

I should be able to fix them for you. If you want me to, just comment
@zwave-js-bot fix lint

@blhoward2
Copy link
Collaborator

I assume this will potentially fix that series of devices where we preserved the root endpoint temporarily so that endpoint 1 didn't change with endpoint 2. I already have the Zooz ones and I emailed Inovelli to see if they'd either send me one, or test on their own, for the LZW36.

@AlCalzone
Copy link
Member Author

Yep, that specifically. We're going to have to remove the workaround then.

@AlCalzone AlCalzone marked this pull request as ready for review July 11, 2021 15:17
@AlCalzone
Copy link
Member Author

AlCalzone commented Jul 11, 2021

@blhoward2 in #2293 we added the root value workaround to the following devices:

  • Zooz ZEN16
  • Zooz ZEN17
  • Zooz ZEN25

Comment: This device improperly reports the state of R2 (endpoint 2) through the root endpoint in a way that also changes the state of R1 (endpoint 1)

And

  • LZW36

Comment: This device improperly reports the state of endpoint 2 through the root endpoint in a way that also changes the state of endpoint 1

Do you remember how exactly this problem manifested or do you still have some of those and can make a new re-interview log? My guess is that those are cases of devices that either don't need any endpoints or that benefit from the simplified Lifeline association setup.

@blhoward2
Copy link
Collaborator

I have the ZEN16 and ZEN17. Upon local operation of the switch for endpoint 2 it would toggle endpoint 1 because it would map the report. I believe I still have logs in my email we sent Zooz and I'll send those to you.

The LZW36 was a similar issue. It has two switches and operating endpoint 2 would send a report across the root endpoint that would map to endpoint 1. There is an entire thread about this over at Inovelli. https://community.inovelli.com/t/re-zwave-js-lzw36-driver/7103/82?u=blhoward2 We also had an issue with logs: #2155

@blhoward2
Copy link
Collaborator

There are some logs of the ZEN17 here. #2379

@AlCalzone
Copy link
Member Author

There are some logs of the ZEN17 here. #2379

Okay, that looks like the device uses the root as a "compound" state. Can you confirm that? Zen16 is probably similar. In that case we should keep the compat flag or that "compound" state will be hidden.

For the LZW36 I don't think we need the flag. The incorrect update was caused by a mapping of a multicast the device sent, probably as a result of the weird OZW association. In any case we're not mapping that anymore.

@AlCalzone AlCalzone force-pushed the no-unnecessary-endpoints branch from f59610f to 927953c Compare July 13, 2021 12:42
@AlCalzone
Copy link
Member Author

I'm going to merge this so I don't have to keep it up to date all the time. We can solve these device-specific issues afterwards.

@AlCalzone AlCalzone enabled auto-merge (squash) July 13, 2021 22:04
@AlCalzone AlCalzone merged commit 10e53af into next Jul 13, 2021
@AlCalzone AlCalzone deleted the no-unnecessary-endpoints branch July 13, 2021 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This is a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants