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

fix(config): add enableBasicSetMapping for device ZD2102 #2386

Merged
merged 2 commits into from Apr 21, 2021
Merged

fix(config): add enableBasicSetMapping for device ZD2102 #2386

merged 2 commits into from Apr 21, 2021

Conversation

trantamjp
Copy link
Contributor

Without enableBasicSetMapping set, it doesn't treat BasicCC::Set as a event but a report

@zwave-js-assistant zwave-js-assistant bot added the config ⚙ Configuration issues or updates label Apr 16, 2021
@blhoward2
Copy link
Collaborator

We don't blindly add compat flags to devices. Could you describe the problem and what is not happening that you believe should happen? Could you post a log with and without? Mapping has been a particular issue that has caused us to revert changes a few times now.

@trantamjp
Copy link
Contributor Author

@blhoward2
Below is the log when I actually open and close the door (sensor close/open). I don't get any event from HA.

Thank you,

2021-04-19T22:51:37.481Z SERIAL « 0x010b0004000203200100b80068                                        (13 bytes)
2021-04-19T22:51:37.482Z SERIAL » [ACK]                                                                   (0x06)
2021-04-19T22:51:37.482Z DRIVER « [Node 002] [REQ] [ApplicationCommand]
                                  └─[BasicCCSet]
                                      target value: 0
2021-04-19T22:51:37.483Z CNTRLR   [Node 002] treating BasicCC::Set as a report
2021-04-19T22:51:37.483Z CNTRLR   [Node 002] [~] [Basic] currentValue: 255 => 0                     [Endpoint 0]
2021-04-19T22:51:37.500Z SERIAL « 0x0112000400020a7105070000ff07020000b600de                          (20 bytes)
2021-04-19T22:51:37.501Z SERIAL » [ACK]                                                                   (0x06)
2021-04-19T22:51:37.501Z DRIVER « [Node 002] [REQ] [ApplicationCommand]
                                  └─[NotificationCCReport]
                                      notification type:   Home Security
                                      notification status: 255
                                      notification state:  Intrusion
2021-04-19T22:51:37.502Z CNTRLR   [Node 002] [~] [Notification] Home Security[Sensor status]: 2 =>  [Endpoint 0]
                                  2

2021-04-19T22:52:02.521Z SERIAL « 0x010b00040002032001ffb1009e                                        (13 bytes)
2021-04-19T22:52:02.522Z SERIAL » [ACK]                                                                   (0x06)
2021-04-19T22:52:02.522Z DRIVER « [Node 002] [REQ] [ApplicationCommand]
                                  └─[BasicCCSet]
                                      target value: 255
2021-04-19T22:52:02.523Z CNTRLR   [Node 002] treating BasicCC::Set as a report
2021-04-19T22:52:02.524Z CNTRLR   [Node 002] [~] [Basic] currentValue: 0 => 255                     [Endpoint 0]
2021-04-19T22:52:02.540Z SERIAL « 0x0112000400020a710507ff00ff07020000b10026                          (20 bytes)
2021-04-19T22:52:02.540Z SERIAL » [ACK]                                                                   (0x06)
2021-04-19T22:52:02.541Z DRIVER « [Node 002] [REQ] [ApplicationCommand]
                                  └─[NotificationCCReport]
                                      notification type:   Home Security
                                      notification status: 255
                                      notification state:  Intrusion
2021-04-19T22:52:02.541Z CNTRLR   [Node 002] [~] [Notification] Home Security[Sensor status]: 2 =>  [Endpoint 0]
                                  2

@blhoward2
Copy link
Collaborator

What's wrong with this being a report? Have you looked under both zwave_js_notification and zwave_js_value_notification?

@AlCalzone
Copy link
Member

@trantamjp maybe it helps if you state your expectations of what should happen. I think this PR is a misunderstanding of what the compat flag does.

The way I see it is that the device reports its current open/closed status with Basic CC Set commands. zwave-js re-interprets them as reports and updates the currentValue accordingly. This is just weird because the device does support Notification CC which has a variable for window/door open/closed.

If it also supported Binary Sensor CC, it would make sense to add the flag, so the Set would get mapped to a Binary Sensor Report. From the manual I've found, this doesn't look like it.

You probably wanted treatBasicSetAsEvent, but I don't think it makes sense to treat a device status as an event. What's the problem with exposing the current status as currentValue?

@trantamjp
Copy link
Contributor Author

Please hold this PR and let me test out with the new code which fixes the interview (from the other PR).

@AlCalzone AlCalzone marked this pull request as draft April 20, 2021 13:47
@trantamjp
Copy link
Contributor Author

trantamjp commented Apr 21, 2021

I have tested this device zd2102 with the PR 2402

First, the good thing is I no longer receive No response to Basic Get command, assuming the node does not support Basic CC... thanks to the PR2402.

Now when I test open/close, I get

2021-04-21T05:04:41.257Z CNTRLR   [Node 071] treating BasicCC::Set as a report
2021-04-21T05:04:41.258Z CNTRLR   [Node 071] [~] [Basic] currentValue: 255 => 0                     [Endpoint 0]

zd2102_ZWJS_inclusion.log
zd2102_JWJS_and_HA.log

Then I enable the enableBasicSetMapping in the device config zd2102 and test it again. This time I get

2021-04-21T05:10:32.244Z CNTRLR   [Node 071] treating BasicCC::Set as a report
2021-04-21T05:10:32.245Z CNTRLR   [Node 071] [~] [Binary Sensor] Any: true => false                 [Endpoint 0]

zd2102_JWJS_and_HA_with_enableBasicSetMapping.log

Lastly, I tested both zd2102 and wadwaz-1. If I remove enableBasicSetMapping from wadwaz-1 config, I get both the same reports.

wadwaz-1_without_enableBasicSetMapping.log

Right now, on the HA integration, wadwaz-1 gives me ON/OFF on binary_sensor while zd2102 gives me 0/255 on sensor. I can live with that but wish if the driver can act the same way for the 2 identical devices: either both use Basic Report or use Binary Sensor Report.

Thank you,

@kpine
Copy link
Contributor

kpine commented Apr 21, 2021

If it also supported Binary Sensor CC, it would make sense to add the flag, so the Set would get mapped to a Binary Sensor Report. From the manual I've found, this doesn't look like it.

Were you possibly looking at the wrong manual? There is a ZD2102 and ZD2102-5, they are identical in appearance. The ZD2102-5 is zwave plus and is a proper notification sensor. This ZD2102 is a binary sensor device type (see interview) and according to the manual I found, it uses Basic Set to indicate off/on. This seems to fit the profile of a device that requires the compat flag.

@trantamjp
Copy link
Contributor Author

trantamjp commented Apr 21, 2021

@kpine

Mine is ZD2102

                                  └─[ManufacturerSpecificCCReport]
                                      manufacturer id: 0x0109
                                      product type:    0x2001
                                      product id:      0x0102

Yes, it use Basic Set. Currently it does not have compat flag. This is why this PR is for, and to match with other device wadwaz-1 that works identical to this ZD2102 while wadwaz-1 has enableBasicSetMapping but this device doesn't.

That's why ZD2102 produces a Binary Sensor report while the other one wadwaz-1 produces Basic report.

@AlCalzone AlCalzone marked this pull request as ready for review April 21, 2021 20:35
@AlCalzone
Copy link
Member

Okay I'm convinced. Let's do this

@AlCalzone AlCalzone merged commit 57758c7 into zwave-js:master Apr 21, 2021
CedricGatay added a commit to CedricGatay/node-zwave-js that referenced this pull request Apr 22, 2021
Fibaro FGK101 with firmware version lower than 2.3 does not report properly state change and uses `BasicCC:Set` to do so.

As per discussed in zwave-js#2417, enabling compat flag to treat the basic set report as a status change (similar to what has been done on zwave-js#2386).

After applying the patch, I have got the following in logs, confirmed working on two FGK101-2.1 devices
```
2021-04-22T06:31:25.722Z CNTRLR [Node 024] treating BasicCC::Set as a report
2021-04-22 08:31:25.736 INFO ZWAVE: Node 24: value updated: 48-0-Any false => true
2021-04-22T06:31:29.816Z CNTRLR [Node 024] treating BasicCC::Set as a report
2021-04-22 08:31:29.823 INFO ZWAVE: Node 24: value updated: 48-0-Any true => false
```
CedricGatay added a commit to CedricGatay/node-zwave-js that referenced this pull request Apr 22, 2021
Fibaro FGK101 with firmware version lower than 2.3 does not report properly state change and uses `BasicCC:Set` to do so.

As per discussed in zwave-js#2417, enabling compat flag to treat the basic set report as a status change (similar to what has been done on zwave-js#2386).

After applying the patch, I have got the following in logs, confirmed working on two FGK101-2.1 devices
```
2021-04-22T06:31:25.722Z CNTRLR [Node 024] treating BasicCC::Set as a report
2021-04-22 08:31:25.736 INFO ZWAVE: Node 24: value updated: 48-0-Any false => true
2021-04-22T06:31:29.816Z CNTRLR [Node 024] treating BasicCC::Set as a report
2021-04-22 08:31:29.823 INFO ZWAVE: Node 24: value updated: 48-0-Any true => false
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config ⚙ Configuration issues or updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants