Skip to content

Conversation

@JPHutchins
Copy link
Collaborator

Related to #73

This is a breaking change.

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!

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 JPHutchins force-pushed the feature/#73/serial-fragmentation-uses-netbuf-param branch from fd8d90b to 523d358 Compare October 12, 2025 21:00
@JPHutchins
Copy link
Collaborator Author

JPHutchins commented Oct 19, 2025

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                │
└──────────────────────┴──────────────────┘

Copy link
Collaborator Author

@JPHutchins JPHutchins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +147 to +150
if isinstance(self._fragmentation_strategy, SMPSerialTransport.Auto):
return self.BufferParams().line_length
else:
return self._fragmentation_strategy.line_length
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants