Add a way to run custom cluster logic for attribute reads #9667

bzbarsky-apple · 2021-09-14T00:43:47Z

Problem

At the moment we can either have cluster attributes living in the "global" attribute store or get an application-level callback called by marking them "external".

Change overview

Add a way to have SDK-internal logic run to produce attribute values, in TLV form only.

This mechanism, when it's used, gets first crack at the value; if it doesn't want to provide one we fall back to asking the attribute store (which might call into the external attribute machinery).

For review purposes: It's probably best to review the three changesets one at a time.

Testing

This part I need to sort out. I don't know how #9587 was tested, but we should presumably do the same tests here to verify that this all works. @yufengwangca

todo · 2021-09-14T00:43:48Z

We should probably clone the writer and convert failures here

connectedhomeip/src/app/util/ember-compatibility-functions.cpp

Lines 206 to 216 in a3951b5

    
           // TODO: We should probably clone the writer and convert failures here 
        
           // into status responses, unless our caller already does that. 
        
           ReturnErrorOnFailure(interceptor->ReadAttributeData(aClusterInfo, apWriter, &dataRead)); 
        
           if (dataRead) 
        
           { 
        
               // TODO: Add DataVersion support 
        
               ReturnErrorOnFailure( 
        
                   apWriter->Put(chip::TLV::ContextTag(AttributeDataElement::kCsTag_DataVersion), kTemporaryDataVersion)); 
        
               return CHIP_NO_ERROR; 
        
           }

This comment was generated by todo based on a `TODO` comment in `a3951b5` in #9667. cc @bzbarsky-apple.

todo · 2021-09-14T00:43:51Z

Add DataVersion support

connectedhomeip/src/app/util/ember-compatibility-functions.cpp

