Skip to content

feat(client): expose the SMP version on the convenience methods - #126

Open
eastagiletracker wants to merge 1 commit into
intercreate:mainfrom
eastagiletracker:agile-board/convenience-methods-smp-version
Open

eastagiletracker wants to merge 1 commit into
intercreate:mainfrom
eastagiletracker:agile-board/convenience-methods-smp-version

Conversation

@eastagiletracker

Copy link
Copy Markdown

This PR proposes exposing the SMP protocol version on the SMPClient convenience methods, so upload(), upload_file(), download_file() and ICUploadClient.ic_upload() can drive a server that predates SMP version 2, defaulting to Version.V2 exactly 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 a version field, so ImageUploadWrite(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() and ICUploadClient.ic_upload() construct ImageUploadWrite / FileUpload / FileDownload without a version, so every frame they put on the wire falls back to smp's Version.V2 default. Someone talking to an SMP version 1 server therefore cannot use them at all, and has to hand-roll against request() the very chunking loop these routines exist to provide.

Reproduced on main at 84e00c2:

$ uv run python - <<'PY'
import inspect
from smpclient import SMPClient
from smpclient.extensions.intercreate import ICUploadClient
for fn in (SMPClient.upload, SMPClient.upload_file, SMPClient.download_file, ICUploadClient.ic_upload):
    print(f"{fn.__qualname__:34} version kwarg: {'version' in inspect.signature(fn).parameters}")
PY
SMPClient.upload                   version kwarg: False
SMPClient.upload_file              version kwarg: False
SMPClient.download_file            version kwarg: False
ICUploadClient.ic_upload           version kwarg: False

The change

A version keyword argument on each of the four routines, last in the signature and defaulting to Version.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 pass version= alongside that header, which stops smp logging Overriding 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_2 is the control for that. It drives all three SMPClient routines with no version argument and asserts every frame is still Version.V2, and it is the one new test that also passes against unmodified src/.

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 on main, they go red:

$ git checkout upstream/main -- src/ && uv run pytest tests -k smp_version -q
FAILED tests/extensions/test_intercreate.py::test_ic_upload_uses_the_requested_smp_version[1]
FAILED tests/test_smp_client.py::test_upload_uses_the_requested_smp_version[0]
FAILED tests/test_smp_client.py::test_upload_uses_the_requested_smp_version[1]
FAILED tests/test_smp_client.py::test_upload_file_uses_the_requested_smp_version[0]
FAILED tests/test_smp_client.py::test_upload_file_uses_the_requested_smp_version[1]
FAILED tests/test_smp_client.py::test_download_file_uses_the_requested_smp_version[0]
FAILED tests/test_smp_client.py::test_download_file_uses_the_requested_smp_version[1]
8 failed, 1 passed, 741 deselected in 4.31s

The single pass there is the default-version control. With the change applied:

$ uv run pytest tests -k smp_version -q
9 passed, 741 deselected in 1.59s

Keeping the new signatures but reverting only the propagation isolates the behavior from the surface: exactly the four Version.V1 cases fail, and the Version.V2 cases 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 on main, 406 passed / 14 skipped here. uv run camas coverage still clears the 91 % gate, and uv run --python 3.10 camas check on 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.

board

If you'd rather not receive contributions like this, reply no-more-prs on 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

@JPHutchins

Copy link
Copy Markdown
Collaborator

Thank you for taking this on!

@JPHutchins JPHutchins left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks good. It makes sense that it only impacts the convenience abstractions since they construct requests directly.

@JPHutchins JPHutchins left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

JPHutchins added a commit to JPHutchins/smp that referenced this pull request Sep 22, 2026
`SMPRequest.to_frame()` declared no parameters, so code programming
against the Protocol could not pass `sequence`, `version`, or `flags`
even though `Data.to_frame()` has always accepted them:

    def client_owned_sequence(
        request: SMPRequest[Any, Any, Any], sequence: int
    ) -> Frame[Any]:
        return request.to_frame(sequence=sequence)

    mypy:    error: Unexpected keyword argument "sequence" for "to_frame"
             of "SMPRequest"  [call-arg]
    pyright: error: No parameter named "sequence"  (reportCallIssue)

`SMPClient.request()` takes an `SMPRequest`, so a client was structurally
forced onto the module-global counter in `smp.message` -- process-global
mutable state shared by every caller in the process. With N concurrent
clients the 8 bit sequence space is consumed N times as fast, and no
caller can reason about its own next sequence, which is what the resync
and retry work downstream needs (intercreate/smpclient#56, #104, #129).

Protocol
--------
`SMPRequest.to_frame()` now declares `sequence`, `version`, and `flags`,
matching `Data.to_frame()`. The same gap hid `version`, which
intercreate/smpclient#126 needs to expose an SMP version on the
convenience methods.

Breaking
--------
`_counter` is gone and `sequence` is required. There is no natural
instance in `smp` to own a counter -- a `Data` is one message, and `smp`
has no session or connection object -- so ownership moves to the entity
whose sequence space it actually is: one `smpclient.SMPClient` per
connection, whose sequences are then monotonic and independent rather
than interleaved with every other client in the process.

`sequence` is positional-or-keyword and leads the signature; every
existing call site already passes it explicitly. Out-of-range values
still fail at the `struct` boundary -- no new validation is introduced,
and the `% 0x100` wrap the counter used to apply is now the caller's
responsibility.

Tests
-----
- tests/test_generics_typing.py drives the issue's repro through the
  Protocol under both mypy and pyright, taking `sequence` positionally
  so the concrete `Data.to_frame` must stay in lockstep with it.
- tests/test_message.py asserts the sequence reaches the wire verbatim,
  that omitting it is a `TypeError`, and that out-of-range still raises
  `struct.error`.
- `assert_frame` asserts `header.sequence == sequence` rather than a
  range; the range check existed only because the library, not the
  caller, produced the value.

Wire format is unchanged: all binary regressions remain byte-exact.

Co-Authored-By: claude-opus-5[1m] <noreply@anthropic.com>
`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.

Assisted-by: Claude Code:claude-opus-5
@eastagiletracker
eastagiletracker force-pushed the agile-board/convenience-methods-smp-version branch from 7aa1eba to 6d39234 Compare September 28, 2026 04:07
@eastagiletracker

Copy link
Copy Markdown
Author

Thanks @JPHutchins! I've amended the commit with an Assisted-by: Claude Code:claude-opus-5 trailer, following the kernel format you linked.

This branch has not been deployed

No deployments
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.

Convenience methods should expose SMP version kwarg

2 participants