-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
matter: samples: Implement factory reset for persistent storage #20803
Conversation
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 project with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 9cb1245d9895695c004516645505677cfed637e2 more detailssdk-nrf:
matter:
Github labels
List of changed files detected by CI (17)
Outputs:ToolchainVersion: acee3b0b2b Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
You can find the documentation preview for this PR 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.
@adigie is the current implementation against the spec for door lock cluster?
@@ -20,12 +20,14 @@ class PersistentStorageSecure { | |||
PSErrorCode _NonSecureLoad(PersistentStorageNode *node, void *data, size_t dataMaxSize, size_t &outSize); | |||
PSErrorCode _NonSecureHasEntry(PersistentStorageNode *node); | |||
PSErrorCode _NonSecureRemove(PersistentStorageNode *node); | |||
PSErrorCode _NonSecureRemoveSubtree(const char *prefix); |
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.
Maybe we could pass the prefix when initializing NonSecure storage. I would make possible to have unified FactoryReset for Secure and NonSecure instead of Settings-specific RemoveSubtree
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 would have to add prefix
argument to SecureInit
and NonSecureInit
and ignore it SecureInit
implementation. Would that be OK?
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.
Yehh I know it's not that obvious which approach is better for the end user. Maybe we could have a default parameter set, or use std::optional? But this would require a bit more logic in both backends. And for the access_storage... maybe some preprocessor magic would work. Just throwing some ides, if none of them works, I'm ok with the current proposal.
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.
Updated implementation.
Yes, please see this comment: |
Remove old code handling clearing users/credentials from fabric as it's unused and not spec compilant. Signed-off-by: Adrian Gielniewski <adrian.gielniewski@nordicsemi.no>
* Handle factory reset for secure/non-secure configurations. * Add `FactoryReset` method to `AccessStorage` and `BridgeStorageManager`. * Add application specific factory reset handlers. Signed-off-by: Adrian Gielniewski <adrian.gielniewski@nordicsemi.no>
Update sdk-connectedhomeip revision Signed-off-by: Adrian Gielniewski <adrian.gielniewski@nordicsemi.no>
|
unused and not spec compilant.
FactoryReset
method toAccessStorage
.