Lines 212 to 221 in a3951b5

    
                   // TODO: Add DataVersion support 
        
                   ReturnErrorOnFailure( 
        
                       apWriter->Put(chip::TLV::ContextTag(AttributeDataElement::kCsTag_DataVersion), kTemporaryDataVersion)); 
        
                   return CHIP_NO_ERROR; 
        
               } 
        
           } 
        
           EmberAfAttributeType attributeType; 
        
           EmberAfStatus status; 
        
           status = emberAfReadAttribute(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mFieldId, CLUSTER_MASK_SERVER,

This comment was generated by todo based on a `TODO` comment in `a3951b5` in #9667. cc @bzbarsky-apple.

src/app/AttributeAccessInterceptor.h

src/app/util/ember-compatibility-functions.cpp

src/app/util/attribute-storage.cpp

src/app/util/ember-compatibility-functions.cpp

src/app/util/attribute-storage.cpp

yufengwangca · 2021-09-14T04:48:22Z

Well, I am not able to find a practical way to verify the correctness of those diagnostic attribute in CI, since we don't have the reference value of those physical info from platform in real-time.

The test was done by reading those diagnostic Ethernet network attributes using chip-tool , such as ./chip-tool ethernetnetworkdiagnostics read packet-rx-count 0, then compare them with the output of ifconfig and netstat.

vivien-apple

As a next step I would be happy to hear your thoughts on how to connect this machinery to ZAP in order to not have to generate useless defaults in this case (thinking about the descriptor cluster use case...).

Since interceptors are not granular at the AttributeId level, it may make it hard to know which attributes needs generated defaults and which one does not.

src/app/util/attribute-storage.cpp

src/app/zap-templates/templates/app/helper.js

src/app/clusters/ethernet_network_diagnostics_server/ethernet_network_diagnostics_server.cpp

src/app/util/ember-compatibility-functions.cpp

src/app/clusters/ethernet_network_diagnostics_server/ethernet_network_diagnostics_server.cpp

src/app/util/attribute-storage.cpp

src/app/clusters/ethernet_network_diagnostics_server/ethernet_network_diagnostics_server.cpp

src/app/util/attribute-storage.cpp

bzbarsky-apple · 2021-09-14T17:21:04Z

As a next step I would be happy to hear your thoughts on how to connect this machinery to ZAP

Yes, we need to do that. The basic problem is that the source of truth for what the cluster is doing is the cluster code, but we need to get information like "don't generate storage for this" into the ZAP pipeline.

I don't have any better ideas so far than @tecimovic's suggestion to have an SDK configuration thing where we basically have a list we provide to ZAP of attributes to not generate storage for....

bzbarsky-apple · 2021-09-15T07:56:08Z

@msandstedt @saurabhst @LuDuda @andy31415 @mspang @jepenven-silabs Please take a look?

…e reads. This allows a cluster implementation to provide custom logic for an attribute read without requiring the attribute to be marked external and needing to trampoline through the app callback.

…tics cluster. This just adds the class and registration machinery for it. It does not produce any data yet. The build system changes are for various example apps that enabled the diagnostics clusters in their ZAP configuration but did not actually compile in the code for them. Those stop linking with this change, since they don't include the cluster init function.

Instead handle ethernet network diagnostics via an attribute override.

woody-apple · 2021-09-15T17:08:22Z

Looks ready for review. @msandstedt @andy31415 @mspang @Damian-Nordic @LuDuda ?

github-actions · 2021-09-15T17:29:50Z

Size increase report for "gn_qpg-example-build" from 4763c91

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	508	508
chip-qpg6100-lighting-example.out	.bss	0	24
chip-qpg6100-lighting-example.out	.data	4	4
chip-qpg6100-lighting-example.out	.heap	0	-24

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,59039
.debug_line,0,3282
.debug_str,0,3130
.debug_loc,0,2656
.debug_abbrev,0,2461
.strtab,0,1203
.symtab,0,768
.text,508,508
.debug_ranges,0,400
.debug_frame,0,268
.debug_aranges,0,104
.bss,24,0
.data,4,4
.shstrtab,0,1
.heap,-24,0
[ELF Program Headers],0,-32
[Unmapped],0,-476

github-actions · 2021-09-15T17:50:37Z

Size increase report for "nrfconnect-example-build" from 4763c91

File	Section	File	VM
chip-lock.elf	text	432	432
chip-lock.elf	rodata	72	72
chip-lock.elf	[LOAD #3 [RW]]	0	19
chip-lock.elf	bss	0	13
chip-lock.elf	init_array	4	4
chip-lock.elf	device_handles	-4	-4

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-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,78306
.debug_line,0,7095
.debug_str,0,5119
.debug_abbrev,0,4659
.debug_loc,0,2837
.strtab,0,1336
.symtab,0,816
text,432,432
.debug_ranges,0,424
.debug_frame,0,312
.debug_aranges,0,136
rodata,72,72
[LOAD #3 [RW]],19,0
bss,13,0
init_array,4,4
device_handles,-4,-4

github-actions · 2021-09-15T18:07:09Z

Size increase report for "esp32-example-build" from 4763c91

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	484	484
chip-all-clusters-app.elf	.flash.rodata	96	96
chip-all-clusters-app.elf	.dram0.bss	0	24
chip-all-clusters-app.elf	.dram0.heap_start	0	-8

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,48668
.debug_line,0,3286
.debug_str,0,3036
.debug_loc,0,1768
.debug_abbrev,0,1364
.strtab,0,1211
.flash.text,484,484
.debug_ranges,0,376
.symtab,0,304
.debug_frame,0,280
.debug_aranges,0,104
.flash.rodata,96,96
.dram0.bss,24,0
.dram0.data,0,8
.shstrtab,0,1
.riscv.attributes,0,-2
.dram0.heap_start,-8,0
[Unmapped],0,-588

yufengwangca · 2021-09-16T17:20:02Z

@jepenven-silabs @msandstedt ?

…ip#9667) * Add machinery to allow a cluster implementation to intercept attribute reads. This allows a cluster implementation to provide custom logic for an attribute read without requiring the attribute to be marked external and needing to trampoline through the app callback. * Add a no-op attribute access override to the Ethernet Network Diagnostics cluster. This just adds the class and registration machinery for it. It does not produce any data yet. The build system changes are for various example apps that enabled the diagnostics clusters in their ZAP configuration but did not actually compile in the code for them. Those stop linking with this change, since they don't include the cluster init function. * Stop using the external attribute callback in lighting-app. Instead handle ethernet network diagnostics via an attribute override. * Disallow registering conflicting attribute access overrides.

bzbarsky-apple requested review from mrjerryjohns and yufengwangca September 14, 2021 00:43

boring-cyborg bot added app examples labels Sep 14, 2021

pullapprove bot requested review from andy31415, chrisdecenzo, Damian-Nordic, hawk248, jepenven-silabs, msandstedt and woody-apple September 14, 2021 00:45

pullapprove bot added the review - pending label Sep 14, 2021

bzbarsky-apple force-pushed the add-attr-interceptor branch from a3951b5 to c2659d0 Compare September 14, 2021 00:48

mrjerryjohns reviewed Sep 14, 2021

View reviewed changes

src/app/AttributeAccessInterceptor.h Outdated Show resolved Hide resolved

mrjerryjohns reviewed Sep 14, 2021

View reviewed changes

src/app/util/ember-compatibility-functions.cpp Outdated Show resolved Hide resolved

mrjerryjohns reviewed Sep 14, 2021

View reviewed changes

src/app/util/attribute-storage.cpp Outdated Show resolved Hide resolved

mrjerryjohns reviewed Sep 14, 2021

View reviewed changes

src/app/util/ember-compatibility-functions.cpp Show resolved Hide resolved

mrjerryjohns reviewed Sep 14, 2021

View reviewed changes

src/app/util/attribute-storage.cpp Outdated Show resolved Hide resolved

bzbarsky-apple force-pushed the add-attr-interceptor branch from c2659d0 to 6216258 Compare September 14, 2021 03:04

bzbarsky-apple requested review from tcarmelveilleux and vivien-apple September 14, 2021 14:16

vivien-apple reviewed Sep 14, 2021

View reviewed changes

src/app/util/attribute-storage.cpp Show resolved Hide resolved

src/app/util/attribute-storage.cpp Outdated Show resolved Hide resolved

yufengwangca reviewed Sep 14, 2021

View reviewed changes

bzbarsky-apple mentioned this pull request Sep 14, 2021

Create a helper for endpoint-independent attribute access interceptors #9694

Open

mrjerryjohns reviewed Sep 14, 2021

View reviewed changes

woody-apple approved these changes Sep 14, 2021

View reviewed changes

woody-apple added the SDK Approved label Sep 14, 2021

bzbarsky-apple force-pushed the add-attr-interceptor branch from 0c05150 to 8bdbe09 Compare September 15, 2021 00:03

boring-cyborg bot added the platform label Sep 15, 2021

mrjerryjohns approved these changes Sep 15, 2021

View reviewed changes

yufengwangca approved these changes Sep 15, 2021

View reviewed changes

bzbarsky-apple added 4 commits September 15, 2021 12:39

Add machinery to allow a cluster implementation to intercept attribut…

ae33570

…e reads. This allows a cluster implementation to provide custom logic for an attribute read without requiring the attribute to be marked external and needing to trampoline through the app callback.

Stop using the external attribute callback in lighting-app.

ee1d9ac

Instead handle ethernet network diagnostics via an attribute override.

Disallow registering conflicting attribute access overrides.

7dbfb82

bzbarsky-apple force-pushed the add-attr-interceptor branch from 8bdbe09 to 7dbfb82 Compare September 15, 2021 16:59

andy31415 approved these changes Sep 15, 2021

View reviewed changes

mrjerryjohns mentioned this pull request Sep 16, 2021

Add support for encode / decode TLV structs from / to native structs #8411

Closed

pullapprove bot requested review from jelderton, jmartinez-silabs, LuDuda and saurabhst September 16, 2021 17:39

jmartinez-silabs approved these changes Sep 17, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Sep 17, 2021

bzbarsky-apple merged commit c1afafe into project-chip:master Sep 17, 2021

todo bot mentioned this pull request Sep 17, 2021

We should probably clone the writer and convert failures here #9789

Closed

bzbarsky-apple deleted the add-attr-interceptor branch September 17, 2021 14:21

yufengwangca mentioned this pull request Sep 21, 2021

Change storage type of Ethernet diagnostic attributes to RAM #9869

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to run custom cluster logic for attribute reads #9667

Add a way to run custom cluster logic for attribute reads #9667

bzbarsky-apple commented Sep 14, 2021 •

edited

Loading

todo bot commented Sep 14, 2021

todo bot commented Sep 14, 2021

yufengwangca commented Sep 14, 2021

vivien-apple left a comment •

edited

Loading

bzbarsky-apple commented Sep 14, 2021

bzbarsky-apple commented Sep 15, 2021

woody-apple commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

yufengwangca commented Sep 16, 2021

Add a way to run custom cluster logic for attribute reads #9667

Add a way to run custom cluster logic for attribute reads #9667

Conversation

bzbarsky-apple commented Sep 14, 2021 • edited Loading

Problem

Change overview

Testing

todo bot commented Sep 14, 2021

We should probably clone the writer and convert failures here

This comment was generated by todo based on a TODO comment in a3951b5 in #9667. cc @bzbarsky-apple.

todo bot commented Sep 14, 2021

Add DataVersion support

This comment was generated by todo based on a TODO comment in a3951b5 in #9667. cc @bzbarsky-apple.

yufengwangca commented Sep 14, 2021

vivien-apple left a comment • edited Loading

Choose a reason for hiding this comment

bzbarsky-apple commented Sep 14, 2021

bzbarsky-apple commented Sep 15, 2021

woody-apple commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

yufengwangca commented Sep 16, 2021

bzbarsky-apple commented Sep 14, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `a3951b5` in #9667. cc @bzbarsky-apple.

This comment was generated by todo based on a `TODO` comment in `a3951b5` in #9667. cc @bzbarsky-apple.

vivien-apple left a comment •

edited

Loading