wip: use mcumgr params for serial transports by JPHutchins · Pull Request #81 · intercreate/smpclient

JPHutchins · 2025-10-12T20:56:55Z

Related to #73

~~This is a breaking change.~~

Note

No longer a breaking change. The 7.1.0 constructor params (max_smp_encoded_frame_size, line_length, line_buffers) still work via @overload + @deprecated over a single implementation: legacy keyword and positional calls construct as before, map onto the equivalent BufferParams, and emit a DeprecationWarning (flagged at the type level too — pyright by default, mypy under --enable-error-code=deprecated). The Auto/BufferSize/BufferParams strategy types are now module-level with a FragmentationStrategy union alias.

I want to setup a simple Zephyr repo to produce test FWs. I'd like to include some QEMU builds so that we can setup automated regression testing for this sorta thing.

Thank you for the research, @joerchan!

Copilot

Pull Request Overview

This PR refactors the SMPSerialTransport class to use mcumgr parameters for configuring serial transport buffers. It introduces a new fragmentation strategy pattern with Auto() and BufferParams classes, replacing the previous direct parameter approach.

Replaces direct constructor parameters with a fragmentation_strategy parameter that accepts either Auto() or BufferParams
Adds automatic buffer configuration based on server's MCUMGR_PARAM buffer size
Updates all test cases and examples to use the new API

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
smpclient/transport/serial.py	Refactors constructor to use fragmentation strategy pattern and adds server buffer size initialization
tests/test_smp_serial_transport.py	Updates tests for new constructor API and adds tests for Auto/BufferParams modes
tests/test_smp_client.py	Updates test transport creation to use new BufferParams syntax
examples/usb/upgrade.py	Updates example code to use new BufferParams constructor pattern

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

JPHutchins · 2025-10-19T22:09:48Z

Great progress! We now have a native_sim fixture that we can use to test stuff out: https://github.com/intercreate/smp-server-fixtures/actions/runs/18636709577

We can always add QEMU in the future - in fact, that would be required to test image management firmware upgrades with MCUBoot - but the native_sim target made this really easy to start out.

In the long run, it probably makes sense to matrix a bunch of configs with twister and then commit the zip to this repo for integration testing.

native_sim terminal:

./build/smp-server/zephyr/zephyr.exe
uart connected to pseudotty: /dev/pts/15
*** Booting Zephyr OS build v4.2.0-6166-g8a72d776c5af ***
[00:00:00.000,000] <inf> littlefs: LittleFS version 2.11, disk version 2.1
[00:00:00.000,000] <inf> littlefs: FS at flash-controller@0:0xfc000 is 4 0x1000-byte blocks with 512 cycle
[00:00:00.000,000] <inf> littlefs: partition sizes: rd 16 ; pr 16 ; ca 64 ; la 32
[00:00:00.000,000] <inf> smp_sample: build time: Oct 19 2025 14:56:16

smpmgr terminal:

smpmgr --port /dev/pts/15 statistics list
⠋ Connecting to /dev/pts/15... OK
⠋ Waiting for response to ListOfGroups... OK
             Statistics Groups
┏━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┓
┃ Group Name           ┃ Number of Groups ┃
┡━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━┩
│ flash_sim_stats      │ 3                │
│ flash_sim_thresholds │ 3                │
│ smp_svr_stats        │ 3                │
└──────────────────────┴──────────────────┘

JPHutchins

I think that more optimized defaults should be added. For example, I think that MCUBoot default is 128 * 8, Zephyr main is 128 * 2, etc. Allows the user to opt out of MCUmgr params negotiation and still get maximum throughput.

JPHutchins · 2025-12-20T23:44:34Z

+        if isinstance(self._fragmentation_strategy, SMPSerialTransport.Auto):
+            return self.BufferParams().line_length
+        else:
+            return self._fragmentation_strategy.line_length


I think that we dropped 3.9 support, so this should be replaced with exhaustive match case.

… suite Launch real Zephyr SMP servers from the vendored smp-server-fixtures release (registry built from manifest.json) and drive them with SMPClient. Covers the buffer-size matrix, serial/shell/udp(IPv4+IPv6) transports, fs file round-trip, img DFU, and MCUboot serial recovery via `os reset boot_mode` (smp 4.1.0). Linux-only; excluded from the default `task test` via the `integration` marker. Also sets the serial Auto line_length default to 128. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 19 out of 35 changed files in this pull request and generated 1 comment.

JPHutchins · 2026-06-03T17:39:59Z

+    @property
+    def _line_buffers(self) -> int:
+        """The number of line buffers."""
+        if isinstance(self._fragmentation_strategy, SMPSerialTransport.Auto):
+            if self._smp_server_transport_buffer_size is not None:
+                return self._smp_server_transport_buffer_size // self.BufferParams().line_length
+            return self.BufferParams().line_buffers
+        else:
+            return self._fragmentation_strategy.line_buffers


Good catch on the floor-division inconsistency — fixed in a714fc8, though the empirical result is the opposite of an overflow.

I probed the real servers (native_sim / QEMU / mps2): buf_size is CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE, which bounds the decoded SMP serial frame uint16 length | message | uint16 CRC16 reassembled into one netbuf — the base64/line framing is stripped before the netbuf. So the limit is simply buf_size - 4 (2-byte length + 2-byte CRC16), and buf_count is irrelevant to a single serial message (one netbuf per message).

Boundary, confirmed at three sizes (client allowed to send past the old cap):

buf_size buf_size - 4 buf_size - 3

96 accepted dropped

256 accepted dropped

384 accepted dropped

So the old base64_max(buf_size) - per_line_framing * (buf_size // line_length) was actually ~25-30% conservative (e.g. buf384 → 255 instead of 380), not over-permissive — and the floor-division line count you flagged fed that wrong framing term.

The fix drops the line-count dependency in Auto: max_unencoded_size = buf_size - (FRAME_LENGTH + CRC16). BufferParams mode (the encoded line-buffer model, e.g. MCUboot's 128×8 UART buffer) is unchanged. Locked by tests: a unit assert (buf_size - 4), an integration assert across the buffer matrix, and test_max_payload_roundtrip actually round-tripping a max-size message (including the sub-line-length buf96) against the real fixtures.

… buffer fixtures - Exclude tests/integration via --ignore (not -m 'not integration') so the marker expression's quoting doesn't break the PowerShell-run task on Windows. - QemuSocketSerialTransport accepts an explicit fragmentation_strategy. - serial recovery test uploads at both 128x1 (client default) and 128x8 (MCUboot default UART buffer), exercising 1- vs 8-line-packet transactions to the bootloader's SMP server. - Vendor qemu_cortex_m0 serial_buf256/buf512 so max-payload fill runs on a non-bursty target at those buffer sizes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…meout A full-buffer echo on the emulated cortex-m0 (nRF51) takes >2.5s under CI load; the default request timeout tripped test_max_payload_roundtrip[qemu...buf512]. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The emulated nRF51 (16 KB RAM) hangs ~9% of the time on a full 512-byte-buffer echo, tripping the request timeout. buf256 and the 384 serial fixture are reliable, so 256-byte buffer fill is retained; reliable 512+ buffer-fill on a real-ish MCU is a build-farm gap (needs a roomier, non-bursty fixture). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…l buffers Tighten test_max_payload_roundtrip to skip a bursty native_sim fixture only when the max-size message needs >2 line packets. This runs the round-trip on buf96 (sub-line-length, line_buffers=0) and buf256 even on native_sim, proving the server actually accepts a message sized at the client's max_unencoded_size -- a regression guard for the Auto buf_size math (re: PR review on non-multiple / sub-line-length buffers). Empirically buf_size bounds the server's decoded netbuf, so the current math is conservative (no overflow), and this test locks that in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…head) The Auto strategy modelled the server's MCUmgr buf_size as an *encoded* budget (base64_max(buf_size) - per-line framing), which both depended on a floor-division line-buffer count (PR review: inconsistent for non-multiples) and left ~25-30% of the buffer unused. But buf_size is CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE, which bounds the *decoded* SMP serial frame `length | message | CRC16` reassembled into one netbuf -- so the message is simply buf_size minus the 2-byte length + 2-byte CRC16. buf_count is irrelevant to a single serial message (one netbuf per message). Verified end-to-end against native_sim/QEMU/mps2 (buf 96/256/384): a buf_size-4 message round-trips; buf_size-3 is dropped. Tests: - unit: max_unencoded_size == buf_size - 4 after Auto initialize. - integration: test_auto_config asserts the exact relation across the matrix, and test_max_payload_roundtrip round-trips a max-size message (incl. buf96/buf256). - per-fixture max_reliable_line_packets steers full-buffer transactions onto roomy targets (native_sim PTY drops >2-packet bursts; emulated nRF51 hangs beyond ~3), so DFU runs to completion on mps2 (Cortex-M3) and skips the constrained nRF51. BufferParams mode (encoded line-buffer model, e.g. MCUboot 128x8) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ion across the matrix Bump vendored SMP server fixtures 2ad0d6bf -> 0eae053d (the build farm closing intercreate/smp-server-fixtures#4 and #5) and lean on the new fixtures: - mps2_an385 serial_recovery buffer matrix (buf400/512/1024/2048): a reliable, link-paced Cortex-M3 target that round-trips a real buffer-filling message where native_sim is bursty and qemu_cortex_m0 is too small. In app mode it advertises MCUmgr params, so Auto drives echo/fs/img at each buf_size. - native_sim serial_bigrx: large UART RX pool, so full multi-fragment messages round-trip on native_sim (no >2-fragment burst drop). - native_sim serial_line512: legacy-interop only (non-128 line length, for non-compliant devices; downstream users should never change line_length). Validate that smpclient MAXIMIZES per-request payload for best link throughput: assert_chunks_maximized() asserts the largest upload stride comes within a small framing budget of max_unencoded_size, wired through DFU, fs, and serial-recovery uploads. test_max_payload_roundtrip now confirms a buf_size-4 message round-trips at 2048 (2044 B unencoded, ~2.8k encoded) on the reliable matrix for the first time. The do-it-all recovery image's RAM-backed littlefs is unreliable for sizeable files on the large-netbuf variants (client uploads fine; server read-back flakes), so the fs round-trip skips recovery buf1024/buf2048; their buffer-fill stays covered by DFU (flash slot) and the max-payload echo. Integration: 199 passed, 65 skipped. task all: lint/mypy clean, 290 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 19 out of 48 changed files in this pull request and generated 4 comments.

+    @override
+    async def connect(self, address: str, timeout_s: float) -> None:
+        self._reset_state()
+        loop = asyncio.get_event_loop()


+
+        ready_pattern = fixture.ready_pattern()
+        endpoint_future: asyncio.Future[Endpoint] | None = (
+            None if ready_pattern is None else asyncio.get_event_loop().create_future()


+    mtu = PropertyMock()
+    max_unencoded_size = PropertyMock()
+


+    # Auto uses the full decoded netbuf: buf_size minus the 2-byte SMP serial frame
+    # length and 2-byte CRC16 (verified on native_sim/QEMU/mps2: buf_size-4 round-trips).
+    assert t.max_unencoded_size == 512 - 4


Add a "Server Buffers & SMP Serial Fragmentation" section: the encoded-vs-decoded frame model (max message = reassembly buffer − 4), a structure diagram and an incremental-decode timing diagram, and a unified mcuboot↔Zephyr terminology table with permalinks to each buffer's Kconfig/source definition. Documents why Auto (decoded netbuf) fills the buffer while BufferParams (encoded model) under-fills. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…buffer Add `SMPSerialTransport.BufferSize(buf_size, line_length=128)` as a first-class member of the `Auto | BufferSize | BufferParams` strategy sum type. It fills the server's decoded reassembly buffer (max message = buf_size - 4) exactly like `Auto`, but for servers that cannot advertise MCUmgr params (e.g. MCUboot serial recovery). Strategy dispatch is now exhaustive `match`/`assert_never`. Unify `_maximize_image_upload_write_packet` and `_maximize_file_upload_packet` into a generic `_maximize_upload_packet[TUploadRequest]`. It fresh-constructs the packet rather than using `model_copy`, which would carry stale serialized bytes (smp message `smp_data` is computed only in `model_post_init`, which copy skips); verified byte-identical to the prior methods. Add unit assertions that the on-wire encoded frame exceeds buf_size (~1.37x) for both `Auto` (post-params) and `BufferSize`, and that the maximizer fills `max_unencoded_size` for image and file uploads. The recovery integration test now uploads at `BufferSize(1024)` (MCUboot's default recovery buffer) and is renamed to `test_upload_to_mcuboot_recovery_smp_server` to reflect that the MCUboot `boot_serial` SMP server is the subject under test. Move the fragmentation-strategy guidance into the serial module docstring (rendered by mkdocstrings); fix and unify the `line_length` docstrings (it is the total fragment line length incl. the 2-byte delimiter + newline, not the base64 payload); the README now points to the API docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

JPHutchins · 2026-06-04T21:22:08Z

@joerchan The outlook is better than I'd hoped. Zephyr and MCUBoot's servers are reporting the unencoded buffer sizes. So SMP client is updated in this PR to allow up to ~1.37x the buffer size encoded. It means that Zephyr's default of 384 can receive ~587 and MCUBoot's default of ~1024 can receive ~1400. Also rolling into a PR over at mcuboot: mcu-tools/mcuboot#2746

JPHutchins · 2026-06-04T21:24:02Z

Integration test suite now has full automated coverage for DFU, all groups, serial fragmentation running on QEMU and native_sim: https://github.com/intercreate/smpclient/actions/runs/26980129038/job/79616993540?pr=81

@Overloads

…types to module level The fragmentation_strategy sum-type API had replaced the 7.1.0 (max_smp_encoded_frame_size, line_length, line_buffers) constructor params. Make that transition non-breaking instead: - __init__ is now three @Overloads over one impl: the modern fragmentation_strategy, plus two @deprecated overloads (keyword- and positional-legacy) so the keyword forms, SMPSerialTransport(256), and (256, 128, 2) all still construct. The impl maps the legacy params onto the equivalent BufferParams (byte-faithful to 7.1.0 for consistent configs, preserving the old frame != line_length*line_buffers log-and-ignore) and emits a DeprecationWarning. An explicit strategy always wins, so a stray arg can never silently rewrite fragmentation. - @deprecated flags the legacy overloads at the type level (pyright by default, mypy via --enable-error-code=deprecated). PEP 702 checkers ignore a name reference, so the message is inlined as a literal and kept in sync with the runtime warning. - Auto/BufferSize/BufferParams move from nested classes to module level, with a FragmentationStrategy = Auto | BufferSize | BufferParams alias. All call sites (unit + integration tests, servers, example) updated. task all green (ruff/pydoclint/mypy/311 tests); mkdocs build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 21 out of 50 changed files in this pull request and generated 1 comment.

+        if not isinstance(fragmentation_strategy, int) and fragmentation_strategy is not None:
+            return fragmentation_strategy  # explicit modern strategy; ignore stray legacy args
+
+        legacy_frame: Final = (
+            max_smp_encoded_frame_size
+            if max_smp_encoded_frame_size is not None
+            else (fragmentation_strategy if isinstance(fragmentation_strategy, int) else None)
+        )
+        if legacy_frame is None and line_length is None and line_buffers is None:
+            return Auto()
+
+        warnings.warn(_LEGACY_PARAMS_DEPRECATION, DeprecationWarning, stacklevel=3)
+        resolved_line_length: Final = _DEFAULT_LINE_LENGTH if line_length is None else line_length
+        resolved_line_buffers: Final = (
+            _LEGACY_LINE_BUFFERS if line_buffers is None else line_buffers
+        )
+        budget: Final = resolved_line_length * resolved_line_buffers
+        if legacy_frame is not None and legacy_frame != budget:
+            logger.warning(
+                f"max_smp_encoded_frame_size={legacy_frame} is not equal to "
+                f"line_length={resolved_line_length} * line_buffers={resolved_line_buffers}; "
+                f"using {budget}"
+            )
+        return BufferParams(line_length=resolved_line_length, line_buffers=resolved_line_buffers)


JPHutchins requested review from JerryCapistrano, KungFuSensai, aldenhaase, Copilot, davedesro, klam20, robhelvestine and swoisz October 12, 2025 20:56

Copilot AI reviewed Oct 12, 2025

View reviewed changes

Comment thread smpclient/transport/serial.py Outdated

Comment thread smpclient/transport/serial.py Outdated

JPHutchins force-pushed the feature/#73/serial-fragmentation-uses-netbuf-param branch from fd8d90b to 523d358 Compare October 12, 2025 21:00

JPHutchins commented Dec 20, 2025

View reviewed changes

This was referenced May 29, 2026

docs: add serial recovery throughput tuning guide mcu-tools/mcuboot#2640

Open

boot: boot_serial: add optional mcumgr parameters command intercreate/mcuboot#3

Closed

boot: boot_serial: add optional mcumgr parameters command mcu-tools/mcuboot#2746

Open

JPHutchins and others added 2 commits June 2, 2026 23:27

wip: use mcumgr params for serial transports

98a8f51

JPHutchins force-pushed the feature/#73/serial-fragmentation-uses-netbuf-param branch from 523d358 to cad229e Compare June 3, 2026 16:27

JPHutchins requested a review from Copilot June 3, 2026 16:27

Copilot started reviewing on behalf of JPHutchins June 3, 2026 16:27 View session

Copilot AI reviewed Jun 3, 2026

View reviewed changes

JPHutchins and others added 6 commits June 3, 2026 09:37

JPHutchins requested a review from Copilot June 4, 2026 00:38

Copilot started reviewing on behalf of JPHutchins June 4, 2026 00:38 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

JPHutchins mentioned this pull request Jun 4, 2026

mgmt/mcumgr/transport: uart: assert UART MTU fits the netbuf intercreate/zephyr#2

Draft

JPHutchins and others added 2 commits June 4, 2026 12:45

JPHutchins requested a review from Copilot June 4, 2026 23:48

Copilot started reviewing on behalf of JPHutchins June 4, 2026 23:48 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip: use mcumgr params for serial transports#81

wip: use mcumgr params for serial transports#81
JPHutchins wants to merge 11 commits into
mainfrom
feature/#73/serial-fragmentation-uses-netbuf-param

JPHutchins commented Oct 12, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

JPHutchins commented Oct 19, 2025 •

edited

Loading

Uh oh!

JPHutchins left a comment

Uh oh!

JPHutchins Dec 20, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

JPHutchins Jun 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

JPHutchins commented Jun 4, 2026

Uh oh!

JPHutchins commented Jun 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

JPHutchins commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

JPHutchins left a comment

Choose a reason for hiding this comment

Uh oh!

JPHutchins Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

JPHutchins Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

JPHutchins commented Jun 4, 2026

Uh oh!

JPHutchins commented Jun 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JPHutchins commented Oct 12, 2025 •

edited

Loading

JPHutchins commented Oct 19, 2025 •

edited

Loading