Initial version of the Descriptor cluster #5020

vivien-apple · 2021-02-25T16:10:58Z

Problem

The Descriptor cluster is not implemented in CHIP. The Descriptor cluster is meant to replace ZDO for the discovery of endpoints and clusters on a target node.

This PR introduce a first version of the Descriptor cluster with basic support for List[T] in the attributes store.

This PR contains the Descriptor cluster source code into src/app/clusters/descriptor. As a result the attributes are defined during the setup phase of the cluster.
I first did an initial version where the values of the attributes were statically defined when ZAP generates the file. It brings some size optimisations to the amount of memory consumes by the global attribute array but it may be hard to fit this model for the Bridge cluster.
I think we may still want to do that for devices with low capacity thought. So maybe a different PR could fix the ZAP tool, to optimise the data if the Bridge cluster is not activated.

Also this PR contains some basic support for List[T]. The tricky part was to get T to work with the current attribute code since this one expects T to be one of the defined basic ZCL types (e.g INT8U).
At the moment the current code support List[BASIC_TYPE] and List[Struct]. It does not yet support nested List. I have started to dig into it with the ACLcluster (intosrc/app/zap-templates/zcl/acl-cluster.xml`) but it it not fully done yet.

Finally, this PR needs to come up with a patch into the ZAP repo (PR # to come).

Summary of Changes

Add the definition of the Descriptor cluster into src/app/zap-templates/zcl/descriptor-cluster.xml
Add the definition of the ACL cluster into src/app/zap-templates/zcl/acl-cluster.xml
Add src/app/clusters/descriptor
Enable the descriptor server side into all-clusters-app.zap
Enable the descriptor server side into controller-clusters.zap
Enable the descriptor client side into chip-tool.zap
Add a template to encode/decode List[T] attribute

yufengwangca · 2021-02-25T19:36:39Z

src/app/zap-templates/zcl/descriptor-cluster.xml

+    <code>0xF003</code>
+    <define>DESCRIPTOR_CLUSTER</define>
+    <description>The Descriptor Cluster is meant to replace the support from the Zigbee Device Object (ZDO) for describing a node, its endpoints and clusters.</description>
+    <attribute side="server" code="0x0000" define="DEVICE" type="ARRAY" entryType="DeviceType" length="254" writable="false" optional="false">device</attribute>


Is there any other way to represent list other than type "ARRAY" to not use fixed length? The spec says the list should have one or more elements, why we set the length to 254 not other number?

Is there any other way to represent list other than type "ARRAY" to not use fixed length? The spec says the list should have one or more elements, why we set the length to 254 not other number?

I agree this is confusing, but the length argument here does not represent the size of the list, but the length in bytes of the related attribute.

In ZAP this argument is used for 2 things.

The first one is to generate the GENERATED_DEFAULTS macro and store potential defaults for the attribute. In this case, it is basically unused, but at some point we could likely generate it directly for simple accessories and it may save some spaces.

The second case where it is used is to set the EmberAfAttributeMetadata size field. This field is then used to allocate some spaces in the generatedData global array, that will be used to store the attribute value itself.

I end up using 254 since this is the maximum value authorised in EmberAfAttributeMedata currently, as it is a uint8_t. But the actual length of the list is stored in the first 2 bytes of the attribute value in this patch and is retrieved as an uint16_t.

The good thing about the current mechanism, where the length in bytes of attributes is defined ahead of time, is that you know how much space you need to store the attributes. The bad thing, is that it makes it harder for things such as list where the number of elements may vary.

I guess at some point we would have to look more deeper into the code that deals with attributes in the current ZCL stack...

vivien-apple · 2021-03-02T17:00:37Z

FTR, this PR needs project-chip/zap#114 to land in order to work properly.

andy31415 · 2021-03-03T19:02:37Z

@vivien-apple - could you fix merge errors (likely regenerate stuff)?

it would be nice to have this within this week - probably TE1 cannot directly use it, but it still seems a good thing to have very early to iterate upon.

CLAassistant · 2021-03-04T13:12:30Z

All committers have signed the CLA.

woody-apple · 2021-03-04T18:20:47Z

ping @vivien-apple

bzbarsky-apple · 2021-03-04T23:25:08Z

src/app/clusters/descriptor/descriptor.cpp

+        // The Descriptor Cluster instance on Endpoint 0 (zero) SHALL have a Parts attribute that lists all endpoint instances on a
+        // node instance.
+        // All local endpoints required by the device type(s) in the Device attribute SHALL be included in the Parts list.
+        if (endpoint == 0x00 || (emberAfDeviceIdFromIndex(endpoint) == emberAfDeviceIdFromIndex(endpointIndex)))


I'm not quite following that second condition. You can have multiple same-type devices on the node, and devices don't span endpoints...

This hierarchical endpoint relationship will need to be expressed in the app config (i.e. the .zap file) and get spit out by codegen somewhere; I don't think our current generated code representes it. Please file a followup issue to do that?

src/app/zap-templates/templates/app/attribute-size-src.zapt

examples/all-clusters-app/esp32/main/component.mk

vivien-apple · 2021-03-10T14:23:19Z

ping @vivien-apple

Sorry. I'm still waiting on project-chip/zap#114 to be reviewed and landed before rebasing. This PR can not land without it as it will break the tree otherwise :/

examples/all-clusters-app/esp32/main/component.mk

examples/chip-tool/templates/commands.zapt

bzbarsky-apple · 2021-04-08T21:14:41Z

examples/chip-tool/templates/commands.zapt

+static void On{{asCamelCased parent.name false}}{{asCamelCased name false}}ListAttributeResponse(void * context, uint16_t count, _{{type}} * entries)
+{{else}}
+static void On{{asCamelCased parent.name false}}{{asCamelCased name false}}ListAttributeResponse(void * context, uint16_t count, {{chipType}} * entries)


As a followup: we should strongly consider making the arg a Span<whatever-type>....

examples/chip-tool/templates/commands.zapt

src/app/clusters/descriptor/descriptor.cpp

src/app/util/attribute-storage.cpp

src/app/zap-templates/templates/app/CHIPClientCallbacks.zapt

src/app/zap-templates/templates/app/attribute-size-src.zapt

bzbarsky-apple

Looks good, with the remaining suggestions applied.

src/app/clusters/descriptor/descriptor.cpp

src/app/util/af.h

src/app/util/attribute-storage.cpp

src/app/zap-templates/templates/app/attribute-size-src.zapt

examples/chip-tool/templates/commands.zapt

src/app/util/af.h

Co-authored-by: Boris Zbarsky <[email protected]>

github-actions · 2021-04-09T19:38:08Z

Size increase report for "esp32-example-build" from 524bafa

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	1648	1648
chip-all-clusters-app.elf	.flash.rodata	1296	1296
chip-all-clusters-app.elf	.dram0.bss	0	1016

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,30791
.debug_line,0,13234
.debug_loc,0,4651
.debug_abbrev,0,3022
.debug_str,0,1858
.flash.text,1648,1648
.flash.rodata,1296,1296
.dram0.bss,1016,0
.strtab,0,543
.debug_frame,0,464
.debug_ranges,0,360
.symtab,0,304
.debug_aranges,0,184
.shstrtab,0,1
[Unmapped],0,-1296

github-actions · 2021-04-09T19:46:16Z

Size increase report for "nrfconnect-example-build" from 524bafa

File	Section	File	VM
chip-lighting.elf	text	208	208
chip-lighting.elf	rodata	48	52
chip-lock.elf	text	208	208
chip-lock.elf	rodata	48	48

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,15320
.debug_loc,0,4296
.debug_line,0,3758
.debug_abbrev,0,1542
.debug_str,0,1000
.debug_frame,0,388
text,208,208
.debug_ranges,0,176
.symtab,0,176
.strtab,0,159
.debug_aranges,0,104
rodata,52,48
.shstrtab,0,-3

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,15320
.debug_loc,0,4300
.debug_line,0,3754
.debug_abbrev,0,1542
.debug_str,0,992
.debug_frame,0,388
text,208,208
.debug_ranges,0,176
.symtab,0,176
.strtab,0,159
.debug_aranges,0,104
rodata,48,48
.shstrtab,0,1

* Add List[T] attribute support to src/app * Update the templates for List[T] supports * Add Descriptor cluster source code * Activate NetworkProvisiong, Descriptor and Group Key Management clusters * Update ZAP repo * Update the templates to fit ZAP update * Update gen/ folder * Update src/app/clusters/descriptor/descriptor.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Apply suggestions from code review * Update src/app/util/af.h * Add 'descriptor' to the list of built cluster for the all-clusters-app Co-authored-by: Boris Zbarsky <[email protected]>

vivien-apple self-assigned this Feb 25, 2021

boring-cyborg bot added the examples label Feb 25, 2021

restyled-io bot mentioned this pull request Feb 25, 2021

Restyle Initial version of the Descriptor cluster #5021

Closed

vivien-apple force-pushed the DescriptorCluster_v1 branch from 38386a3 to 17b8482 Compare February 25, 2021 16:56

pullapprove bot requested review from andy31415, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, msandstedt, mspang, saurabhst and woody-apple February 25, 2021 16:58

vivien-apple force-pushed the DescriptorCluster_v1 branch 3 times, most recently from fb0d5d2 to 34f6a7b Compare February 25, 2021 18:11

yufengwangca reviewed Feb 25, 2021

View reviewed changes

yufengwangca mentioned this pull request Feb 25, 2021

Implement the Descriptor Cluster #4921

Closed

andy31415 approved these changes Mar 2, 2021

View reviewed changes

jelderton approved these changes Mar 2, 2021

View reviewed changes

bzbarsky-apple reviewed Mar 4, 2021

View reviewed changes

dhrishi reviewed Mar 9, 2021

View reviewed changes

examples/all-clusters-app/esp32/main/component.mk Show resolved Hide resolved

pullapprove bot requested review from jepenven-silabs and jmartinez-silabs March 9, 2021 22:52

pullapprove bot requested a review from Damian-Nordic March 10, 2021 20:29

woody-apple requested a review from dhrishi April 8, 2021 00:19

vivien-apple force-pushed the DescriptorCluster_v1 branch from 8e9ab5c to 6af9d6e Compare April 8, 2021 09:19

bzbarsky-apple requested changes Apr 8, 2021

View reviewed changes

vivien-apple force-pushed the DescriptorCluster_v1 branch from 6af9d6e to 98dddbe Compare April 9, 2021 10:38

vivien-apple requested a review from bzbarsky-apple April 9, 2021 10:39

vivien-apple mentioned this pull request Apr 9, 2021

Passing write or readLength = 0 as an argument to emAfReadOrWriteAttribute is unsafe #5897

Closed

bzbarsky-apple approved these changes Apr 9, 2021

View reviewed changes

restyled-io bot mentioned this pull request Apr 9, 2021

Restyle Initial version of the Descriptor cluster #5906

Closed

bzbarsky-apple reviewed Apr 9, 2021

View reviewed changes

src/app/util/af.h Outdated Show resolved Hide resolved

msandstedt approved these changes Apr 9, 2021

View reviewed changes

vivien-apple and others added 11 commits April 9, 2021 20:28

Add List[T] attribute support to src/app

1e99d77

Update the templates for List[T] supports

f573794

Add Descriptor cluster source code

725d350

Activate NetworkProvisiong, Descriptor and Group Key Management clusters

6a521af

Update ZAP repo

8929a82

Update the templates to fit ZAP update

3d4377d

Update gen/ folder

824097b

Update src/app/clusters/descriptor/descriptor.cpp

bfe405a

Co-authored-by: Boris Zbarsky <[email protected]>

Apply suggestions from code review

fbc3dbc

Update src/app/util/af.h

8f6f762

Add 'descriptor' to the list of built cluster for the all-clusters-app

cfa1db0

vivien-apple force-pushed the DescriptorCluster_v1 branch from f17ba9b to cfa1db0 Compare April 9, 2021 18:37

mspang approved these changes Apr 9, 2021

View reviewed changes

mspang merged commit fca1757 into project-chip:master Apr 9, 2021

vivien-apple mentioned this pull request Apr 10, 2021

attribute-size.cpp is not included in the list of files to build for … #5936

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial version of the Descriptor cluster #5020

Initial version of the Descriptor cluster #5020

vivien-apple commented Feb 25, 2021

yufengwangca Feb 25, 2021

vivien-apple Mar 1, 2021

vivien-apple commented Mar 2, 2021

andy31415 commented Mar 3, 2021

CLAassistant commented Mar 4, 2021 •

edited

Loading

woody-apple commented Mar 4, 2021

bzbarsky-apple Mar 4, 2021

vivien-apple commented Mar 10, 2021

bzbarsky-apple Apr 8, 2021

bzbarsky-apple left a comment

github-actions bot commented Apr 9, 2021

github-actions bot commented Apr 9, 2021

Initial version of the Descriptor cluster #5020

Initial version of the Descriptor cluster #5020

Conversation

vivien-apple commented Feb 25, 2021

Problem

Summary of Changes

yufengwangca Feb 25, 2021

Choose a reason for hiding this comment

vivien-apple Mar 1, 2021

Choose a reason for hiding this comment

vivien-apple commented Mar 2, 2021

andy31415 commented Mar 3, 2021

CLAassistant commented Mar 4, 2021 • edited Loading

woody-apple commented Mar 4, 2021

bzbarsky-apple Mar 4, 2021

Choose a reason for hiding this comment

vivien-apple commented Mar 10, 2021

bzbarsky-apple Apr 8, 2021

Choose a reason for hiding this comment

bzbarsky-apple left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 9, 2021

github-actions bot commented Apr 9, 2021

CLAassistant commented Mar 4, 2021 •

edited

Loading