project-chip / zap

ZAP stands for ZCL Advanced Platform. It is used to configure clusters, attributes and other entities for Matter and ZigbeePro applications.
Apache License 2.0
133 stars 82 forks source link

[Bug / Matter Non Conformance] Missing Binding Clusters for Simple device types #1161

Closed jvmahon closed 11 months ago

jvmahon commented 11 months ago

Issue: When creating endpoints with Zap, there are a number of cases where endpoints for "Simple" device types are created without the "Binding" cluster. I understand this as non-conforming under Section 1.1.7 of the Matter Cluster Library Specification Version 1.1.

The table below shows the device types that I have identified as missing their Binding cluster.

Can this be corrected (I would do a PR but am not familiar enough with Zap to correct).

"Simple" Device Type Name Binding Cluster Status

lighting 0x0100|On/Off Light|Binding Cluster missing 0x0101|Dimmable Light|Binding Cluster missing 0x010C|Color Temperature Light|Binding Cluster missing 0x010D|Extended Color Light|Binding Cluster missing smart plugs/outlets and other actuators|| 0x010A|On/Off Plug-in Unit|Binding Cluster missing 0x010B|Dimmable Plug-In Unit|Binding Cluster missing 0x0303|Pump|Binding Cluster missing switches and controls 0x0103|On/Off Light Switch|Good 0x0104|Dimmer Switch|Good 0x0105|Color Dimmer Switch|Good 0x0840|Control Bridge|Good 0x0304|Pump Controller|Good 0x000F|Generic Switch|Binding Cluster Missing sensors 0x0015|Contact Sensor|Binding Cluster missing 0x0106|Light Sensor|Binding Cluster missing 0x0107|Occupancy Sensor|Binding Cluster missing 0x0302|Temperature Sensor|Binding Cluster missing 0x0305|Pressure Sensor|Binding Cluster missing 0x0306|Flow Sensor|Binding Cluster missing 0x0307|Humidity Sensor|Binding Cluster missing 0x0850|On/Off Sensor|Good closures 0x000A|Door Lock|Binding Cluster missing 0x000B|Door Lock Controller|Good 0x0202|Window Covering|Binding Cluster missing 0x0203|Window Covering Controller|Good HVAC 0x0300|Heating/Cooling Unit|Good 0x0301|Thermostat|Binding Cluster missing 0x002B|Fan|Binding Cluster missing media 0x0028|Basic Video Player|Binding Cluster missing 0x0023|Casting Video Player|Binding Cluster missing 0x0022|Speaker|Binding Cluster missing 0x0024|Content App|Binding Cluster missing 0x0029|Casting Video Client|Good 0x002A|Video Remote Control|Good generic 0x0027|Mode Select|Binding Cluster missing

jvmahon commented 11 months ago

It also looks like all of these endpoints are created as Version 1 even where the Matter specification may specify version 2 or later as the Matter version. I think it would be preferable to default to the latest version available.

brdandu commented 11 months ago

The clusters defaults for device types are meta data driven so this will be a CHIP repo issue. These need to be updated https://github.com/project-chip/connectedhomeip/blob/master/src/app/zap-templates/zcl/data-model/chip/matter-devices.xml#L194 here. The device type versioning is not meta data driven however and defaults to 1 for now.

jvmahon commented 11 months ago

@brdandu - A few more things ...

Is it worth me trying to help

I see you assigned it to Silicon Labs - I could probably hand-code the entries, but would that be helpful? Or does Silicon Labs have the tools and like to maintain control over this file?

Unless you indicate that it would be helpful for me to try and touch up the file and generate a PR, I'll leave it to Si Labs internal work.

"All" versus "Legal" versus "Enabled" clusters filters

On the Zap tool, you can filter clusters for an endpoint by All, Legal, Enabled. The way the tool works for All and Enabled make sense. When I select "Legal", I'd expect to see all the clusters that I could enable for Matter, regardless of whether they have actually been enabled. That doesn't seem to be how it works though - when I select "Legal", I think I'm getting the same as "Enabled". Is that a misunderstanding of how the tool is supposed to work?

A later follow-up on cluster "Client" versus "Server"

