-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Set Identify ClusterRevision as external and implement read handler #41287
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
Set Identify ClusterRevision as external and implement read handler #41287
Conversation
…and use the generated MetaData Revision constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements external cluster revision handling for the Identify cluster to align with specification changes. The revision is updated from 5 to 6 and the implementation moves from RAM storage to external read handlers.
- Makes the Identify cluster's ClusterRevision attribute external and implements read handler using generated constants
- Updates the cluster XML to sync with spec revision 6 using alchemy
- Regenerates all .zap and .matter files to reflect the external attribute configuration
Reviewed Changes
Copilot reviewed 126 out of 126 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/zap-templates/zcl/zcl.json | Adds Identify cluster to external attributes list |
| src/app/zap-templates/zcl/zcl-with-test-extensions.json | Adds Identify cluster to external attributes list |
| src/app/zap-templates/zcl/data-model/chip/identify-cluster.xml | Updates cluster revision from 5 to 6 and other spec alignment changes |
| src/app/clusters/identify-server/identify-server.h | Adds IdentifyAttrAccess class declaration for external attribute handling |
| src/app/clusters/identify-server/identify-server.cpp | Implements read handler for ClusterRevision attribute using Identify::kRevision constant |
| zzz_generated/app-common/clusters/Identify/Metadata.h | Updates revision constant from 5 to 6 |
| zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.h | Removes ClusterRevision accessor declarations |
| zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp | Removes ClusterRevision accessor implementations |
| scripts/tools/zap/tests/outputs/.../endpoint_config.h | Updates attribute configuration to external storage |
| examples/.../...matter | Updates cluster revision from 5 to 6 and changes from ram to callback attributes |
Comments suppressed due to low confidence (1)
|
PR #41287: Size comparison from b0f2fa3 to 75f1bb3 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
This is probably completely superseeded by #41232 . However it also is probably low risk and shows that doing the right thing adds a flash overhead by itself. I guess merge will not be too bad and then we see overhead of code driven compared to AAI on the other PR ... so we can merge this one. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41287 +/- ##
==========================================
+ Coverage 50.96% 50.97% +0.01%
==========================================
Files 1378 1378
Lines 100597 100610 +13
Branches 13023 13018 -5
==========================================
+ Hits 51267 51284 +17
+ Misses 49330 49326 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
74ceb39 to
6ec64bd
Compare
|
PR #41287: Size comparison from 42525d0 to 6ec64bd Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41287: Size comparison from cfa7f13 to 7ad735e Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Summary
The Cluster Revision should not be set by a Zap config. It shall follow the spec changes and xml updates we do, based on the spec/alchemy results.
This PR is one of a series I'll complete over the next couple of days.
What is done :
Set the Cluster Revision attribute of the Identify cluster to always be external by listing the attribute in
Implement the Read handler for the Cluster Revision Attribute in
src/app/clusters/identify-server/identify-server.cppUse the generated constant
Identify::kRevision, fromclusters/Identify/Metadata.h, as the ClusterRevision value to be encoded.The Identify.xml was out of sync with the spec by 1 revision. Changes are small so I integrated it in this pr.
Commit 2 Updates src/app/zap-templates/zcl/data-model/chip/identify-cluster.xml using alchemy
alchemy zap --sdkRoot=./connectedhomeip --specRoot=./connectedhomeip-spec './connectedhomeip-spec/src/app_clusters/Identify.adoc'Finally, in a separate commit. I ran ./scripts/tools/zap_regen_all.py to update all .zap and .matter files as the attribute changes from RAM to External
Related issues
n/a
Testing
Build and commission the SIlabs Lighting-app. Use chip-tool to read the ClusterRevision of the Identify cluster.
Readability checklist
commit 1 is my manual changes
commit 2 is alchemy generation changes
commit 3 is zap regen changes