emds: loading flow implementation #22895

alxelax · 2025-06-19T14:47:36Z

PR:

adds new loading flow implementation
adapts mesh light control sample to the new API statuses

NordicBuilder · 2025-06-19T14:50:59Z

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: 0136b4cab27b68b45a0b67681f720168fe7430a9

more details

sdk-nrf:

PR head: 0136b4cab27b68b45a0b67681f720168fe7430a9
merge base: f411e0a82d678cf283a1df48bcd721f5a6cae3c1
target head (collab-emds-next): f411e0a82d678cf283a1df48bcd721f5a6cae3c1
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 (6)

include
│  ├── emds
│  │  │ emds.h
samples
│  ├── bluetooth
│  │  ├── mesh
│  │  │  ├── light_ctrl
│  │  │  │  ├── src
│  │  │  │  │  │ main.c
subsys
│  ├── emds
│  │  ├── Kconfig
│  │  ├── emds_flash_new.c
│  │  ├── emds_flash_new.h
│  │  │ emds_new.c

Outputs:

Toolchain

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

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

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 12
🟠 Integration tests
- 🟠 test-fw-nrfconnect-ble_mesh

Disabled integration tests

- test-fw-nrfconnect-nrf_lrcs_mosh
- test-fw-nrfconnect-nrf_lrcs_positioning
- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- 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_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-ps-main
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread-main
- test-low-level
- test-sdk-audio
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-wifi
- test-secdom-samples-public

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

subsys/emds/emds_flash_new.c

Balaklaka · 2025-06-20T13:23:52Z

subsys/emds/emds_flash_new.c

+		}
+	}
+
+	return 0;


This function will return 0 if failures reach CONFIG_EMDS_SCANNING_FAILURES number. Is that correct? So it is success if all crc are wrong. First time run is maybe success.

Yep, this is correct.
Having snapshot with wrong CRC or not having anything in emds at all are valid scenarios.
The function works with partitions.
There might be another partition has correct snapshot.
The final decision about fail or success should be done by user of emds_flash_scan_partition.
This function just scans partition and returns result.
It fails only if scanning fails itself (e.g. flash driver failure etc).

I find it a little strange that failing crc is not reported as error. But I having a hard time arguing why that is really important, so I will just leave it for now.

subsys/emds/emds_new.c

alxelax · 2025-06-23T10:02:28Z

github added @tejlmand and @nrfconnect/ncs-test-leads due to collaboration branch was rebased on top the latest main and there were some collisions.
Actually, nothing to review for them after collisions were solved. I removed them from PR as reviewers.

subsys/emds/Kconfig

subsys/emds/emds_flash_new.c

omkar3141 · 2025-06-24T07:51:54Z

subsys/emds/emds_flash_new.c

+				    struct emds_snapshot_metadata *metadata)
+{
+	const struct flash_area *fa = partition->fa;
+	uint8_t data_chunk[CHUNK_SIZE(partition->fp)];


This creates VLA at runtime. We should not be using dynamic memory allocation. I think it is safer to simply freeze the data chunk size at compile time in multiples of 16.

Hm... Why is VLA under obstruction?
Heap and VLA are already in use in many components. For example, in old emds here:
https://github.com/nrfconnect/sdk-nrf/blob/main/subsys/emds/emds_flash.c#L343
https://github.com/nrfconnect/sdk-nrf/blob/main/subsys/emds/emds_flash.c#L428

In 2018, we removed all usage of VLAs in nRF5 Mesh. I don't know how this got into existing emds. Even zephyr guidelines prevent VLAs here.

Hm... but this is not nrf5 and not Zephyr. Does Nordic concern about MISRA rules?
Seems using VLA wasn't a problem before.

is wrong. Because dev_uuid is already declared as array. Example in one is not VLA declaration, it is pointer access.
2,3, 4: this is not vla. this will be treated as pointer.

A declaration of a parameter as ‘‘array of type’’ shall be adjusted to ‘‘qualified pointer to type’’, where the type qualifiers (if any) are those specified within the [ and ] of the
array type derivation. If the keyword static also appears within the [ and ] of the
array type derivation, then for each call to the function, the value of the corresponding
actual argument shall provide access to the first element of an array with at least as many
elements as specified by the size expression.

So we don't have VLAs in NCS mesh so far. Probably copilot is getting confused by array like syntax and confidently stating that this is a VLA.

btw, I decided to ask same question like you to cursor, and it seems, it is able to provide correct answer:

subsys/emds/emds_flash_new.c

subsys/emds/emds_flash_new.h

subsys/emds/emds_new.c

Commit adds new loading flow implementation with searching the freshest data on two partitions. Signed-off-by: Aleksandr Khromykh <aleksandr.khromykh@nordicsemi.no>

Commit adapts light control sample to the new emds API statuses. Signed-off-by: Aleksandr Khromykh <aleksandr.khromykh@nordicsemi.no>

Balaklaka · 2025-06-24T14:56:14Z

subsys/emds/emds_flash_new.c

+		}
+	}
+
+	return 0;