I noticed in some cases the tool lets you pick Client or Server for certain cluster endpoints even though only one or the other is valid. Is there a way of coding the xml so as to only permit the valid choice (i.e., user can't select a "wrong" choice). If so, I could try to flag where the wrong choice is being permitted. If not, this would be a great addition to the tool to reduce user confusion.

This also relates to a feature request issue I had posted about here: https://github.com/project-chip/zap/issues/1150

A later follow-up on cluster attributes

FYI - I believe I'll have a subsequent follow-up on individual clusters where I've found attributes that, in a similar pattern, are either missing or present where they shouldn't be (specifically, I noticed that for FeatureMap a few times, but I didn't track that well, so I'd have to go back and look at the clusters again).

brdandu commented 11 months ago

@brdandu - A few more things ...

Is it worth me trying to help

There is some work being done to make this automated but that could take a while so until then yes it will be helpful if you can do this. I suggest you create a PR for the CHIP repo to update the xml as per the latest spec. (You can create a PR into https://github.com/project-chip/connectedhomeip from your own fork) . By the way I just put Silabs internal for most of the new and open issues which come up but we are always open to any development into our repo through PRs. :)

"All" versus "Legal" versus "Enabled" clusters filters

I believe Legal should show you all clusters associated to a device type as per the spec (be it enabled or disabled) if I am not wrong. @tecimovic is my understanding correct here?

A later follow-up on cluster "Client" versus "Server"

There are a couple of use cases where users may want to build on top of the ZCL/Data Model device type selected so we do not restrict the user from making such a selection. However, having an option such as strict mode is a good suggestion. We should implement that.

A later follow-up on cluster attributes

FeatureMap is a global attribute so it will have an instance for each cluster.

jvmahon commented 11 months ago

@brdandu

I've made the changes and the updated matter-devices.xml file can be found in my fork, here: https://github.com/jvmahon/connectedhomeip/tree/master/src/app/zap-templates/zcl/data-model/chip

I note that it looks like there were a few cases where I flagged a missing "Binding" cluster, but they were in the file -- is there a separate copy of this data that belongs in the Zap repository and gets pulled into the Zap executable downloads that would explain this?

I was ready to do a PR, but then noticed in a prior update, @bzbarsky-apple removed the "binding" cluster from the door lock 0x000A with a note that the binding was non-compliant. Since this is one of the devices where I am adding it, I didn't want to enter this until that could be checked.

For the Door Lock binding, I'm reading the Matter Device Library Specification Version 1.1 (the "MDL Spec") as requiring Binding for door lock, but there is some odd wording in the MDL Spec

Specifically, if I look at the spec for Door Lock, the cluster restriction section 8.1.5 of the MDL Spec says "It is OPTIONAL for a Door Lock controller device to support finding and binding". This wording may be a Zigbee holdover or may belong in 8.2.5. In any case, the intent is not clear. I say this because (a) the wording in Section 8.1.5 refers to "Door Lock controller" rather than just "Door Lock" (Door Lock "controller" is in Section 8.2), and (b) the wording refers to "finding and binding" which is a Zigbee concept.

Then I noticed in 8.2.5 of the Specification, which is for a Door Lock Controller device and that says ""It is OPTIONAL for a Door Lock Controller device to support EZ-Mode finding and binding"

Here the phrase more clearly limits what is optional to an "EZ-Mode" finding and binding implementation. I don't know why a Matter spec has this odd Zigbee "EZ-Mode" reference, but it does create some confusion about what should happen in Matter.

So, before entering this as a PR, the question is what the spec really means. I'm thinking Door Lock gets a Binding cluster, but wanted to be sure.

jvmahon commented 11 months ago

I think I misread Section 1.1.7 of the Base Cluster Requirements for Matter standard and Binding is not required for Simple devices unless they also have Client clusters, so this comment was wrong.

brdandu commented 11 months ago

@jvmahon I would not be surprised if the Matter spec has some Zigbee carry ons. Best to file those as issues in the CHIP repo if it feels incorrect. ZAP as a standalone does contain the xml which is mostly used for internal testing so you may be seeing a very old xml which is primarily used for testing(For eg when you run npm run zap).

In order to get the correct meta data for Matter within ZAP you need to run ZAP using the correct zcl.json(xml reference file) and gen-templates.json(code generation template reference) files. An easy way to do that is to use https://github.com/jvmahon/connectedhomeip/blob/master/scripts/tools/zap/run_zaptool.sh script

jvmahon commented 11 months ago

Thank you. I'm definitely seeing some "old" stuff in zap (or at least in some of the .zap example files - though its possible the examples were generated a while ago and problems are less now).

I appreciate the tip on starting Zap from the script folder. I'll do that going forward.

jvmahon commented 11 months ago

One more question. What does !Matter as shown in the image below mean (this particular example is from section 7.1.5 of the Device Library spec, but there are numerous examples like this). Does it mean: a) The QRY bit of the FeatureMap of Identify is set to 1 only if the device is not Matter or b) The QRY bit of the FeatureMap of Identify is set to 1 but only if the device is Matter or c) The QRY bit of the FeatureMap of Identify is set to 0 only if the device is not Matter or d) The QRY bit of the FeatureMap of Identify is set to 0 but only if the device is Matter or e) Something else?

Section 7.3.6 of the Matter Specification Version 1.1 attempts to explain this, but the wording is a bit convoluted.

Thanks!

image

bzbarsky-apple commented 11 months ago

What does !Matter as shown in the image below mean

That means "the QRY bit is 1 if and only if the device is not Matter".

Or in other words "QRY is Mandatory of not Matter and disallowed if Matter".

Basically conformance consists of a boolean condition and possible optionality annotation with O or []. If the optionality stuff is not present then the thing in question is required if the boolean is true, and disallowed if the boolean is false. If the optionality bits are present, then the thing is allowed if the boolean is true, and disallowed if the boolean is false.

bzbarsky-apple commented 11 months ago

I don't know why a Matter spec has this odd Zigbee "EZ-Mode" reference

Mostly because copy/paste, but also because people want to use the same text here for both Matter and Zigbee.... hence all the Matter/!Matter and Zigbee/!Zigbee conformance bits to limit which parts of the spec apply to which.