breaking: remove address from SMPClient; SMPTransport owns address by JPHutchins · Pull Request #58 · intercreate/smpclient

JPHutchins · 2024-12-10T05:35:04Z

I've long thought that passing the SMP server "address" to the SMPClient class instead of the SMPTransport implementation classes was a mistake. "address" means different things to the different transports. Notably, this change allows the UDP transport to clearly take an address and a port argument, rather than just an address.

Unfortunately this is a breaking change. While all existing address args should still be valid, they must be moved from "1st arg of SMPClient() to 1st arg of SMPTransport".

@M1cha this is what I have in mind as the breaking change required to continue implementing the unique requirements of each transport for smpmgr.

M1cha · 2024-12-10T05:51:07Z

    @override
-    async def connect(self, address: str, timeout_s: float) -> None:
-        logger.debug(f"Scanning for {address=}")
+    async def connect(self, timeout_s: float) -> None:


This PR didn't change the behavior, but it got me wondering: What is the reasoning behind having a single parameter for both name and address? While this is probably fine for 99% of use-cases, you can easily make this connect to the wrong device by giving a device a name, that looks the same as the BLE address of another device 🙈 😅

Yeah, I don't love it, and thought about creating a union type that's name or address, but honestly, if someone uses a MAC address as the device name, I WANT them to open a ticket - force them out into the wild! 🤣

M1cha

I didn't test this, yet but it looks good, thx.

GHF · 2025-05-02T22:18:13Z

I've long thought that passing the SMP server "address" to the SMPClient class instead of the SMPTransport implementation classes was a mistake.

I really like the direction this is going and I want to suggest that this could go even further. In this case, it's SMPClient dictating to SMPTransport what its connection parameters should be, when the transport-specific class would know better. I think the lifetime of a transport's connection session should be decoupled from SMPClient's session too.

We've been integrating smpclient into an application that has a standing BLE link and the semantics of SMPClient (the class and async context) owning the lifetime of the transport's session has been awkward. It wants to make a BLE connection, but the establishment procedure isn't idempotent (scanning fails if the peripheral stops advertising with an existing link), and it disconnects the link through bleak when the SMPClient context closes.

Instead, if the semantics were: an SMPTransport instance exists only after a session is established and SMPClient can assume that a transport is available to send and receive as long as it's open, then it becomes more flexible to reuse a BLE link.

Ideally it'd probably look something like:

async with SMPBLETransport(address) as transport:
	async with SMPClient(transport) as client:
        ...

JPHutchins · 2025-05-03T00:51:42Z

I've long thought that passing the SMP server "address" to the SMPClient class instead of the SMPTransport implementation classes was a mistake.

I really like the direction this is going and I want to suggest that this could go even further. In this case, it's SMPClient dictating to SMPTransport what its connection parameters should be, when the transport-specific class would know better. I think the lifetime of a transport's connection session should be decoupled from SMPClient's session too.

We've been integrating smpclient into an application that has a standing BLE link and the semantics of SMPClient (the class and async context) owning the lifetime of the transport's session has been awkward. It wants to make a BLE connection, but the establishment procedure isn't idempotent (scanning fails if the peripheral stops advertising with an existing link), and it disconnects the link through bleak when the SMPClient context closes.

Instead, if the semantics were: an SMPTransport instance exists only after a session is established and SMPClient can assume that a transport is available to send and receive as long as it's open, then it becomes more flexible to reuse a BLE link.

Ideally it'd probably look something like:
async with SMPBLETransport(address) as transport:
	async with SMPClient(transport) as client:
        ...

I think that I'm understanding that with this design the connected transport is an invariant of the SMPClient instance. This is nice, because it does away with SMPClient having a connect method and skirts around the general issue of python not having an __ainit__() constructor. It allows for client errors to occur without ending the transport lifetime, but semantically shows that a transport lifetime must live at least as long as a client.

Am I understanding correctly?

JPHutchins · 2026-07-18T00:40:55Z

Warning

LLM Disclosure

This comment was authored by claude-opus-4-8 on behalf of @JPHutchins, coordinating this breaking change with the cross-repo migration.

