Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

adigie
Copy link
Member

@adigie adigie commented Mar 7, 2025

  • Remove old code handling clearing users/credentials from fabric as it's
    unused and not spec compilant.
  • Handle factory reset for secure/non-secure configurations.
  • Add FactoryReset method to AccessStorage.
  • Add application specific factory reset handler.
  • Update sdk-connectedhomeip revision.

@adigie adigie requested review from a team as code owners March 7, 2025 15:26
@github-actions github-actions bot added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Mar 7, 2025
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Mar 7, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
matter nrfconnect/sdk-connectedhomeip@4c46941 nrfconnect/sdk-connectedhomeip#569 nrfconnect/sdk-connectedhomeip#569/files

DNM label due to: 1 project with PR revision

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Mar 7, 2025

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 3

Inputs:

Sources:

sdk-nrf: PR head: 9cb1245d9895695c004516645505677cfed637e2
matter: PR head: 6ce9516c5d601c4b0cfa293aba82d73a09107a1d

more details

sdk-nrf:

PR head: 9cb1245d9895695c004516645505677cfed637e2
merge base: 889f9297e37700d95c4c1836a6b0dbc3afd29aa6
target head (main): 889f9297e37700d95c4c1836a6b0dbc3afd29aa6
Diff

matter:

PR head: 6ce9516c5d601c4b0cfa293aba82d73a09107a1d
merge base: 4c46941ef17b7b755c96c57e4ba42306cc524e1c
target head (master): 2c3f0a05295196b56e2c658686a1136f75d993df
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (17)
applications
│  ├── matter_bridge
│  │  ├── src
│  │  │  │ main.cpp
modules
│  ├── lib
│  │  ├── matter
│  │  │  ├── src
│  │  │  │  ├── platform
│  │  │  │  │  ├── Zephyr
│  │  │  │  │  │  │ ConfigurationManagerImpl.cpp
samples
│  ├── matter
│  │  ├── common
│  │  │  ├── src
│  │  │  │  ├── bridge
│  │  │  │  │  ├── bridge_storage_manager.cpp
│  │  │  │  │  │ bridge_storage_manager.h
│  │  │  │  ├── persistent_storage
│  │  │  │  │  ├── backends
│  │  │  │  │  │  ├── persistent_storage_secure.cpp
│  │  │  │  │  │  ├── persistent_storage_secure.h
│  │  │  │  │  │  ├── persistent_storage_settings.cpp
│  │  │  │  │  │  │ persistent_storage_settings.h
│  │  │  │  │  ├── persistent_storage.h
│  │  │  │  │  │ persistent_storage_impl.h
│  │  ├── lock
│  │  │  ├── Kconfig
│  │  │  ├── src
│  │  │  │  ├── access
│  │  │  │  │  ├── access_manager.h
│  │  │  │  │  ├── access_manager_credentials.cpp
│  │  │  │  │  ├── access_storage.cpp
│  │  │  │  │  │ access_storage.h
│  │  │  │  │ main.cpp
west.yml

Outputs:

Toolchain

Version: acee3b0b2b
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:acee3b0b2b_e579f9fbe6

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 90
  • ✅ Integration tests
    • ✅ test-fw-nrfconnect-chip
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_cloud
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-proprietary_esb
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-find-my
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-sidewalk
    • test-sdk-wifi
    • test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

Copy link

github-actions bot commented Mar 7, 2025

You can find the documentation preview for this PR here.

Copy link
Contributor

@markaj-nordic markaj-nordic left a 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);
Copy link
Contributor

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

Copy link
Member Author

@adigie adigie Mar 10, 2025

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?

https://github.com/nrfconnect/sdk-nrf/blob/main/samples/matter/lock/src/access/access_storage.cpp#L50-L65

Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated implementation.

@adigie
Copy link
Member Author

adigie commented Mar 10, 2025

@adigie is the current implementation against the spec for door lock cluster?

Yes, please see this comment:
project-chip/connectedhomeip#37900 (comment)

@adigie adigie requested review from markaj-nordic and a team March 10, 2025 15:09
adigie added 3 commits March 11, 2025 13:50
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>
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
4.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@adigie adigie marked this pull request as draft March 13, 2025 15:47
@adigie adigie closed this Mar 14, 2025
@adigie adigie deleted the doorlock branch March 14, 2025 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. DNM manifest manifest-matter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants