ZwaveJS multicast CLASS_BASIC fails to validate (and send)across multiple different devices

davidbbs commented 3 years ago

The problem

With Core 2021.7.3 I could send a COMMAND_CLASS_BASIC (32) ZwaveJS multicast to multiple nodes on endpoint 0 and have them turn/on off. With Core 2021.8.6 this same command fails with: File "/usr/local/lib/python3.9/site-packages/zwave_js_server/model/node.py", line 414, in async_set_value raise NotFoundError(f"Value {val} not found on node {self}") zwave_js_server.exceptions.NotFoundError: Value 19-32-0-targetValue not found on node Node(node_id=19)

Two things appear to have changed/broken:

COMMAND_CLASS_BASIC commands to set the targetValue no longer work
I need to send to endpoint 1 now using device-specific (SWITCH_BINARY or SWITCH_MULTILEVEL)

Note that there is nothing in the zwave-js log when I try this (even on the high debug levels) because the command never gets issued due to the validation error above.

This suggests that the zwave-js value set command is rejecting "targetValue" for the BASIC command class, which should be valid for all devices, and was working before.

For reference: The service I am testing is:

service: zwave_js.multicast_set_value
data:
  command_class: '32'
  property: targetValue
  value: 0
  endpoint: 0
target:
  entity_id:
          - light.kitchen_counter_switch
          - switch.kitchen_bar_switch

light.kitchen_counter_switch is a Fibaro FGD-212 dimmer that has worked for over a year, and works fine in all non-multicast uses. switch.kitchen_bar_switch is a Fiabro FGS-223 dual switch that has worked for over a year, and works fine in all non-multicast uses.

Here is what I can get working:

If I send a COMMAND_CLASS_SWITCH_BINARY (37) with "true" to the switch entity on endpoint 1 it works.
If I send a COMMAND_CLASS_SWITCH_MULTILEVEL (38) with 99 to the light entity endpoint 1 it works.
Sending either of those commands to the other entities results in the same type of error. (I expect the MULTILEVEL to fail on the binary switch, but not the other way around.)

What is version of Home Assistant Core has the issue?

core-2021.8.6

What was the last working version of Home Assistant Core?

core-2021.7.3

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Zwave-JS

Link to integration documentation on our website

https://www.home-assistant.io/integrations/zwave_js/

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

probot-home-assistant[bot] commented 3 years ago

Hey there @home-assistant/z-wave, mind taking a look at this issue as it has been labeled with an integration (zwave_js) you are listed as a code owner for? Thanks! _{^{(message by CodeOwnersMention)}}

zwave_js documentation zwave_js source _{^{(message by IssueLinks)}}

MartinHjelmare commented 3 years ago

Please create a dump of your Z-Wave network and upload the dump as a text file here.

The dump tool is available in the configuration panel of the Z-Wave JS integration, reached from the integrations page in the Home Assistant GUI.

The network dump will help us troubleshoot your device.

Thanks!

davidbbs commented 3 years ago

Here's a dump from currently and one (Aug) I found from a month ago (July). Diffing them may be of help. I saw that the dimmer nodes list endpoint 0 as supporting Multi Channel while the new one doesn't. E.g., line 5259 (July) vs. line 5352 (Aug).

zwave_js_dumps.zip

kpine commented 3 years ago

The old and new dump show that there never was a Basic targetValue value ID, which is what the error is complaining about. The difference is, the upstream Python library was updated to include additional error checks. See https://github.com/home-assistant-libs/zwave-js-server-python/pull/262 (and because of that one, the error message comes from https://github.com/home-assistant-libs/zwave-js-server-python/pull/260). Prior to that first PR, the value ID given for a multicast command would not be checked and sent straight to node-zwave-js. node-zwave-js doesn't actually care if the value ID passed to setValue actually exists or not, AFAIK, as long as the CC is supported and the value ID properties are supported, it will issue the corresponding command.

I would have to guess that the Basic Set/Report is mapped to Multilevel Switch and Binary CCs, which is why there was never a value ID created. For some reason Basic CC is not listed in the node's array of "commandClasses" either, but it is in the "mandatorySupportedCCs". Regardless if there's a value ID, it should be possible to send a Basic Set. I think nearly all (if not all) actuator devices are required to support Basic CC. Maybe an exception should be added for Basic CC, or let the driver handle the error checking instead?

Sending either of those commands to the other entities results in the same type of error. (I expect the MULTILEVEL to fail on the binary switch, but not the other way around.)

