-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Migrate binding cluster to be code driven #40607
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
Conversation
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 migrates the binding cluster from using generated code patterns to a code-driven implementation, centralizing all binding-related functionality under the cluster directory.
- Migrates binding cluster to use the new code-driven architecture with a dedicated BindingCluster class
- Moves and renames EmberBindingTableEntry to BindingTableEntry in the cluster directory
- Relocates unit tests from the app/tests directory to the cluster-specific test directory
Reviewed Changes
Copilot reviewed 80 out of 81 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/lib/core/CHIPConfig.h | Adds configuration for maximum binding entries per fabric |
src/app/clusters/binding-server/binding-cluster.h | New cluster implementation with attribute handling methods |
src/app/clusters/binding-server/binding-cluster.cpp | Implementation of the code-driven binding cluster with read/write handling |
src/app/clusters/binding-server/binding-table.h | Moved and renamed binding table structures and class |
src/app/clusters/binding-server/binding-table.cpp | Moved binding table implementation with namespace updates |
src/app/clusters/binding-server/CodegenIntegration.cpp | New integration file for registering code-driven cluster |
src/app/util/types_stub.h | Removes old binding type definitions |
zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.h | Removes generated attribute accessors for binding |
Comments suppressed due to low confidence (1)
src/app/clusters/binding-server/CodegenIntegration.cpp:79
- Error message incorrectly mentions 'OTA' instead of 'Binding' when logging unregistration failure
ChipLogError(AppServer, "Failed to unregister OTA on endpoint %u: %" CHIP_ERROR_FORMAT, endpointId, err.Format());
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Code Review
This pull request is a significant and well-executed refactoring to migrate the Binding cluster to be a code-driven implementation. The changes are extensive, involving moving the binding table implementation, renaming EmberBindingTableEntry
to BindingTableEntry
, and updating numerous files to use the new structure. The new implementation is more modular and robust. I've found one issue in the new binding cluster implementation that could cause valid write operations to fail.
cb05fae
to
7dbbe74
Compare
d2c1645
to
3516539
Compare
PR #40607: Size comparison from b58b999 to 3516539 Increases above 0.2%:
Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
examples/tv-casting-app/tv-casting-common/include/CHIPProjectAppConfig.h
Show resolved
Hide resolved
PR #40607: Size comparison from 49cf915 to 6b7be68 Increases above 0.2%:
Full report (43 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
PR #40607: Size comparison from 2f77e85 to f6200c3 Full report (5 builds for cc32xx, realtek, stm32)
|
PR #40607: Size comparison from 2f77e85 to 799f5d0 Increases above 0.2%:
Full report (44 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
PR #40607: Size comparison from 2f77e85 to e243577 Increases above 0.2%:
Full report (44 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Summary
src/app/utils/
Related issues
Part of #36538
Testing