breaking: the effect-stack lifecycle (#58): transports own their link, options, and sizing - #144
Open
JPHutchins wants to merge 15 commits into
Open
JPHutchins wants to merge 15 commits into
JPHutchins wants to merge 15 commits into
Conversation
`SMPUDPTransport.max_unencoded_size` overrode the base implementation with the MSS alone, so the MCUmgr parameters that `SMPClient` reads on connect never reached the UDP transport: `initialize(buf_size)` stored the value and nothing read it. Zephyr's UDP SMP transport receives each request as a single datagram into one MCUmgr buffer of `CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE` bytes, the value advertised as `buf_size`. A request larger than that cannot be received. The defaults (2048-byte buffer, 1500-byte MTU) hide this. A build that lowers the buffer, e.g. to its 384-byte non-UDP default, does not. The payload is now `min(MSS, buf_size)`. Before the params are known, `buf_size or mtu` makes that the MSS, so nothing changes for a server that does not advertise them. Verified: `camas check` green; the new parametrized test fails on the 384-byte case without the fix. https://github.com/zephyrproject-rtos/zephyr/blob/70be2ff0b565a3313128f5577f51cfeb3ebcf602/subsys/mgmt/mcumgr/grp/os_mgmt/src/os_mgmt.c#L551-L554 https://github.com/zephyrproject-rtos/zephyr/blob/70be2ff0b565a3313128f5577f51cfeb3ebcf602/subsys/mgmt/mcumgr/transport/Kconfig#L33-L58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
fix(udp): cap the payload at the server's advertised buffer
JPHutchins
commented
Sep 23, 2026
| """ | ||
|
|
||
| from smpclient.transport.serial.encoded import Auto as Auto | ||
| from smpclient.transport import Auto as Auto |
Collaborator
Author
There was a problem hiding this comment.
Why re-export these?
Collaborator
Author
There was a problem hiding this comment.
The existing re-exports were needed becuase of the encoded/unencoded split
Collaborator
Author
There was a problem hiding this comment.
Warning
LLM Disclosure
This reply was authored by claude-opus-5-5[1m] on behalf of @JPHutchins. It answers this thread for the force-pushed revision of the PR.
These re-exports are gone in the new revision. smpclient/transport/serial/__init__.py no longer re-exports any shared smpclient.transport type. Auto is imported from smpclient.transport, as are the shared BufferSize and Unfragmented.
What it does export are serial's own types, defined in its encoded, unencoded, and common submodules. That is the same split-driven reason as before: serial's own BufferSize (unchanged from main), plus the new SerialOptions and SerialPort from common.
Collaborator
Author
|
Warning LLM Disclosure This review was authored by Read it as a self-review. The implementer and the Summary
Bugs to fix (9), each with a proposed fix
Smaller items:
Your decisions (4)A. The cost of the timeout policy (
B. The sequence number of the params read (review #2 and #12, advisor). The read hardcodes sequence 0, outside the client's injectable sequence space, which is the principle smp#71 set.
C. Naming (your call during review). Rename the concept to
The prefixes are there because the console and raw transports share the D. Split the PR? Most of the churn is forced by the design:
What could come out, if you want a smaller PR:
Public surface you haven't approved yet
Pushed back on (1)
Next: once A–D are answered, one fix commit for bugs 1–9 and the smaller items, then the naming rename, then re-gate and re-run integration. 🤖 Generated with Claude Code |
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…request
A verbatim move with no behavior change: the TypeVars, the `success`/`error`
narrowers, `wrapping_sequence`, the validation diagnostics, and the body of
`SMPClient.request`, which becomes `_request.exchange(transport, request,
sequence, timeout_s)`. `SMPClient.request` delegates to it, and `smpclient`
re-exports every public name unchanged.
The next commit needs this: a transport reads the server's MCUmgr parameters
while it connects, before any `SMPClient` exists, so the exchange must sit below
the client. Review with `git show --color-moved`; 252 of the 284 changed lines
are moved.
`smpclient` imports `_request` as a module (`from smpclient import _request`),
so the package attribute stays the submodule. An `import ... as _request` of the
function would shadow it and break `mock.patch("smpclient._request.…")`. The
function is named `exchange` so that its `request` parameter doesn't shadow it.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…lient never opens a link `SMPClient(transport, address, timeout_s)` becomes `SMPClient(transport, *, timeout_s, sequence)`. `connect()`, `disconnect()`, `address`, `__aenter__`/`__aexit__` and `_initialize()` are removed. The client only sends and receives over a transport that is already open. It holds the `SMPTransport` Protocol, which no longer has `connect` or `disconnect`, so it can't control the transport's side effects. Each transport now takes its address in the constructor (#58), along with `connect_timeout_s` and `sequence`. `connect()` and `disconnect()` take no arguments, and their bodies are unchanged: - serial: the old `connect` body becomes `_open()`; `connect()` is `_open()` then `negotiate()` - UDP: `SMPUDPTransport(address, port=1337, *, mtu, ...)`. The port is a real parameter, the point of #58. - bleak: `connect()` wraps `_connect(address, timeout_s)`, which is unchanged - bumble: `connect()` is the old flow, reading the address from `self`. `use_connection` becomes `borrow`, and the module helper `borrowed_connection()` becomes the method `borrowed()`. A private base, `_ConnectableTransport`, adds the encouraged bracket, `async with transport.connected():`. It connects, yields the transport, and disconnects best-effort. The primitives remain for lifetimes a lexical scope can't express, e.g. a standing link held for an application's lifetime. The MCUmgr parameters read moves out of `SMPClient._initialize` into the transport's `negotiate()`, with the same warnings and the same fallback on an error or a timeout (`_request.read_mcumgr_parameters`). `negotiate()` runs inside `connect()` and `borrow()`, and it is public, for re-negotiating: the integration harness uses it after a server boots, and a borrowed link can negotiate at all. Behavior is unchanged here: the read is still unconditional. The next commit makes it conditional on each transport's fragmentation strategy. `connect()` is all-or-nothing on every transport: a failed or cancelled negotiation closes the link it just opened. Tests move to address-first constructors and argument-free `connect()`. A `skip_negotiation` fixture answers the params read with `None`, so tests that drive `connect()` over mocked I/O don't wait for a server. The integration harness enters `transport.connected()` and re-negotiates after the echo wait, where it used to call `client._initialize()`. Its skip for a UDP fixture on a non-default port is gone. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e only when asked Each transport now declares how it sizes SMP messages with its own fragmentation strategy union, and reads the server's MCUmgr parameters only when that strategy asks for them. A pinned strategy never issues the read, so a server without the params command, like mcuboot serial recovery, never sees it. The shared vocabulary lives in `smpclient.transport`: - `Auto`: read `buf_size` while connecting. On a timeout or an error response, warn and fall back to the transport's conservative default. - `BufferSize(buf_size)`: a known server buffer; nothing is read. - `Unfragmented`: GATT only. Like `Auto`, but one message per write, for a server built without `CONFIG_MCUMGR_TRANSPORT_BT_REASSEMBLY`. The per-transport unions use the prefixed names: - `SerialFragmentationStrategy = Auto | BufferSize | BufferParams` (serial's own `BufferSize(buf_size, line_length)` and `BufferParams` are unchanged from main) - `RawSerialFragmentationStrategy = Auto | BufferSize` - `UDPFragmentationStrategy = Auto | BufferSize`, always capped at the MSS - `GATTFragmentationStrategy = Auto | Unfragmented | BufferSize`, shared by `SMPBLETransport` and `SMPBumbleTransport` through a `_GATTTransport` mixin `SMPTransport.initialize()` and `_smp_server_transport_buffer_size` are gone. Each transport's `negotiate()` matches its strategy exhaustively and stores `_negotiated_buf_size`; `max_unencoded_size` is derived from the strategy. Breaking: - `smpclient.transport.serial.FragmentationStrategy` is renamed `SerialFragmentationStrategy`. - `Auto` moves to `smpclient.transport`, since every transport uses it. - `SMPSerialRawTransport(port, mtu=384)` becomes `SMPSerialRawTransport(port, fragmentation_strategy=Auto())`; pin the old behavior with `BufferSize(384)`. Its `mtu` now reports `max_unencoded_size`, one whole message. - The serial "pinned size exceeds the server's buffer" warnings are removed: a pinned strategy no longer reads the parameters it would compare against. Tests: `tests/support.py` adds `advertise(buf_size)`, which patches the params read, and `negotiated(transport, buf_size)`. Each transport tests that a pinned strategy never reads, and how `Auto` and `Unfragmented` cap the size. The integration raw transport defaults to `Auto()`, so the suite exercises negotiation against the real fixtures (229 passed, 101 skipped, the same as before). Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The eleven pyserial keyword arguments are replaced by one
`options: SerialOptions = SerialOptions()` on `SMPSerialTransport`
(all three constructor overloads and the implementation) and on
`SMPSerialRawTransport`. The settings are declared once, in
`smpclient.transport.serial.common`, and exported from
`smpclient.transport.serial`; they are no longer repeated across the
four encoded signatures, the raw signature, and the base.
SMPSerialTransport(port, baudrate=9600)
SMPSerialTransport(port, options=SerialOptions(baudrate=9600))
`test_serial_options_lock_pyserial` locks the field names, their order,
and their defaults to `inspect.signature(serial.Serial)`. The one
deliberate difference is `baudrate`: 115200 here, 9600 in pyserial.
pyserial is effectively unmaintained, so drift isn't expected, but the
test fails loudly if it happens. A renamed field and a changed default
were each confirmed to fail the test.
Refs #58
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… flush fails `_open()` now runs pyserial's blocking `open()` and `reset_input_buffer()` in `asyncio.to_thread`, so a slow open (USB CDC ACM still enumerating, for example) no longer stalls the event loop. The leak, which is pre-existing on main: `reset_input_buffer()` was inside the retry `try`. When the flush raised `SerialException` after a successful `open()`, the loop called `open()` again on the open port. pyserial refuses that with another `SerialException`, so the loop spun until `connect_timeout_s` and raised `TimeoutError`, leaving the first fd open. Only `open()` is retried now (`try/except/else`), and `connect()` wraps `_open()` too in its all-or-nothing `except (Exception, CancelledError): close()`. A failed flush, or a cancellation during the open, closes the port and re-raises. A cancellation that lands while the worker thread is still inside `open()` cannot interrupt that thread. The best-effort `close()` runs either way, and is a no-op if the open hadn't finished. Tests: `test_connect_closes_the_port_when_the_flush_fails` fails on the previous code. `test_connect_closes_the_port_when_cancelled_while_negotiating` covers the cancel path. Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…cket chardev `_SerialTransportBase` gains the same borrow primitives the GATT transports have: - `await t.borrow(port)` adopts a caller's open `SerialPort`, clears the framing state, then runs `negotiate()`. It is all-or-nothing: if negotiation raises or is cancelled, the transport reverts to its own port. - `async with t.borrowed(port):` wraps borrow and disconnect in a bracket. - `disconnect()` returns a borrowed port without closing it; the acquirer releases. It still closes the transport's own `Serial`. `SerialPort` is the Protocol for the four members the transports use: `port`, `out_waiting`, `write`, and `read_all`. `serial.Serial` satisfies it, and so does a `serial_for_url` port that reports `out_waiting`. Both it and `SerialOptions` are exported from `smpclient.transport.serial`. Which port is live is a sum type, `_Link = _Owned | _Borrowed(port)`. `_conn` becomes a property that matches on it, and `_serial` is the transport's own `Serial`, still constructed closed in `__init__`. The unit tests' `t._conn.<attr> = MagicMock(...)` assignments still land on the owned mock, so they keep working unchanged. Integration harness: the `QemuSocketSerialTransport` and `QemuSocketSerialRawTransport` subclasses are gone. They overrode `_open` and replaced the `Final` `_conn` with `object.__setattr__`. Now `socket_link(transport, url)` opens the emulator's `socket://` chardev (paced for the raw transport), lends it with `transport.borrowed()`, and closes it on exit, so the suite drives the real public API. `ConnectedServer` carries its link as an `AsyncExitStack`, and `reboot_into_recovery` releases it with `link.aclose()` before opening the recovery link it is handed. Integration: 229 passed, 101 skipped, the same as before, with every socket fixture going through `borrowed()`. Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`SMPBLETransport` gains the borrow primitives the other transports have: - `await t.borrow(client)` adopts a caller's connected `BleakClient`, finds the SMP characteristic, sizes writes to the link, subscribes, and then runs `negotiate()`. It is all-or-nothing: on failure or cancellation the transport returns the client and re-raises. - `async with t.borrowed(client):` wraps borrow and disconnect in a bracket. - `disconnect()` on a borrowed client unsubscribes and never disconnects it; the acquirer releases. The `stop_notify` is bounded by `connect_timeout_s`, and a failure is logged rather than raised, so returning the client can't hang or mask the caller's error when the owner has already dropped the link. The part of `_connect()` after the link comes up is now `_start_smp()`, shared by connect and borrow, and it clears the receive buffer. Ownership is a sum type, `_Link = _Owned | _Borrowed(client)`. `_active_client` matches on it; `_client` stays the transport's own client, so the unit tests that assign it keep working. Disconnect detection: bleak takes `disconnected_callback` only when the client is constructed, and the owner holds it. So `_until_disconnected()` waits on the transport's event when it owns the client, and polls `client.is_connected` every 100 ms when it borrows one. The poll runs only inside a receive or GATT wait. There is no watcher task, so nothing outlives the primitive that started it. `_notify_or_disconnect` now reaps its two sub-tasks in a `finally`, like `_await_or_disconnect`. Before, cancelling a waiting `receive()` leaked both tasks; with a borrowed client, that would leave a poll loop running for as long as the owner's link stayed up. The old `except CancelledError: pass` around the reaping `gather` also swallowed a cancellation of the waiter itself; a positional `gather(..., return_exceptions=True)` doesn't. `test_borrowed_receive_leaves_no_task_polling_when_cancelled` fails without the `finally`. Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`SMPBLETransport(address, bluez=BlueZClientArgs(adapter="hci1"))`
scans for the device and connects to it on that adapter, instead of
bleak's default. The options are bleak's own type, so the
`BlueZClientArgs` passes to `BleakClient`, and its keys, a subset of
`BlueZScannerArgs`, pass to `find_device_by_address` and
`find_device_by_name`. `SMPBLETransport.scan()` takes
`bluez: BlueZScannerArgs` too. Both default to `{}`, which is bleak's
default adapter, so behavior is unchanged.
This sits beside the existing `winrt=WinRTClientArgs(...)`, which
already carries #90's `use_cached_services`. bleak is pinned `>=3.0.2`,
and the BlueZ adapter args date from 3.0.
Closes #103
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`SMPBumbleTransport.bonded_devices()`, `.clear_bond(address)`, and
`.clear_bonds()` become module functions in `smpclient.transport.bumble`:
await bonded_devices(keystore=..., host_address=...)
await clear_bond(address, keystore=..., host_address=...)
await clear_bonds(keystore=..., host_address=...)
They only ever read the transport's `keystore` and `host_address`, the
keystore namespace, and never its link. So listing or clearing bonds
no longer means building a transport for a device address you don't
plan to connect to. The defaults match the transport's, `Tempfile()`
and `DEFAULT_HOST_ADDRESS`, so a call with none of the options sees the
same bonds a default transport writes. The private
`_standalone_keystore()` helper is gone.
The functions had no tests; `test_bond_functions_manage_the_hosts_bonds`
seeds a keystore, then covers list, per-host isolation, clearing one
bond, and clearing all.
Refs #58
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ibe only ourselves Three lifecycle gaps from the #144 review: - A cancelled `connect()` leaked its partial state. The teardown arm was `except Exception`, which `CancelledError` bypasses, so a cancel mid `device.connect()` left the state at `Connecting` with the HCI transport open. Every later `connect()` then raised "called while in state Connecting". A `CancelledError` arm now tears down and re-raises, logged at debug: a cancel is the caller's decision, not an error. This is pre-existing on main. - `borrow()` was not all-or-nothing. If `negotiate()` raised or was cancelled, the transport stayed `ConnectedBorrowed`, subscribed, with its disconnection listener attached. It now returns the connection (`disconnect()` → `_teardown_borrowed`) and re-raises, like serial and bleak `borrow()`. - Returning a borrowed connection called `smp_characteristic.unsubscribe()` with no subscriber. bumble reads that as "drop every subscriber" and writes the CCCD to zero, cutting off the owner's own notifications on the shared characteristic. It now passes `self._on_notification`. bumble keys subscriber proxies by the subscriber, and a bound method compares equal each time it's looked up, so only this transport's proxy is removed, and the CCCD is cleared only if no subscriber is left. The owned teardown still unsubscribes everything; it owns the whole link. Each new test fails on the previous code. Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ises SMPTransportDisconnected `_Link` is now `_Closed | _Owned(client) | _Borrowed(client)`, and the separate `_client` attribute is gone. `connect()` creates `_Owned(BleakClient(...))`; `borrow()` makes `_Borrowed(client)`; and `disconnect()` ends in `_Closed()` from any state, so it stays idempotent. This closes a gap that was also on `main`. Once a transport that only ever borrowed had returned its client, `disconnect()` switched back to the old `_Owned()` marker, and `_active_client` read a `self._client` that only `connect()` assigns. `send()`/`receive()` raised `AttributeError: _client` instead of `SMPTransportDisconnected`; the same was true on `main` for a transport that never connected. With the client inside the variant, "no client" is its own case: - `_active_client` raises `SMPTransportDisconnected` on `_Closed`. - `_until_disconnected` returns at once on `_Closed`. - `_best_effort_disconnect` delegates to `disconnect()`, dropping its defensive `getattr`. - `_set_disconnected_event` still rejects a callback from a client other than the owned one. After our own `disconnect()` the link is `_Closed`, so bleak's callback for that disconnect is accepted. Tests inject `t._link = _Owned(client)`, or read the owned client with `_owned_client(t)`, instead of assigning `t._client`. Four sizing tests dropped a client assignment they never used. `test_a_returned_borrow_raises_disconnected` fails on the previous code with the `AttributeError`, and `test_disconnect` now also checks idempotence and a send after close. Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
JPHutchins
force-pushed
the
feat/58-effect-stack-lifecycle
branch
from
September 23, 2026 22:11
a76b962 to
d35531e
Compare
Collaborator
Author
|
Warning LLM Disclosure This comment was authored by Force-pushed the new revision:
At the tip:
|
…al's settings everywhere
The lock test read `inspect.signature(serial.Serial)`, which only works
where `serial.Serial` inherits `SerialBase.__init__`: POSIX. On Windows,
`serial.Serial` is `serialwin32.Serial`, whose
`__init__(self, *args, **kwargs)` sets up the overlapped handles and
forwards to `SerialBase.__init__`. The signature there reads `('args',)`,
which failed every Windows job on #144.
`SerialBase` in `serial.serialutil` declares the settings on every
platform, so the test reads its signature, and first asserts that
`serial.Serial` subclasses it. The transport already passes
`**options._asdict()` through `serial.Serial` to `SerialBase`.
Refs #58
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
JPHutchins
commented
Sep 23, 2026
JPHutchins
left a comment
JPHutchins
left a comment
Collaborator
Author
There was a problem hiding this comment.
Going the right direction. Overall poor code quality, needs some review and revision. Better typing, less mutation, etc etc.
Comment on lines
+180
to
+181
| sequence: The SMP sequence space the MCUmgr parameters read draws from; | ||
| defaults to `wrapping_sequence()`. |
Collaborator
Author
There was a problem hiding this comment.
Yu say it defaults to wrapping_sequence, yet you default it to None in the sig
Collaborator
Author
There was a problem hiding this comment.
The correct approach is for it to be structural (type level) not "prose".
Comment on lines
+184
to
+186
| self._fragmentation_strategy = fragmentation_strategy | ||
| self._connect_timeout_s = connect_timeout_s | ||
| self._sequence = _request.wrapping_sequence() if sequence is None else sequence |
Collaborator
Author
There was a problem hiding this comment.
Are these final?
|
|
||
|
|
||
| async def bonded_devices( | ||
| *, keystore: KeystoreStrategy = Tempfile(), host_address: Address = DEFAULT_HOST_ADDRESS |
Collaborator
Author
There was a problem hiding this comment.
why kwargs only?
Comment on lines
+130
to
+131
| self._connect_timeout_s = connect_timeout_s | ||
| self._sequence = _request.wrapping_sequence() if sequence is None else sequence |
Collaborator
Author
There was a problem hiding this comment.
Are these final?
Comment on lines
+143
to
+149
| SerialFragmentationStrategy: TypeAlias = Auto | BufferSize | BufferParams | ||
| """How `SMPSerialTransport` sizes SMP messages: `Auto`, `BufferSize`, or `BufferParams`. | ||
|
|
||
| With `Auto`, connecting reads the server's `buf_size` (the decoded reassembly buffer) and | ||
| the transport sends messages up to `buf_size - 4`, filling that buffer; until the parameters | ||
| are read, or if the server doesn't provide them, it assumes a conservative line budget. | ||
| """ |
Collaborator
Author
There was a problem hiding this comment.
llm doc slop - restates the code and other doc strings
Comment on lines
-65
to
-71
| def initialize(self, smp_server_transport_buffer_size: int) -> None: # pragma: no cover | ||
| """Initialize the `SMPTransport` with the server transport buffer size. | ||
|
|
||
| Args: | ||
| smp_server_transport_buffer_size: The SMP server transport buffer size, in 8-bit bytes. | ||
| """ | ||
| self._smp_server_transport_buffer_size = smp_server_transport_buffer_size |
Collaborator
Author
There was a problem hiding this comment.
Isn't negotiate required now?
Collaborator
Author
There was a problem hiding this comment.
Errr, I guess not, since client doesn't call it.
Comment on lines
+123
to
+129
| async def connect(self) -> None: # pragma: no cover | ||
| """Open the link, then `negotiate()`.""" | ||
| ... | ||
|
|
||
| async def disconnect(self) -> None: # pragma: no cover | ||
| """Close the link.""" | ||
| ... |
Collaborator
Author
There was a problem hiding this comment.
Add note to not use them; dangerous loss of side effect control
Comment on lines
+134
to
+135
| winrt: WinRT backend arguments, e.g. `use_cached_services`. | ||
| bluez: BlueZ backend arguments, e.g. the `adapter` to scan and connect with. |
Collaborator
Author
There was a problem hiding this comment.
You're telling me that we accpet both? On macos too? At the same time. FUCKING GARBAGE. This is a sum type.
| self._address: Final = address | ||
| self._fragmentation_strategy = fragmentation_strategy | ||
| self._connect_timeout_s = connect_timeout_s | ||
| self._sequence = _request.wrapping_sequence() if sequence is None else sequence |
| return summary, details | ||
|
|
||
|
|
||
| class SMPClient: |
Collaborator
Author
There was a problem hiding this comment.
Should be generic over the transport, so that users can access the transport in a type safe way.
This branch has not been deployed
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
1 participant
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.
Warning
LLM Disclosure
This PR was authored by
claude-opus-5-5[1m]on behalf of @JPHutchins. @JPHutchins found the previous revision far more invasive than expected. She asked for a smaller diff on the idiom she settled on: transports are valid while closed, standardconnect()/disconnect()/borrow()primitives with theconnected()/borrowed()brackets encouraged, andSMPClientunable to control transport side effects. This revision is a fresh branch of individually green commits, force-pushed over the old one.Note
Targets
screaming-goblin, the breaking branch (#143, epic intercreate/smpmgr#103). Implements the design from #58. Closes #90. Closes #103. It supersedes the previous revision (a76b962) and its review.A transport is constructed closed and owns its address, options, and sizing.
SMPClient(transport)is a plain object: it can't open or close a link.SMPTransport, whatSMPClientsees, is onlysend,receive,send_and_receive,mtu, andmax_unencoded_size. It has no lifecycle.connect()anddisconnect()on every transport, plusborrow(resource)on serial, bleak, and bumble. Theconnected()andborrowed()brackets are the encouraged form. They add best-effort cleanup and release on cancellation.connect()andborrow(), and through a publicnegotiate(). It sends the MCUmgr params read only when the fragmentation strategy asks:Auto(), or GATTUnfragmented(). A timeout or error response warns and falls back.Commits (each passes
camas checkon its own)40c6d36smpclient._request; no API changea353b5bSMPClientlosesconnect/disconnect/address/async with78c58ee*FragmentationStrategyunions sharingAuto/BufferSize/Unfragmented; conditionalnegotiate()9d00fd4options=SerialOptions(...), locked toserial.Serial's signature by a test9a5eb94asyncio.to_thread; a failed input flush no longer leaks the fd (pre-existing onmain)c8ccc02borrow(port)/borrowed(port); the integration harness borrows itssocket://chardev instead ofobject.__setattr__03306eaborrow(client)/borrowed(client), pollingis_connected;receive()no longer leaks tasks on cancel0ecf44cbluez=BlueZClientArgs(adapter=...)for both the scan and the client (#103)2bd7554bonded_devices/clear_bond/clear_bondsbecome module functionsbdc979econnect();borrow()is all-or-nothing; return a borrowed link by unsubscribing only this transportd35531eSMPTransportDisconnectedrather thanAttributeError5d58791SerialOptionstoSerialBase, sinceserial.Serial.__init__on Windows is*args, **kwargsBreaking changes
main)SMPClient(transport, address, timeout_s), thenasync with clientorclient.connect()async with Transport(address, ...).connected() as t:, thenSMPClient(t, timeout_s=...)SMPClient.connect()/.disconnect()/.addressSMPTransport.connect(address, timeout_s)/.disconnect()/.initialize(buf_size)connect_timeout_sin the constructor,connect()takes no arguments, andinitialize()becomesnegotiate()SMPUDPTransport(mtu)+connect(address, timeout_s, port=1337)SMPUDPTransport(address, port=1337, *, mtu=, fragmentation_strategy=)SMPBLETransport(winrt=)+connect(address, timeout_s)SMPBLETransport(address, *, winrt=, bluez=, fragmentation_strategy=); newborrow(client)use_connection(c)/borrowed_connection(t, c)t.borrow(c)/t.borrowed(c)SMPBumbleTransport.bonded_devices()/.clear_bond()/.clear_bonds()keystore=andhost_address=SMPSerialTransport(strategy, <pyserial kwargs>)+connect(port, timeout_s)SMPSerialTransport(port, strategy, *, options=SerialOptions(...))smpclient.transport.serial.FragmentationStrategy/.AutoSerialFragmentationStrategy/smpclient.transport.AutoSMPSerialRawTransport(mtu=384)SMPSerialRawTransport(port, fragmentation_strategy=Auto());BufferSize(384)pins the old sizeSerial's
BufferSize(buf_size, line_length)andBufferParams(line_length, line_buffers)are unchanged frommain, as are the deprecated 7.1.0 params. Their removal is left to a follow-up.Decisions, for review
BufferSizetypes. Serial keeps its own, which also carries aline_length. The other transports sharesmpclient.transport.BufferSize(buf_size), and the type checkers catch a mix-up.smpclient.transport.serialdoesn't re-export the shared types; it exports only serial's own, defined in theencoded/unencoded/commonsubmodules._Owned | _Borrowed(port); its ownSerialexists, closed, from construction. bleak uses_Closed | _Owned(client) | _Borrowed(client); its own client is created byconnect(). I/O goes through a property that matches on it, and a transport with no link raisesSMPTransportDisconnected.disconnect()returns a borrowed resource and never closes it; the acquirer releases. bumble keeps its existing state machine.client.is_connectedat 100 ms, and only while a receive or GATT wait is running. bleak accepts a disconnect callback only at construction, and the owner holds it. No watcher task outlives a call.borrow()is all-or-nothing on every transport: ifnegotiate()fails or is cancelled, the resource is returned before re-raising.borrow(), because its client class is private.winrt=WinRTClientArgs(use_cached_services=True)) was already served by thewinrt=kwarg onmain; it is kept as is.Verification
camas check(ruff, pydoclint, mypy, pyright, tests) green.5d58791: 28/28 (Linux x64/arm64, macOS, Windows × 3.10–3.14, extras, integration).camas check401 passed, 14 skipped; coverage 94.1% (floor 91%).camas matrixgreen on 3.10–3.14, andcamas test_integration229 passed, 101 skipped, the same as breaking: port to smp screaming-goblin (Frame[T] on msgspec) #137's baseline. Every socket-serial fixture, including serial recovery over all four framings, now runs through the publicborrowed()API.connect()borrow()negotiation failureAttributeErrorafter a borrow returnsSerialOptionslock test was confirmed to fail on a renamed field and on a changed default.🤖 Generated with Claude Code