applications: nrf_desktop: Switch to ZMS for RRAM-based devices #17932

mkapala-nordic · 2024-10-16T13:18:25Z

Switched to using the Zephyr Memory Storage (ZMS) instead of Non-Volatile Storage (NVS) for all devices that uses the RRAM non-volatile memory (currently only the nRF54L15).

Jira: NCSDK-29634

NordicBuilder · 2024-10-16T13:23:38Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 5

Inputs:

Sources:

sdk-nrf: PR head: c98b2a25af6a12a97c1ac9a2c66e0d533ed4e3e5

more details

sdk-nrf:

PR head: c98b2a25af6a12a97c1ac9a2c66e0d533ed4e3e5
merge base: c673d2b5c544d59260a08a8b4531017c11dcd0a2
target head (main): c673d2b5c544d59260a08a8b4531017c11dcd0a2
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 (1)

applications
│  ├── nrf_desktop
│  │  ├── src
│  │  │  ├── modules
│  │  │  │  │ Kconfig.caf_settings_loader.default

Outputs:

Toolchain

Version: 3dd8985b56
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:3dd8985b56_912848a074

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

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 72
✅ Integration tests
- ✅ desktop52_verification

Disabled integration tests

- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-chip
- 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_nrf_provisioning
- 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

applications/nrf_desktop/src/modules/Kconfig.caf_settings_loader.default

NordicBuilder · 2024-10-17T09:34:29Z

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.

applications/nrf_desktop/src/modules/Kconfig.caf_settings_loader.default

kapi-no

Looking good, eventual fine-tuning of ZMS can be done in follow-up PR

mkapala-nordic · 2024-10-18T07:56:27Z

@nrfconnect/ncs-co-build-system please review

nordicjm · 2024-10-18T08:00:17Z

applications/nrf_desktop/src/modules/Kconfig.caf_settings_loader.default

+choice DESKTOP_SETTINGS_BACKEND
+	prompt "Settings backend"
+	default DESKTOP_SETTINGS_BACKEND_ZMS if SOC_FLASH_NRF_RRAM
+	default DESKTOP_SETTINGS_BACKEND_NVS
+	help
+	  By default, nRF Desktop application, depending on the non-volatile
+	  memory technology used by the device, uses either the Zephyr Memory
+	  Storage (ZMS) or Non-Volatile Storage (NVS) settings backend.
+	  ZMS is used for the devices with the RRAM non-volatile memory that
+	  do not require explicit erase. Otherwise, the NVS is used.
+
+config DESKTOP_SETTINGS_BACKEND_ZMS
+	bool "Zephyr Memory Storage (ZMS) settings backend"
+	select ZMS
+	help
+	  This option enables the Zephyr Memory Storage (ZMS) as the settings
+	  backend for the nRF Desktop application.
+
+config DESKTOP_SETTINGS_BACKEND_NVS
+	bool "Non-Volatile Storage (NVS) settings backend"
+	select NVS
+	help
+	  This option enables the Non-Volatile Storage (NVS) as the settings
+	  backend for the nRF Desktop application.
+
+endchoice


why do you need this, just imply in the above, if someone wants to change because of external flash then they can change from SETTINGS_ZMS to SETTINGS_NVS, this choice adds nothing

FYI @MarekPieta

I agree this could be simplified. We can skip configuring SETTINGS_BACKEND below as this is autoselected based on FS.
Then you could remove DESKTOP_SETTINGS_BACKEND altogether and simply add

config DESKTOP_SETTINGS_LOADER imply ZMS if SOC_FLASH_NRF_RRAM imply NVS

just check if the end values are correct

I am worried it might lead to problems if multiple filesystem support is enabled in configuration. E.g. enabling ZMS for any purpose other than settings would silently change the used settings backend - probably not expected. That's why I suggested to make configuration more explicit here.

Can we keep it because of that?

No, the correct backend would be on if SETTINGS_NVS or SETTINGS_ZMS is used, this configuration option is completely pointless, it just mirrors another one adding nothing

Keep in mind that FS might be also used "as is" for any purpose (i.e. not as settings backend). The approach currently presented in the PR should ensure correct handling of case with multiple filesystems too. Maybe we can stick with it for now?

Keep in mind that FS might be also used "as is" for any purpose (i.e. not as settings backend).

Then you wouldn't enable SETTINGS_NVS or SETTINGS_ZMS, I'm not really sure why this discussion is still going on

Keep in mind that FS might be also used "as is" for any purpose (i.e. not as settings backend).

Then you wouldn't enable SETTINGS_NVS or SETTINGS_ZMS, I'm not really sure why this discussion is still going on

You actually would if you have one FS used for settings and another for provisioning. I think fast pair case has two storage partitions.
I expect the problem would be if these are handled using two different FSs. Likely not the use case at the moment.

Note that this should be owners and maintainers call of this code to make the decision. If @MarekPieta believes this is the best path forward, let's keep it as is. The code is not wrong and indeed there is no point in arguing.

You might use file system "B" (for an application-specific purpose) next to the Zephyr settings using a file system "A". No one forbids enabling multiple file systems for various non-volatile memories (even though IIRC we currently do not support multiple settings instances). That's why I suggested to add the application-specific Kconfig option that ensures no side effects here.

Still, if you insist we will remove the application-specific Kconfigs and stick to the configuration that relies only on implies (and has a potential side effect).

Applied @nordicjm suggestion to switch to implies as the current application-specific Kconfig would not be accepted.

Switched to using the Zephyr Memory Storage (ZMS) instead of Non-Volatile Storage (NVS) for all devices that uses the RRAM non-volatile memory (currently only the nRF54L15). Jira: NCSDK-29634 Signed-off-by: Mateusz Kapala <[email protected]>

mkapala-nordic · 2024-10-22T08:30:25Z

Rebased

mkapala-nordic requested a review from a team October 16, 2024 13:18

mkapala-nordic requested a review from a team as a code owner October 16, 2024 13:18

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 16, 2024

MarekPieta requested changes Oct 17, 2024

View reviewed changes

mkapala-nordic force-pushed the nrf_desktop/zms branch from 5d8ac18 to a3a261b Compare October 17, 2024 09:11

mkapala-nordic requested a review from MarekPieta October 17, 2024 09:16

MarekPieta approved these changes Oct 17, 2024

View reviewed changes

applications/nrf_desktop/src/modules/Kconfig.caf_settings_loader.default Outdated Show resolved Hide resolved

applications/nrf_desktop/src/modules/Kconfig.caf_settings_loader.default Outdated Show resolved Hide resolved

mkapala-nordic force-pushed the nrf_desktop/zms branch from a3a261b to 7cb4475 Compare October 17, 2024 10:20

kapi-no approved these changes Oct 17, 2024

View reviewed changes

kapi-no added this to the 2.8.0 milestone Oct 17, 2024

nordicjm requested changes Oct 18, 2024

View reviewed changes

pdunaj approved these changes Oct 21, 2024

View reviewed changes

mkapala-nordic force-pushed the nrf_desktop/zms branch from 7cb4475 to 42528ac Compare October 22, 2024 08:20

mkapala-nordic requested review from MarekPieta, nordicjm, pdunaj and kapi-no October 22, 2024 08:23

mkapala-nordic force-pushed the nrf_desktop/zms branch from 42528ac to c98b2a2 Compare October 22, 2024 08:30

MarekPieta approved these changes Oct 22, 2024

View reviewed changes

kapi-no approved these changes Oct 22, 2024

View reviewed changes

nordicjm approved these changes Oct 22, 2024

View reviewed changes

nordicjm merged commit ced1763 into nrfconnect:main Oct 23, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

applications: nrf_desktop: Switch to ZMS for RRAM-based devices #17932

applications: nrf_desktop: Switch to ZMS for RRAM-based devices #17932

mkapala-nordic commented Oct 16, 2024

NordicBuilder commented Oct 16, 2024 •

edited

Loading

NordicBuilder commented Oct 17, 2024

kapi-no left a comment

mkapala-nordic commented Oct 18, 2024

nordicjm Oct 18, 2024

mkapala-nordic Oct 18, 2024

pdunaj Oct 18, 2024

MarekPieta Oct 18, 2024 •

edited

Loading

nordicjm Oct 18, 2024

MarekPieta Oct 18, 2024

nordicjm Oct 21, 2024

pdunaj Oct 21, 2024

MarekPieta Oct 21, 2024

mkapala-nordic Oct 22, 2024

mkapala-nordic commented Oct 22, 2024

applications: nrf_desktop: Switch to ZMS for RRAM-based devices #17932

applications: nrf_desktop: Switch to ZMS for RRAM-based devices #17932

Conversation

mkapala-nordic commented Oct 16, 2024

NordicBuilder commented Oct 16, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

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

NordicBuilder commented Oct 17, 2024

kapi-no left a comment

Choose a reason for hiding this comment

mkapala-nordic commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarekPieta Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkapala-nordic commented Oct 22, 2024

NordicBuilder commented Oct 16, 2024 •

edited

Loading

MarekPieta Oct 18, 2024 •

edited

Loading