What do you mean by this, are you expecting the FGD-212 Dimmer to succeed with a Binary Switch CC Set command? It doesn't support that command class, so it won't work.

davidbbs commented 3 years ago

That would explain it.

The spec says "Any device SHOULD support the Basic Command Class" (4.13.1) but it also says "The Basic Command Class MUST NOT be advertised in the Node Information Frame". That might suggest that an exception may need to be made for Basic CC if it is not likely to be advertised.

And since it worked before, I would conclude that the device does support it.

Do you know where this class is missing? I looked at the device in the database but I couldn't figure out where the basic class (template?) should have been included. Or does it need to be manually specified in each device in the database?

With regards to your question: I had expected that multilevel was a subclass of binary switch, since a multilevel switch can also be turned on/off, but if it doesn't list that then I understand it wouldn't work.

kpine commented 3 years ago

The spec says "Any device SHOULD support the Basic Command Class" (4.13.1) but it also says "The Basic Command Class MUST NOT be advertised in the Node Information Frame".

And since it worked before, I would conclude that the device does support it.

To make it clear, I didn't mean suggest the device does not support Basic CC. It's a mandatory CC and it supports it, there's no argument from me. I was explaining what the dump files show. HA does not have any indication that Basic CC is supported because it uses the value API, and there are no values for that CC. Before HA did not care about the value being present, now it does.

Either node-zwave-js should always expose Basic CC target values when available, or HA needs to allow it to be set regardless of whether the value exists or not.

Do you know where this class is missing?

I was referring to the dump files you posted, literally the "commandClasses" field of the node data. Maybe node-zwave-js just doesn't include it in the list of CCs if the Basic CC is mapped to another CC. 🤷🏻‍♂️ The z-wave alliance product DB lists it as a supported class, and it's a mandatory CC. It is not necessary for node-zwave-js to have this in its device config files. This is simply an issue of HA restricting functionality.

With regards to your question: I had expected that multilevel was a subclass of binary switch, since a multilevel switch can also be turned on/off, but if it doesn't list that then I understand it wouldn't work.

Multilevel Switch CC and Binary Switch CC are independent command classes, there's not really such a thing as a "subclass". Typically a dimmer switch would only implement Multilevel Switch CC. Implementing Binary Switch CC would be redundant.

MartinHjelmare commented 3 years ago

@AlCalzone what do you think should change here?

Either node-zwave-js should always expose Basic CC target values when available, or HA needs to allow it to be set regardless of whether the value exists or not.

AlCalzone commented 3 years ago

I've chosen to not expose the Basic values when there is a better CC supported, because you'd end up with a ton of unnecessary values that never get updated.

IMO you should allow always setting the basic values. Just keep in mind that it is possible that the automatic value refresh won't always work entirely.

AlCalzone commented 3 years ago

Thinking about it, it would make sense to not hide the Basic CC targetValue on virtual nodes (multicast/broadcast) on the driver side. I've raised an issue to track that.

davidbbs commented 3 years ago

Thanks. That sounds like the right approach if we expect Basic CC to always be supported. Shall I close this?

MartinHjelmare commented 3 years ago

We can keep it open until it's resolved. 👍

AlCalzone commented 3 years ago

I guess you couldn't keep it open 🤣 The robot overlords had a different plan.

davidbbs commented 3 years ago

Hi, I saw that it looks like https://github.com/zwave-js/node-zwave-js/pull/3504 has been included in the current HA release but I still can't issue a Basic CC to a device. Do either of you know if there is further work required on the HA side to make that function?

kpine commented 3 years ago

That fix isn't related to this issue. This one requires a change in HA (or the python library, to be exact).

e10kstarfire commented 2 years ago

Hello, I just wondered if it's still planned to do this fix in HA as I need the same for this problem https://community.home-assistant.io/t/new-siren-integration-cannot-find-the-service/327185 ?

davidbbs commented 2 years ago

@e10kstarfire I haven't seen any updates on this, so I do not think this is prioritized.

github-actions[bot] commented 2 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

davidbbs commented 2 years ago

Hi there: is there anything I can/should do to get this resolved? (I understand it is not prioritized.) thanks!

kpine commented 2 years ago

@davidbbs Can this be closed now?

issue-triage-workflows[bot] commented 1 year ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

kristofer84 commented 1 year ago

Seems to be fixed, works for me now.

issue-triage-workflows[bot] commented 1 year ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

home-assistant / core