Split out Attribute Persistance between safe (can be at libCHIP level) and ember-specific (placed in codegen level) #36658
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
- keeps persistence and safe persistence as interfaces - implementations are now Separate - let server initialize the default safe persistence - update codegen provider to initialize the ember side
|
Review changes with |
github-actions
bot
added
documentation
|
PR #36658: Size comparison from 7805664 to 300fb85 Full report (22 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, qpg, stm32, telink, tizen)
|
|
PR #36658: Size comparison from 7805664 to 067968a Full report (39 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36658: Size comparison from 7805664 to 4f95762 Full report (80 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
…s for a run to timeout, set individual tests to short
|
PR #36658: Size comparison from 2474fc3 to 3a409df Full report (80 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
harimau-qirex
approved these changes
Dec 3, 2024
Co-authored-by: Andy Salisbury <[email protected]>
arkq
approved these changes
Dec 5, 2024
|
PR #36658: Size comparison from b0fd385 to 0665c32 Full report (80 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| #include <app/codegen-data-model-provider/CodegenDataModelProvider.h> | ||
| #include <app/codegen-data-model-provider/Instance.h> | ||
|
|
||
| namespace chip { | ||
| namespace app { | ||
|
|
||
| DataModel::Provider * CodegenDataModelProviderInstance() | ||
| DataModel::Provider * CodegenDataModelProviderInstance(PersistentStorageDelegate * delegate) |
There was a problem hiding this comment.
Having this mutate the state of the data model provider on each get is really weird. Why are we doing that?
It seems like the fundamental issue is that we have this global getter at all, instead of just letting people who want a CodegenDataModelProvider instantiate a CodegenDataModelProvider.
There was a problem hiding this comment.
I could not come up with a saner "object is defined elsewhere" solution.
Technically this could be part of the constructor, but only if applications are responsible to allocate such an object.
Would accept any proposals to make this different.
There was a problem hiding this comment.
but only if applications are responsible to allocate such an object.
Right. Why are we not doing that?
2 tasks
yyzhong-g
pushed a commit
to yyzhong-g/connectedhomeip
that referenced
this pull request
Dec 12, 2024
…) and ember-specific (placed in codegen level) (project-chip#36658) * Separate out logic between Safe and Ember storage persistence. - keeps persistence and safe persistence as interfaces - implementations are now Separate - let server initialize the default safe persistence - update codegen provider to initialize the ember side * Some manual updates * Scripted update * Restyle * Some manual updates again * More manual updates. Only tests remain (and these are odd now ....) * Update tests and support for nullptr, manual fix for one compilation * Remove ember dependency from server * Fix more typos for compilation * Restyle * Add a note about cadmin 1_19 being slow * Fix up link to PR for persistence * Restyled by prettier-markdown * Mark one more slow test * Add a few more dependencies to make esp32 link * fix data model paths * Fix qpg and nrf compile * Fix a few more tests * Restyled by clang-format * Fix another typo for NRF compile * Add persistence cpp files to darwin project * Fix typo * Drop timeouts for java tests: it is not reasonable to wait 360 minutes for a run to timeout, set individual tests to short * Add back deleted line from before * Restyled by clang-format * Update src/app/DefaultSafeAttributePersistenceProvider.h Co-authored-by: Andy Salisbury <[email protected]> --------- Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Andy Salisbury <[email protected]>
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 #36476
Fixes #36472
Changes
Set...andGet...methods out fromDefaultPersistenceProviderand place those together with the cpp corresponding to the interface header.StorageDelegateWrapperwhich places a key/bytespan layer on top of a storage provider delegate since this seems to be a reusable unit across our attribute persistence logic.