feat(client): expose the SMP version on the convenience methods - #126
Open
eastagiletracker wants to merge 1 commit into
Open
Conversation
`request()` can send any SMP version, because its caller builds the request. The routines that build their own requests could not: `upload()`, `upload_file()`, `download_file()` and `ICUploadClient.ic_upload()` always put SMP version 2 on the wire, so a server that predates it could only be driven by hand-rolling the chunking loop that these routines exist to provide. Add a `version` keyword argument to each, defaulting to `Version.V2` so existing callers are unaffected, and carry the version through the packet-maximizing helpers so every chunk of a multi-packet transfer uses it. The helpers now pass `version` alongside the header they build, which also keeps `smp` from logging an "Overriding self.version" warning for each maximized chunk when the two disagree.
Collaborator
|
Thank you for taking this on! |
JPHutchins
approved these changes
Aug 19, 2026
JPHutchins
left a comment
Collaborator
There was a problem hiding this comment.
This looks good. It makes sense that it only impacts the convenience abstractions since they construct requests directly.
JPHutchins
requested changes
Aug 21, 2026
JPHutchins
left a comment
Collaborator
There was a problem hiding this comment.
The PR appears to be authored by an LLM. Please ammend the commit to add the model ID. See https://docs.kernel.org/process/coding-assistants.html
Co-Authored-By and linux form are both acceptable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR proposes exposing the SMP protocol version on the
SMPClientconvenience methods, soupload(),upload_file(),download_file()andICUploadClient.ic_upload()can drive a server that predates SMP version 2, defaulting toVersion.V2exactly as #43 asks. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/406. You can sign in with your GitHub ID to claim ownership of the project.Fixes #43.
What is missing today
request()can send any SMP version, because its caller builds the request:smp's message classes take aversionfield, soImageUploadWrite(off=0, data=b"", image=0, len=4, version=Version.V1)works today. The four routines that build their own requests cannot be told which version to use.upload(),upload_file(),download_file()andICUploadClient.ic_upload()constructImageUploadWrite/FileUpload/FileDownloadwithout aversion, so every frame they put on the wire falls back tosmp'sVersion.V2default. Someone talking to an SMP version 1 server therefore cannot use them at all, and has to hand-roll againstrequest()the very chunking loop these routines exist to provide.Reproduced on
mainat 84e00c2:The change
A
versionkeyword argument on each of the four routines, last in the signature and defaulting toVersion.V2, threaded into every request they build._maximize_upload_packet()and_ic_maximize_packet()already carried the version across from the header they are handed, so a multi-packet transfer keeps it; they now also passversion=alongside that header, which stopssmploggingOverriding self.version=<Version.V2: 1> with self.header.version=<Version.V1: 0>once per maximized chunk.Backward compatibility: the new argument is last and its default is the version these routines already sent, so no existing call changes behavior.
test_convenience_methods_default_to_smp_version_2is the control for that. It drives all threeSMPClientroutines with noversionargument and asserts every frame is stillVersion.V2, and it is the one new test that also passes against unmodifiedsrc/.Verification
Each new test asserts the version of every frame the routine puts on the wire across a 4097-byte transfer that spans several chunks, so the packet-maximizing path is covered rather than just the first request. Against
src/as it stands onmain, they go red:The single pass there is the default-version control. With the change applied:
Keeping the new signatures but reverting only the propagation isolates the behavior from the surface: exactly the four
Version.V1cases fail, and theVersion.V2cases plus the default control stay green.uv run camas check(ruff check,pydoclint,mypy,pytest) is green both before and after, with no new failures: 397 passed / 14 skipped onmain, 406 passed / 14 skipped here.uv run camas coveragestill clears the 91 % gate, anduv run --python 3.10 camas checkon the oldest supported interpreter is green too.One merge note: #58 also edits
src/smpclient/__init__.py. If that lands first this needs a small rebase, since the change here is confined to the request-building arguments.How this was managed
We imported this repository's issues and pull requests onto an agile board as 111 stories, and used it to manage this work: the story this PR delivers is Convenience methods should expose SMP version kwarg, on the board at https://eastagiletracker.com/projects/406.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com