Staging note for the transport-subcommands epic (intercreate/smpmgr#103): breaking work lands on a per-repo screaming-goblin branch, and smpclient's screaming-goblin will pin smp's screaming-goblin (git dependency) so we can migrate smp → smpclient → smpmgr in lockstep before smp cuts its breaking release; the pin then moves to the released version.

As the main breaking prerequisite, this PR belongs on that screaming-goblin branch rather than main — i.e. retarget the base to smpclient's screaming-goblin when we cut it (the BREAKING/move-address-to-transport branch itself is unchanged). That keeps main releasable while the breaking set assembles alongside the smp Frame[T]/Data + generics migration (JPHutchins/smp#65, JPHutchins/smp#62).

JPHutchins · 2026-09-22T23:27:24Z

Warning

LLM Disclosure

This comment was authored by claude-opus-5-5[1m] on behalf of @JPHutchins. We iterated on the design for this change over several rounds; @JPHutchins asked me to post the result here before implementing, and to close this PR as superseded.

The design has grown past this PR, so I'm closing it as superseded. The implementation will be one new PR on screaming-goblin, per the epic (intercreate/smpmgr#103). @GHF @M1cha: this is the shape we're building, and feedback is welcome during review.

async with (
    SMPSerialTransport.connected("/dev/ttyACM0") as transport,  # opens the link; closes it on exit
    SMPClient(transport) as client,                            # negotiates sizing if the transport asks
):
    await client.upload(image)

# @GHF's standing link: smpclient never establishes or drops it
async with SMPBLETransport.borrow(bleak_client) as transport:
    async with SMPClient(transport) as client:
        ...

The transport owns its address, its options, and its link. Each transport's constructor wraps an already-open resource, so an unconnected transport can't be held. .connected(...) opens one; .borrow(...) wraps one you already own, subscribing on entry and unsubscribing on exit.
SMPClient holds a type that cannot close the link. connect/disconnect leave the SMPTransport Protocol, and SMPClient.connect(), .disconnect() and .address go away.
MCUmgr params negotiation is the transport's policy, and it's opt-in. Each transport declares a sizing sum type. The client's bracket reads the params only when the transport was built with Auto().

@GHF's standing-link requirements, and how this meets them

From #58 (comment):

A caller-owned BLE link backs an SMPClient without smpclient establishing it. SMPBLETransport.borrow(bleak_client), and the same for SMPBumbleTransport.borrow(connection).
The client going out of scope never drops the link. SMPClient holds an SMPTransport, and nothing on that Protocol opens or closes a link. Reaching for one is a type error under mypy and pyright.
Two clients can use one link in sequence without re-establishing it. Negotiation is idempotent transport state.
The link lives at least as long as the client. The nesting of the brackets enforces it.

Sizing: per-transport sum types sharing one Auto()

The server's MCUmgr buf_size is one global buffer size, but each transport turns it into a message cap differently. Zephyr main @ 70be2ff0: os_mgmt.c.

Transport	Sizing	`Auto()` gives
console serial	`Auto \| BufferSize \| BufferParams` (unchanged)	`buf_size - 4` (the length and CRC16 share the buffer)
raw serial, COBS	`Auto \| BufferSize`	`buf_size`
BLE (bleak, bumble)	`Auto \| Unfragmented \| BufferSize`	`buf_size`, fragmented across writes. This assumes the server enables `CONFIG_MCUMGR_TRANSPORT_BT_REASSEMBLY`, as Zephyr's `smp_svr` sample does. `Unfragmented()` means one message per write, for servers that don't reassemble.
UDP	`Auto \| BufferSize`	`min(buf_size, MSS)`, since the server receives each request as one datagram into one buffer (the same fix is going to `main` in #142)

The mechanism: SMPClient.__aenter__ calls await transport.negotiate(read), where read sends the params request. Only the transport's Auto() arm calls read(), inside an exhaustive match over its sizing type. So the policy is decided in one place, and a pinned size never costs a round trip. If the server answers with an error (no params support), the transport warns and falls back to its default size. If it doesn't answer at all, TimeoutError propagates.

Transport options: #90 and #103

Options move onto each transport's .connected(...). For BLE they are bleak's own backend argument types, passed through unchanged:

Bleak transport options #90 (cached GATT services on Windows, from Feature request: Allow forcing cached GATT services (Bleak/WinRT) smpmgr#82): winrt=WinRTClientArgs(use_cached_services=True).
Add HCI option to BLE Transport #103 (choose the HCI adapter): bluez=BlueZClientArgs(adapter="hci1"), applied to both the scan and the connection.
UDP takes address and port, instead of port arriving through a connect() kwarg the Protocol never declared. That was this PR's original motivation.

Breaking changes

SMPClient(transport, address, ...) becomes SMPClient(transport, ...), and is an async with bracket that does no link I/O. SMPClient.connect(), .disconnect() and .address are removed.
Transports are obtained from Transport.connected(...) or .borrow(...) rather than Transport(...) followed by connect(address, timeout_s). The connect timeout moves to .connected(...), and SMPClient's timeout_s remains the request timeout.
SMPTransport loses connect/disconnect, and initialize(buf_size) becomes negotiate(read).
For a transport sized with Auto(), a params timeout now raises; it used to be logged and ignored.
No deprecation aliases: screaming-goblin is the breaking release.

JPHutchins · 2026-09-22T23:34:08Z

async with (
    SMPSerialTransport.connected("/dev/ttyACM0") as transport,  # opens the link; closes it on exit
    SMPClient(transport) as client,                            # negotiates sizing if the transport asks
):
    await client.upload(image)

# @GHF's standing link: smpclient never establishes or drops it
async with SMPBLETransport.borrow(bleak_client) as transport:
    async with SMPClient(transport) as client:
        ...

My only note here is that the double context manager is only required if negotiating mcumgr params. Yet we're forcing everything to use 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>

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

…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>

Per review on #144: the overloads existed only for backwards compatibility, and this is the breaking release, so they go now rather than in the follow-up planned earlier. `SMPSerialTransport(port, fragmentation_strategy=Auto(), *, ...)` is now the only signature. Removed: - the three `__init__` overloads, including the two `@deprecated` ones for `max_smp_encoded_frame_size`/`line_length`/`line_buffers` - `_LegacyParams` and `_ResolvedStrategy` - `_resolve_fragmentation_strategy` - the `_LEGACY_FRAME_SIZE` constant and `_LEGACY_PARAMS_DEPRECATION` - the `_LegacyParams` match arms in the sizing properties - the six tests covering the 7.1.0 reproduction The constructor now validates the strategy directly. `_LEGACY_LINE_BUFFERS` survives as `_AUTO_LINE_BUFFERS`, the line buffers `Auto` assumes before the server's parameters are read. `typing_extensions.deprecated` is no longer used, and the pyproject comment about typing-extensions' minimum no longer names it. Migration: `max_smp_encoded_frame_size=n, line_length=l, line_buffers=b` becomes `BufferParams(line_length=l, line_buffers=b)`, or better, `BufferSize(buf_size=...)` for the server's decoded buffer. Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

… no Optional sizing state Per review on #144 ("Are these final?", "you say it defaults to wrapping_sequence, yet you default it to None in the sig", "less mutation"): - `_ConnectableTransport` is now a concrete base, generic over the transport's fragmentation strategy union. Its `__init__` sets `_fragmentation_strategy`, `_connect_timeout_s`, and `_sequence` as `Final`. As Protocol members they could not be `Final`: a `Final` assignment in an implementer conflicts with a writable Protocol attribute. `connect`/`disconnect`/`negotiate` are `@abstractmethod`, and their docstrings point at the `connected()` bracket. - `sequence: Iterator[u8] | None = None` becomes `sequence: Callable[[], Iterator[u8]] = wrapping_sequence` on every transport and on `SMPClient`, so the signature states the default. A factory rather than an iterator, because a default iterator is evaluated once at definition time and would be shared across instances. The "defaults to `wrapping_sequence()`" prose is gone. - `_negotiated_buf_size: int | None` is gone. The configured strategy stays `Final`, and one slot, `_sizing`, holds the strategy as `negotiate()` resolved it. `Auto` resolves to `BufferSize(n)` when the server advertises `n`; GATT `Unfragmented` resolves to `BufferSize(min(mtu, n))`. An unadvertised read resets to the unresolved strategy, so a re-negotiation never keeps a stale size from an earlier server. Every sizing property now matches `_sizing` with no `is None` branches; `Auto` there only means "the server advertised nothing". - Attributes that are never reassigned are `Final`: UDP's `_mtu`, bleak's `_buffer`/`_notify_condition`/`_disconnected_event`/`_winrt`, and `SMPClient._timeout_s`. Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

… the constructor Per review on #144 ("Port wouldn't be required if we achieve via SMPSerialTransport.borrowed()... right?"): a transport that only borrows never needed a port or address. The constructor now holds only config: the fragmentation strategy, options, timeouts, and sequence. The target belongs to the primitive that opens a link. async with SMPSerialTransport(BufferSize(1024)).connected("/dev/ttyACM0") as t: ... async with SMPSerialTransport().borrowed(open_port) as t: ... async with SMPUDPTransport().connected("192.168.1.1", 1337) as t: ... async with SMPBLETransport().connected("AA:BB:CC:DD:EE:FF") as t: ... async with SMPBumbleTransport(hci="usb:0").connected("AA:BB:CC:DD:EE:FF") as t: ... - `connect(target)` and `connected(target)` are defined per transport: serial `port`, BLE/bumble `address`, UDP `address, port=1337`. The signatures differ, so `_ConnectableTransport` no longer declares `connect`. What each bracket shares is `_released_on_exit()`, which yields the link and releases it best-effort on every exit. Both `connected()` and `borrowed()` use it. - The primitives' docstrings point at their bracket, and the base class docstring says what the bare primitives give up. - The positional constructor arguments are now the ones `main` had: serial and raw serial take `fragmentation_strategy`; UDP takes `mtu`. - Tests, the integration harness (`_link`, `socket_link`, the recovery and line-length tests), the examples, the `SMPClient` docstring example, and the bumble CLI all pass the target to `connect`/`connected`. Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Per review on #144 ("Should be generic over the transport, so that users can access the transport in a type safe way."): TTransport = TypeVar("TTransport", bound=SMPTransport) class SMPClient(Generic[TTransport]): ... @Property def transport(self) -> TTransport: ... `SMPClient(SMPSerialTransport())` is an `SMPClient[SMPSerialTransport]`, so `client.transport.read_serial()` type-checks with no cast. `ICUploadClient` is generic over the same `TTransport`. `SMPClient.__init__` gains its missing `-> None`. `tests/test_generics_typing.py` asserts the type of `client.transport` for both classes under mypy and pyright. Typing the property as plain `SMPTransport` fails that check. Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

… positional bond args Per review on #144 ("llm doc slop - restates the code and other doc strings", "why kwargs only?"): - The strategy aliases no longer list their own members or restate the sizing rules the properties implement. `SerialFragmentationStrategy`, `RawSerialFragmentationStrategy`, and `UDPFragmentationStrategy` are each one line. The constructors' `fragmentation_strategy` Args no longer repeat the union. - `_request.exchange` and `_request.read_mcumgr_parameters` are private helpers, so their docstrings are one line. `exchange` was a copy of `SMPClient.request`'s docstring, which remains the documented contract. - The nested `match` on the params read (`int | None`) ended in a bare capture, which rules out an `assert_never` arm. Each now matches `case int() as buf_size:` and closes with `case _ as unreachable: assert_never(unreachable)`. That covers all five transports' `negotiate()`, including encoded serial's guarded arm. - `bonded_devices`, `clear_bond`, and `clear_bonds` drop the `*`. There is no ambiguity for keyword-only to guard against. - Test comments no longer mention the removed 7.1.0 params. - `serial.common` reuses `smpclient.transport._TStrategy` instead of redeclaring an identical TypeVar. Refs #58 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

breaking: remove address from SMPClient; SMPTransport owns address

2f1c022

M1cha reviewed Dec 10, 2024

View reviewed changes

M1cha approved these changes Dec 10, 2024

View reviewed changes

JPHutchins assigned davedesro, KungFuSensai, robhelvestine, JerryCapistrano, aldenhaase, klam20 and swoisz Dec 10, 2024

JPHutchins mentioned this pull request Jun 7, 2025

Feature request: Allow setting custom connection timeout in SMPClient() #69

Closed

This was referenced Aug 9, 2025

Can negotiated MTU w/ MCUMgr params work for serial (uart/usb/shell transport) #73

Closed

fix: use smpclient upstream mtu defaults intercreate/smpmgr#56

Merged

JPHutchins mentioned this pull request Dec 19, 2025

refactor: use a single configurable timeout value #94

Merged

JPHutchins mentioned this pull request Jul 14, 2026

Epic: transport subcommands intercreate/smpmgr#103

Open

eastagiletracker mentioned this pull request Aug 19, 2026

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

Open

JPHutchins closed this Sep 22, 2026

This was referenced Sep 22, 2026

breaking: screaming-goblin #143

Draft

breaking: the effect-stack lifecycle (#58): transports own their link, options, and sizing #144

Open

JPHutchins added a commit that referenced this pull request Sep 23, 2026

Merge main into the #58 work (brings #142, the UDP buffer cap)

a231a36

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Navigation Menu

breaking: remove address from SMPClient; SMPTransport owns address - #58

breaking: remove address from SMPClient; SMPTransport owns address#58
JPHutchins wants to merge 1 commit into
mainfrom
BREAKING/move-address-to-transport

JPHutchins commented Dec 10, 2024 •

edited

Loading

Uh oh!

M1cha Dec 10, 2024

Uh oh!

JPHutchins Dec 10, 2024

Uh oh!

M1cha left a comment

Uh oh!

GHF commented May 2, 2025

Uh oh!

JPHutchins commented May 3, 2025

Uh oh!

JPHutchins commented Jul 18, 2026

Uh oh!

JPHutchins commented Sep 22, 2026

Uh oh!

JPHutchins commented Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Conversation

JPHutchins commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

M1cha Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

JPHutchins Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

M1cha left a comment

Choose a reason for hiding this comment

Uh oh!

GHF commented May 2, 2025

Uh oh!

JPHutchins commented May 3, 2025

Uh oh!

JPHutchins commented Jul 18, 2026

Uh oh!

JPHutchins commented Sep 22, 2026

Uh oh!

JPHutchins commented Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

JPHutchins commented Dec 10, 2024 •

edited

Loading