Migrate Group Key Management Cluster to be Code Driven #40504
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zaid-google
added
the
cluster-decoupling
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively migrates the Group Key Management cluster to the new code-driven format, which is a great step towards decoupling. The changes are well-structured, including the new GroupKeyManagementCluster implementation, the CodegenIntegration.cpp for backward compatibility, and the addition of unit tests.
I've found a critical issue in the InvokeCommand implementation where Decode is called with an incorrect signature for several commands, which will cause a compilation failure. I've also pointed out a minor code hygiene issue regarding unused headers.
Once these issues are addressed, this PR will be in excellent shape.
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.h
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively migrates the Group Key Management cluster to the modern, code-driven model. The refactoring is clean, correctly removing the old attribute access and command callback mechanisms and replacing them with the DefaultServerCluster implementation. The addition of unit tests for the new cluster's metadata is a great touch. I have one piece of feedback regarding a missing attribute change notification, which is important for client synchronization.
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
|
PR #40504: Size comparison from 188dec4 to cd12528 Full report (3 builds for cc32xx, stm32)
|
zaid-google
force-pushed
the
code_driven_key_management
branch
from
bf91eac to
da3a681
Compare
|
PR #40504: Size comparison from 16070f1 to da3a681 Full report (33 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly migrates the Group Key Management cluster to the code-driven model. The changes are well-structured, including updates to the cluster implementation, build files, and the addition of unit tests. The refactoring follows the established patterns for code-driven clusters. I've found a critical issue in the WriteAttribute implementation that will prevent compilation and also misses attribute change notifications. I've also suggested adding a missing header file to support the fix. Once these are addressed, the PR should be in good shape.
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40504 +/- ##
=======================================
Coverage 50.68% 50.68%
=======================================
Files 1350 1352 +2
Lines 99236 99258 +22
Branches 12910 12916 +6
=======================================
+ Hits 50297 50309 +12
- Misses 48939 48949 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #40504: Size comparison from 16070f1 to 1b36526 Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
|
PR #40504: Size comparison from 16070f1 to d7a5560 Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
There was a problem hiding this comment.
Code Review
This pull request correctly refactors the Group Key Management cluster to the new code-driven model. The changes are well-structured and follow the established patterns for this kind of migration. The new implementation in group-key-mgmt-cluster.cpp is clean and the command/attribute handling logic is sound. One area for improvement is in testing. While the new test file TestGroupKeyManagementCluster.cpp is a good start, it only verifies the cluster's metadata (attributes and commands). I recommend adding more comprehensive unit tests that cover the actual logic of the command handlers and attribute accessors to ensure correctness and prevent future regressions.
src/app/clusters/group-key-mgmt-server/tests/TestGroupKeyManagementCluster.cpp
Show resolved
Hide resolved
|
PR #40504: Size comparison from 180c8ae to 35f1f58 Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Aug 13, 2025
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Aug 13, 2025
andy31415
reviewed
Aug 13, 2025
andy31415
reviewed
Aug 13, 2025
andy31415
reviewed
Aug 13, 2025
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Aug 13, 2025
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Aug 13, 2025
src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp
Outdated
Show resolved
Hide resolved
andy31415
approved these changes
Aug 13, 2025
There was a problem hiding this comment.
Generally looks good - since previous cluster used AAI, the existing code migration looks quite clean. Any improvements would likely apply to existing code as well and I am concerned that if we apply fixes we will actually also incur a flash overhead that is not due to migration.
One thing that is interesting is that this now correctly sets the cluster revision from spec and not from a hardcoded value (so we will start claiming revions 2 instead of 1). CS feature and its (non)implementation is awkward... we should probably completely remove that and not have partial non-working TODO.
Co-authored-by: Andrei Litvin <andy314@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
soares-sergio
approved these changes
Aug 13, 2025
This comment was marked as outdated.
This comment was marked as outdated.
This was referenced Aug 13, 2025
|
PR #40504: Size comparison from c16ef0f to c51aa3d Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
From what I can tell PSOC6 is not compiled with size-optimization for debug builds (which presumably is what our CI builds) so the size increase there seems larger. I think real size is the rest that seem to be at O(200 bytes) and often under. |
I checked flags and it does use -Os. I am unclear why this compiles differently... |
andy31415
added
the
sdk-maintainer-approved
|
Maintainer approved: changes in size seem to be 200 bytes or lower for most cases. PSOC6 is larger for an unknown reason (I checked we compile with Pinged silabs folks about efr32 oddities (one build shows small size is possible, but other boards/apps show a larger size increase which I do not understand). We have to check that separately or accept it. @jepenven-silabs @jmartinez-silabs |
dsavitsky-dsr
pushed a commit
to dsavitsky-dsr/connectedhomeip
that referenced
this pull request
Aug 19, 2025
…40504) * Initial seperation of cluster * Update config * Codegen * Complete initial cluster impl, update build files * Add codegen integration * Fix build issues * Use codegen mandatory attributes * Use anon namespace, remove old cluster code * Add unit tests * Restyled by whitespace * Restyled by clang-format * Restyled by gn * Remove old header includes * Remove unused variable * Update BUILD.gn for test * Update config and CodegenIntegration * Codegen * Remove unneeded chip::app references * Restyled by clang-format * Codegen * Add NotifyAttributeChangedIfSuccess on WriteAttribute * Fix CodegenIntegration callback * Update src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp Co-authored-by: Andrei Litvin <andy314@gmail.com> * Address review comments * Restyled by whitespace * Restyled by clang-format --------- Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Andrei Litvin <andy314@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR is re-writing the group key management cluster to follow the new code-driven format, as part of the cluster decoupling effort. These changes include:
group-key-mgmt-server.cppfilegroup-key-mgmt-cluster.h/cppwhich will hold code for the cluster functionality. This newGroupKeyManagementClusterextends theDefaultServerClusterand implements the required functions from it to maintain old functionalityCodgenIntegration.cppfor backwards compatibilityRelated issues
#40263
Testing