-
Notifications
You must be signed in to change notification settings - Fork 31
[SL-UP] Full integration of CTM credentials and migration. #541
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
base: release_2.7-1.4
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR integrates CTM (Chip Token Manager) credentials and migration by moving all provisioning data to nvm3 to prevent collision with other stack tokens in CMP apps. The PR updates the configuration system to use the new token manager API and implements migration functionality.
- Refactored configuration system to use new CTM token manager API instead of direct NVM3 access
- Added migration functionality to migrate existing attestation credentials to the new storage format
- Updated build files to include new utility functions and moved chip stack initialization after provisioning
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/silabs/efr32/BUILD.gn | Added SilabsConfigUtils source files to the build |
| src/platform/silabs/SiWx917/BUILD.gn | Added SilabsConfigUtils source files to the build |
| src/platform/silabs/SilabsConfigUtils.h | New utility header for NVM3 error mapping |
| src/platform/silabs/SilabsConfigUtils.cpp | New utility implementation for NVM3 error mapping |
| src/platform/silabs/SilabsConfigCTM.cpp | Updated to use new token manager API and extracted MapNvm3Error |
| src/platform/silabs/SilabsConfig.h | Reorganized key definitions and conditionals for CTM support |
| src/platform/silabs/SilabsConfig.cpp | Removed duplicate MapNvm3Error function |
| src/platform/silabs/MigrationManager.h | Added MigrateCTM function declaration |
| src/platform/silabs/MigrationManager.cpp | Added CTM migration to migration table |
| examples/platform/silabs/provision/ProvisionStorageFlash.cpp | Added stub MigrateAttestationCredentialAPI method |
| examples/platform/silabs/provision/ProvisionStorageDefault.cpp | Implemented CTM migration and updated credential storage methods |
| examples/platform/silabs/MatterConfig.cpp | Moved chip stack initialization after provisioning setup |
e65bdcf to
0f7800d
Compare
| static constexpr Key kConfigKey_Creds_Dac = SilabsConfigKey(kMatterFactory_KeyBase, 0x31); | ||
| static constexpr Key kConfigKey_Creds_Pai = SilabsConfigKey(kMatterFactory_KeyBase, 0x32); | ||
| static constexpr Key kConfigKey_Creds_CD = SilabsConfigKey(kMatterFactory_KeyBase, 0x33); |
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.
need follow-up/aggrement on where to store does. I don't think nvm3 is the way to go.
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.
NVM3 would have been the easy way to get series 2 on board as well.
| constexpr inline uint32_t SilabsSecureTokenKey(uint8_t keyBaseOffset, uint8_t id) | ||
| { | ||
| return SL_TOKEN_TYPE_STATIC_SECURE | static_cast<uint32_t>(keyBaseOffset) << 8 | id; | ||
| } |
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.
what is the story with this SL_TOKEN_TYPE_STATIC_SECURE and now removal?
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.
We were going to use NVM3, then we weren't, now we are going to use it again.
| { .migrationGroup = 2, .migrationFunc = MigrateDacProvider }, | ||
| { .migrationGroup = 3, .migrationFunc = MigrateCounterConfigs }, | ||
| { .migrationGroup = 4, .migrationFunc = MigrateHardwareVersion }, | ||
| { .migrationGroup = 5, .migrationFunc = MigrateCTM }, |
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.
Warning !!!
I think this migration group is in use in CSA for doorlock credentials.
We need to CP the migration PR before adding this migration.
There is also a catch22 here. Once this is merged, all apps must use CTM by default, else Migration will be done when CTM is not in use and will never be called again.
IF CTM implementation can become default, then we shouldn't keep SilabsConfig and SilabsConfigCTM. Does the Mig also work for 917?
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.
We are keeping it since Series 2 cannot move to CTM right now because in series 2 the token interface relies on zigbee or embernet.
Regarding 917, I do not think the CTM supports it but I have to confirm.
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.
Regarding the catch 22, the nvm location will be the same for all, however since Series2 cannot use the CTM API currently, we need to keep the SilabsConfig.
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.
The final version of the DoorLock migration do not use the MigrationManager. It required to move the migrations later on on the initialization, which I deemed too risky, so the DoorLock migration end up being a standalone process.
| #ifdef SL_TOKEN_MANAGER_DEFINES_H | ||
| sl_status_t status = sl_token_manager_set_data(token_key, const_cast<uint8_t *>(value.data()), value.size()); | ||
| #else | ||
| sl_status_t status = nvm3_writeData(nvm3_defaultHandle, token_key, const_cast<uint8_t *>(value.data()), value.size()); |
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.
shouldn't that be using SilabsConfig.h API that abstracts this already?
| #ifdef SL_TOKEN_MANAGER_DEFINES_H | ||
| sl_status_t status = sl_token_manager_get_data(token_key, value.data(), key_size); | ||
| #else | ||
| sl_status_t status = nvm3_readData(nvm3_defaultHandle, token_key, value.data(), key_size); | ||
| #endif // SL_TOKEN_MANAGER_DEFINES_H |
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.
same comment about the SilabsConfig API
Summary
We are integrating all our matter apps with the CTM by moving all provisioning data to nvm3 to prevent collision with other stack tokens in CMP apps.
Note: Moved the Chip Stack Init after the provisioning init since our migration function needs to use the provisioning manager in an initialized state.
In the future, we might migrate to user data once :
Related PRs:
matter_support:
SiliconLabsSoftware/matter_support#149
matter_extension:
https://stash.silabs.com/projects/WMN_TOOLS/repos/matter_extension/pull-requests/593/overview
Related issues
https://jira.silabs.com/browse/MATTER-4883
https://jira.silabs.com/browse/MATTER-5195
Testing
Provision -> Build -> Flash -> Read NVM3 to confirm migration.
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines