Add ListNotifications on DataModelProvider #37526
Merged
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 |
|
PR #37526: Size comparison from 9c8cb33 to 5d233a2 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
added a commit
to andy31415/connectedhomeip
that referenced
this pull request
Feb 12, 2025
This defines a ServerClusterInterface class along with a registry. Also slight update to build_coverage to run on more (specifically my) machines without errors. Implementation notes: - Registry does NOT allow anymore to have "wildcard register" on endpoints. This is because we expect to have attribute members (no more "free/implicit" storage from ember buffers). - Interface now includes metadata for attributes (AAI does not) and maintains the cluster version. - This does not yet include changes in project-chip#37526 (so no AAI replacement for list begin/end) as that interface gets finalized. It will have it once that PR is finalized and merged. - Registry has no "cache" but uses a "move found item to front of list". This is ok for individual requests however for full iterations it would be slower (about 2x if we always iterate over all clusters ... since then average search would be N instead of N/2 for a list that never changes). We can revisit this approach at any time This is currently unused. I expect usages to add flash & RAM cost, that will be slowly offset (especially RAM) as we move clusters around: - RAM for linked list will be one list instead of 2 (AAI to CHI) so any cluster moved that uses both would save 1 pointer metadata will increase slightly, can be reduced if we get ember to stop generating metadata for included clusters - RAM can decrease by FeatureMap and ClusterRevision if we stop ember from allocating space for it (however we will offset this by flash "encode const value"). - RAM usage increases slightly during conversion for "store version data" which ember currently allocates for all clusters. - Very long term: if we replace all clusters, we can drop AAI/CHI and that would save some flash. Still expect no savings because new interface does all of AAI and CHI and a bit more. TLDR on resourcing: probably ok on RAM over time, there is a flash overhead for this, claiming this is important for testable and maintainable code in the future.
|
PR #37526: Size comparison from 9c8cb33 to 1a61b04 Increases above 0.2%:
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tersal
changed the title
andy31415
approved these changes
Feb 18, 2025
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
|
PR #37526: Size comparison from 78e5932 to f6d0f1c Increases above 0.2%:
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
approved these changes
Feb 27, 2025
andy31415
added a commit
to andy31415/connectedhomeip
that referenced
this pull request
Mar 3, 2025
This defines a ServerClusterInterface class along with a registry.
Also slight update to build_coverage to run on more (specifically my) machines without errors.
Implementation notes:
- Registry does NOT allow anymore to have "wildcard register" on endpoints. This is because we expect to have
attribute members (no more "free/implicit" storage from ember buffers).
- Interface now includes metadata for attributes (AAI does not) and maintains the cluster version.
- This does not yet include changes in project-chip#37526 (so no AAI replacement for list begin/end) as that interface
gets finalized. It will have it once that PR is finalized and merged.
- Registry has no "cache" but uses a "move found item to front of list". This is ok for individual requests
however for full iterations it would be slower (about 2x if we always iterate over all clusters ... since
then average search would be N instead of N/2 for a list that never changes). We can revisit this approach at any time
This is currently unused. I expect usages to add flash & RAM cost, that will be slowly offset (especially RAM) as
we move clusters around:
- RAM for linked list will be one list instead of 2 (AAI to CHI) so any cluster moved
that uses both would save 1 pointer metadata will increase slightly, can be reduced
if we get ember to stop generating metadata for included clusters
- RAM can decrease by FeatureMap and ClusterRevision if we stop ember from allocating
space for it (however we will offset this by flash "encode const value").
- RAM usage increases slightly during conversion for "store version data" which ember currently
allocates for all clusters.
- Very long term: if we replace all clusters, we can drop AAI/CHI and that would save some flash.
Still expect no savings because new interface does all of AAI and CHI and a bit more.
TLDR on resourcing: probably ok on RAM over time, there is a flash overhead for this,
claiming this is important for testable and maintainable code in the future.
gmarcosb
pushed a commit
to gmarcosb/connectedhomeip
that referenced
this pull request
Mar 4, 2025
* Add ListNotifications on DataModelProvider * Restyled by whitespace * Restyled by clang-format * Rename enum values and add more information in comments. * Update src/app/data-model-provider/Provider.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Remove BitFlags from Provider --------- Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Boris Zbarsky <bzbarsky@apple.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.
Fixes #37307
The intention is to remove direct calls to AAI from WriteHandler, instead doing it through calls to DataModel::Provider. This to allow a more complete decoupling between these layers.
Testing
Tested with Unit Test suites.