boot: boot_serial: add optional mcumgr parameters command#2746
boot: boot_serial: add optional mcumgr parameters command#2746JPHutchins wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds optional support for the mcumgr OS group "MCUmgr parameters" command (ID 6) to MCUboot's serial recovery, allowing SMP clients to query the transport buffer size/count for negotiating serial fragmentation.
Changes:
- New Kconfig
BOOT_MGMT_MCUMGR_PARAMS(gated on default 128-byte line length) and correspondingMCUBOOT_BOOT_MGMT_MCUMGR_PARAMSmacro mapping. - New
bs_mcumgr_params()handler inboot_serial.cdispatched fromboot_serial_input()forNMGR_ID_MCUMGR_PARAMS(6). - Documentation, release note, and sample.yaml test config updates.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| boot/boot_serial/src/boot_serial.c | Implements the new CBOR response handler and wires it into the command dispatch. |
| boot/boot_serial/src/boot_serial_priv.h | Adds the NMGR_ID_MCUMGR_PARAMS command ID constant. |
| boot/zephyr/Kconfig.serial_recovery | New Kconfig option with dependency on default line length. |
| boot/zephyr/include/mcuboot_config/mcuboot_config.h | Maps Kconfig symbol to MCUboot macro. |
| boot/zephyr/sample.yaml | Enables the new option in the serial mgmt all-options test. |
| docs/serial_recovery.md | Documents the new supported command. |
| docs/release-notes.d/serial-recovery-mcumgr-params.md | Adds a release note entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nordicjm
left a comment
There was a problem hiding this comment.
Good addition, just some styling parts then ready to merge
| if ( | ||
| zcbor_map_start_encode(cbor_state, 10) && | ||
| zcbor_tstr_put_lit_cast(cbor_state, "buf_size") && | ||
| zcbor_uint32_put(cbor_state, MCUBOOT_SERIAL_MAX_RECEIVE_SIZE) && | ||
| zcbor_tstr_put_lit_cast(cbor_state, "buf_count") && | ||
| zcbor_uint32_put(cbor_state, 1) && | ||
| zcbor_map_end_encode(cbor_state, 10) |
There was a problem hiding this comment.
use in-tree style with bool:
mcuboot/boot/boot_serial/src/boot_serial.c
Line 1098 in 43e95b5
| If enabled, support for the mcumgr OS group "MCUmgr parameters" | ||
| command (command ID 6) is added, mirroring the command provided by | ||
| Zephyr's SMP server. It reports the SMP transport buffer size | ||
| (BOOT_SERIAL_MAX_RECEIVE_SIZE) and buffer count so that SMP clients | ||
| can negotiate optimal serial fragmentation. The parameters do not | ||
| carry the transport line length, so this option requires | ||
| BOOT_MAX_LINE_INPUT_LEN to remain at the standard 128-byte fragment | ||
| size. buf_count will always be 1. |
43e95b5 to
ee0a699
Compare
|
@nordicjm Converted back to draft while I investigate an important finding. I've added emulated integration tests over at smpclient and confirmed that Zephyr's smp server mcumgr params reports the unencoded, not encoded, buffer size. For mcuboot, the simplest backwards compatible solution is to report 685B (calculated at compile time) "netbuf" size for its 1024 buffer. This is the worst case base64 cost - 4 for framing. This also suggests the Zephyr's smp server has a larger incoming-encoded buffer than it's decoded buffer (what is reported by mcumgr params). I will take a look at the kconfig + compile time assertions to double check that it's safe. |
Following up on my note above — I traced both code bases to pin down whether Client side:
|
I suspect it's all OK. So MCUBoot has a 1024 byte decoded packet buffer. The smp client can (and should) send the "max encoded packet" of ~1,368 bytes. Fragmented across ~11 128 byte encoded SMP frames. Assuming that each frame is decoded and written to the dec_buf in its decoded form as soon as it is received. Unrelated, but that means that the incoming buffer only needs to be size 128, but it doesn't matter, mcuboot should always have tons of RAM to spare. |
|
Traced both code bases (read-only) to settle this: the PR is correct as written, and no other mcuboot change is needed beyond an optional doc/assert tidy-up. Pinned to mcuboot@ 1.
|
ee0a699 to
48afec5
Compare
|
Moved out of draft again after doing some exhaustive integration testing and research. I'm happy with it, but make sure to review the Kconfig that I decided to update based on the research. |
| if enabled, support for the mcumgr echo command is being added. | ||
|
|
||
| config BOOT_MGMT_MCUMGR_PARAMS | ||
| bool "SMP server buffer size" |
There was a problem hiding this comment.
would change the prompt name to have e.g. MCUmgr OS mgmt params command as SMP server buffer size is not what it is
There was a problem hiding this comment.
Fixed, thank you!
Add support for the mcumgr OS management group "MCUmgr parameters" command (group 0, command ID 6), gated behind the new CONFIG_BOOT_MGMT_MCUMGR_PARAMS option. The read-only command reports the SMP transport buffer parameters as a CBOR map, mirroring the command provided by Zephyr's SMP server so that existing SMP clients can query them unchanged. The serial recovery reassembles one command at a time into a single buffer, so the response reports a buf_size of CONFIG_BOOT_SERIAL_MAX_RECEIVE_SIZE and a buf_count of 1. The parameters do not carry the transport line length, which SMP serial clients assume to be 128 bytes, so the option depends on CONFIG_BOOT_MAX_LINE_INPUT_LEN remaining at its default of 128. Clarify the BOOT_SERIAL_MAX_RECEIVE_SIZE help to describe it as the maximum decoded SMP frame (the value reported as buf_size) rather than an encoded line-buffer product, and add a Kconfig range so that a buffer too small to hold a fragment fails at configuration time instead of silently dropping frames. Signed-off-by: JP Hutchins <jp@intercreate.io> Assisted-By: Claude:opus-4.8
48afec5 to
2a207c6
Compare
Summary
Adds an optional, Kconfig-gated mcumgr MCUmgr parameters command to the
serial-recovery SMP server (OS management group
0, command ID6,read-only), mirroring the command provided by Zephyr's SMP server.
When
CONFIG_BOOT_MGMT_MCUMGR_PARAMSis enabled the server answers with theCBOR map
{"buf_size": <CONFIG_BOOT_SERIAL_MAX_RECEIVE_SIZE>, "buf_count": 1},allowing SMP clients (e.g. smpclient)
to auto-negotiate serial fragmentation instead of hardcoding buffer sizes.
buf_countis1because serial recovery reassembles one command at a timeinto a single buffer.
Why the 128-byte line-length requirement
mcumgr parameters do not carry the transport line length (MTU); SMP serial
clients assume 128-byte fragments and derive
line_buffers = buf_size // 128.To keep the advertised
buf_sizeconsistent, the optiondepends on BOOT_MAX_LINE_INPUT_LEN = 128— setting a different line lengthmakes Kconfig refuse the option.
Compatibility
Identical wire contract to upstream Zephyr's
os_mgmt_mcumgr_params— samegroup/id, read-only, same
buf_size/buf_countkeys andzcbor_uint32_putencoding — so existing SMP clients read it unchanged.
ROM / RAM cost
Measured on
nrf52840dk/nrf52840withserial_recovery.conf, with vs.without the option: +116 B flash, +0 B RAM.
Test coverage
Added
CONFIG_BOOT_MGMT_MCUMGR_PARAMS=yto theserial_recovery_all_optionstwister scenario, so the feature is build-covered on every PR via the existing
Zephyr twister CI.
🤖 Generated with Claude Code