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

feat(client): expose the SMP version on the convenience methods#126
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

`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.
@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.

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