Closure control cluster server code implementation #37561
Draft
+587
−1
Conversation
This file contains 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
|
Review changes with Changed Files
|
|
PR #37561: Size comparison from 1b3f616 to 651fb3b Full report (3 builds for cc32xx, stm32)
|
src/app/clusters/closure-control-server/closure-control-server.cpp
Outdated
Show resolved
Hide resolved
jmartinez-silabs
approved these changes
Feb 14, 2025
bzbarsky-apple
requested changes
Feb 15, 2025
|
|
||
| // ------------------------------------------------------------------ | ||
| // Get attribute methods | ||
| virtual DataModel::Nullable<uint32_t> GetCountdownTime() = 0; |
There was a problem hiding this comment.
This is very likely to lead to buggy implementations that do not properly report attribute changes.
Small bits of state like the countdown time, overall state, resting procedure, etc should be owned by the cluster instance and pushed to it by the application. The cluster instance can then make sure to do the dirty-marking needed for subscriptions to work properly, implement the Q bits for CountdownTime, etc, etc.
There was a problem hiding this comment.
thanks for your input.
Sure will change the implementation to add Qbits for CountDownTime and change the Mainstate implementation to enable getter and setter for the attribute. Overall state and Target state will be handled by application as they are feature speicfic like (positioning, latching,speed and extra info).
Resting procedure and others are being part of the fallback feature, which is now being removed in spec. so will take it up after the spec changes.(https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/10879)
There was a problem hiding this comment.
They can be handled by the application, but stored in the cluster, by the way, if the copying of the data is not too much of a problem.
|
PR #37561: Size comparison from 7c1d6f7 to 236a68b Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
sabollim-silabs
changed the title
sabollim-silabs
changed the title
|
PR #37561: Size comparison from 5621ff6 to 5c130df Full report (3 builds for cc32xx, stm32)
|
| CHIP_ERROR Instance::SetOverallTarget(const DataModel::Nullable<Structs::OverallTargetStruct::Type> & aOverallTarget) | ||
| { | ||
| mOverallTarget = aOverallTarget; | ||
| MatterReportingAttributeChangeCallback(mDelegate.GetEndpointId(), mClusterId, Attributes::OverallTarget::Id); |
There was a problem hiding this comment.
Should we not check if it was really a different value before reporting calling MatterReportingAttributeChangeCallback ? For SetMainState you do add the check
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.
Testing
Enabled the cluster in all clusters app and build the server code locally.