-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
manifest: sdk-zephyr: ZMS a new memory storage system #17502
Conversation
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
c1a0cb1
to
6db1ded
Compare
You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds. Note: This comment is automatically posted by the Documentation Publishing GitHub Action. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 7eaa1f439ee5e1ffa5c35199dc8f41dbd25310b0 more detailssdk-nrf:
matter:
zephyr:
Github labels
List of changed files detected by CI (114)
Outputs:ToolchainVersion: 3dd8985b56 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
8cb2764
to
2cbf0a8
Compare
217b8ca
to
bc25fcc
Compare
Conversions of some remaining ble samples. fyi @miha-nordic I have not covered mesh nor anything beyond samples/bluetooth. I have not checked if it works on my desktop. |
5f7ad0d
to
6ffc4d2
Compare
6ffc4d2
to
7eaa1f4
Compare
@nrfconnect/ncs-co-build-system @nrfconnect/ncs-si-muffin @nrfconnect/ncs-thread Ping, please prioritize this. |
@rghaddab Subrepo PRs are merged, please update SHAs. |
# In nRF54L15 we place the TF-M non-secure storage partitions after the | ||
# TF-M non-secure application to avoid splitting the secure/non-secure | ||
# partitions more than necessary. | ||
#if defined(CONFIG_SOC_NRF54L15_ENGA_CPUAPP) || defined(CONFIG_SOC_NRF54L15_CPUAPP) |
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.
CONFIG_SOC_NRF54L15_ENGA_CPUAPP
can go
#endif | ||
size: CONFIG_PM_PARTITION_SIZE_ZMS_STORAGE | ||
|
||
|
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.
select ZMS if SOC_FLASH_NRF_RRAM | ||
select NVS if !SOC_FLASH_NRF_RRAM |
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.
Should be imply so it can be changed to allow for relocating to/from external storage
config SETTINGS | ||
default y |
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.
this isn't needed
@@ -31,8 +31,6 @@ CONFIG_BT_SETTINGS=y | |||
CONFIG_FLASH=y | |||
CONFIG_FLASH_PAGE_LAYOUT=y | |||
CONFIG_FLASH_MAP=y | |||
CONFIG_NVS=y | |||
CONFIG_SETTINGS=y |
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.
add CONFIG_SETTINGS=y back here
#ifdef CONFIG_NVS | ||
nvs_fs *sStorage = nullptr; | ||
#elif CONFIG_ZMS | ||
zms_fs *sStorage = nullptr; | ||
#endif |
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.
#ifdef CONFIG_NVS | |
nvs_fs *sStorage = nullptr; | |
#elif CONFIG_ZMS | |
zms_fs *sStorage = nullptr; | |
#endif | |
#if defined(CONFIG_NVS) | |
... | |
#elif defined(CONFIG_ZMS) | |
... | |
#endif |
and apply throughout
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.
Applied as a follow-up here: #17939 please have a look.
if (CONFIG_SOC_SERIES_NRF54LX AND CONFIG_SETTINGS_NVS) | ||
message(WARNING " ###################################################################################\n" | ||
" # #\n" | ||
" # Your application uses NVS backend for the settings storage that is not #\n" |
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.
get rid of the excess spacing on the right here. Also seems a bit odd that this is being added for openthread?
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.
Why is it odd? This is because the Zephyr OpenThread L2 layer implies NVS and we cannot fix it reasonably fast now. The ZMS was enabled in all OpenThread samples, but still someone may have an own application copy that will not be automatically aligned, so the check will be done for all applications running OpenThread.
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.
It should be a global thing for any code so users can know if they've got something set up that will kill their device in half the time as one would expect
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.
Yeah, I agree, but I was asked only to handle integration with OpenThread samples, so I've taken care only about this use case. Maybe it could be moved and used in some generic place if others agree on that.
# This script checks if NVS settings backend is used with nRF54L SoC target and prints | ||
# the warning that the recommended backend is ZMS due to the RRAM design. | ||
|
||
if (CONFIG_SOC_SERIES_NRF54LX AND CONFIG_SETTINGS_NVS) |
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.
does not take into consideration if NVS is being placed on external flash
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.
Yes, because it does consider only OpenThread module use case, which uses only settings. This is not generic warning for all application use cases.
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.
it does not work though because if I use NVS on external flash with the Kconfig enabled for that, I wrongly get this giant warning when NVS isn't on the internal RRAM, it's on an external flash
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.
Ok, so do you mean settings placed on the external flash or NVS used for some custom use case on the external flash?
Automatically created by Github Action Signed-off-by: Nordic Builder <[email protected]>
This adds the support of ZMS for the partition manager Signed-off-by: Riadh Ghaddab <[email protected]>
makes the partition manager take into account ZMS storage partition when its CONFIG is enabled. Signed-off-by: Riadh Ghaddab <[email protected]>
if the settings backend is enabled for the trusted storage, it can also select SETTINGS_ZMS as a backend for settings Signed-off-by: Riadh Ghaddab <[email protected]>
When RRAM device is being used select ZMS rather then NVS as a file system for settings. Signed-off-by: Pawel Dunaj <[email protected]>
Select settings and fs in kconfig instead of prj.conf. Select file system depending on NV memory type. Signed-off-by: Pawel Dunaj <[email protected]>
Enabled ZMS fs backend for all Matter samples that work on devices which use RRAM, like nRF54l15. Signed-off-by: Arkadiusz Balys <[email protected]>
The settings backend for the nRF54L SoC family should be selected to the ZMS, so it was applied for all OpenThread samples. Additionally, added a cmake check that prints warning in case someone uses NVS with nRF54L SoC and informs about the consequences. Signed-off-by: Kamil Kasperczyk <[email protected]>
7eaa1f4
to
43ceb65
Compare
cannot wait with this PR anymore. It was decided to merge it as it is. |
@rghaddab Thanks, please address any remaining review comments in a separate PR. |
includes changes from this :
nrfconnect/sdk-zephyr#2045