nonblocking image upload by michalek-no · Pull Request #613 · nrfconnect/sdk-nrf-bm

michalek-no · 2026-01-13T18:26:17Z

No description provided.

github-actions · 2026-01-13T18:29:23Z

You can find the documentation preview for this PR here.

lemrey · 2026-01-16T08:46:40Z

@michalek-no it would help us to review this PR if you could point to the functionality that you changed/added from the Zephyr implementation, and fix the errors that GitHub is complaining about in the Files changed tab.

lemrey · 2026-01-29T08:21:15Z

I have a PR for bm_storage where I add a flag to enable automatic padding of write operations #611, and a bunch of other stuff.

nvlsianpu

LGTM

michalek-no · 2026-02-23T12:26:38Z

rebase

nvlsianpu · 2026-03-09T12:55:42Z

@@ -423,19 +478,63 @@ int img_mgmt_write_image_data(unsigned int offset, const void *data, unsigned in
 int img_mgmt_write_image_data(unsigned int offset, const void *data, unsigned int num_bytes,


some comments in code on what is done? why? would be very helpfull.

nvlsianpu · 2026-03-09T13:09:05Z

This file is so much different then sdk-zephyr origin that I would:

move NCS-BM implementation of img_mgmt_read() & int img_mgmt_write_image_data() to a brand new file.

cut this fille content as you already did (+ removal of what the new file implements instead).

Not necessary in this PR.

lemrey · 2026-03-16T09:22:30Z

+
+		ring_buf_put(&ring_buf, data, pad);
+		rb_size = ring_buf_get_claim(&ring_buf, &rb_data, PKTBUF_SZ);
+		ring_buf_get_finish(&ring_buf, rb_size);


I don't think it's correct to call ring_buf_get_finish() here. That functions allows the data you claimed using ring_buf_get_claim() to be overwritten in the buffer, but at this point the data may not have been written yet. You need to keep this data unchanged while it is being written to NVM, meaning until you receive the event from bm_storage.

Not calling finish() causes size_get() to return incorrect values. 'ongoing' var keeps track on available space.

can we use value from ring_buf_get_claim() instead of size_get() here? Then we can release at the bm_event.

lemrey · 2026-03-16T09:27:47Z

+		ring_buf_put(&ring_buf, data, num_bytes);
+	}
+
+	if (last) {


When last, it seems you retrieve up to CHUNK_SZ bytes first, then write those bytes, then retrieve everything else that's left and write that. I don't think you need to write twice in this case; could you not simply retrieve everything that's left and write it at once?

This handles possibility we hit ring buffer wrap around

lemrey · 2026-03-16T09:28:46Z

+static uint8_t ongoing;
+static int write_offset;
+
+static void bm_storage_evt_handler_writes(struct bm_storage_evt *evt)


Instead you should inspect the event from bm_storage and free the data there.

de-nordic

Comments left.

de-nordic · 2026-03-17T13:17:08Z

+	}
+}
+
+struct bm_storage_config s0_config = {


Shouldn't this be static const?

de-nordic · 2026-03-17T13:18:50Z

+	},
+};
+
+struct bm_storage_config s1_config = {


static const?

de-nordic · 2026-03-17T13:25:30Z

+			rb_data, rb_size, NULL);
+
+		if (err) {
+			rc = IMG_MGMT_ERR_INVALID_IMAGE_TOO_LARGE;


There are three places where we can get IMG_MGMT_ERR_INVALID_IMAGE_TOO_LARGE here, shouldn't we at least have LOG_DBG, in all of them, with the err being reported?

de-nordic · 2026-03-17T13:26:12Z

+		if (ring_buf_size_get(&ring_buf)) {
+			rb_size = ring_buf_get_claim(&ring_buf, &rb_data, PKTBUF_SZ);
+			ring_buf_get_finish(&ring_buf, rb_size);
+			int err = bm_storage_write(&s0_storage,


Eclipsing err variable declaration in upper scope.

eivindj-nordic

some nits.

eivindj-nordic · 2026-03-16T08:24:58Z

+			int err = bm_storage_write(&s0_storage,
+				write_offset, rb_data, rb_size, NULL);


nit: unaligned

This is still unaligned

michalek-no · 2026-03-23T09:20:53Z

rebase

eivindj-nordic · 2026-03-23T12:16:13Z

+			int err = bm_storage_write(&s0_storage,
+				write_offset, rb_data, rb_size, NULL);


This is still unaligned

eivindj-nordic · 2026-03-23T12:36:23Z

+int img_mgmt_find_by_hash(uint8_t *find, struct image_version *ver);
+int img_mgmt_find_by_ver(struct image_version *find, uint8_t *hash);
+int img_mgmt_state_read(struct smp_streamer *ctxt);
+int img_mgmt_state_write(struct smp_streamer *njb);
+int img_mgmt_flash_area_id(int slot);


nit: Missing descriptions

I see the rest are removed, will be good with some description for img_mgmt_state_read().

eivindj-nordic · 2026-03-23T13:19:36Z

+
+		ring_buf_put(&ring_buf, data, pad);
+		rb_size = ring_buf_get_claim(&ring_buf, &rb_data, PKTBUF_SZ);
+		ring_buf_get_finish(&ring_buf, rb_size);


can we use value from ring_buf_get_claim() instead of size_get() here? Then we can release at the bm_event.

eivindj-nordic · 2026-03-24T12:01:47Z

+	struct zcbor_string img_data;
+	struct zcbor_string data_sha;


Add descriptions here?

let's leave this minor stuff for the future

eivindj-nordic · 2026-03-25T08:24:20Z

@nrfconnect/ncs-co-build-system Please review! We need to get this in this week.

nordicjm · 2026-03-25T09:51:48Z

- * @param ver		output buffer for image version
- * @param hash		output buffer for image hash
- * @param flags		output buffer for image flags
+ * @param image_slot	Image slot number.


why not fix these issues upstream and bring the fixed version in?

nordicjm · 2026-03-25T09:52:26Z

Have not reviewed C code, first 3 comments mandatory, last one a suggestion

img_mgmt.h img_mgmt_priv.h img_mgmt.c zephyr_img_mgmt.c cherry-picked from sdk-zephyr dbcfb77ce3d20784406d4d73a1de3f7f1bcea07c Signed-off-by: Mateusz Michalek <mateusz.michalek@nordicsemi.no>

nvlsianpu · 2026-03-25T13:26:18Z

@nordicjm My comment to what you posted there:
comment 3rd- that maybe can be improved, comment 4th - no sens to procced. We did deep copy of this. comment 2nd - we can do a workaround for the thing - but this will be hack just for applying request.

access to slot0 and slot1 partitions are handled via bm_storage subsystem Signed-off-by: Mateusz Michalek <mateusz.michalek@nordicsemi.no>

removed bm unneeded stuff Signed-off-by: Mateusz Michalek <mateusz.michalek@nordicsemi.no>

github-actions Bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 13, 2026

michalek-no force-pushed the rram_split_poc branch 5 times, most recently from 7e95fc5 to 7d19184 Compare January 20, 2026 11:42

anhmolt reviewed Jan 21, 2026

View reviewed changes

Comment thread subsys/mgmt/mcumgr/grp/img_mgmt/src/zephyr_img_mgmt.c Outdated

Comment thread subsys/mgmt/mcumgr/grp/img_mgmt/src/zephyr_img_mgmt.c Outdated

michalek-no force-pushed the rram_split_poc branch from 7d19184 to 26a859c Compare January 22, 2026 09:30

anhmolt reviewed Jan 26, 2026

View reviewed changes

Comment thread subsys/mgmt/mcumgr/grp/img_mgmt/src/zephyr_img_mgmt.c Outdated

michalek-no force-pushed the rram_split_poc branch 2 times, most recently from 8a30e6c to f26f1a3 Compare February 5, 2026 14:37

michalek-no force-pushed the rram_split_poc branch 5 times, most recently from 1d5e92d to 62ca667 Compare February 16, 2026 10:44

nvlsianpu reviewed Feb 18, 2026

View reviewed changes

Comment thread subsys/mgmt/mcumgr/grp/img_mgmt/src/zephyr_img_mgmt.c

michalek-no force-pushed the rram_split_poc branch 7 times, most recently from 6a43719 to 3b7bbaf Compare February 23, 2026 12:25

michalek-no force-pushed the rram_split_poc branch 2 times, most recently from ee821c2 to 05f0f3e Compare February 23, 2026 13:53

nvlsianpu approved these changes Mar 9, 2026

View reviewed changes

michalek-no added the DNM Do not merge label Mar 10, 2026

michalek-no force-pushed the rram_split_poc branch 6 times, most recently from fbfe7c8 to 1cf28f0 Compare March 13, 2026 12:44

michalek-no removed the DNM Do not merge label Mar 13, 2026

lemrey reviewed Mar 16, 2026

View reviewed changes

eivindj-nordic added this to the v2.0.0 milestone Mar 16, 2026

michalek-no force-pushed the rram_split_poc branch from 1cf28f0 to f352147 Compare March 16, 2026 11:54

de-nordic reviewed Mar 17, 2026

View reviewed changes

eivindj-nordic reviewed Mar 17, 2026

View reviewed changes

eivindj-nordic reviewed Mar 23, 2026

View reviewed changes

Comment thread subsys/mgmt/mcumgr/grp/img_mgmt/src/bm_img_mgmt.c Outdated

eivindj-nordic reviewed Mar 23, 2026

View reviewed changes

peknis requested changes Mar 24, 2026

View reviewed changes

eivindj-nordic reviewed Mar 24, 2026

View reviewed changes

peknis approved these changes Mar 24, 2026

View reviewed changes

eivindj-nordic approved these changes Mar 24, 2026

View reviewed changes

b-gent approved these changes Mar 25, 2026

View reviewed changes

nordicjm requested changes Mar 25, 2026

View reviewed changes

mgmt: mcumgr: copy from zephyr

8aeaa5b

img_mgmt.h img_mgmt_priv.h img_mgmt.c zephyr_img_mgmt.c cherry-picked from sdk-zephyr dbcfb77ce3d20784406d4d73a1de3f7f1bcea07c Signed-off-by: Mateusz Michalek <mateusz.michalek@nordicsemi.no>

michalek-no added 2 commits March 25, 2026 14:38

firmware_loader: redo image backend to bm_storage

120cf40

access to slot0 and slot1 partitions are handled via bm_storage subsystem Signed-off-by: Mateusz Michalek <mateusz.michalek@nordicsemi.no>

mcumgr: img_mgmt: cut down

9d95476

removed bm unneeded stuff Signed-off-by: Mateusz Michalek <mateusz.michalek@nordicsemi.no>

nordicjm approved these changes Mar 25, 2026

View reviewed changes

		@@ -423,19 +478,63 @@ int img_mgmt_write_image_data(unsigned int offset, const void *data, unsigned in
		int img_mgmt_write_image_data(unsigned int offset, const void *data, unsigned int num_bytes,

		int err = bm_storage_write(&s0_storage,
		write_offset, rb_data, rb_size, NULL);

Conversation

michalek-no commented Jan 13, 2026

Uh oh!

github-actions Bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lemrey commented Jan 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lemrey commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvlsianpu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michalek-no commented Feb 23, 2026

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!

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!

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!

de-nordic left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eivindj-nordic left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

michalek-no commented Mar 23, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jan 13, 2026 •

edited

Loading

lemrey commented Jan 29, 2026 •

edited

Loading