-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add ServerClusterShim: A ServerClusterInterface wrapper for legacy (codegen/ember) clusters #39393
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
base: master
Are you sure you want to change the base?
Add ServerClusterShim: A ServerClusterInterface wrapper for legacy (codegen/ember) clusters #39393
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.
Code Review
This pull request introduces CodegenServerCluster
, a shim class implementing the ServerClusterInterface
to wrap Ember-based clusters. This is a valuable step for decoupling from Ember and enabling testing with the CodeDrivenDataModelProvider
. The implementation appears to be a good thin wrapper, and the added unit tests cover the new path validation logic effectively.
I have a few suggestions and questions regarding some TODOs and a potentially redundant check.
Summary of Findings
- Incomplete Attribute Quality Flags: TODOs exist for setting
kFabricScoped
,kFabricSensitive
, andkChangesOmitted
flags inAttributeEntryFrom
. Clarification is needed on whether these are in scope for the current shim. - AAI Control Over Data Versioning: A TODO in
WriteAttribute
notes that AAIs currently lack control over data version increments after a successful AAI write. This could be problematic for attributes with special versioning requirements. - Handling of
ChangesOmitted
Flag: A TODO inWriteAttribute
indicates thatdataInput.SetMarkDirty()
isn't yet configured based on theChangesOmitted
flag. - Incomplete
GetClusterFlags
Implementation: TheGetClusterFlags
method inCodegenServerCluster.h
is marked as a TODO and currently returns empty flags. - Redundant Path Check: There appears to be a redundant
ContainsClusterPath
check inCodegenServerCluster::InvokeCommand
after an initial check for the same path. - Copyright Year: New files use copyright year 2025. This is a minor issue and was not commented on due to review settings.
Merge Readiness
The pull request is a good step towards decoupling from Ember. However, there are several medium-severity issues, primarily around TODOs for unimplemented or partially implemented functionality related to attribute and cluster metadata (quality flags, data versioning with AAI, ChangesOmitted
flag handling) and a redundant check.
I recommend addressing these points, or at least clarifying the scope and future plans for the TODOs, before merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from other team members.
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.
My understanding was that the plan was to do this migration on the provider level (by having a multiplexing provider which redirected to both a codegen provider as needed and a non-codegen one as needed), not by jamming all the clusters into the "non-codegen provider" codepath... What changed?
In particular, what is the backwards compat story here?
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.
Nothing changes for legacy apps, they can continue to use the CodegenDataModelProvider
, which supports both legacy ember-based clusters and modern ones based on the ServerClusterInterface
.
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.
Then I am really confused by why we need this wrapper to start with, I guess. Why is the right thing to do to force existing cluster impls into the new provider setup instead of the plan we had initially (which is to allow multiple providers, one of which would handle the codegen bits and one of which handles whatever code-driven stuff we want to do)?
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.
Why is the right thing to do to force existing cluster impls
This PR is not forcing it. It doesn't change anything for existing clusters or providers. It just offers an opportunity for legacy clusters to be wrapped in the ServerClusterInterface
, totally optional.
What's your main concern here? Is it the location where I put those files? I could move them somewhere else if it makes it clearer that we're not changing the existing codegen model.
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.
I moved this under examples/common
and renamed it to ServerClusterShim
, let me know if it makes it clearer.
…es and added a readme
PR #39393: Size comparison from 60841f5 to c631367 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
PR #39393: Size comparison from ebbcfb7 to 1408a02 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
This reverts commit abda30a.
PR #39393: Size comparison from ebbcfb7 to fa958a1 Full report (50 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
PR #39393: Size comparison from ebbcfb7 to 7c228f6 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Server Cluster Shim
Overview
The
ServerClusterShim
provides a way to adapt legacy (codegen/ember-based) server cluster implementations to the newServerClusterInterface
. It acts as an intermediary layer, translating between the new interface methods and legacyemberAf...
calls. This allows developers to isolate the dependency on code generation to the cluster layer, instead of the leaking it to the data model provider layer.This
ServerClusterShim
enables us to develop a new Data Model Provider (fully code-driven) that doesn't depend on codegen/ember and can be tested against real clusters (while the clusters themselves are not all migrated to fully code-drviven yet).Note: this is meant to be a temporary option, until all cluster implementations have been migrated to implement the new interface. This is an effort that's currently in progress the SDK.
Usage
To use the
ServerClusterShim
, you need to:ServerClusterShim
providing the endpoint and cluster ID in to the constructorServerClusterShim
with multiple paths or one for each cluster.ServerClusterShim::Startup
to initialize the cluster with aServerClusterContext
.ServerClusterInterface
methods such as:ReadAttribute
,WriteAttribute
,InvokeCommand
, etc.See some other examples in
TestServerClusterShim.cpp
.Flash Size Impact
This PR has no flash size impact on existing implementation. It will be used in follow up PRs to build a new example application that demonstrates the new code driven data model provider. It's usage is totally optional, existing applications won't be forced to use this and can continue to use the existing cluster implementations with the
CodegenDataModelProvider
.Note: there is some code duplication with the
CodegenDataModelProvider
and that's OK because this is meant to be temporary. When we tried to the CodegenModel reuse the same code, we ended up with flash size increase, see 38265 for additional context, so we decided to use this as an optional supporting resource.Testing
You can build and run the unit tests with the following command:
ninja -C out examples/common/server-cluster-shim/tests:TestServerClusterShim && out/tests/TestServerClusterShim