Conversation
There was a problem hiding this comment.
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_strategyparameter that accepts eitherAuto()orBufferParams - 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.
fd8d90b to
523d358
Compare
|
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: smpmgr terminal: |
JPHutchins
left a comment
There was a problem hiding this comment.
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.
| if isinstance(self._fragmentation_strategy, SMPSerialTransport.Auto): | ||
| return self.BufferParams().line_length | ||
| else: | ||
| return self._fragmentation_strategy.line_length |
There was a problem hiding this comment.
I think that we dropped 3.9 support, so this should be replaced with exhaustive match case.
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!