-
Notifications
You must be signed in to change notification settings - Fork 23
wip: use mcumgr params for serial transports #81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
wip: use mcumgr params for serial transports #81
Conversation
There was a problem hiding this 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_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.
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.
| 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.
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.
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!