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

[Bug]: node_notification event parameter 2 is always null #2382

Closed
3 tasks done
j9brown opened this issue Apr 21, 2022 · 2 comments · Fixed by #2386
Closed
3 tasks done

[Bug]: node_notification event parameter 2 is always null #2382

j9brown opened this issue Apr 21, 2022 · 2 comments · Fixed by #2386
Labels
bug Something isn't working

Comments

@j9brown
Copy link
Contributor

j9brown commented Apr 21, 2022

Checklist

  • I am not using Home Assistant. Or: a developer has told me to come here.
  • I have checked the troubleshooting section and my problem is not described there.
  • I have read the changelog and my problem is not mentioned there.

Deploy method

Docker

Zwavejs2Mqtt version

6.7.2

ZwaveJS version

9.0.2

Describe the bug

I'm trying to handle MultilevelSwitchChange notifications from ZwaveJS by subscribing to the node_notification MQTT topic.

I receive an object like this (pruned for clarity):

[
  { /* tons of information about the node */ },
  {
    id: "57-38-0-5"
    nodeId: 57
    commandClass: 38
    commandClassName: "Multilevel Switch"
    property: 5
    propertyName: 5
   },
  null
]

That null is the problem. It should be an object containing an object with eventType and direction fields as can be seen in the last parameter of the call to emit() in ZwaveJS.

Without having read too deeply into ZwaveJS2MQTT, I suspect this parameter is not being serialized properly when the MQTT message is generated, resulting in a null instead of an object.

To Reproduce

Interact with a Zwave device that generates mutlilevel switch change messages. In my case, press the up or down arrow buttons on a VRCS4 or VRCZ4.

Expected behavior

The second parameter should contain an object with the event data that was supplied in the call to emit(), not null.

Additional context

Here's the pull request that added this feature to ZwaveJS in case it's of use to you: zwave-js/node-zwave-js#4282

@j9brown j9brown added the bug Something isn't working label Apr 21, 2022
@robertsLando
Copy link
Member

Notifications are catched here: https://github.com/zwave-js/zwavejs2mqtt/blob/master/lib/ZwaveClient.ts#L3543

The third parameter in my case is args: https://github.com/zwave-js/zwavejs2mqtt/blob/master/lib/ZwaveClient.ts#L3355

In case of amultilevel switch notification data is parsed using args.eventData so if that is null it means that field is empty: https://github.com/zwave-js/zwavejs2mqtt/blob/master/lib/ZwaveClient.ts#L3381

cc @AlCalzone

@AlCalzone
Copy link
Member

AlCalzone commented Apr 21, 2022

Not sure where you get the eventData from as this is specific to the Entry Control CC. Not enough TypeScript enabled, heh? ;)

The third parameter of the "notification" event depends on the CC that emits it (2nd parameter):
https://github.com/zwave-js/node-zwave-js/blob/e7897140cde5b8fac1064dabe9ec92ddc47b23a9/packages/zwave-js/src/lib/node/Types.ts#L87-L94
and is currently one of these:
Notification CC
https://github.com/zwave-js/node-zwave-js/blob/76c76fb90161bb1d68f0559bf9156261e15f0c23/packages/zwave-js/src/lib/commandclass/NotificationCC.ts#L71-L83

Entry Control CC
https://github.com/zwave-js/node-zwave-js/blob/76c76fb90161bb1d68f0559bf9156261e15f0c23/packages/zwave-js/src/lib/commandclass/EntryControlCC.ts#L94-L99

Powerlevel CC (handled internally)
https://github.com/zwave-js/node-zwave-js/blob/76c76fb90161bb1d68f0559bf9156261e15f0c23/packages/zwave-js/src/lib/commandclass/PowerlevelCC.ts#L63-L68

Multilevel Switch CC
https://github.com/zwave-js/node-zwave-js/blob/3ef1368bd1f2fd1e41ceac43bc9f89e0d1ffe550/packages/zwave-js/src/lib/commandclass/MultilevelSwitchCC.ts#L127-L135


IMO when passing these through MQTT, you shouldn't be parsing/converting anything. Well... maybe not the entire node object, but the entire payload (3rd argument) is relevant.

You can do some conversion when writing to special topics, but this handling must be CC specific.

robertsLando added a commit that referenced this issue Apr 22, 2022
* fix: correctly parse MultilevelSwitchCC notifications

Fixes #2382

* fix: better typescript support

* fix: use ZWaveNotificationCallback

* fix: let TS do the work

Co-authored-by: Dominic Griesel <dominic.griesel@nabucasa.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants