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.

Sign up for GitHub

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

Jump to bottom

Add ZMS to Zephyr port #493

Merged
merged 2 commits into from
Oct 16, 2024
Merged

Add ZMS to Zephyr port #493

merged 2 commits into from
Oct 16, 2024

Conversation

ArekBalysNordic
Copy link
Contributor

Add ZMS to Zephyr port, Enable ZMS in nrfconnect if RRAM is in use.

@ArekBalysNordic ArekBalysNordic requested a review from a team October 11, 2024 08:37
This was referenced Oct 11, 2024
Closed
Merged
maciejbaczmanski
maciejbaczmanski reviewed Oct 11, 2024
View reviewed changes
@ArekBalysNordic ArekBalysNordic force-pushed the zms_matter branch 2 times, most recently from 18b9635 to fea9e01 Compare October 11, 2024 11:06
LuDuda
LuDuda reviewed Oct 11, 2024
View reviewed changes
config/zephyr/Kconfig Outdated
device-specific entries.
device-specific entries. It makes sense only for NVS filesystem.

config CHIP_FACTORY_RESET_ERASE_ZMS
Copy link
Collaborator

@LuDuda LuDuda Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's fine for now and will accept this, however, what do you think to have one Matter config like CHIP_FACTORY_RESET_NVM_PARTITION_ERASE (or sth similar) which would use ZMS or NVS underneath based on the selected backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but it impacts many platforms that use Zephyr... We can do it, but not so easily. :/ I think we can create a follow up task for that after the release. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I also think that using two different Kconfigs for that is not the best idea. Why is it not that easy? Isn't it just about changing CHIP_FACTORY_RESET_ERASE_NVS->CHIP_FACTORY_RESET_ERASE_SETTINGS and then adding depends on depends on SETTINGS_NVS || SETTINGS_ZMS? Then we can use zms_clear or nvs_clear in the cpp file based on SETTINGS_NVS/SETTINGS_ZMS selection, because it's a choice. I don't think it would affect anyone much unless he will use SETTINGS_ZMS.

config/zephyr/Kconfig Outdated
device-specific entries. It makes sense only for NVS filesystem.

config CHIP_FACTORY_RESET_ERASE_ZMS
bool "Erase ZMS flash pages on factory reset"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bool "Erase ZMS flash pages on factory reset"
bool "Erase ZMS non-volatile pages on factory reset"

config/zephyr/Kconfig Outdated

config CHIP_FACTORY_RESET_ERASE_ZMS
bool "Erase ZMS flash pages on factory reset"
depends on SETTINGS_ZMS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have protection on setting this config when DAC migration is turned on? I mean we should not allow for setting this config if DAC migration is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I've added it to the nrfconnect's .Kconfig file, but please double check it :)

@github-actions github-actions bot added documentation Improvements or additions to documentation nrf connect labels Oct 14, 2024
@ArekBalysNordic ArekBalysNordic force-pushed the zms_matter branch 2 times, most recently from 241fd44 to 655b3a8 Compare October 14, 2024 08:20
kkasperczyk-no
kkasperczyk-no reviewed Oct 14, 2024
View reviewed changes
config/nrfconnect/chip-module/Kconfig Outdated
default y
depends on NVS || KVS
Copy link
Contributor

Choose a reason for hiding this comment

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

KVS?

src/platform/Zephyr/ConfigurationManagerImpl.cpp Outdated
if (status)
{
ChipLogError(DeviceLayer, "Factory reset failed: %d", status);
}
#else
#else // CONFIG_SETTINGS_NVS || CONFIG_SETTINGS_ZMS
Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant comment.

kkasperczyk-no
kkasperczyk-no approved these changes Oct 14, 2024
View reviewed changes
src/platform/nrfconnect/FactoryDataProvider.cpp Outdated
@@ -160,6 +160,10 @@ CHIP_ERROR FactoryDataProvider<FlashFactoryData>::MoveDACPrivateKeyToSecureStora
}

#ifdef CONFIG_CHIP_CRYPTO_PSA_MIGRATE_DAC_PRIV_KEY
#ifdef CONFIG_CHIP_FACTORY_RESET_ERASE_SETTINGS
#error "Do not use both CONFIG_CHIP_FACTORY_RESET_ERASE_SETTINGS and CONFIG_CHIP_CRYPTO_PSA_MIGRATE_DAC_PRIV_KEY kconfig options " \
"because you will lose the DAC private key permanently from the device."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"because you will lose the DAC private key permanently from the device."
"because you will permanently loose the DAC private key from the device."

Copy link
Contributor

Choose a reason for hiding this comment

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

"lose", not "loose".

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right :)

docs/guides/nrfconnect_examples_configuration.md Outdated
@@ -207,8 +207,8 @@ set the `CONFIG_CHIP_NFC_COMMISSIONING` option.
By default, the factory reset procedure implemented in the Matter stack removes
Matter-related settings only. If your application does not depend on any
device-lifelong data stored in the non-volatile storage, set the
`CONFIG_CHIP_FACTORY_RESET_ERASE_NVS` option to fully erase the NVS partition at
the factory reset. This approach is more robust and regains the original NVS
`CONFIG_CHIP_FACTORY_RESET_ERASE_SETTINGS` option to fully erase the non-volatile partition at
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`CONFIG_CHIP_FACTORY_RESET_ERASE_SETTINGS` option to fully erase the non-volatile partition at
`CONFIG_CHIP_FACTORY_RESET_ERASE_SETTINGS` option to fully erase the non-volatile settings partition at

@doublemis1
Copy link
Contributor

Can we somehow highlight what Configs have been changed?
Clients may use them in their configuration and during migration bump into issues with config renaming.
Maybe add it to migration guideline

@ArekBalysNordic
[nrf toup][Zephyr] Add ZMS to Zephyr port
3d783ff 
Added a possibility to use ZMS fs backend in Zephyr.
NVS fs backend imply is now controlled by the nrfconnect platform
configuration directly.
All other platforms use NVS fs backend by default.

Signed-off-by: Arkadiusz Balys <arkadiusz.balys@nordicsemi.no>
@ArekBalysNordic
[nrf toup][nrfconnect] Enable ZMS in nrfconnect if RRAM is in use.
2bd04ee 
Disabled NVS and enabled ZMS fs backend for all devices that
uses RRAM.

Signed-off-by: Arkadiusz Balys <arkadiusz.balys@nordicsemi.no>
@NordicBuilder NordicBuilder mentioned this pull request Oct 15, 2024
Merged
@rlubos rlubos merged commit 9a4ea8e into nrfconnect:master Oct 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LuDuda LuDuda LuDuda left review comments

@kkasperczyk-no kkasperczyk-no kkasperczyk-no approved these changes

@wiba-nordic wiba-nordic wiba-nordic left review comments

@maciejbaczmanski maciejbaczmanski Awaiting requested review from maciejbaczmanski

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

Assignees
No one assigned
Labels
config documentation Improvements or additions to documentation nrf connect platform zephyr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ArekBalysNordic @doublemis1 @LuDuda @kkasperczyk-no @wiba-nordic @maciejbaczmanski @rlubos