I find it a little strange that failing crc is not reported as error. But I having a hard time arguing why that is really important, so I will just leave it for now.

Balaklaka · 2025-06-24T14:58:07Z

include/emds/emds.h

@@ -144,7 +144,9 @@ int emds_store(void);
 * previously stored data.
 *
 * @retval 0 Success
- * @retval -ERRNO errno code if error
+ * @retval -ECANCELED errno code if it was called before @ref emds_init
+ * @retval -ENOENT errno code if no valid snapshot was found in any partition


Maybe we should write in documentation that first time run will give this error, as snapshot is empty.

Balaklaka · 2025-06-24T15:01:56Z

subsys/emds/emds_new.c

-	return 0;
+	for (int i = 0; i < PARTITIONS_NUM_MAX; i++) {
+		if (emds_flash_scan_partition(&partition[i], &metadata)) {
+			LOG_ERR("Failed to scan partition %d:", i);


Should this be a LOG_DBG (LOG_WRN) as possible no error code will be supplied?

omkar3141

Looks good. Lots of interesting discussions during this PR. Thanks for clarifications.

alxelax requested review from a team and Balaklaka as code owners June 19, 2025 14:47

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 19, 2025

alxelax force-pushed the load_flow branch from 0930b13 to 06b812b Compare June 19, 2025 14:52

nordicjm approved these changes Jun 20, 2025

View reviewed changes

alxelax force-pushed the load_flow branch from 06b812b to 59530e4 Compare June 20, 2025 13:06

Balaklaka reviewed Jun 20, 2025

View reviewed changes

alxelax force-pushed the load_flow branch from 59530e4 to 1fa8950 Compare June 23, 2025 09:09

alxelax requested a review from Balaklaka June 23, 2025 09:10

alxelax force-pushed the collab-emds-next branch from 17a71e7 to f411e0a Compare June 23, 2025 09:55

alxelax requested review from tejlmand and a team as code owners June 23, 2025 09:55

alxelax force-pushed the load_flow branch from 1fa8950 to 12ab06d Compare June 23, 2025 09:58

alxelax removed request for a team and tejlmand June 23, 2025 09:59

alxelax removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 23, 2025

omkar3141 requested changes Jun 24, 2025

View reviewed changes

alxelax force-pushed the load_flow branch from 12ab06d to 61598d4 Compare June 24, 2025 13:07

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 24, 2025

alxelax requested a review from omkar3141 June 24, 2025 13:08

alxelax removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 24, 2025

alxelax added 2 commits June 24, 2025 17:02

emds: loading flow

693e9e4

Commit adds new loading flow implementation with searching the freshest data on two partitions. Signed-off-by: Aleksandr Khromykh <aleksandr.khromykh@nordicsemi.no>

samples: bluetooth: mesh: adapt light control to new emds

0136b4c

Commit adapts light control sample to the new emds API statuses. Signed-off-by: Aleksandr Khromykh <aleksandr.khromykh@nordicsemi.no>

alxelax force-pushed the load_flow branch from 61598d4 to 0136b4c Compare June 24, 2025 15:02

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 24, 2025

Balaklaka approved these changes Jun 24, 2025

View reviewed changes

omkar3141 approved these changes Jun 24, 2025

View reviewed changes

emds: loading flow implementation #22895

Are you sure you want to change the base?

emds: loading flow implementation #22895

Conversation

alxelax commented Jun 19, 2025

Uh oh!

NordicBuilder commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxelax Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alxelax commented Jun 23, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omkar3141 Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omkar3141 Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omkar3141 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NordicBuilder commented Jun 19, 2025 •

edited

Loading

alxelax Jun 23, 2025 •

edited

Loading

omkar3141 Jun 24, 2025 •

edited

Loading

omkar3141 Jun 24, 2025 •

edited

Loading