diff --git a/examples/ble/helloworld.py b/examples/ble/helloworld.py index b9c0341..2cb0ca3 100644 --- a/examples/ble/helloworld.py +++ b/examples/ble/helloworld.py @@ -16,7 +16,8 @@ async def main() -> None: print(f"Found {len(smp_servers)} SMP servers: {smp_servers}") print("Connecting to the first SMP server...", end="", flush=True) - async with SMPClient(SMPBLETransport(), smp_servers[0].address) as client: + async with SMPBLETransport().connected(smp_servers[0].address) as transport: + client = SMPClient(transport) print("OK") print("Sending request...", end="", flush=True) diff --git a/examples/ble/imagestate.py b/examples/ble/imagestate.py index f9b8f30..99216b3 100644 --- a/examples/ble/imagestate.py +++ b/examples/ble/imagestate.py @@ -16,7 +16,8 @@ async def main() -> None: print(f"Found {len(smp_servers)} SMP servers: {smp_servers}") print("Connecting to the first SMP server...", end="", flush=True) - async with SMPClient(SMPBLETransport(), smp_servers[0].address) as client: + async with SMPBLETransport().connected(smp_servers[0].address) as transport: + client = SMPClient(transport) print("OK") print("Sending request...", end="", flush=True) diff --git a/examples/ble/mcumgrparameters.py b/examples/ble/mcumgrparameters.py index 57d0134..a7c4700 100644 --- a/examples/ble/mcumgrparameters.py +++ b/examples/ble/mcumgrparameters.py @@ -16,7 +16,8 @@ async def main() -> None: print(f"Found {len(smp_servers)} SMP servers: {smp_servers}") print("Connecting to the first SMP server...", end="", flush=True) - async with SMPClient(SMPBLETransport(), smp_servers[0].address) as client: + async with SMPBLETransport().connected(smp_servers[0].address) as transport: + client = SMPClient(transport) print("OK") print(f"Client MTU is {client._transport.mtu}B") print(f"Client max unencoded size is {client._transport.max_unencoded_size}B") diff --git a/examples/ble/upgrade.py b/examples/ble/upgrade.py index 1b68d19..7a5bf5a 100644 --- a/examples/ble/upgrade.py +++ b/examples/ble/upgrade.py @@ -64,7 +64,8 @@ async def main() -> None: print("OK") print("Connecting to A SMP DUT...", end="", flush=True) - async with SMPClient(SMPBLETransport(), a_smp_dut.name or a_smp_dut.address) as client: + async with SMPBLETransport().connected(a_smp_dut.name or a_smp_dut.address) as transport: + client = SMPClient(transport) print("OK") async def ensure_request(request: SMPRequest[TRep, TEr1, TEr2]) -> TRep: @@ -119,7 +120,8 @@ async def ensure_request(request: SMPRequest[TRep, TEr1, TEr2]) -> TRep: b_smp_dut = cast(BLEDevice, b_smp_dut) print("Connecting to B SMP DUT...", end="", flush=True) - async with SMPClient(SMPBLETransport(), b_smp_dut.name or b_smp_dut.address) as client: + async with SMPBLETransport().connected(b_smp_dut.name or b_smp_dut.address) as transport: + client = SMPClient(transport) print("OK") print() diff --git a/examples/ble/upload.py b/examples/ble/upload.py index 025cb92..e71a6d9 100644 --- a/examples/ble/upload.py +++ b/examples/ble/upload.py @@ -30,9 +30,10 @@ async def main() -> None: print(f"Found {len(smp_servers)} SMP servers: {smp_servers}") print("Connecting to the first SMP server...", end="", flush=True) - async with SMPClient( - SMPBLETransport(), smp_servers[0].name or smp_servers[0].address - ) as client: + async with SMPBLETransport().connected( + smp_servers[0].name or smp_servers[0].address + ) as transport: + client = SMPClient(transport) print("OK") print("Sending request...", end="", flush=True) diff --git a/examples/udp/helloworld.py b/examples/udp/helloworld.py index 799f8db..4bdbc71 100644 --- a/examples/udp/helloworld.py +++ b/examples/udp/helloworld.py @@ -18,7 +18,8 @@ async def main() -> None: parser.add_argument("address", help="The IP address to connect to") address = parser.parse_args().address - async with SMPClient(SMPUDPTransport(), address) as client: + async with SMPUDPTransport().connected(address) as transport: + client = SMPClient(transport) print("OK") print("Sending request...", end="", flush=True) diff --git a/examples/usb/download_file.py b/examples/usb/download_file.py index 6f6e683..9faeece 100644 --- a/examples/usb/download_file.py +++ b/examples/usb/download_file.py @@ -16,7 +16,8 @@ async def main() -> None: port = args.port file_location = args.file_location - async with SMPClient(SMPSerialTransport(), port) as client: + async with SMPSerialTransport().connected(port) as transport: + client = SMPClient(transport) start_s = time.time() file_data = await client.download_file(file_location) end_s = time.time() diff --git a/examples/usb/helloworld.py b/examples/usb/helloworld.py index 2373dd4..a9fc50c 100644 --- a/examples/usb/helloworld.py +++ b/examples/usb/helloworld.py @@ -15,7 +15,8 @@ async def main() -> None: parser.add_argument("port", help="The serial port to connect to") port = parser.parse_args().port - async with SMPClient(SMPSerialTransport(), port) as client: + async with SMPSerialTransport().connected(port) as transport: + client = SMPClient(transport) print("OK") print("Sending request...", end="", flush=True) diff --git a/examples/usb/upgrade.py b/examples/usb/upgrade.py index 9cc0e45..f3c3506 100644 --- a/examples/usb/upgrade.py +++ b/examples/usb/upgrade.py @@ -105,15 +105,10 @@ async def main() -> None: await asyncio.sleep(1) print("Connecting to SMP DUT...", end="", flush=True) - async with SMPClient( - SMPSerialTransport( - fragmentation_strategy=BufferParams( - line_length=line_length, - line_buffers=line_buffers, - ) - ), - port_a.device, - ) as client: + async with SMPSerialTransport( + BufferParams(line_length=line_length, line_buffers=line_buffers) + ).connected(port_a.device) as transport: + client = SMPClient(transport) print("OK") async def ensure_request(request: SMPRequest[TRep, TEr1, TEr2]) -> TRep: @@ -185,15 +180,10 @@ async def ensure_request(request: SMPRequest[TRep, TEr1, TEr2]) -> TRep: print(f"OK - found DUT B at {port_b.device}") print("Connecting to B SMP DUT...", end="", flush=True) - async with SMPClient( - SMPSerialTransport( - fragmentation_strategy=BufferParams( - line_length=line_length, - line_buffers=line_buffers, - ) - ), - port_b.device, - ) as client: + async with SMPSerialTransport( + BufferParams(line_length=line_length, line_buffers=line_buffers) + ).connected(port_b.device) as transport: + client = SMPClient(transport) print("OK") print() diff --git a/examples/usb/upload_file.py b/examples/usb/upload_file.py index d5aadeb..b19561c 100644 --- a/examples/usb/upload_file.py +++ b/examples/usb/upload_file.py @@ -64,7 +64,8 @@ async def main() -> None: Etiam elit velit, posuere ut pulvinar ac, condimentum eget justo. Fusce a erat velit. Vivamus imperdiet ultrices orci in hendrerit. """ - async with SMPClient(SMPSerialTransport(), port) as client: + async with SMPSerialTransport().connected(port) as transport: + client = SMPClient(transport) start_s = time.time() async for offset in client.upload_file(file_data=file_data, file_path=file_path): print( diff --git a/pyproject.toml b/pyproject.toml index c5d9db3..752d004 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,7 +25,7 @@ dependencies = [ "smp @ git+https://github.com/JPHutchins/smp@screaming-goblin", "msgspec>=0.21.1", "intelhex>=2.3.0", -# `TypeIs` landed in 4.10; `override`/`assert_never`/`deprecated` are older +# `TypeIs` landed in 4.10; `override`/`assert_never` are older "typing-extensions>=4.10", "async-timeout>=5.0.1; python_version < '3.11'", ] diff --git a/src/smpclient/__init__.py b/src/smpclient/__init__.py index c471166..56550a0 100644 --- a/src/smpclient/__init__.py +++ b/src/smpclient/__init__.py @@ -37,101 +37,37 @@ from __future__ import annotations -import asyncio -import itertools import logging -import traceback -from collections.abc import AsyncIterator, Iterator +from collections.abc import AsyncIterator, Callable, Iterator from hashlib import sha256 -from types import TracebackType -from typing import TYPE_CHECKING, Any, Final, TypeVar, Union, cast +from typing import TYPE_CHECKING, Final, Generic, TypeVar import msgspec from smp import SMPRequest -from smp import error as smperror from smp import header as smpheader from smp import message as smpmsg from smp.file_management import FileDownloadRequest, FileUploadRequest from smp.image_management import ImageUploadWriteRequest -from smp.os_management import MCUMgrParametersReadRequest from smp.user import intercreate as smpic -from typing_extensions import TypeIs, assert_never - -from smpclient.exceptions import SMPBadSequence, SMPUploadError, SMPValidationException +from typing_extensions import assert_never + +from smpclient import _request +from smpclient._request import TEr1 as TEr1 +from smpclient._request import TEr2 as TEr2 +from smpclient._request import TRep as TRep +from smpclient._request import error as error +from smpclient._request import error_v1 as error_v1 +from smpclient._request import error_v2 as error_v2 +from smpclient._request import success as success +from smpclient._request import wrapping_sequence as wrapping_sequence +from smpclient.exceptions import SMPUploadError from smpclient.transport import SMPTransport if TYPE_CHECKING: from types_bits import u8 -try: - from asyncio import timeout # type: ignore -except ImportError: # backport for Python3.10 and below - from async_timeout import timeout # type: ignore - logger = logging.getLogger(__name__) -TEr1 = TypeVar("TEr1", bound=smperror.ErrorV1) -"""Type of SMP Error V1.""" - -TEr2 = TypeVar("TEr2", bound=smperror.ErrorV2) -"""Type of SMP Error V2.""" - -TRep = TypeVar("TRep", bound=Union[smpmsg.ReadResponse, smpmsg.WriteResponse]) -"""Type of successful SMP Response (ReadResponse or WriteResponse).""" - - -def error_v1(response: smpmsg.Response) -> TypeIs[smperror.ErrorV1]: - """`TypeIs` that returns `True` if the `response` is an `ErrorV1`. - - Args: - response: The response to check. - - Returns: - `True` if the `response` is an `ErrorV1`. - """ - return response.RESPONSE_TYPE == smpmsg.ResponseType.ERROR_V1 - - -def error_v2(response: smpmsg.Response) -> TypeIs[smperror.ErrorV2[Any]]: - """`TypeIs` that returns `True` if the `response` is an `ErrorV2`. - - Args: - response: The response to check. - - Returns: - `True` if the `response` is an `ErrorV2`. - """ - return response.RESPONSE_TYPE == smpmsg.ResponseType.ERROR_V2 - - -def error( - response: smpmsg.Response, -) -> TypeIs[Union[smperror.ErrorV1, smperror.ErrorV2[Any]]]: - """`TypeIs` that returns `True` if the `response` is an `ErrorV1` or `ErrorV2`. - - Args: - response: The response to check. - - Returns: - `True` if the `response` is an `ErrorV1` or `ErrorV2`. - """ - return error_v1(response) or error_v2(response) - - -def success( - response: smpmsg.Response, -) -> TypeIs[Union[smpmsg.ReadResponse, smpmsg.WriteResponse]]: - """`TypeIs` that returns `True` if the `response` is a successful `Response`. - - Args: - response: The response to check. - - Returns: - `True` if the `response` is a successful `Response`. - """ - return response.RESPONSE_TYPE == smpmsg.ResponseType.SUCCESS - - TUploadRequest = TypeVar( "TUploadRequest", ImageUploadWriteRequest, @@ -140,50 +76,12 @@ def success( ) """A single-shot upload request whose `data` field is filled to maximize throughput.""" +TTransport = TypeVar("TTransport", bound=SMPTransport) +"""The type of the client's transport.""" -def wrapping_sequence() -> Iterator[u8]: - """The default SMP sequence space: `0x00`-`0xFF`, wrapping.""" - return cast("Iterator[u8]", itertools.cycle(range(0x100))) - - -def _hexdump(frame: bytes) -> str: - """Format `frame` as an offset/hex/printable-ASCII dump for readable debug logging.""" - - def row(offset: int) -> str: - chunk: Final = frame[offset : offset + 16] - columns: Final = " ".join(f"{byte:02x}" for byte in chunk) - printable: Final = "".join(chr(byte) if 0x20 <= byte <= 0x7E else "." for byte in chunk) - return f"\t{offset:04x} {columns:<47} {printable}" - - return "\n".join(row(offset) for offset in range(0, len(frame), 16)) - - -def _validation_failure( - header: smpheader.Header, - frame: bytes, - errors: tuple[tuple[type[smpmsg.Response], msgspec.DecodeError], ...], -) -> tuple[str, str]: - """Return the `(summary, details)` describing why `frame` matched none of `errors`' types.""" - summary: Final = ( - "\nFrame could not be parsed as any of:\n" - f"\t{[response.__name__ for response, _ in errors]}\n" - ) - details: Final = "\n".join( - ( - f"Header:\n\t{header}", - f"Frame:\n{_hexdump(frame)}", - "Errors:", - *( - f"\tCould not be parsed as {response.__name__}: {error}" - for response, error in errors - ), - ) - ) - return summary, details - -class SMPClient: - """Create a client to the SMP server `address`, using `transport`. +class SMPClient(Generic[TTransport]): + """Create a client to the SMP server at the other end of the live `transport`. This class provides a high-level interface to an SMP server. Other than the `request` method, all methods are abstractions of common SMP routines, @@ -193,10 +91,9 @@ class SMPClient: the response or error. Args: - transport: the `SMPTransport` to use - address: the address of the SMP server, see `smpclient.transport` for details + transport: the connected `SMPTransport`; the client never opens or closes it timeout_s: the default timeout in seconds for SMP requests - sequence: this client's SMP sequence space; defaults to `wrapping_sequence()` + sequence: this client's SMP sequence space Example: ```python @@ -206,7 +103,8 @@ class SMPClient: from smpclient.transport.ble import SMPBLETransport async def main(): - async with SMPClient(SMPBLETransport(), "00:11:22:33:44:55") as client: + async with SMPBLETransport().connected("00:11:22:33:44:55") as transport: + client = SMPClient(transport) response = await client.request(EchoWriteRequest(d="Hello, World!")) if success(response): @@ -221,30 +119,19 @@ async def main(): def __init__( # noqa: DOC301 self, - transport: SMPTransport, - address: str, + transport: TTransport, + *, timeout_s: float = 2.5, - sequence: Iterator[u8] | None = None, - ): + sequence: Callable[[], Iterator[u8]] = wrapping_sequence, + ) -> None: self._transport: Final = transport - self._address: Final = address - self._timeout_s = timeout_s - self._sequence: Final = wrapping_sequence() if sequence is None else sequence - - async def connect(self, connect_timeout_s: float | None = None) -> None: - """Connect to the SMP server. - - Args: - connect_timeout_s: the timeout for the connection attempt in seconds - """ - connect_timeout_s = connect_timeout_s if connect_timeout_s is not None else self._timeout_s + self._timeout_s: Final = timeout_s + self._sequence: Final = sequence() - await self._transport.connect(self._address, connect_timeout_s) - await self._initialize(self._timeout_s) - - async def disconnect(self) -> None: - """Disconnect from the SMP server.""" - await self._transport.disconnect() + @property + def transport(self) -> TTransport: + """The live transport this client exchanges requests over.""" + return self._transport async def request( self, request: SMPRequest[TRep, TEr1, TEr2], timeout_s: float | None = None @@ -300,45 +187,13 @@ async def request( assert_never(response) ``` - """ - timeout_s = timeout_s if timeout_s is not None else self._timeout_s - - request_frame: Final = request.to_frame(next(self._sequence)) - - try: - async with timeout(timeout_s): - frame = await self._transport.send_and_receive(bytes(request_frame)) - except asyncio.TimeoutError: - timeout_message: Final = f"Timeout ({timeout_s}s) waiting for request {request}" - logger.error(timeout_message) - raise TimeoutError(timeout_message) - - header = smpheader.Header.loads(frame[: smpheader.Header.SIZE]) - - if header.sequence != request_frame.header.sequence: - raise SMPBadSequence( - f"Bad sequence {header.sequence}, expected {request_frame.header.sequence}" - ) - - # `SMPMalformed` and `SMPMismatchedGroupId` are not caught: they fail all three - # candidates identically, so they are transport errors rather than a mismatch. - errors: list[tuple[type[smpmsg.Response], msgspec.DecodeError]] = [] - try: - return request._Response.loads(frame).data # type: ignore[return-value] - except msgspec.DecodeError as error: - errors.append((request._Response, error)) - try: - return request._ErrorV1.loads(frame).data - except msgspec.DecodeError as error: - errors.append((request._ErrorV1, error)) - try: - return request._ErrorV2.loads(frame).data - except msgspec.DecodeError as error: - errors.append((request._ErrorV2, error)) - - summary, details = _validation_failure(header, frame, tuple(errors)) - logger.error(summary + details) - raise SMPValidationException(summary, details) from None + """ # noqa: DOC502 + return await _request.exchange( + self._transport, + request, + next(self._sequence), + timeout_s if timeout_s is not None else self._timeout_s, + ) async def upload( self, @@ -544,25 +399,6 @@ async def download_file( logger.info("Download complete") return file_data - @property - def address(self) -> str: - """The SMP server address.""" - return self._address - - async def __aenter__(self) -> "SMPClient": - await self.connect() - return self - - async def __aexit__( - self, - exc_type: type[BaseException] | None, - exc_value: BaseException | None, - tb: TracebackType | None, - ) -> None: - if exc_value is not None: - logger.error(f"Exception in SMPClient:\n{traceback.format_exc()}") - await self.disconnect() - @staticmethod def _cbor_integer_size(integer: int) -> int: """CBOR integers are packed as small as possible.""" @@ -619,21 +455,3 @@ def _maximize_upload_packet(self, request: TUploadRequest, data: bytes) -> TUplo data_size: Final = min(max_data_size, len(data) - request.off) return msgspec.structs.replace(request, data=data[request.off : request.off + data_size]) - - async def _initialize(self, timeout_s: float | None = None) -> None: - """Gather initialization information from the SMP server.""" - timeout_s = timeout_s if timeout_s is not None else self._timeout_s - - try: - mcumgr_parameters = await self.request( - MCUMgrParametersReadRequest(), timeout_s=timeout_s - ) - if success(mcumgr_parameters): - logger.debug(f"MCUMgr parameters: {mcumgr_parameters}") - self._transport.initialize(mcumgr_parameters.buf_size) - elif error(mcumgr_parameters): - logger.warning(f"Error reading MCUMgr parameters: {mcumgr_parameters}") - else: - assert_never(mcumgr_parameters) - except TimeoutError: - logger.warning("Timeout waiting for MCUMgr parameters") diff --git a/src/smpclient/_request.py b/src/smpclient/_request.py new file mode 100644 index 0000000..979ae80 --- /dev/null +++ b/src/smpclient/_request.py @@ -0,0 +1,199 @@ +"""The SMP request/response exchange over a live `SMPTransport`.""" + +from __future__ import annotations + +import asyncio +import itertools +import logging +from collections.abc import Iterator +from typing import TYPE_CHECKING, Any, Final, TypeVar, Union, cast + +import msgspec +from smp import SMPRequest +from smp import error as smperror +from smp import header as smpheader +from smp import message as smpmsg +from smp.os_management import MCUMgrParametersReadRequest, MCUMgrParametersReadResponse +from typing_extensions import TypeIs, assert_never + +from smpclient.exceptions import SMPBadSequence, SMPValidationException + +if TYPE_CHECKING: + from types_bits import u8 + + from smpclient.transport import SMPTransport + +try: + from asyncio import timeout # type: ignore +except ImportError: # backport for Python3.10 and below + from async_timeout import timeout # type: ignore + +logger = logging.getLogger(__name__) + +TEr1 = TypeVar("TEr1", bound=smperror.ErrorV1) +"""Type of SMP Error V1.""" + +TEr2 = TypeVar("TEr2", bound=smperror.ErrorV2) +"""Type of SMP Error V2.""" + +TRep = TypeVar("TRep", bound=Union[smpmsg.ReadResponse, smpmsg.WriteResponse]) +"""Type of successful SMP Response (ReadResponse or WriteResponse).""" + + +def error_v1(response: smpmsg.Response) -> TypeIs[smperror.ErrorV1]: + """`TypeIs` that returns `True` if the `response` is an `ErrorV1`. + + Args: + response: The response to check. + + Returns: + `True` if the `response` is an `ErrorV1`. + """ + return response.RESPONSE_TYPE == smpmsg.ResponseType.ERROR_V1 + + +def error_v2(response: smpmsg.Response) -> TypeIs[smperror.ErrorV2[Any]]: + """`TypeIs` that returns `True` if the `response` is an `ErrorV2`. + + Args: + response: The response to check. + + Returns: + `True` if the `response` is an `ErrorV2`. + """ + return response.RESPONSE_TYPE == smpmsg.ResponseType.ERROR_V2 + + +def error( + response: smpmsg.Response, +) -> TypeIs[Union[smperror.ErrorV1, smperror.ErrorV2[Any]]]: + """`TypeIs` that returns `True` if the `response` is an `ErrorV1` or `ErrorV2`. + + Args: + response: The response to check. + + Returns: + `True` if the `response` is an `ErrorV1` or `ErrorV2`. + """ + return error_v1(response) or error_v2(response) + + +def success( + response: smpmsg.Response, +) -> TypeIs[Union[smpmsg.ReadResponse, smpmsg.WriteResponse]]: + """`TypeIs` that returns `True` if the `response` is a successful `Response`. + + Args: + response: The response to check. + + Returns: + `True` if the `response` is a successful `Response`. + """ + return response.RESPONSE_TYPE == smpmsg.ResponseType.SUCCESS + + +def wrapping_sequence() -> Iterator[u8]: + """The default SMP sequence space: `0x00`-`0xFF`, wrapping.""" + return cast("Iterator[u8]", itertools.cycle(range(0x100))) + + +def _hexdump(frame: bytes) -> str: + """Format `frame` as an offset/hex/printable-ASCII dump for readable debug logging.""" + + def row(offset: int) -> str: + chunk: Final = frame[offset : offset + 16] + columns: Final = " ".join(f"{byte:02x}" for byte in chunk) + printable: Final = "".join(chr(byte) if 0x20 <= byte <= 0x7E else "." for byte in chunk) + return f"\t{offset:04x} {columns:<47} {printable}" + + return "\n".join(row(offset) for offset in range(0, len(frame), 16)) + + +def _validation_failure( + header: smpheader.Header, + frame: bytes, + errors: tuple[tuple[type[smpmsg.Response], msgspec.DecodeError], ...], +) -> tuple[str, str]: + """Return the `(summary, details)` describing why `frame` matched none of `errors`' types.""" + summary: Final = ( + "\nFrame could not be parsed as any of:\n" + f"\t{[response.__name__ for response, _ in errors]}\n" + ) + details: Final = "\n".join( + ( + f"Header:\n\t{header}", + f"Frame:\n{_hexdump(frame)}", + "Errors:", + *( + f"\tCould not be parsed as {response.__name__}: {error}" + for response, error in errors + ), + ) + ) + return summary, details + + +async def exchange( + transport: SMPTransport, + request: SMPRequest[TRep, TEr1, TEr2], + sequence: u8, + timeout_s: float, +) -> TRep | TEr1 | TEr2: + """The core of `SMPClient.request`, as SMP sequence number `sequence`.""" + request_frame: Final = request.to_frame(sequence) + + try: + async with timeout(timeout_s): + frame = await transport.send_and_receive(bytes(request_frame)) + except asyncio.TimeoutError: + timeout_message: Final = f"Timeout ({timeout_s}s) waiting for request {request}" + logger.error(timeout_message) + raise TimeoutError(timeout_message) + + header = smpheader.Header.loads(frame[: smpheader.Header.SIZE]) + + if header.sequence != request_frame.header.sequence: + raise SMPBadSequence( + f"Bad sequence {header.sequence}, expected {request_frame.header.sequence}" + ) + + # `SMPMalformed` and `SMPMismatchedGroupId` are not caught: they fail all three + # candidates identically, so they are transport errors rather than a mismatch. + errors: list[tuple[type[smpmsg.Response], msgspec.DecodeError]] = [] + try: + return request._Response.loads(frame).data # type: ignore[return-value] + except msgspec.DecodeError as error: + errors.append((request._Response, error)) + try: + return request._ErrorV1.loads(frame).data + except msgspec.DecodeError as error: + errors.append((request._ErrorV1, error)) + try: + return request._ErrorV2.loads(frame).data + except msgspec.DecodeError as error: + errors.append((request._ErrorV2, error)) + + summary, details = _validation_failure(header, frame, tuple(errors)) + logger.error(summary + details) + raise SMPValidationException(summary, details) from None + + +async def read_mcumgr_parameters( + transport: SMPTransport, sequence: u8, timeout_s: float +) -> MCUMgrParametersReadResponse | None: + """The server's MCUmgr parameters, or `None` (warned) if it answers an error or not at all.""" + try: + response: Final = await exchange( + transport, MCUMgrParametersReadRequest(), sequence, timeout_s + ) + except TimeoutError: + logger.warning("Timeout waiting for MCUMgr parameters") + return None + if success(response): + logger.debug(f"MCUMgr parameters: {response}") + return response + elif error(response): + logger.warning(f"Error reading MCUMgr parameters: {response}") + return None + else: + assert_never(response) diff --git a/src/smpclient/extensions/intercreate.py b/src/smpclient/extensions/intercreate.py index c8a7343..0516303 100644 --- a/src/smpclient/extensions/intercreate.py +++ b/src/smpclient/extensions/intercreate.py @@ -4,11 +4,11 @@ from smp.user import intercreate as ic -from smpclient import SMPClient, error, success +from smpclient import SMPClient, TTransport, error, success from smpclient.exceptions import SMPUploadError -class ICUploadClient(SMPClient): +class ICUploadClient(SMPClient[TTransport]): """Support for Intercreate Group Upload.""" async def ic_upload(self, data: bytes, image: int = 0) -> AsyncIterator[int]: diff --git a/src/smpclient/transport/__init__.py b/src/smpclient/transport/__init__.py index 100004c..61398d9 100644 --- a/src/smpclient/transport/__init__.py +++ b/src/smpclient/transport/__init__.py @@ -1,8 +1,23 @@ """Simple Management Protocol (SMP) Client Transport Protocol.""" -from typing import Final, Protocol +from __future__ import annotations + +import logging +from abc import abstractmethod +from collections.abc import AsyncIterator, Callable, Iterator +from contextlib import asynccontextmanager +from typing import TYPE_CHECKING, Final, Generic, NamedTuple, Protocol, TypeAlias, TypeVar from uuid import UUID +from typing_extensions import Self, assert_never, override + +from smpclient import _request + +if TYPE_CHECKING: + from types_bits import u8 + +logger: Final = logging.getLogger(__name__) + SMP_SERVICE_UUID: Final = UUID("8D53DC1D-1DB7-4CD3-868B-8A527460AA84") """The 128-bit GATT service UUID for an SMP server. @@ -18,23 +33,30 @@ class SMPTransportDisconnected(Exception): """Raised when the SMP transport is disconnected.""" -class SMPTransport(Protocol): - _smp_server_transport_buffer_size: int | None = None - """The SMP server transport buffer size, in 8-bit bytes.""" +class Auto(NamedTuple): + """Size messages from the server's MCUmgr parameters, read while the transport connects.""" - async def connect(self, address: str, timeout_s: float) -> None: # pragma: no cover - """Connect the `SMPTransport`. - Args: - address: The SMP server address. - timeout_s: The connection timeout in seconds. - """ - ... +class BufferSize(NamedTuple): + """Size messages from a known server buffer; the server's parameters are not read.""" - async def disconnect(self) -> None: # pragma: no cover - """Disconnect the `SMPTransport`.""" - ... + buf_size: int + """The server's SMP reassembly buffer (`CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE`).""" + + +class Unfragmented(NamedTuple): + """Like `Auto`, but one SMP message per GATT write. + + For a server that does not reassemble a message split across writes (Zephyr without + `CONFIG_MCUMGR_TRANSPORT_BT_REASSEMBLY`). + """ + + +GATTFragmentationStrategy: TypeAlias = Auto | Unfragmented | BufferSize +"""How a GATT transport (`ble`, `bumble`) sizes SMP messages.""" + +class SMPTransport(Protocol): async def send(self, data: bytes) -> None: # pragma: no cover """Send the encoded `SMPRequest` `data`. @@ -62,14 +84,6 @@ async def send_and_receive(self, data: bytes) -> bytes: # pragma: no cover """ ... - 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 - @property def mtu(self) -> int: # pragma: no cover """The Maximum Transmission Unit (MTU) in 8-bit bytes.""" @@ -88,5 +102,94 @@ def max_unencoded_size(self) -> int: # pragma: no cover # an error in some write, then some of the writes that have already been # sent out are no longer valid. That is, the response to each # concurrent write needs to be tracked very carefully! + ... + - return self._smp_server_transport_buffer_size or self.mtu +_TStrategy = TypeVar("_TStrategy") +"""A transport's fragmentation strategy union.""" + + +class _ConnectableTransport(SMPTransport, Generic[_TStrategy]): + """An `SMPTransport` that opens and closes its own link. + + `SMPClient` sees only the `SMPTransport` part. Open a link with the `connected()` or + `borrowed()` bracket: the bare `connect()`, `borrow()`, and `disconnect()` primitives give + up the bracket's guarantee that the link is released on error and on cancellation. + """ + + def __init__( + self, + fragmentation_strategy: _TStrategy, + connect_timeout_s: float, + sequence: Callable[[], Iterator[u8]], + ) -> None: + self._fragmentation_strategy: Final = fragmentation_strategy + self._sizing: _TStrategy = fragmentation_strategy + """The fragmentation strategy as `negotiate()` resolved it.""" + self._connect_timeout_s: Final = connect_timeout_s + self._sequence: Final = sequence() + + @abstractmethod + async def disconnect(self) -> None: # pragma: no cover + """Release the link; prefer the bracket that opened it, which releases it on every exit.""" + + @abstractmethod + async def negotiate(self) -> None: # pragma: no cover + """Adopt the server's MCUmgr parameters, if the fragmentation strategy asks for them.""" + + async def _read_buf_size(self) -> int | None: + """The server's advertised `buf_size`, or `None` if it doesn't provide one.""" + params: Final = await _request.read_mcumgr_parameters( + self, next(self._sequence), self._connect_timeout_s + ) + return None if params is None else params.buf_size + + @asynccontextmanager + async def _released_on_exit(self) -> AsyncIterator[Self]: + """Yield the open link, then release it best-effort on every exit.""" + try: + yield self + finally: + try: + await self.disconnect() + except Exception as e: + logger.warning(f"Error during disconnect: {e}") + + +class _GATTTransport(_ConnectableTransport[GATTFragmentationStrategy]): + """A `_ConnectableTransport` that writes SMP messages to a GATT characteristic.""" + + @override + async def negotiate(self) -> None: + match self._fragmentation_strategy: + case Auto(): + match await self._read_buf_size(): + case None: + self._sizing = Auto() + case int() as buf_size: + self._sizing = BufferSize(buf_size) + case _ as unreachable: + assert_never(unreachable) + case Unfragmented(): + match await self._read_buf_size(): + case None: + self._sizing = Unfragmented() + case int() as buf_size: + self._sizing = BufferSize(min(self.mtu, buf_size)) + case _ as unreachable: + assert_never(unreachable) + case BufferSize(): + pass + case _ as unreachable: + assert_never(unreachable) + + @property + @override + def max_unencoded_size(self) -> int: + match self._sizing: + case Auto() | Unfragmented(): + return self.mtu + case BufferSize(buf_size=buf_size): + return buf_size + case _ as unreachable: + assert_never(unreachable) diff --git a/src/smpclient/transport/ble.py b/src/smpclient/transport/ble.py index 54b6107..045bcd5 100644 --- a/src/smpclient/transport/ble.py +++ b/src/smpclient/transport/ble.py @@ -1,15 +1,19 @@ """A Bluetooth Low Energy (BLE) SMPTransport.""" +from __future__ import annotations + import asyncio import logging import re import sys -from collections.abc import Coroutine -from typing import Any, Final, Protocol, TypeAlias, TypeGuard, TypeVar +from collections.abc import AsyncIterator, Callable, Coroutine, Iterator +from contextlib import asynccontextmanager +from typing import TYPE_CHECKING, Any, Final, NamedTuple, Protocol, TypeAlias, TypeGuard, TypeVar from uuid import UUID try: from bleak import BleakClient, BleakScanner + from bleak.args.bluez import BlueZClientArgs, BlueZScannerArgs from bleak.args.winrt import WinRTClientArgs from bleak.backends.characteristic import BleakGATTCharacteristic from bleak.backends.client import BaseBleakClient @@ -19,16 +23,22 @@ raise ImportError("BLE transport requires the 'ble' extra. Use smpclient[ble]") from e raise from smp import header as smphdr -from typing_extensions import override +from typing_extensions import Self, assert_never, override +from smpclient import _request from smpclient.exceptions import SMPClientException from smpclient.transport import ( SMP_CHARACTERISTIC_UUID, SMP_SERVICE_UUID, - SMPTransport, + Auto, + GATTFragmentationStrategy, SMPTransportDisconnected, + _GATTTransport, ) +if TYPE_CHECKING: + from types_bits import u8 + if sys.platform == "linux": from bleak.backends.bluezdbus.client import BleakClientBlueZDBus else: # stub for mypy @@ -65,6 +75,46 @@ def _session(self) -> GattSession: ... ) +class PlatformDefault(NamedTuple): + """bleak's defaults for the platform's backend.""" + + +class BlueZ(NamedTuple): + """Options for bleak's BlueZ backend (Linux), e.g. the `adapter` to scan and connect with.""" + + args: BlueZClientArgs + + +class WinRT(NamedTuple): + """Options for bleak's WinRT backend (Windows), e.g. `use_cached_services`.""" + + args: WinRTClientArgs + + +BleakBackend: TypeAlias = PlatformDefault | BlueZ | WinRT +"""The bleak backend options that `SMPBLETransport` scans and connects with.""" + + +def _bluez_args(backend: BleakBackend) -> BlueZClientArgs: + match backend: + case BlueZ(args=args): + return args + case PlatformDefault() | WinRT(): + return {} + case _ as unreachable: + assert_never(unreachable) + + +def _winrt_args(backend: BleakBackend) -> WinRTClientArgs: + match backend: + case WinRT(args=args): + return args + case PlatformDefault() | BlueZ(): + return {} + case _ as unreachable: + assert_never(unreachable) + + class SMPBLETransportException(SMPClientException): """Base class for SMP BLE transport exceptions.""" @@ -81,55 +131,150 @@ class SMPBLETransportNotSMPServer(SMPBLETransportException): _T = TypeVar("_T") +_BORROWED_DISCONNECT_POLL_S: Final = 0.1 +"""How often a wait on a borrowed client checks `is_connected`; its owner holds the callback.""" + + +class _Closed(NamedTuple): + """No link: not yet connected, disconnected, or a borrowed client returned.""" + + +class _Owned(NamedTuple): + """The link is the transport's own `BleakClient`, which `connect()` created.""" + + client: BleakClient + + +class _Borrowed(NamedTuple): + """The link is a caller's connected `BleakClient`, which the caller disconnects.""" + + client: BleakClient -class SMPBLETransport(SMPTransport): + +_Link: TypeAlias = _Closed | _Owned | _Borrowed + + +class SMPBLETransport(_GATTTransport): """A Bluetooth Low Energy (BLE) SMPTransport.""" - def __init__(self, winrt: WinRTClientArgs = {}) -> None: - self._buffer = bytearray() - self._notify_condition = asyncio.Condition() - self._disconnected_event = asyncio.Event() + def __init__( + self, + *, + backend: BleakBackend = PlatformDefault(), + fragmentation_strategy: GATTFragmentationStrategy = Auto(), + connect_timeout_s: float = 2.5, + sequence: Callable[[], Iterator[u8]] = _request.wrapping_sequence, + ) -> None: + """Initialize the BLE transport. + + Args: + backend: The bleak backend options to scan and connect with. + fragmentation_strategy: How to size SMP messages. + connect_timeout_s: Bounds scanning and connecting, and reading the server's + MCUmgr parameters. + sequence: The SMP sequence space the MCUmgr parameters read draws from. + """ + super().__init__(fragmentation_strategy, connect_timeout_s, sequence) + self._buffer: Final = bytearray() + self._notify_condition: Final = asyncio.Condition() + self._disconnected_event: Final = asyncio.Event() self._disconnected_event.set() - self._winrt = winrt + self._backend: Final = backend + self._link: _Link = _Closed() self._max_write_without_response_size = 20 """Initially set to BLE minimum; may be mutated by the `connect()` method.""" logger.debug(f"Initialized {self.__class__.__name__}") - @override - async def connect(self, address: str, timeout_s: float) -> None: + async def connect(self, address: str) -> None: + """Scan for and connect to `address`, then `negotiate()`; prefer `connected()`. + + Args: + address: The device's MAC address, macOS UUID, or advertised name. + """ # noqa: DOC501, DOC503 try: - await asyncio.wait_for(self._connect(address, timeout_s), timeout=timeout_s) + await asyncio.wait_for( + self._connect(address, self._connect_timeout_s), + timeout=self._connect_timeout_s, + ) + await self.negotiate() except (Exception, asyncio.CancelledError): await self._best_effort_disconnect() raise + @asynccontextmanager + async def connected(self, address: str) -> AsyncIterator[Self]: + """Connect to `address` for the duration of the `async with`, then disconnect.""" + await self.connect(address) + async with self._released_on_exit(): + yield self + async def _connect(self, address: str, timeout_s: float) -> None: logger.debug(f"Scanning for {address=}") device: BLEDevice | None = ( - await BleakScanner.find_device_by_address(address, timeout=timeout_s) + await BleakScanner.find_device_by_address( + address, timeout=timeout_s, bluez=BlueZScannerArgs(**_bluez_args(self._backend)) + ) if MAC_ADDRESS_PATTERN.match(address) or UUID_PATTERN.match(address) - else await BleakScanner.find_device_by_name(address, timeout=timeout_s) + else await BleakScanner.find_device_by_name( + address, timeout=timeout_s, bluez=BlueZScannerArgs(**_bluez_args(self._backend)) + ) ) if type(device) is BLEDevice: - self._client = BleakClient( - device, - services=(str(SMP_SERVICE_UUID),), - winrt=self._winrt, - timeout=timeout_s, - disconnected_callback=self._set_disconnected_event, + self._link = _Owned( + BleakClient( + device, + services=(str(SMP_SERVICE_UUID),), + winrt=_winrt_args(self._backend), + bluez=_bluez_args(self._backend), + timeout=timeout_s, + disconnected_callback=self._set_disconnected_event, + ) ) else: raise SMPBLETransportDeviceNotFound(f"Device '{address}' not found") logger.debug(f"Found device: {device=}, connecting...") - await self._client.connect() + await self._active_client.connect() self._disconnected_event.clear() logger.debug(f"Connected to {device=}") + await self._start_smp() + + async def borrow(self, client: BleakClient) -> None: + """Adopt the caller's connected `client`, then `negotiate()`; prefer `borrowed()`.""" + self._link = _Borrowed(client) + try: + await self._start_smp() + await self.negotiate() + except (Exception, asyncio.CancelledError): + await self.disconnect() + raise + + @asynccontextmanager + async def borrowed(self, client: BleakClient) -> AsyncIterator[Self]: + """Borrow the caller's connected `client` for the duration of the `async with`.""" + await self.borrow(client) + async with self._released_on_exit(): + yield self - smp_characteristic = self._client.services.get_characteristic(SMP_CHARACTERISTIC_UUID) + @property + def _active_client(self) -> BleakClient: + match self._link: + case _Closed(): + raise SMPTransportDisconnected(f"{self.__class__.__name__} is not connected") + case _Owned(client=client) | _Borrowed(client=client): + return client + case _ as unreachable: + assert_never(unreachable) + + async def _start_smp(self) -> None: + """Find the SMP characteristic, size writes to the link, and subscribe to it.""" + self._buffer.clear() + smp_characteristic = self._active_client.services.get_characteristic( + SMP_CHARACTERISTIC_UUID + ) if smp_characteristic is None: raise SMPBLETransportNotSMPServer("Missing the SMP characteristic UUID.") @@ -137,7 +282,7 @@ async def _connect(self, address: str, timeout_s: float) -> None: logger.info(f"{smp_characteristic.max_write_without_response_size=}") self._max_write_without_response_size = smp_characteristic.max_write_without_response_size if ( - self._winrt_backend(self._client._backend) + self._winrt_backend(self._active_client._backend) and self._max_write_without_response_size == 20 ): # https://github.com/hbldh/bleak/pull/1552#issuecomment-2105573291 @@ -146,38 +291,54 @@ async def _connect(self, address: str, timeout_s: float) -> None: ) await asyncio.sleep(2) smp_characteristic._max_write_without_response_size = ( # pyright: ignore[reportAttributeAccessIssue] - self._client._backend._session.max_pdu_size - 3 # type: ignore + self._active_client._backend._session.max_pdu_size - 3 # type: ignore ) self._max_write_without_response_size = ( smp_characteristic.max_write_without_response_size ) logger.warning(f"{smp_characteristic.max_write_without_response_size=}") - elif self._bluez_backend(self._client._backend): + elif self._bluez_backend(self._active_client._backend): logger.debug("Getting MTU from BlueZ backend") - await self._client._backend._acquire_mtu() - logger.debug(f"Got MTU: {self._client.mtu_size}") - self._max_write_without_response_size = self._client.mtu_size - 3 + await self._active_client._backend._acquire_mtu() + logger.debug(f"Got MTU: {self._active_client.mtu_size}") + self._max_write_without_response_size = self._active_client.mtu_size - 3 logger.info(f"{self._max_write_without_response_size=}") self._smp_characteristic = smp_characteristic logger.debug(f"Starting notify on {SMP_CHARACTERISTIC_UUID=}") await self._await_or_disconnect( - self._client.start_notify(SMP_CHARACTERISTIC_UUID, self._notify_callback) + self._active_client.start_notify(SMP_CHARACTERISTIC_UUID, self._notify_callback) ) logger.debug(f"Started notify on {SMP_CHARACTERISTIC_UUID=}") @override async def disconnect(self) -> None: - logger.debug(f"Disonnecting from {self._client.address}") - await self._client.disconnect() - logger.debug(f"Disconnected from {self._client.address}") + match self._link: + case _Closed(): + pass + case _Owned(client=client): + logger.debug(f"Disonnecting from {client.address}") + self._link = _Closed() + await client.disconnect() + logger.debug(f"Disconnected from {client.address}") + case _Borrowed(client=client): + logger.debug(f"Returning the borrowed client for {client.address}") + self._link = _Closed() + try: + await asyncio.wait_for( + client.stop_notify(SMP_CHARACTERISTIC_UUID), timeout=self._connect_timeout_s + ) + except Exception as e: + logger.warning(f"Error unsubscribing from the borrowed client: {e}") + case _ as unreachable: + assert_never(unreachable) @override async def send(self, data: bytes) -> None: logger.debug(f"Sending {len(data)} bytes, {self.mtu=}") for offset in range(0, len(data), self.mtu): - await self._client.write_gatt_char( + await self._active_client.write_gatt_char( self._smp_characteristic, data[offset : offset + self.mtu], response=False ) logger.debug(f"Sent {len(data)} bytes") @@ -227,12 +388,12 @@ def mtu(self) -> int: return self._max_write_without_response_size @staticmethod - async def scan(timeout: int = 5) -> list[BLEDevice]: - """Scan for BLE devices.""" + async def scan(timeout: int = 5, backend: BleakBackend = PlatformDefault()) -> list[BLEDevice]: + """Scan for BLE devices with the bleak `backend` options.""" logger.debug(f"Scanning for BLE devices for {timeout} seconds") - devices: Final = await BleakScanner(service_uuids=[str(SMP_SERVICE_UUID)]).discover( - timeout=timeout, return_adv=True - ) + devices: Final = await BleakScanner( + service_uuids=[str(SMP_SERVICE_UUID)], bluez=BlueZScannerArgs(**_bluez_args(backend)) + ).discover(timeout=timeout, return_adv=True) smp_servers: Final = [ d for d, a in devices.values() if SMP_SERVICE_UUID in {UUID(u) for u in a.service_uuids} ] @@ -248,28 +409,44 @@ def _winrt_backend(client_backend: _ClientBackend) -> TypeGuard[BleakClientWinRT return client_backend.__class__.__name__ == "BleakClientWinRT" def _set_disconnected_event(self, client: BleakClient) -> None: - if client is not self._client: - raise SMPBLETransportException( - f"Unexpected client disconnected: {client=}, {self._client=}" - ) + match self._link: + case _Owned(client=owned) if owned is not client: + raise SMPBLETransportException( + f"Unexpected client disconnected: {client=}, {owned=}" + ) + case _Closed() | _Owned() | _Borrowed(): + pass + case _ as unreachable: + assert_never(unreachable) logger.warning(f"Disconnected from {client.address}") self._disconnected_event.set() + async def _until_disconnected(self) -> None: + match self._link: + case _Closed(): + pass + case _Owned(): + await self._disconnected_event.wait() + case _Borrowed(client=client): + while client.is_connected: + await asyncio.sleep(_BORROWED_DISCONNECT_POLL_S) + case _ as unreachable: + assert_never(unreachable) + async def _notify_or_disconnect(self) -> None: - disconnected_task: Final = asyncio.create_task(self._disconnected_event.wait()) + disconnected_task: Final = asyncio.create_task(self._until_disconnected()) notify_task: Final = asyncio.create_task(self._notify_condition.wait()) - done, pending = await asyncio.wait( - (disconnected_task, notify_task), return_when=asyncio.FIRST_COMPLETED - ) - for task in pending: - task.cancel() try: - await asyncio.gather(*pending) - except asyncio.CancelledError: - pass + done, _ = await asyncio.wait( + (disconnected_task, notify_task), return_when=asyncio.FIRST_COMPLETED + ) + finally: + for task in (disconnected_task, notify_task): + task.cancel() + await asyncio.gather(disconnected_task, notify_task, return_exceptions=True) if disconnected_task in done: raise SMPTransportDisconnected( - f"{self.__class__.__name__} disconnected from {self._client.address}" + f"{self.__class__.__name__} disconnected from {self._active_client.address}" ) async def _await_or_disconnect(self, coro: Coroutine[Any, Any, _T]) -> _T: @@ -280,7 +457,7 @@ async def _await_or_disconnect(self, coro: Coroutine[Any, Any, _T]) -> _T: https://github.com/intercreate/smpmgr/issues/97. """ op_task: Final = asyncio.create_task(coro) - disconnected_task: Final = asyncio.create_task(self._disconnected_event.wait()) + disconnected_task: Final = asyncio.create_task(self._until_disconnected()) try: done, _ = await asyncio.wait( (op_task, disconnected_task), return_when=asyncio.FIRST_COMPLETED @@ -292,16 +469,13 @@ async def _await_or_disconnect(self, coro: Coroutine[Any, Any, _T]) -> _T: await asyncio.gather(op_task, disconnected_task, return_exceptions=True) if disconnected_task in done: raise SMPTransportDisconnected( - f"{self.__class__.__name__} disconnected from {self._client.address}" + f"{self.__class__.__name__} disconnected from {self._active_client.address}" ) return op_task.result() async def _best_effort_disconnect(self) -> None: """Best-effort cleanup after a failed `connect()`; never raises.""" - client: Final = getattr(self, "_client", None) - if client is None: - return try: - await client.disconnect() + await self.disconnect() except Exception: logger.warning("Best-effort disconnect after failed connect raised", exc_info=True) diff --git a/src/smpclient/transport/bumble/__init__.py b/src/smpclient/transport/bumble/__init__.py index 9e64ac5..47de27f 100644 --- a/src/smpclient/transport/bumble/__init__.py +++ b/src/smpclient/transport/bumble/__init__.py @@ -1,10 +1,13 @@ """A bumble-backed `SMPTransport` driving an external HCI controller over GATT.""" +from __future__ import annotations + import asyncio import logging +from collections.abc import Callable, Iterator from contextlib import asynccontextmanager from dataclasses import dataclass -from typing import AsyncIterator, Final, NamedTuple, Protocol, TypeAlias +from typing import TYPE_CHECKING, AsyncIterator, Final, NamedTuple, Protocol, TypeAlias from uuid import UUID try: @@ -12,7 +15,6 @@ from bumble.device import Connection, Device, Peer from bumble.gatt_client import CharacteristicProxy from bumble.hci import Address, HCI_ErrorCode - from bumble.keys import KeyStore from bumble.pairing import PairingConfig, PairingDelegate from bumble.smp import AuthReq from bumble.transport import open_transport @@ -25,15 +27,21 @@ raise from smp import header as smphdr -from typing_extensions import assert_never, override +from typing_extensions import Self, assert_never, override +from smpclient import _request from smpclient.exceptions import SMPClientException from smpclient.transport import ( SMP_CHARACTERISTIC_UUID, SMP_SERVICE_UUID, - SMPTransport, + Auto, + GATTFragmentationStrategy, SMPTransportDisconnected, + _GATTTransport, ) + +if TYPE_CHECKING: + from types_bits import u8 from smpclient.transport.bumble.device import ( DEFAULT_HCI_TRANSPORT, DEFAULT_HOST_ADDRESS, @@ -122,7 +130,7 @@ class ConnectedBorrowed(NamedTuple): _State: TypeAlias = Disconnected | Connecting | Connected | ConnectedBorrowed -class SMPBumbleTransport(SMPTransport): +class SMPBumbleTransport(_GATTTransport): """An `SMPTransport` backed by Google's bumble Bluetooth stack.""" def __init__( @@ -136,6 +144,9 @@ def __init__( pair_on_connect: PairingDelegate | None = None, pair_timeout_s: float = DEFAULT_PAIR_TIMEOUT_S, settle_s: float = DEFAULT_POST_PAIR_SETTLE_S, + fragmentation_strategy: GATTFragmentationStrategy = Auto(), + connect_timeout_s: float = 2.5, + sequence: Callable[[], Iterator[u8]] = _request.wrapping_sequence, ) -> None: """Initialize the bumble transport. @@ -160,7 +171,12 @@ def __init__( `pair_on_connect` and `pair()`. settle_s: Wait between successful pair and proceeding (or disconnecting) so the peer can finalize bonding. + fragmentation_strategy: How to size SMP messages. + connect_timeout_s: Bounds scanning for a name, and reading the server's + MCUmgr parameters. + sequence: The SMP sequence space the MCUmgr parameters read draws from. """ + super().__init__(fragmentation_strategy, connect_timeout_s, sequence) self._hci: Final = hci self._host_address: Final = host_address self._host_name: Final = host_name @@ -185,8 +201,15 @@ def __init__( logger.debug(f"Initialized {self.__class__.__name__}(hci={hci!r})") - @override - async def connect(self, address: str, timeout_s: float) -> None: + async def connect(self, address: str) -> None: + """Connect to `address`, then `negotiate()`; prefer `connected()`. + + Args: + address: The peer's BD_ADDR, or an advertised name to scan for. + + Raises: + SMPBumbleTransportException: if the transport already has a link. + """ # noqa: DOC503 if not isinstance(self._state, Disconnected): raise SMPBumbleTransportException( f"connect() called while in state {type(self._state).__name__}" @@ -221,7 +244,7 @@ async def connect(self, address: str, timeout_s: float) -> None: ) await self._state.device.power_on() - target = await _resolve_target(self._state.device, address, timeout_s) + target = await _resolve_target(self._state.device, address, self._connect_timeout_s) logger.info(f"Connecting to {target}") self._state.connection = await self._state.device.connect(Address(target)) self._state.connection.on(Connection.EVENT_DISCONNECTION, self._on_disconnection) @@ -263,6 +286,11 @@ async def connect(self, address: str, timeout_s: float) -> None: max_write=max_write, ) logger.info(f"Connected to {target}, max_write={max_write}") + await self.negotiate() + except asyncio.CancelledError: + logger.debug("connect() cancelled; tearing down partial state") + await self.disconnect() + raise except Exception: logger.exception("connect() failed; tearing down partial state") await self.disconnect() @@ -331,16 +359,16 @@ async def scan( async with bumble_device(hci=hci) as device: return await scan_for_devices(device, timeout_s, mode, service_uuid=service_uuid) - async def use_connection( + async def borrow( self, connection: Connection, *, peer: Peer | None = None, ) -> None: - """Adopt a caller-owned `Connection`; `disconnect()` only unsubscribes.""" + """Adopt a caller-owned `Connection`, then `negotiate()`; prefer `borrowed()`.""" if not isinstance(self._state, Disconnected): raise SMPBumbleTransportException( - f"use_connection() called while in state {type(self._state).__name__}" + f"borrow() called while in state {type(self._state).__name__}" ) while not self._notifications.empty(): @@ -362,23 +390,30 @@ async def use_connection( max_write=max_write, ) logger.info(f"Borrowing connection to {connection.peer_address}, max_write={max_write}") + try: + await self.negotiate() + except (Exception, asyncio.CancelledError): + await self.disconnect() + raise - async def bonded_devices(self) -> tuple[str, ...]: - """Return the BD_ADDRs of peers currently in the keystore.""" - return tuple(addr for addr, _keys in await self._standalone_keystore().get_all()) - - async def clear_bond(self, address: str) -> None: - """Delete the bond for `address` from the keystore.""" - await self._standalone_keystore().delete(address) - logger.info(f"Cleared bond for {address}") - - async def clear_bonds(self) -> None: - """Delete every bond from the keystore.""" - await self._standalone_keystore().delete_all() - logger.info("Cleared all bonds") - - def _standalone_keystore(self) -> KeyStore: - return resolve_keystore(self._keystore, namespace=str(self._host_address)) + @asynccontextmanager + async def borrowed( + self, + connection: Connection, + *, + peer: Peer | None = None, + ) -> AsyncIterator[Self]: + """Borrow the caller's `connection` for the duration of the `async with`.""" + await self.borrow(connection, peer=peer) + async with self._released_on_exit(): + yield self + + @asynccontextmanager + async def connected(self, address: str) -> AsyncIterator[Self]: + """Connect to `address` for the duration of the `async with`, then disconnect.""" + await self.connect(address) + async with self._released_on_exit(): + yield self async def pair( self, @@ -544,7 +579,7 @@ async def _next_chunk(self) -> bytes: async def _teardown_borrowed(self) -> None: assert isinstance(self._state, ConnectedBorrowed) try: - await self._state.smp_characteristic.unsubscribe() + await self._state.smp_characteristic.unsubscribe(self._on_notification) except Exception as e: logger.warning(f"smp_characteristic.unsubscribe failed: {e}") try: @@ -555,6 +590,34 @@ async def _teardown_borrowed(self) -> None: logger.warning(f"remove_listener(EVENT_DISCONNECTION) failed: {e}") +async def bonded_devices( + keystore: KeystoreStrategy = Tempfile(), host_address: Address = DEFAULT_HOST_ADDRESS +) -> tuple[str, ...]: + """Return the BD_ADDRs of peers in the keystore that `host_address` bonds with.""" + return tuple( + addr + for addr, _keys in await resolve_keystore(keystore, namespace=str(host_address)).get_all() + ) + + +async def clear_bond( + address: str, + keystore: KeystoreStrategy = Tempfile(), + host_address: Address = DEFAULT_HOST_ADDRESS, +) -> None: + """Delete the bond for `address` from the keystore that `host_address` bonds with.""" + await resolve_keystore(keystore, namespace=str(host_address)).delete(address) + logger.info(f"Cleared bond for {address}") + + +async def clear_bonds( + keystore: KeystoreStrategy = Tempfile(), host_address: Address = DEFAULT_HOST_ADDRESS +) -> None: + """Delete every bond from the keystore that `host_address` bonds with.""" + await resolve_keystore(keystore, namespace=str(host_address)).delete_all() + logger.info("Cleared all bonds") + + async def _resolve_target(device: Device, address: str, timeout_s: float) -> str: if MAC_ADDRESS_PATTERN.match(address): return address @@ -625,18 +688,3 @@ def _find_smp_characteristic(peer: Peer) -> CharacteristicProxy[bytes]: f"SMP characteristic {SMP_CHARACTERISTIC_UUID} not found on peer" ) return characteristics[0] - - -@asynccontextmanager -async def borrowed_connection( - transport: SMPBumbleTransport, - connection: Connection, - *, - peer: Peer | None = None, -) -> AsyncIterator[SMPBumbleTransport]: - """`async with`-friendly wrapper around `use_connection()` + `disconnect()`.""" - await transport.use_connection(connection, peer=peer) - try: - yield transport - finally: - await transport.disconnect() diff --git a/src/smpclient/transport/bumble/__main__.py b/src/smpclient/transport/bumble/__main__.py index 06e3892..19914be 100644 --- a/src/smpclient/transport/bumble/__main__.py +++ b/src/smpclient/transport/bumble/__main__.py @@ -93,10 +93,12 @@ async def _pair(args: _PairArgs) -> int: async def _echo(args: _EchoArgs) -> int: - async with SMPClient( - SMPBumbleTransport(hci=args.hci), args.address, timeout_s=args.timeout - ) as client: - response = await client.request(EchoWriteRequest(d=args.message)) + async with SMPBumbleTransport(hci=args.hci, connect_timeout_s=args.timeout).connected( + args.address + ) as transport: + response = await SMPClient(transport, timeout_s=args.timeout).request( + EchoWriteRequest(d=args.message) + ) if success(response): print(response.r) return 0 diff --git a/src/smpclient/transport/serial/__init__.py b/src/smpclient/transport/serial/__init__.py index eaa3cfc..499e924 100644 --- a/src/smpclient/transport/serial/__init__.py +++ b/src/smpclient/transport/serial/__init__.py @@ -3,11 +3,17 @@ In addition to UART, these transports can be used with USB CDC ACM and CAN. """ -from smpclient.transport.serial.encoded import Auto as Auto +from smpclient.transport.serial.common import SerialOptions as SerialOptions +from smpclient.transport.serial.common import SerialPort as SerialPort from smpclient.transport.serial.encoded import BufferParams as BufferParams from smpclient.transport.serial.encoded import BufferSize as BufferSize -from smpclient.transport.serial.encoded import FragmentationStrategy as FragmentationStrategy +from smpclient.transport.serial.encoded import ( + SerialFragmentationStrategy as SerialFragmentationStrategy, +) from smpclient.transport.serial.encoded import SMPSerialTransport as SMPSerialTransport from smpclient.transport.serial.framing import SerialFraming as SerialFraming from smpclient.transport.serial.framing.cobs import Cobs as Cobs +from smpclient.transport.serial.unencoded import ( + RawSerialFragmentationStrategy as RawSerialFragmentationStrategy, +) from smpclient.transport.serial.unencoded import SMPSerialRawTransport as SMPSerialRawTransport diff --git a/src/smpclient/transport/serial/common.py b/src/smpclient/transport/serial/common.py index aa0866e..fdc93ad 100644 --- a/src/smpclient/transport/serial/common.py +++ b/src/smpclient/transport/serial/common.py @@ -1,10 +1,13 @@ """Shared connection management for the encoded and unencoded serial transports.""" +from __future__ import annotations + import asyncio import logging -from contextlib import contextmanager +from collections.abc import AsyncIterator, Callable, Iterator +from contextlib import asynccontextmanager, contextmanager from time import monotonic -from typing import Final, Generator, final +from typing import TYPE_CHECKING, Final, Generator, NamedTuple, Protocol, TypeAlias, final try: from serial import Serial, SerialException @@ -14,24 +17,92 @@ "Serial transport requires the 'serial' extra. Use smpclient[serial]" ) from e raise -from typing_extensions import override +from typing_extensions import Self, assert_never, override + +from smpclient.transport import SMPTransportDisconnected, _ConnectableTransport, _TStrategy -from smpclient.transport import SMPTransport, SMPTransportDisconnected +if TYPE_CHECKING: + from _typeshed import ReadableBuffer + from types_bits import u8 logger = logging.getLogger(__name__) -class _SerialTransportBase(SMPTransport): +class SerialPort(Protocol): + """The part of an open `pyserial` port that the serial transports use. + + Satisfied by `serial.Serial`, and by a `serial.serial_for_url` port that reports + `out_waiting`. + """ + + @property + def port(self) -> str | None: ... # pragma: no cover + @property + def out_waiting(self) -> int: ... # pragma: no cover + def write(self, b: ReadableBuffer, /) -> int | None: ... # pragma: no cover + def read_all(self) -> bytes | None: ... # pragma: no cover + + +class _Owned(NamedTuple): + """The link is the transport's own `Serial`, which `connect()` opens.""" + + +class _Borrowed(NamedTuple): + """The link is a caller's open port, which the caller closes.""" + + port: SerialPort + + +_Link: TypeAlias = _Owned | _Borrowed + + +class SerialOptions(NamedTuple): + """The `pyserial` port settings, named as `serial.Serial` names them.""" + + baudrate: int = 115200 + """The baudrate of the serial connection. OK to ignore for USB CDC ACM.""" + + bytesize: int = 8 + """The number of data bits.""" + + parity: str = "N" + """The parity setting.""" + + stopbits: float = 1 + """The number of stop bits.""" + + timeout: float | None = None + """The read timeout.""" + + xonxoff: bool = False + """Enable software flow control.""" + + rtscts: bool = False + """Enable hardware (RTS/CTS) flow control.""" + + write_timeout: float | None = None + """The write timeout.""" + + dsrdtr: bool = False + """Enable hardware (DSR/DTR) flow control.""" + + inter_byte_timeout: float | None = None + """The inter-byte timeout.""" + + exclusive: bool | None = None + """Set exclusive access mode (POSIX only). A port cannot be opened in exclusive access + mode if it is already open in exclusive access mode.""" + + +class _SerialTransportBase(_ConnectableTransport[_TStrategy]): """Connection-management base class for serial-port-backed SMP transports. - Holds the `pyserial` `Serial` instance, the open/retry connect loop, disconnect, - and the small TX/RX helpers that wrap `SerialException` into - `SMPTransportDisconnected`. + Holds the `pyserial` `Serial` instance, the open/retry connect loop, borrowing a + caller's open port (e.g. an emulator's `socket://` chardev), disconnect, and the small + TX/RX helpers that wrap `SerialException` into `SMPTransportDisconnected`. - Subclasses implement `send` and `receive` with their framing of choice, may - override `_reset_state` to clear per-connection state on `connect`, and may - override `connect` to back the transport with a byte pipe other than a local - serial port (e.g. an emulator's `socket://` chardev). + Subclasses implement `send` and `receive` with their framing of choice, and may + override `_reset_state` to clear per-connection state on `connect` and `borrow`. """ _POLLING_INTERVAL_S: Final = 0.005 @@ -39,80 +110,97 @@ class _SerialTransportBase(SMPTransport): def __init__( self, - baudrate: int = 115200, - bytesize: int = 8, - parity: str = "N", - stopbits: float = 1, - timeout: float | None = None, - xonxoff: bool = False, - rtscts: bool = False, - write_timeout: float | None = None, - dsrdtr: bool = False, - inter_byte_timeout: float | None = None, - exclusive: bool | None = None, + fragmentation_strategy: _TStrategy, + connect_timeout_s: float, + sequence: Callable[[], Iterator[u8]], + options: SerialOptions, ) -> None: - """Initialize the underlying `pyserial` `Serial` instance. - - Args: - baudrate: The baudrate of the serial connection. OK to ignore for - USB CDC ACM. - bytesize: The number of data bits. - parity: The parity setting. - stopbits: The number of stop bits. - timeout: The read timeout. - xonxoff: Enable software flow control. - rtscts: Enable hardware (RTS/CTS) flow control. - write_timeout: The write timeout. - dsrdtr: Enable hardware (DSR/DTR) flow control. - inter_byte_timeout: The inter-byte timeout. - exclusive: Set exclusive access mode (POSIX only). A port cannot be - opened in exclusive access mode if it is already open in - exclusive access mode. - """ - self._conn: Final = Serial( - baudrate=baudrate, - bytesize=bytesize, - parity=parity, - stopbits=stopbits, - timeout=timeout, - xonxoff=xonxoff, - rtscts=rtscts, - write_timeout=write_timeout, - dsrdtr=dsrdtr, - inter_byte_timeout=inter_byte_timeout, - exclusive=exclusive, - ) + """Hold a closed `Serial` with the `options` until `connect()` opens it.""" + super().__init__(fragmentation_strategy, connect_timeout_s, sequence) + self._serial: Final = Serial(**options._asdict()) + self._link: _Link = _Owned() + + @property + def _conn(self) -> SerialPort: + match self._link: + case _Owned(): + return self._serial + case _Borrowed(port=port): + return port + case _ as unreachable: + assert_never(unreachable) def _reset_state(self) -> None: """Reset any per-connection state. Subclasses override as needed.""" - @override - async def connect(self, address: str, timeout_s: float) -> None: + async def connect(self, port: str) -> None: + """Open `port`, then `negotiate()`; prefer `connected()`.""" + try: + await self._open(port) + await self.negotiate() + except (Exception, asyncio.CancelledError): + self._serial.close() + raise + + async def borrow(self, port: SerialPort) -> None: + """Adopt the caller's open `port`, then `negotiate()`; prefer `borrowed()`.""" self._reset_state() - self._conn.port = address - logger.debug(f"Connecting to {self._conn.port=}") + self._link = _Borrowed(port) + try: + await self.negotiate() + except (Exception, asyncio.CancelledError): + self._link = _Owned() + raise + + @asynccontextmanager + async def connected(self, port: str) -> AsyncIterator[Self]: + """Open `port` for the duration of the `async with`, then close it.""" + await self.connect(port) + async with self._released_on_exit(): + yield self + + @asynccontextmanager + async def borrowed(self, port: SerialPort) -> AsyncIterator[Self]: + """Borrow the caller's open `port` for the duration of the `async with`.""" + await self.borrow(port) + async with self._released_on_exit(): + yield self + + async def _open(self, port: str) -> None: + """Open `port` off the event loop, retrying until `connect_timeout_s`.""" + self._reset_state() + self._serial.port = port + logger.debug(f"Connecting to {self._serial.port=}") start_time: Final = monotonic() - while monotonic() - start_time <= timeout_s: + while monotonic() - start_time <= self._connect_timeout_s: try: - self._conn.open() - self._conn.reset_input_buffer() - logger.debug(f"Connected to {self._conn.port=}") - return + await asyncio.to_thread(self._serial.open) except SerialException as e: logger.debug( - f"Failed to connect to {self._conn.port=}: {e}, " + f"Failed to connect to {self._serial.port=}: {e}, " f"retrying in {self._CONNECTION_RETRY_INTERVAL_S} seconds" ) await asyncio.sleep(self._CONNECTION_RETRY_INTERVAL_S) + else: + await asyncio.to_thread(self._serial.reset_input_buffer) + logger.debug(f"Connected to {self._serial.port=}") + return - raise TimeoutError(f"Failed to connect to {address=}") + raise TimeoutError(f"Failed to connect to {port=}") @final @override async def disconnect(self) -> None: - logger.debug(f"Disconnecting from {self._conn.port=}") - self._conn.close() - logger.debug(f"Disconnected from {self._conn.port=}") + match self._link: + case _Owned(): + logger.debug(f"Disconnecting from {self._serial.port=}") + self._serial.close() + logger.debug(f"Disconnected from {self._serial.port=}") + case _Borrowed(port=port): + logger.debug(f"Returning the borrowed {port.port=}") + self._link = _Owned() + case _ as unreachable: + assert_never(unreachable) @final @override diff --git a/src/smpclient/transport/serial/encoded.py b/src/smpclient/transport/serial/encoded.py index adbe673..47d5785 100644 --- a/src/smpclient/transport/serial/encoded.py +++ b/src/smpclient/transport/serial/encoded.py @@ -15,21 +15,28 @@ `smpclient.transport.serial.unencoded`. The transport fills that decoded buffer for best throughput; how it learns the buffer -size is the `fragmentation_strategy` (`FragmentationStrategy`) -- see `Auto` (the +size is the `fragmentation_strategy` (`SerialFragmentationStrategy`) -- see `Auto` (the default), `BufferSize`, and `BufferParams`. """ +from __future__ import annotations + import asyncio import logging import math -import warnings +from collections.abc import Callable, Iterator from enum import IntEnum, unique -from typing import Final, NamedTuple, TypeAlias +from typing import TYPE_CHECKING, Final, NamedTuple, TypeAlias from smp import packet as smppacket -from typing_extensions import assert_never, deprecated, overload, override +from typing_extensions import assert_never, override + +from smpclient import _request +from smpclient.transport import Auto +from smpclient.transport.serial.common import SerialOptions, _SerialTransportBase -from smpclient.transport.serial.common import _SerialTransportBase +if TYPE_CHECKING: + from types_bits import u8 logger = logging.getLogger(__name__) @@ -53,11 +60,8 @@ def _base64_max(size: int) -> int: _DEFAULT_LINE_LENGTH: Final = 128 """The SMP serial line length convention: base64 chars per line on the wire.""" -_LEGACY_LINE_BUFFERS: Final = 2 -"""The 7.1.0 default `line_buffers`, preserved for the deprecated constructor params.""" - -_LEGACY_FRAME_SIZE: Final = 256 -"""The 7.1.0 default `max_smp_encoded_frame_size`, preserved for the deprecated params.""" +_AUTO_LINE_BUFFERS: Final = 2 +"""The line buffers `Auto` assumes until it reads the server's parameters.""" _MIN_LINE_LENGTH: Final = 8 """The smallest `line_length` that can carry a base64 payload. @@ -84,29 +88,6 @@ def _encoded_budget(mtu: int, line_buffers: int) -> int: return _base64_max(mtu) - packet_framing_size -_LEGACY_PARAMS_DEPRECATION: Final = ( - "max_smp_encoded_frame_size, line_length, and line_buffers are deprecated; pass a " - "fragmentation_strategy (Auto, BufferSize, or BufferParams) instead." -) -"""The runtime `DeprecationWarning` message. - -The `@deprecated` overload decorators must repeat this text as a string *literal* -- -PEP 702 type checkers ignore a name reference -- so keep the two in sync. -""" - - -class Auto(NamedTuple): - """Discover the server's reassembly buffer from its MCUmgr params. - - On connect the client reads the server's `buf_size` - (`CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE`) -- the decoded SMP frame reassembly - buffer -- and sends messages up to `buf_size - 4` (the frame length and CRC16 - share that buffer), filling it for best throughput. Before the params are read, - or when the server does not support the params command, a single 128-byte line - buffer is assumed. - """ - - class BufferSize(NamedTuple): """Manually specify the server's decoded reassembly buffer size. @@ -145,36 +126,11 @@ class BufferParams(NamedTuple): """The number of encoded line buffers the budget spans.""" -FragmentationStrategy: TypeAlias = Auto | BufferSize | BufferParams -"""How `SMPSerialTransport` sizes SMP messages: `Auto`, `BufferSize`, or `BufferParams`.""" - - -class _LegacyParams(NamedTuple): - """The deprecated 7.1.0 `(max_smp_encoded_frame_size, line_length, line_buffers)` sizing. - - Constructed only by the deprecated constructor params; it reproduces 7.1.0 - byte-for-byte. Unlike `BufferParams`, `mtu` is the *explicit* - `max_smp_encoded_frame_size` (independent of `line_length * line_buffers`, exactly - as 7.1.0 stored it), while the per-line framing still spans `line_buffers`. Not part - of the public `FragmentationStrategy` API -- prefer `Auto`, `BufferSize`, or - `BufferParams`. - """ +SerialFragmentationStrategy: TypeAlias = Auto | BufferSize | BufferParams +"""How `SMPSerialTransport` sizes SMP messages.""" - max_smp_encoded_frame_size: int - """The encoded frame size that `mtu` reports verbatim (7.1.0 semantics).""" - line_length: int - """The maximum length of one fragment (line) on the wire.""" - - line_buffers: int - """The number of encoded line buffers the framing budget spans.""" - - -_ResolvedStrategy: TypeAlias = Auto | BufferSize | BufferParams | _LegacyParams -"""The internal strategy a constructor call resolves to (adds the deprecated `_LegacyParams`).""" - - -class SMPSerialTransport(_SerialTransportBase): +class SMPSerialTransport(_SerialTransportBase[SerialFragmentationStrategy]): @unique class BufferState(IntEnum): SMP = 0 @@ -187,133 +143,26 @@ class BufferState(IntEnum): `_buffer` is being parsed as serial data. """ - @overload - def __init__( - self, - fragmentation_strategy: FragmentationStrategy = ..., - *, - baudrate: int = ..., - bytesize: int = ..., - parity: str = ..., - stopbits: float = ..., - timeout: float | None = ..., - xonxoff: bool = ..., - rtscts: bool = ..., - write_timeout: float | None = ..., - dsrdtr: bool = ..., - inter_byte_timeout: float | None = ..., - exclusive: bool | None = ..., - ) -> None: ... - - @overload - @deprecated( - "max_smp_encoded_frame_size, line_length, and line_buffers are deprecated; pass a " - "fragmentation_strategy (Auto, BufferSize, or BufferParams) instead." - ) - def __init__( - self, - *, - max_smp_encoded_frame_size: int = ..., - line_length: int = ..., - line_buffers: int = ..., - baudrate: int = ..., - bytesize: int = ..., - parity: str = ..., - stopbits: float = ..., - timeout: float | None = ..., - xonxoff: bool = ..., - rtscts: bool = ..., - write_timeout: float | None = ..., - dsrdtr: bool = ..., - inter_byte_timeout: float | None = ..., - exclusive: bool | None = ..., - ) -> None: ... - - @overload - @deprecated( - "max_smp_encoded_frame_size, line_length, and line_buffers are deprecated; pass a " - "fragmentation_strategy (Auto, BufferSize, or BufferParams) instead." - ) def __init__( self, - max_smp_encoded_frame_size: int, - line_length: int = ..., - line_buffers: int = ..., - /, + fragmentation_strategy: SerialFragmentationStrategy = Auto(), *, - baudrate: int = ..., - bytesize: int = ..., - parity: str = ..., - stopbits: float = ..., - timeout: float | None = ..., - xonxoff: bool = ..., - rtscts: bool = ..., - write_timeout: float | None = ..., - dsrdtr: bool = ..., - inter_byte_timeout: float | None = ..., - exclusive: bool | None = ..., - ) -> None: ... - - def __init__( # noqa: DOC301 - self, - fragmentation_strategy: FragmentationStrategy | int | None = None, - line_length: int | None = None, - line_buffers: int | None = None, - *, - max_smp_encoded_frame_size: int | None = None, - baudrate: int = 115200, - bytesize: int = 8, - parity: str = "N", - stopbits: float = 1, - timeout: float | None = None, - xonxoff: bool = False, - rtscts: bool = False, - write_timeout: float | None = None, - dsrdtr: bool = False, - inter_byte_timeout: float | None = None, - exclusive: bool | None = None, + connect_timeout_s: float = 2.5, + sequence: Callable[[], Iterator[u8]] = _request.wrapping_sequence, + options: SerialOptions = SerialOptions(), ) -> None: """Initialize the serial transport. Args: - fragmentation_strategy: how to size SMP messages; one of `Auto` - (default), `BufferSize`, or `BufferParams`. - line_length: Deprecated; pass `BufferParams(line_length=...)` (or `BufferSize`). - line_buffers: Deprecated; pass `BufferParams(line_buffers=...)`. - max_smp_encoded_frame_size: Deprecated, but still honored for backward - compatibility -- it drives `mtu` exactly as in 7.1.0. Prefer an explicit - `BufferSize(buf_size=...)` (decoded netbuf) for new code. - baudrate: The baudrate of the serial connection. OK to ignore for - USB CDC ACM. - bytesize: The number of data bits. - parity: The parity setting. - stopbits: The number of stop bits. - timeout: The read timeout. - xonxoff: Enable software flow control. - rtscts: Enable hardware (RTS/CTS) flow control. - write_timeout: The write timeout. - dsrdtr: Enable hardware (DSR/DTR) flow control. - inter_byte_timeout: The inter-byte timeout. - exclusive: The exclusive access timeout. + fragmentation_strategy: How to size SMP messages. + connect_timeout_s: Bounds opening the port, and reading the server's MCUmgr + parameters. + sequence: The SMP sequence space the MCUmgr parameters read draws from. + options: The `pyserial` port settings. """ - super().__init__( - baudrate=baudrate, - bytesize=bytesize, - parity=parity, - stopbits=stopbits, - timeout=timeout, - xonxoff=xonxoff, - rtscts=rtscts, - write_timeout=write_timeout, - dsrdtr=dsrdtr, - inter_byte_timeout=inter_byte_timeout, - exclusive=exclusive, - ) - - self._fragmentation_strategy: Final = self._resolve_fragmentation_strategy( - fragmentation_strategy, max_smp_encoded_frame_size, line_length, line_buffers - ) + self._validate_strategy(fragmentation_strategy) + super().__init__(fragmentation_strategy, connect_timeout_s, sequence, options) self._smp_packet_queue: asyncio.Queue[bytes] = asyncio.Queue() """Contains full SMP packets.""" @@ -327,83 +176,14 @@ def __init__( # noqa: DOC301 logger.debug(f"Initialized {self.__class__.__name__}") @staticmethod - def _resolve_fragmentation_strategy( - fragmentation_strategy: FragmentationStrategy | int | None, - max_smp_encoded_frame_size: int | None, - line_length: int | None, - line_buffers: int | None, - ) -> _ResolvedStrategy: - """Normalize the constructor inputs into a fragmentation strategy. - - An explicit `fragmentation_strategy` always wins; it is validated, and any - stray deprecated args passed alongside it are logged and ignored. Otherwise - the deprecated 7.1.0 params -- `max_smp_encoded_frame_size`, `line_length`, - `line_buffers`, or a legacy positional `int` frame size -- reproduce 7.1.0 - exactly via `_LegacyParams` (`mtu == max_smp_encoded_frame_size`, defaulting to - the 7.1.0 256/128/2) and emit a `DeprecationWarning`. A frame size that - disagrees with `line_length * line_buffers` is logged at the level 7.1.0 used, - but -- as in 7.1.0 -- the explicit frame size still drives `mtu`. - """ - if not isinstance(fragmentation_strategy, int) and fragmentation_strategy is not None: - ignored: Final = { - name: value - for name, value in ( - ("max_smp_encoded_frame_size", max_smp_encoded_frame_size), - ("line_length", line_length), - ("line_buffers", line_buffers), - ) - if value is not None - } - if ignored: - logger.warning( - f"explicit fragmentation_strategy={fragmentation_strategy!r} takes " - f"precedence; ignoring deprecated {ignored}" - ) - SMPSerialTransport._validate_strategy(fragmentation_strategy) - return fragmentation_strategy - - legacy_frame: Final = ( - max_smp_encoded_frame_size - if max_smp_encoded_frame_size is not None - else (fragmentation_strategy if isinstance(fragmentation_strategy, int) else None) - ) - if legacy_frame is None and line_length is None and line_buffers is None: - return Auto() - - warnings.warn(_LEGACY_PARAMS_DEPRECATION, DeprecationWarning, stacklevel=3) - resolved_frame: Final = _LEGACY_FRAME_SIZE if legacy_frame is None else legacy_frame - resolved_line_length: Final = _DEFAULT_LINE_LENGTH if line_length is None else line_length - resolved_line_buffers: Final = ( - _LEGACY_LINE_BUFFERS if line_buffers is None else line_buffers - ) - budget: Final = resolved_line_length * resolved_line_buffers - if resolved_frame < budget: - logger.error( - f"max_smp_encoded_frame_size={resolved_frame} is less than " - f"line_length={resolved_line_length} * line_buffers={resolved_line_buffers}!" - ) - elif resolved_frame != budget: - logger.warning( - f"max_smp_encoded_frame_size={resolved_frame} is not equal to " - f"line_length={resolved_line_length} * line_buffers={resolved_line_buffers}!" - ) - return _LegacyParams( - max_smp_encoded_frame_size=resolved_frame, - line_length=resolved_line_length, - line_buffers=resolved_line_buffers, - ) - - @staticmethod - def _validate_strategy(strategy: FragmentationStrategy) -> None: + def _validate_strategy(strategy: SerialFragmentationStrategy) -> None: """Raise `ValueError` for a modern strategy that cannot carry a message. Guards `BufferSize`/`BufferParams` against configs that would otherwise fail far downstream: a `line_length` too small for `smppacket.encode` to make progress (it would emit empty packets forever), a `buf_size` at or below the frame overhead, or - an encoded budget too small for a single byte. The deprecated 7.1.0 params are - intentionally *not* validated -- `_LegacyParams` reproduces 7.1.0 behavior, latent - edge cases and all. `Auto` defers to `initialize`, where the server's advertised - buffer size is known. + an encoded budget too small for a single byte. `Auto` defers to `negotiate`, where + the server's advertised buffer size is known. """ match strategy: case Auto(): @@ -447,15 +227,13 @@ def _reset_state(self) -> None: @property def _line_length(self) -> int: """The base64 line length used to fragment outgoing frames.""" - match self._fragmentation_strategy: + match self._sizing: case Auto(): return _DEFAULT_LINE_LENGTH case BufferSize(line_length=line_length): return line_length case BufferParams(line_length=line_length): return line_length - case _LegacyParams(line_length=line_length): - return line_length case _ as unreachable: assert_never(unreachable) @@ -463,88 +241,59 @@ def _line_length(self) -> int: def _line_buffers(self) -> int: """The number of encoded line buffers spanned by the configured budget. - Meaningful for `BufferParams`/legacy params, where it sets the encoded budget. + Meaningful for `BufferParams`, where it sets the encoded budget. For the decoded-netbuf strategies (`Auto`/`BufferSize`) it is a diagnostic line count, clamped to at least 1 (never the misleading `0` of a sub-`line_length` - buffer); `Auto` falls back to the conservative legacy default until the server's + buffer); `Auto` falls back to a conservative default until the server's params are read. """ - match self._fragmentation_strategy: + match self._sizing: case Auto(): - if self._smp_server_transport_buffer_size is not None: - return max(1, self._smp_server_transport_buffer_size // self._line_length) - return _LEGACY_LINE_BUFFERS + return _AUTO_LINE_BUFFERS case BufferSize(buf_size=buf_size): return max(1, buf_size // self._line_length) case BufferParams(line_buffers=line_buffers): return line_buffers - case _LegacyParams(line_buffers=line_buffers): - return line_buffers case _ as unreachable: assert_never(unreachable) @property def _max_smp_encoded_frame_size(self) -> int: """The configured buffer size that the MTU reports.""" - match self._fragmentation_strategy: + match self._sizing: case Auto(): - if self._smp_server_transport_buffer_size is not None: - return self._smp_server_transport_buffer_size return self._line_length * self._line_buffers case BufferSize(buf_size=buf_size): return buf_size case BufferParams(line_length=line_length, line_buffers=line_buffers): return line_length * line_buffers - case _LegacyParams(max_smp_encoded_frame_size=frame_size): - return frame_size case _ as unreachable: assert_never(unreachable) @override - def initialize(self, smp_server_transport_buffer_size: int) -> None: - """Initialize with the server's buffer size from MCUMGR_PARAM. - - Args: - smp_server_transport_buffer_size: The server's CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE - - Raises: - ValueError: in `Auto` mode, if the server's advertised buffer is too small to - hold a framed message (`<= ` the frame overhead). - """ - super().initialize(smp_server_transport_buffer_size) - + async def negotiate(self) -> None: + """For `Auto`, adopt the server's `buf_size`; `ValueError` if it can't hold a frame.""" match self._fragmentation_strategy: case Auto(): - if smp_server_transport_buffer_size <= _FRAME_OVERHEAD: - raise ValueError( - f"server buffer size ({smp_server_transport_buffer_size}) must exceed " - f"the {_FRAME_OVERHEAD}-byte frame overhead to carry a message" - ) - logger.info( - f"Auto-configured from server buf_size={smp_server_transport_buffer_size}: " - f"mtu={self.mtu}, max_unencoded_size={self.max_unencoded_size}, " - f"line_length={self._line_length}" - ) - case BufferSize(buf_size=buf_size): - if buf_size > smp_server_transport_buffer_size: - logger.warning( - f"BufferSize buf_size ({buf_size}) exceeds the server's advertised " - f"buffer size ({smp_server_transport_buffer_size})" - ) - case BufferParams(line_length=line_length, line_buffers=line_buffers): - calculated_size = line_length * line_buffers - if calculated_size > smp_server_transport_buffer_size: - logger.warning( - f"BufferParams (line_length={line_length} * " - f"line_buffers={line_buffers} = {calculated_size}) " - f"exceeds server buffer size ({smp_server_transport_buffer_size})" - ) - case _LegacyParams(max_smp_encoded_frame_size=frame_size): - if frame_size > smp_server_transport_buffer_size: - logger.warning( - f"deprecated max_smp_encoded_frame_size ({frame_size}) exceeds the " - f"server's advertised buffer size ({smp_server_transport_buffer_size})" - ) + match await self._read_buf_size(): + case None: + self._sizing = Auto() + case int() as buf_size if buf_size <= _FRAME_OVERHEAD: + raise ValueError( + f"server buffer size ({buf_size}) must exceed the " + f"{_FRAME_OVERHEAD}-byte frame overhead to carry a message" + ) + case int() as buf_size: + self._sizing = BufferSize(buf_size=buf_size) + logger.info( + f"Auto-configured from server buf_size={buf_size}: " + f"mtu={self.mtu}, max_unencoded_size={self.max_unencoded_size}, " + f"line_length={self._line_length}" + ) + case _ as unreachable: + assert_never(unreachable) + case BufferSize() | BufferParams(): + pass case _ as unreachable: assert_never(unreachable) @@ -726,24 +475,19 @@ def max_unencoded_size(self) -> int: (Verified against native_sim/QEMU/mps2: a `buf_size - 4` message round-trips; `buf_size - 3` is dropped.) - `BufferParams`, the deprecated 7.1.0 params, and `Auto` before initialization - instead bound the message by an *encoded* line-buffer budget: how many + `BufferParams`, and `Auto` before initialization, instead bound the message by an *encoded* line-buffer budget: how many unencoded bytes survive base64 expansion and per-line framing within `mtu`. SMP serial framing (the 2-byte length + 2-byte CRC16): https://docs.zephyrproject.org/latest/services/device_mgmt/smp_transport.html """ - match self._fragmentation_strategy: + match self._sizing: case Auto(): - if self._smp_server_transport_buffer_size is not None: - return self._smp_server_transport_buffer_size - _FRAME_OVERHEAD return self._encoded_budget_max_unencoded_size() case BufferSize(buf_size=buf_size): return buf_size - _FRAME_OVERHEAD case BufferParams(): return self._encoded_budget_max_unencoded_size() - case _LegacyParams(): - return self._encoded_budget_max_unencoded_size() case _ as unreachable: assert_never(unreachable) diff --git a/src/smpclient/transport/serial/unencoded.py b/src/smpclient/transport/serial/unencoded.py index 6cd488b..39b5174 100644 --- a/src/smpclient/transport/serial/unencoded.py +++ b/src/smpclient/transport/serial/unencoded.py @@ -11,76 +11,60 @@ `smpclient.transport.serial.encoded`. """ +from __future__ import annotations + import asyncio import logging -from typing import Final +from collections.abc import Callable, Iterator +from typing import TYPE_CHECKING, Final, TypeAlias from smp import header as smphdr -from typing_extensions import override +from typing_extensions import assert_never, override +from smpclient import _request from smpclient.exceptions import SMPClientException -from smpclient.transport.serial.common import _SerialTransportBase +from smpclient.transport import Auto, BufferSize +from smpclient.transport.serial.common import SerialOptions, _SerialTransportBase from smpclient.transport.serial.framing import SerialFraming +if TYPE_CHECKING: + from types_bits import u8 + logger = logging.getLogger(__name__) -class SMPSerialRawTransport(_SerialTransportBase): +_DEFAULT_BUF_SIZE: Final = 384 +"""The server buffer `Auto` assumes until it reads the MCUmgr parameters: Zephyr's default +`CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE`.""" + +RawSerialFragmentationStrategy: TypeAlias = Auto | BufferSize +"""How `SMPSerialRawTransport` sizes SMP messages.""" + + +class SMPSerialRawTransport(_SerialTransportBase[RawSerialFragmentationStrategy]): def __init__( self, - mtu: int = 384, + fragmentation_strategy: RawSerialFragmentationStrategy = Auto(), *, framing: SerialFraming | None = None, - baudrate: int = 115200, - bytesize: int = 8, - parity: str = "N", - stopbits: float = 1, - timeout: float | None = None, - xonxoff: bool = False, - rtscts: bool = False, - write_timeout: float | None = None, - dsrdtr: bool = False, - inter_byte_timeout: float | None = None, - exclusive: bool | None = None, + connect_timeout_s: float = 2.5, + sequence: Callable[[], Iterator[u8]] = _request.wrapping_sequence, + options: SerialOptions = SerialOptions(), ) -> None: """Initialize the raw serial transport. Args: - mtu: The maximum size of one SMP message (header + payload), in - bytes. A serial link has no MTU of its own, but the SMP - server's receive buffer does -- this should match the server's - `CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE` (Zephyr default 384). + fragmentation_strategy: How to size one SMP message (header + payload). A + serial link has no MTU of its own, but the SMP server's receive buffer + (`CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE`) does. framing: optional wire framing for each SMP message (e.g. `Cobs()`); `None` sends the bare `[header][payload]`. - baudrate: The baudrate of the serial connection. OK to ignore for - USB CDC ACM. - bytesize: The number of data bits. - parity: The parity setting. - stopbits: The number of stop bits. - timeout: The read timeout. - xonxoff: Enable software flow control. - rtscts: Enable hardware (RTS/CTS) flow control. - write_timeout: The write timeout. - dsrdtr: Enable hardware (DSR/DTR) flow control. - inter_byte_timeout: The inter-byte timeout. - exclusive: Set exclusive access mode (POSIX only). A port cannot be - opened in exclusive access mode if it is already open in - exclusive access mode. + connect_timeout_s: Bounds opening the port, and reading the server's MCUmgr + parameters. + sequence: The SMP sequence space the MCUmgr parameters read draws from. + options: The `pyserial` port settings. """ - super().__init__( - baudrate=baudrate, - bytesize=bytesize, - parity=parity, - stopbits=stopbits, - timeout=timeout, - xonxoff=xonxoff, - rtscts=rtscts, - write_timeout=write_timeout, - dsrdtr=dsrdtr, - inter_byte_timeout=inter_byte_timeout, - exclusive=exclusive, - ) - self._mtu: Final = mtu + super().__init__(fragmentation_strategy, connect_timeout_s, sequence, options) self._framing: Final = framing logger.debug(f"Initialized {self.__class__.__name__}") @@ -173,7 +157,34 @@ async def _poll_read_into(self, buf: bytearray) -> None: else: await asyncio.sleep(self._POLLING_INTERVAL_S) + @override + async def negotiate(self) -> None: + match self._fragmentation_strategy: + case Auto(): + match await self._read_buf_size(): + case None: + self._sizing = Auto() + case int() as buf_size: + self._sizing = BufferSize(buf_size) + case _ as unreachable: + assert_never(unreachable) + case BufferSize(): + pass + case _ as unreachable: + assert_never(unreachable) + @property @override def mtu(self) -> int: - return self._mtu + return self.max_unencoded_size + + @property + @override + def max_unencoded_size(self) -> int: + match self._sizing: + case Auto(): + return _DEFAULT_BUF_SIZE + case BufferSize(buf_size=buf_size): + return buf_size + case _ as unreachable: + assert_never(unreachable) diff --git a/src/smpclient/transport/udp.py b/src/smpclient/transport/udp.py index 9e4e568..6e63ba0 100644 --- a/src/smpclient/transport/udp.py +++ b/src/smpclient/transport/udp.py @@ -1,17 +1,25 @@ """A UDP SMPTransport for Network connections like Wi-Fi or Ethernet.""" +from __future__ import annotations + import asyncio import logging +from collections.abc import AsyncIterator, Callable, Iterator +from contextlib import asynccontextmanager from socket import AF_INET6 -from typing import Final +from typing import TYPE_CHECKING, Final, TypeAlias from smp import header as smphdr -from typing_extensions import override +from typing_extensions import Self, assert_never, override +from smpclient import _request from smpclient.exceptions import SMPClientException -from smpclient.transport import SMPTransport +from smpclient.transport import Auto, BufferSize, _ConnectableTransport from smpclient.transport._udp_client import Addr, UDPClient +if TYPE_CHECKING: + from types_bits import u8 + logger = logging.getLogger(__name__) IPV4_HEADER_SIZE: Final = 20 @@ -36,30 +44,60 @@ PMTU to avoid fragmentation.""" -class SMPUDPTransport(SMPTransport): - def __init__(self, mtu: int = 1500) -> None: +UDPFragmentationStrategy: TypeAlias = Auto | BufferSize +"""How `SMPUDPTransport` sizes SMP messages.""" + + +class SMPUDPTransport(_ConnectableTransport[UDPFragmentationStrategy]): + def __init__( + self, + mtu: int = 1500, + *, + fragmentation_strategy: UDPFragmentationStrategy = Auto(), + connect_timeout_s: float = 2.5, + sequence: Callable[[], Iterator[u8]] = _request.wrapping_sequence, + ) -> None: """Initialize the SMP UDP transport. Args: mtu: The Maximum Transmission Unit (MTU) of the link layer in bytes. IP and UDP header overhead will be subtracted to calculate the maximum UDP payload size (MSS) to avoid fragmentation per RFC 8085 section 3.2. + fragmentation_strategy: How to size SMP messages. + connect_timeout_s: Bounds connecting, and reading the server's MCUmgr + parameters. + sequence: The SMP sequence space the MCUmgr parameters read draws from. """ - self._mtu = mtu + super().__init__(fragmentation_strategy, connect_timeout_s, sequence) + self._mtu: Final = mtu self._is_ipv6 = False self._client: Final = UDPClient() - @override - async def connect(self, address: str, timeout_s: float, port: int = 1337) -> None: + async def connect(self, address: str, port: int = 1337) -> None: + """Connect to `address`:`port`, then `negotiate()`; prefer `connected()`.""" logger.debug(f"Connecting to {address=} {port=}") - await asyncio.wait_for(self._client.connect(Addr(host=address, port=port)), timeout_s) + await asyncio.wait_for( + self._client.connect(Addr(host=address, port=port)), self._connect_timeout_s + ) if sock := self._client._transport.get_extra_info('socket'): self._is_ipv6 = sock.family == AF_INET6 logger.debug(f"Detected {'IPv6' if self._is_ipv6 else 'IPv4'} connection") logger.info(f"Connected to {address=} {port=}") + try: + await self.negotiate() + except (Exception, asyncio.CancelledError): + await self.disconnect() + raise + + @asynccontextmanager + async def connected(self, address: str, port: int = 1337) -> AsyncIterator[Self]: + """Connect to `address`:`port` for the duration of the `async with`, then disconnect.""" + await self.connect(address, port) + async with self._released_on_exit(): + yield self @override async def disconnect(self) -> None: @@ -131,13 +169,35 @@ async def send_and_receive(self, data: bytes) -> bytes: def mtu(self) -> int: return self._mtu + @override + async def negotiate(self) -> None: + match self._fragmentation_strategy: + case Auto(): + match await self._read_buf_size(): + case None: + self._sizing = Auto() + case int() as buf_size: + self._sizing = BufferSize(buf_size) + case _ as unreachable: + assert_never(unreachable) + case BufferSize(): + pass + case _ as unreachable: + assert_never(unreachable) + @property @override def max_unencoded_size(self) -> int: - """Maximum UDP payload size (MSS) to avoid fragmentation. + """Maximum UDP payload size (MSS) to avoid fragmentation, capped at the server's buffer. Subtracts IPv4/IPv6 and UDP header overhead from MTU per RFC 8085 section 3.2. The IP version is auto-detected after connection. """ - overhead = IPV6_UDP_OVERHEAD if self._is_ipv6 else IPV4_UDP_OVERHEAD - return self._mtu - overhead + mss: Final = self._mtu - (IPV6_UDP_OVERHEAD if self._is_ipv6 else IPV4_UDP_OVERHEAD) + match self._sizing: + case Auto(): + return mss + case BufferSize(buf_size=buf_size): + return min(mss, buf_size) + case _ as unreachable: + assert_never(unreachable) diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..52b377c --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,14 @@ +"""Fixtures shared by the unit tests.""" + +from collections.abc import Generator +from typing import Any +from unittest.mock import AsyncMock, patch + +import pytest + + +@pytest.fixture +def skip_negotiation() -> Generator[AsyncMock, Any, None]: + """Answer every transport's MCUmgr parameters read with `None`, as a server without them.""" + with patch("smpclient._request.read_mcumgr_parameters", AsyncMock(return_value=None)) as read: + yield read diff --git a/tests/extensions/test_intercreate.py b/tests/extensions/test_intercreate.py index 00d7c16..cc45bb0 100644 --- a/tests/extensions/test_intercreate.py +++ b/tests/extensions/test_intercreate.py @@ -24,7 +24,7 @@ async def test_upload_hello_world_bin_encoded(mock_mtu: PropertyMock) -> None: image = f.read() m = SMPSerialTransport() - s = ICUploadClient(m, "address") + s = ICUploadClient(m) assert s._transport.mtu == 127 assert s._transport.max_unencoded_size < 127 diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 645e250..23f3440 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -13,9 +13,9 @@ import logging import re from collections.abc import AsyncIterator, Awaitable, Callable -from contextlib import asynccontextmanager +from contextlib import AbstractAsyncContextManager, AsyncExitStack, asynccontextmanager from pathlib import Path -from typing import NamedTuple +from typing import Final, NamedTuple import pytest import pytest_asyncio @@ -26,19 +26,17 @@ from smpclient import SMPClient, success from smpclient.exceptions import SMPBadSequence -from smpclient.transport import SMPTransport from smpclient.transport.serial import SMPSerialRawTransport, SMPSerialTransport from smpclient.transport.udp import SMPUDPTransport from tests.integration.servers import ( FIXTURES, Endpoint, PtyEndpoint, - QemuSocketSerialRawTransport, - QemuSocketSerialTransport, ServerFixture, SocketSerialEndpoint, UdpEndpoint, serve, + socket_link, ) logger = logging.getLogger(__name__) @@ -46,12 +44,17 @@ _READY_PROBE = "smpclient-integration-ready" +FixtureTransport = SMPSerialTransport | SMPSerialRawTransport | SMPUDPTransport + + class ConnectedServer(NamedTuple): - """A live `SMPClient`, the `ServerFixture` it is connected to, and its `Endpoint`.""" + """A live `SMPClient`, its transport, its `ServerFixture`, `Endpoint`, and open link.""" client: SMPClient + transport: FixtureTransport fixture: ServerFixture endpoint: Endpoint + link: AsyncExitStack def fixture_params( @@ -71,18 +74,16 @@ def fixture_params( ] -_SMP_UDP_DEFAULT_PORT = 1337 -"""`SMPUDPTransport.connect`'s default port; `SMPClient.connect` cannot override it.""" - - -def _build_transport(fixture: ServerFixture, endpoint: Endpoint) -> tuple[SMPTransport, str]: +def _link( + fixture: ServerFixture, endpoint: Endpoint +) -> AbstractAsyncContextManager[FixtureTransport]: match endpoint: case PtyEndpoint(pty): match fixture.transport: case "serial" | "shell": - return SMPSerialTransport(), pty + return SMPSerialTransport().connected(pty) case "serial_raw": - return SMPSerialRawTransport(), pty + return SMPSerialRawTransport().connected(pty) case "udp": pytest.fail("UDP fixtures do not present as a PTY serial endpoint") case _ as unreachable: @@ -90,20 +91,15 @@ def _build_transport(fixture: ServerFixture, endpoint: Endpoint) -> tuple[SMPTra case SocketSerialEndpoint(url): match fixture.transport: case "serial" | "shell": - return QemuSocketSerialTransport(url), url + return socket_link(SMPSerialTransport(), url) case "serial_raw": - return QemuSocketSerialRawTransport(url), url + return socket_link(SMPSerialRawTransport(), url) case "udp": pytest.fail("UDP fixtures do not present as a socket serial endpoint") case _ as unreachable: assert_never(unreachable) case UdpEndpoint(host, port): - if port != _SMP_UDP_DEFAULT_PORT: - pytest.skip( - f"UDP fixture port {port} is unreachable: SMPClient.connect cannot pass a " - f"non-default UDP port (only {_SMP_UDP_DEFAULT_PORT})" - ) - return SMPUDPTransport(), host + return SMPUDPTransport().connected(host, port) case _: assert_never(endpoint) @@ -146,7 +142,7 @@ async def echoes(c: SMPClient) -> bool: return success(response) and response.r == _READY_PROBE if not await _poll_until_answering(client, echoes, attempts=attempts): - raise TimeoutError(f"{client.address} never answered an echo") + raise TimeoutError("the SMP server never answered an echo") def signed_image(fixture: ServerFixture) -> Path: @@ -233,16 +229,16 @@ def assert_chunks_maximized( @asynccontextmanager async def reboot_into_recovery( - app_client: SMPClient, - transport: SMPSerialTransport | SMPSerialRawTransport, - address: str, + app: ConnectedServer, + recovery: AbstractAsyncContextManager[SMPSerialTransport | SMPSerialRawTransport], ) -> AsyncIterator[SMPClient]: """Reboot the device into MCUboot serial recovery and yield a recovery-connected client. - The app at `app_client` reboots via `os reset boot_mode=BOOTLOADER` (smp 4.1.0); - `transport` then connects to the bootloader on the same serial endpoint, probed until - it answers (the recovery server speaks the img group, not echo). + The app at `app` reboots via `os reset boot_mode=BOOTLOADER` (smp 4.1.0) and its link + closes; the `recovery` link then opens to the bootloader on the same serial endpoint, + probed until it answers (the recovery server speaks the img group, not echo). """ + app_client: Final = app.client assert success(await app_client.request(ImageStatesReadRequest())) try: assert success( @@ -252,40 +248,31 @@ async def reboot_into_recovery( ) except TimeoutError: pass # some servers reset before sending the response - await app_client.disconnect() + await app.link.aclose() await asyncio.sleep(2.0) # let MCUboot serial recovery come up async def lists_images(c: SMPClient) -> bool: return success(await c.request(ImageStatesReadRequest(), timeout_s=1.0)) - bootloader = SMPClient(transport, address) - await bootloader.connect() - try: + async with recovery as transport: + bootloader = SMPClient(transport) if not await _poll_until_answering(bootloader, lists_images, interval_s=0.2): pytest.fail("MCUboot serial recovery SMP server never answered") yield bootloader - finally: - await bootloader.disconnect() @asynccontextmanager async def connected(fixture: ServerFixture) -> AsyncIterator[ConnectedServer]: """Launch `fixture`, connect an `SMPClient`, and wait until the server answers.""" - async with serve(fixture) as endpoint: - transport, address = _build_transport(fixture, endpoint) - client = SMPClient(transport, address) - await client.connect() + async with serve(fixture) as endpoint, AsyncExitStack() as link: + # Tolerant on exit: `connected()` closes best-effort, and a recovery test may have + # rebooted the server out from under us. + transport = await link.enter_async_context(_link(fixture, endpoint)) + client = SMPClient(transport) await _wait_until_answering(client) - # Re-initialize in case the first MCUMgr parameter read raced server boot. - await client._initialize() - try: - yield ConnectedServer(client, fixture, endpoint) - finally: - # Tolerant: a recovery test may have rebooted the server out from under us. - try: - await client.disconnect() - except Exception as e: - logger.debug(f"disconnect during teardown failed: {e}") + # Re-negotiate in case the first MCUMgr parameter read raced server boot. + await transport.negotiate() + yield ConnectedServer(client, transport, fixture, endpoint, link) @pytest_asyncio.fixture(params=fixture_params()) diff --git a/tests/integration/servers.py b/tests/integration/servers.py index edde8d1..ac553f1 100644 --- a/tests/integration/servers.py +++ b/tests/integration/servers.py @@ -33,19 +33,14 @@ from contextlib import asynccontextmanager, closing from hashlib import sha256 from pathlib import Path -from typing import TYPE_CHECKING, Final, Literal, NamedTuple +from typing import TYPE_CHECKING, Final, Literal, NamedTuple, TypeVar import serial as pyserial from serial.urlhandler.protocol_socket import Serial as _SocketSerial -from typing_extensions import override +from typing_extensions import assert_never, override from smpclient.transport import SMPTransportDisconnected -from smpclient.transport.serial import ( - FragmentationStrategy, - SerialFraming, - SMPSerialRawTransport, - SMPSerialTransport, -) +from smpclient.transport.serial import SMPSerialRawTransport, SMPSerialTransport if TYPE_CHECKING: from _typeshed import ReadableBuffer @@ -286,84 +281,47 @@ def write(self, b: ReadableBuffer, /) -> int: FIXTURES: Final = _load_fixtures() -async def _connect_socket_chardev( - transport: SMPSerialTransport | SMPSerialRawTransport, - url: str, - timeout_s: float, - chardev: type[_SocketChardev] = _SocketChardev, -) -> None: - """Back `transport` with an emulator's `socket://` serial chardev, retrying until it accepts. +_CHARDEV_CONNECT_TIMEOUT_S: Final = 2.5 +_CHARDEV_RETRY_INTERVAL_S: Final = 0.5 - Replaces the `Final` pyserial `_conn` with a socket-backed `Serial`, sidestepping the - PTY held-byte quirk of an emulated UART. Shared by the encoded and raw socket - transports, which differ only in their on-wire framing. +_LinkedTransport = TypeVar("_LinkedTransport", bound=SMPSerialTransport | SMPSerialRawTransport) - Args: - transport: the socket-backed serial transport whose `_conn` to (re)bind. - url: the emulator's `socket://host:port` chardev URL. - timeout_s: how long to keep retrying before the socket must have accepted. - chardev: the chardev class to bind; `_PacedSocketChardev` for the raw transport. - Raises: - TimeoutError: if the emulator's serial socket never accepts within `timeout_s`. - """ - transport._reset_state() +def _chardev_for(transport: SMPSerialTransport | SMPSerialRawTransport) -> type[_SocketChardev]: + match transport: + case SMPSerialRawTransport(): + return _PacedSocketChardev + case SMPSerialTransport(): + return _SocketChardev + case _ as unreachable: + assert_never(unreachable) + + +async def _open_socket_chardev(url: str, chardev: type[_SocketChardev]) -> _SocketChardev: + """Open an emulator's `socket://` serial chardev, retrying until it accepts.""" loop = asyncio.get_running_loop() - deadline = loop.time() + timeout_s + deadline = loop.time() + _CHARDEV_CONNECT_TIMEOUT_S while True: try: - conn = chardev(url, timeout=0, write_timeout=_WRITE_TIMEOUT_S) + return chardev(url, timeout=0, write_timeout=_WRITE_TIMEOUT_S) except (OSError, pyserial.SerialException) as e: if loop.time() >= deadline: raise TimeoutError(f"emulator serial socket {url} never accepted: {e}") - await asyncio.sleep(transport._CONNECTION_RETRY_INTERVAL_S) - continue - # `_conn` is `Final` on the base class; replace it for the socket backend. - object.__setattr__(transport, "_conn", conn) - logger.debug(f"Connected to {url}") - return - + await asyncio.sleep(_CHARDEV_RETRY_INTERVAL_S) -class QemuSocketSerialTransport(SMPSerialTransport): - """`SMPSerialTransport` whose byte pipe is a TCP socket (an emulator's serial chardev). - - Only `connect` differs -- it binds a `socket://` chardev instead of a local serial - port, sidestepping the PTY held-byte quirk of an emulated UART. Framing, - fragmentation, `send`, and `receive` are inherited unchanged, so the suite exercises - the real transport rather than a copy of it. - """ - def __init__( # noqa: DOC301 - self, - url: str, - fragmentation_strategy: FragmentationStrategy | None = None, - ) -> None: - if fragmentation_strategy is None: - super().__init__() - else: - super().__init__(fragmentation_strategy=fragmentation_strategy) - self._url: Final = url - - @override - async def connect(self, address: str, timeout_s: float) -> None: - await _connect_socket_chardev(self, self._url, timeout_s) - - -class QemuSocketSerialRawTransport(SMPSerialRawTransport): - """`SMPSerialRawTransport` whose byte pipe is a TCP socket (an emulator's serial chardev). +@asynccontextmanager +async def socket_link(transport: _LinkedTransport, url: str) -> AsyncIterator[_LinkedTransport]: + """Lend `transport` an emulator's `socket://` serial chardev, closing it on exit. - The raw counterpart of `QemuSocketSerialTransport`: only `connect` differs; the raw - `[header][payload]` framing, `send`, and `receive` are inherited from - `SMPSerialRawTransport` unchanged. + The socket sidesteps the PTY held-byte quirk of an emulated UART. The transport + borrows it, so its framing, fragmentation, `send`, and `receive` run unchanged -- the + suite exercises the real transport rather than a copy of it. """ - - def __init__(self, url: str, mtu: int = 384, framing: SerialFraming | None = None) -> None: # noqa: DOC301 - super().__init__(mtu=mtu, framing=framing) - self._url: Final = url - - @override - async def connect(self, address: str, timeout_s: float) -> None: - await _connect_socket_chardev(self, self._url, timeout_s, _PacedSocketChardev) + with closing(await _open_socket_chardev(url, _chardev_for(transport))) as chardev: + async with transport.borrowed(chardev): + logger.debug(f"Borrowing {url}") + yield transport def _verify_sha256(artifact: Path) -> str | None: diff --git a/tests/integration/test_fragmentation.py b/tests/integration/test_fragmentation.py index a028cc3..4578c19 100644 --- a/tests/integration/test_fragmentation.py +++ b/tests/integration/test_fragmentation.py @@ -124,12 +124,10 @@ async def test_non_default_line_length(fixture: ServerFixture) -> None: """ async with serve(fixture) as endpoint: assert isinstance(endpoint, PtyEndpoint) - transport = SMPSerialTransport( - fragmentation_strategy=BufferParams(line_length=512, line_buffers=1) - ) - client = SMPClient(transport, endpoint.pty) - await client.connect() - try: + async with SMPSerialTransport(BufferParams(line_length=512, line_buffers=1)).connected( + endpoint.pty + ) as transport: + client = SMPClient(transport) await _wait_until_answering(client) assert transport._line_length == 512 @@ -141,5 +139,3 @@ async def test_non_default_line_length(fixture: ServerFixture) -> None: response = await client.request(request, timeout_s=10.0) assert success(response) assert response.r == text - finally: - await client.disconnect() diff --git a/tests/integration/test_serial_recovery.py b/tests/integration/test_serial_recovery.py index aae2b39..0adb81b 100644 --- a/tests/integration/test_serial_recovery.py +++ b/tests/integration/test_serial_recovery.py @@ -33,7 +33,8 @@ from typing_extensions import assert_never from smpclient import success -from smpclient.transport.serial import Auto, BufferSize, Cobs, SMPSerialTransport +from smpclient.transport import Auto +from smpclient.transport.serial import BufferSize, Cobs, SMPSerialRawTransport, SMPSerialTransport from smpclient.transport.serial.encoded import _FRAME_OVERHEAD from tests.integration.conftest import ( RECOVERY_UPLOAD_TIMEOUT_S, @@ -45,15 +46,14 @@ ) from tests.integration.servers import ( FIXTURES, - QemuSocketSerialRawTransport, - QemuSocketSerialTransport, ServerFixture, SocketSerialEndpoint, + socket_link, ) pytestmark = [pytest.mark.integration, pytest.mark.asyncio] -_SocketTransport = QemuSocketSerialTransport | QemuSocketSerialRawTransport +_SocketTransport = SMPSerialTransport | SMPSerialRawTransport class Console(NamedTuple): @@ -99,14 +99,14 @@ def _fixture(variant: _Recovery) -> tuple[str, str]: assert_never(unreachable) -def _build_transport(variant: _Recovery, url: str) -> _SocketTransport: +def _build_transport(variant: _Recovery) -> _SocketTransport: match variant: case Console(strategy=strategy): - return QemuSocketSerialTransport(url, fragmentation_strategy=strategy) + return SMPSerialTransport(strategy) case Raw(): - return QemuSocketSerialRawTransport(url) + return SMPSerialRawTransport() case RawCobs(): - return QemuSocketSerialRawTransport(url, framing=Cobs()) + return SMPSerialRawTransport(framing=Cobs()) case _ as unreachable: assert_never(unreachable) @@ -168,10 +168,11 @@ async def test_upload_to_mcuboot_recovery(variant: _Recovery, fixture: ServerFix async with connected(fixture) as cs: assert isinstance(cs.endpoint, SocketSerialEndpoint) - transport = _build_transport(variant, cs.endpoint.url) + transport = _build_transport(variant) - async with reboot_into_recovery(cs.client, transport, cs.endpoint.url) as bootloader: - await bootloader._initialize() # negotiate buf_size (a no-op for explicit BufferSize) + async with reboot_into_recovery(cs, socket_link(transport, cs.endpoint.url)) as bootloader: + # Re-negotiate in case the first read raced the bootloader coming up. + await transport.negotiate() params = await bootloader.request(MCUMgrParametersReadRequest(), timeout_s=2.0) assert success(params) diff --git a/tests/support.py b/tests/support.py new file mode 100644 index 0000000..16397df --- /dev/null +++ b/tests/support.py @@ -0,0 +1,29 @@ +"""Helpers shared by the transport tests.""" + +from collections.abc import Iterator +from contextlib import contextmanager +from typing import TypeVar +from unittest.mock import AsyncMock, patch + +from smp.os_management import MCUMgrParametersReadResponse + +from smpclient.transport import _ConnectableTransport + +_T = TypeVar("_T", bound=_ConnectableTransport) + + +@contextmanager +def advertise(buf_size: int) -> Iterator[AsyncMock]: + """Answer every transport's MCUmgr parameters read as a server whose buffer is `buf_size`.""" + with patch( + "smpclient._request.read_mcumgr_parameters", + AsyncMock(return_value=MCUMgrParametersReadResponse(buf_size=buf_size, buf_count=1)), + ) as read: + yield read + + +async def negotiated(transport: _T, buf_size: int) -> _T: + """`transport`, after negotiating against a server whose buffer is `buf_size`.""" + with advertise(buf_size): + await transport.negotiate() + return transport diff --git a/tests/test_generics_typing.py b/tests/test_generics_typing.py index c61f0c8..e6b09a9 100644 --- a/tests/test_generics_typing.py +++ b/tests/test_generics_typing.py @@ -21,6 +21,14 @@ from typing_extensions import assert_never, assert_type from smpclient import SMPClient, error, error_v1, error_v2, success +from smpclient.extensions.intercreate import ICUploadClient +from smpclient.transport.serial import SMPSerialTransport + + +def _check_client_keeps_its_transport_type(transport: SMPSerialTransport) -> None: + """A client's `transport` is the transport it was given, e.g. for `read_serial()`.""" + assert_type(SMPClient(transport).transport, SMPSerialTransport) + assert_type(ICUploadClient(transport).transport, SMPSerialTransport) async def _check_exhaustive_narrowing(client: SMPClient) -> None: diff --git a/tests/test_smp_ble_transport.py b/tests/test_smp_ble_transport.py index a911e9d..942f6ab 100644 --- a/tests/test_smp_ble_transport.py +++ b/tests/test_smp_ble_transport.py @@ -1,25 +1,34 @@ """Tests for `SMPBLETransport`.""" import asyncio -from typing import cast +from typing import Final, cast from unittest.mock import AsyncMock, MagicMock, patch from uuid import UUID import pytest from bleak import BleakClient +from bleak.args.bluez import BlueZClientArgs +from bleak.args.winrt import WinRTClientArgs from bleak.backends.characteristic import BleakGATTCharacteristic from bleak.backends.device import BLEDevice +from bleak.exc import BleakError from smp.os_management import EchoWriteResponse -from smpclient.transport import SMPTransportDisconnected +from smpclient.transport import BufferSize, SMPTransportDisconnected, Unfragmented from smpclient.transport.ble import ( MAC_ADDRESS_PATTERN, SMP_CHARACTERISTIC_UUID, SMP_SERVICE_UUID, UUID_PATTERN, + BleakBackend, + BlueZ, + PlatformDefault, SMPBLETransport, SMPBLETransportDeviceNotFound, + WinRT, + _Owned, ) +from tests.support import advertise, negotiated class MockBleakClient: @@ -31,6 +40,12 @@ def __new__(cls, *args, **kwargs) -> "MockBleakClient": # type: ignore return client +pytestmark = pytest.mark.usefixtures("skip_negotiation") + +ADDRESS = "00:00:00:00:00:00" +"""An address; bleak is mocked, so nothing is scanned for.""" + + def test_constructor() -> None: t = SMPBLETransport() assert t._buffer == bytearray() @@ -85,35 +100,34 @@ async def test_connect( mock_find_device_by_address: MagicMock, ) -> None: # assert that it searches by name if MAC or UUID is not provided - await SMPBLETransport().connect("device name", 1.0) - mock_find_device_by_name.assert_called_once_with("device name", timeout=1.0) + await SMPBLETransport(connect_timeout_s=1.0).connect("device name") + mock_find_device_by_name.assert_called_once_with("device name", timeout=1.0, bluez={}) mock_find_device_by_name.reset_mock() # assert that it searches by MAC if MAC is provided - await SMPBLETransport().connect("00:00:00:00:00:00", 1.0) - mock_find_device_by_address.assert_called_once_with("00:00:00:00:00:00", timeout=1.0) + await SMPBLETransport(connect_timeout_s=1.0).connect("00:00:00:00:00:00") + mock_find_device_by_address.assert_called_once_with("00:00:00:00:00:00", timeout=1.0, bluez={}) mock_find_device_by_address.reset_mock() # assert that it searches by UUID if UUID is provided - await SMPBLETransport().connect(UUID("00000000-0000-4000-8000-000000000000").hex, 1.0) + await SMPBLETransport(connect_timeout_s=1.0).connect( + UUID("00000000-0000-4000-8000-000000000000").hex + ) mock_find_device_by_address.assert_called_once_with( - "00000000000040008000000000000000", timeout=1.0 + "00000000000040008000000000000000", timeout=1.0, bluez={} ) mock_find_device_by_address.reset_mock() # assert that it raises an exception if the device is not found mock_find_device_by_address.return_value = None with pytest.raises(SMPBLETransportDeviceNotFound): - await SMPBLETransport().connect("00:00:00:00:00:00", 1.0) + await SMPBLETransport(connect_timeout_s=1.0).connect("00:00:00:00:00:00") mock_find_device_by_address.reset_mock() # assert that connect is awaited - t = SMPBLETransport() - await t.connect("name", 1.0) - t._client = cast(MagicMock, t._client) - t._client.reset_mock() - await t.connect("name", 1.0) - t._client.connect.assert_awaited_once_with() + t = SMPBLETransport(connect_timeout_s=1.0) + await t.connect("name") + _owned_client(t).connect.assert_awaited_once_with() # these are hard to mock now because the _client is created in the connect method # reenable these after the SMPTransport Protocol is updated to take address @@ -135,25 +149,205 @@ async def test_connect( # assert t._smp_characteristic is m # assert that SMP characteristic notifications are started - t._client.start_notify.assert_called_once_with(SMP_CHARACTERISTIC_UUID, t._notify_callback) + _owned_client(t).start_notify.assert_called_once_with( + SMP_CHARACTERISTIC_UUID, t._notify_callback + ) + + +@pytest.mark.parametrize( + "backend, bluez, winrt", + [ + pytest.param(PlatformDefault(), {}, {}, id="default"), + pytest.param(BlueZ(BlueZClientArgs(adapter="hci1")), {"adapter": "hci1"}, {}, id="bluez"), + pytest.param( + WinRT(WinRTClientArgs(use_cached_services=True)), + {}, + {"use_cached_services": True}, + id="winrt", + ), + ], +) +@patch( + "smpclient.transport.ble.BleakScanner.find_device_by_address", + return_value=BLEDevice(ADDRESS, "name", None), +) +@patch("smpclient.transport.ble.BleakClient", side_effect=MockBleakClient) +@pytest.mark.asyncio +async def test_connect_passes_bleak_only_the_chosen_backend( + mock_bleak_client: MagicMock, + mock_find_device_by_address: MagicMock, + backend: BleakBackend, + bluez: BlueZClientArgs, + winrt: WinRTClientArgs, +) -> None: + await SMPBLETransport(backend=backend, connect_timeout_s=1.0).connect(ADDRESS) + + mock_find_device_by_address.assert_called_once_with(ADDRESS, timeout=1.0, bluez=bluez) + assert mock_bleak_client.call_args.kwargs["bluez"] == bluez + assert mock_bleak_client.call_args.kwargs["winrt"] == winrt + + +@patch("smpclient.transport.ble.BleakScanner") +@pytest.mark.asyncio +async def test_scan_uses_the_bluez_adapter(mock_bleak_scanner: MagicMock) -> None: + mock_bleak_scanner.return_value.discover = AsyncMock(return_value={}) + + assert await SMPBLETransport.scan(backend=BlueZ(BlueZClientArgs(adapter="hci1"))) == [] + + mock_bleak_scanner.assert_called_once_with( + service_uuids=[str(SMP_SERVICE_UUID)], bluez={"adapter": "hci1"} + ) @pytest.mark.asyncio async def test_disconnect() -> None: + client: Final = MagicMock(spec=BleakClient) + t = SMPBLETransport() + t._link = _Owned(client) + + await t.disconnect() + await t.disconnect() + + client.disconnect.assert_awaited_once_with() + with pytest.raises(SMPTransportDisconnected): + await t.send(b"Hello pytest!") + + +def _owned_client(t: SMPBLETransport) -> MagicMock: + match t._link: + case _Owned(client=client): + return cast(MagicMock, client) + case _: + pytest.fail(f"expected an owned link, got {t._link}") + + +def _borrowable_client(max_write: int = 244) -> MagicMock: + """A caller's connected `BleakClient` that serves the SMP characteristic.""" + client = MagicMock(spec=BleakClient, name="BorrowedBleakClient") + client._backend = MockBleakClient.Backend() + client.address = ADDRESS + client.is_connected = True + client.services.get_characteristic.return_value = MagicMock( + spec=BleakGATTCharacteristic, max_write_without_response_size=max_write + ) + return client + + +@pytest.mark.asyncio +async def test_borrowed_subscribes_and_leaves_the_client_connected() -> None: + client: Final = _borrowable_client(max_write=244) t = SMPBLETransport() - t._client = MagicMock(spec=BleakClient) + + async with t.borrowed(client) as borrowed: + assert borrowed is t + assert t.mtu == 244 + await t.send(b"Hello pytest!") + + client.start_notify.assert_awaited_once_with(SMP_CHARACTERISTIC_UUID, t._notify_callback) + client.write_gatt_char.assert_awaited_once_with( + t._smp_characteristic, b"Hello pytest!", response=False + ) + client.stop_notify.assert_awaited_once_with(SMP_CHARACTERISTIC_UUID) + client.disconnect.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_a_returned_borrow_raises_disconnected() -> None: + t = SMPBLETransport() + async with t.borrowed(_borrowable_client()): + pass + + with pytest.raises(SMPTransportDisconnected): + await t.send(b"Hello pytest!") + with pytest.raises(SMPTransportDisconnected): + await asyncio.wait_for(t.receive(), timeout=1.0) await t.disconnect() - t._client.disconnect.assert_awaited_once_with() + + +@pytest.mark.asyncio +async def test_borrowed_receive_raises_once_the_client_disconnects() -> None: + """The owner holds the client's disconnect callback, so a borrowed wait polls instead.""" + client: Final = _borrowable_client() + t = SMPBLETransport() + + async with t.borrowed(client): + client.is_connected = False + with pytest.raises(SMPTransportDisconnected): + await asyncio.wait_for(t.receive(), timeout=1.0) + + +@pytest.mark.asyncio +async def test_borrowed_receive_leaves_no_task_polling_when_cancelled() -> None: + t = SMPBLETransport() + + async with t.borrowed(_borrowable_client()): + tasks_before: Final = asyncio.all_tasks() + receive: Final = asyncio.create_task(t.receive()) + await asyncio.sleep(0.01) # the receive is waiting on a notify or a disconnect + receive.cancel() + with pytest.raises(asyncio.CancelledError): + await receive + + assert asyncio.all_tasks() == tasks_before + + +@pytest.mark.asyncio +async def test_borrow_negotiates_the_fragmentation_strategy() -> None: + t = SMPBLETransport() + + with advertise(2048) as read_mcumgr_parameters: + await t.borrow(_borrowable_client()) + + read_mcumgr_parameters.assert_awaited_once() + assert t.max_unencoded_size == 2048 + + +@pytest.mark.asyncio +async def test_borrow_returns_the_client_when_negotiation_fails() -> None: + client: Final = _borrowable_client() + t = SMPBLETransport() + + with ( + patch( + "smpclient._request.read_mcumgr_parameters", + AsyncMock(side_effect=asyncio.CancelledError), + ), + pytest.raises(asyncio.CancelledError), + ): + await t.borrow(client) + + client.stop_notify.assert_awaited_once_with(SMP_CHARACTERISTIC_UUID) + client.disconnect.assert_not_awaited() + + +async def _never_returns(*_args: object) -> None: + await asyncio.Event().wait() + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "stop_notify", [BleakError("Not connected"), _never_returns], ids=["raises", "hangs"] +) +async def test_returning_a_borrowed_client_survives_its_unsubscribe(stop_notify: object) -> None: + client: Final = _borrowable_client() + client.stop_notify.side_effect = stop_notify + t = SMPBLETransport(connect_timeout_s=0.1) + + async with t.borrowed(client): + pass + + client.disconnect.assert_not_awaited() @pytest.mark.asyncio async def test_send() -> None: + client: Final = MagicMock(spec=BleakClient) t = SMPBLETransport() - t._client = MagicMock(spec=BleakClient) + t._link = _Owned(client) t._smp_characteristic = MagicMock(spec=BleakGATTCharacteristic) t._smp_characteristic.max_write_without_response_size = 20 await t.send(b"Hello pytest!") - t._client.write_gatt_char.assert_awaited_once_with( + client.write_gatt_char.assert_awaited_once_with( t._smp_characteristic, b"Hello pytest!", response=False ) @@ -161,7 +355,7 @@ async def test_send() -> None: @pytest.mark.asyncio async def test_receive() -> None: t = SMPBLETransport() - t._client = MagicMock(spec=BleakClient) + t._link = _Owned(MagicMock(spec=BleakClient)) t._smp_characteristic = MagicMock(spec=BleakGATTCharacteristic) t._smp_characteristic.uuid = str(SMP_CHARACTERISTIC_UUID) t._disconnected_event.clear() # pretend t.connect() was successful @@ -201,16 +395,33 @@ async def test_send_and_receive() -> None: def test_max_unencoded_size() -> None: t = SMPBLETransport() - t._client = MagicMock(spec=BleakClient) t._max_write_without_response_size = 42 assert t.max_unencoded_size == 42 -def test_max_unencoded_size_mcumgr_param() -> None: +@pytest.mark.asyncio +async def test_max_unencoded_size_mcumgr_param() -> None: t = SMPBLETransport() - t._client = MagicMock(spec=BleakClient) - t._smp_server_transport_buffer_size = 9001 - assert t.max_unencoded_size == 9001 + t._max_write_without_response_size = 42 + assert (await negotiated(t, 9001)).max_unencoded_size == 9001 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("buf_size, expected", [(9001, 42), (30, 30)]) +async def test_unfragmented_caps_at_the_write_size(buf_size: int, expected: int) -> None: + """One message per write: never more than one write, nor more than the server holds.""" + t = SMPBLETransport(fragmentation_strategy=Unfragmented()) + t._max_write_without_response_size = 42 + assert (await negotiated(t, buf_size)).max_unencoded_size == expected + + +@pytest.mark.asyncio +async def test_buffer_size_never_reads() -> None: + t = SMPBLETransport(fragmentation_strategy=BufferSize(512)) + with advertise(9001) as read: + await t.negotiate() + read.assert_not_awaited() + assert t.max_unencoded_size == 512 class _HangingBleakClient: @@ -251,41 +462,43 @@ async def test_connect_raises_on_peer_disconnect_during_start_notify( When the peer disconnects mid-`start_notify` (e.g. failed pairing), `connect()` must surface `SMPTransportDisconnected` rather than hang. """ - t = SMPBLETransport() + t = SMPBLETransport(connect_timeout_s=5.0) - async def _trip_disconnect_callback() -> None: + async def _trip_disconnect_callback() -> MagicMock: # Wait until the transport reaches start_notify and clears the event, # then simulate the bleak `disconnected_callback` firing. while t._disconnected_event.is_set(): await asyncio.sleep(0) await asyncio.sleep(0) # let start_notify await begin - t._set_disconnected_event(t._client) + client: Final = _owned_client(t) + t._set_disconnected_event(client) + return client - connect_task = asyncio.create_task(t.connect("00:00:00:00:00:00", 5.0)) + connect_task = asyncio.create_task(t.connect(ADDRESS)) trip_task = asyncio.create_task(_trip_disconnect_callback()) with pytest.raises(SMPTransportDisconnected): await connect_task - await trip_task # `_best_effort_disconnect` should have been called to release the client. - t._client.disconnect.assert_awaited() # type: ignore[attr-defined] + (await trip_task).disconnect.assert_awaited() @patch( "smpclient.transport.ble.BleakScanner.find_device_by_address", return_value=BLEDevice("00:00:00:00:00:00", "name", None), ) -@patch("smpclient.transport.ble.BleakClient", new=_HangingBleakClient) +@patch("smpclient.transport.ble.BleakClient", return_value=_HangingBleakClient()) @pytest.mark.asyncio async def test_connect_raises_on_timeout_during_start_notify( + mock_bleak_client: MagicMock, _mock_find_device_by_address: MagicMock, ) -> None: - """`connect()` must honor `timeout_s` even when `start_notify` hangs.""" - t = SMPBLETransport() + """`connect()` must honor `connect_timeout_s` even when `start_notify` hangs.""" + t = SMPBLETransport(connect_timeout_s=0.05) with pytest.raises(asyncio.TimeoutError): - await t.connect("00:00:00:00:00:00", 0.05) - t._client.disconnect.assert_awaited() # type: ignore[attr-defined] + await t.connect(ADDRESS) + mock_bleak_client.return_value.disconnect.assert_awaited() @patch( @@ -298,10 +511,10 @@ async def test_connect_does_not_leak_tasks_on_external_cancel( _mock_find_device_by_address: MagicMock, ) -> None: """Caller-driven cancellation must not leave `_await_or_disconnect` sub-tasks running.""" - t = SMPBLETransport() + t = SMPBLETransport(connect_timeout_s=60.0) tasks_before = {id(task) for task in asyncio.all_tasks()} - connect_task = asyncio.create_task(t.connect("00:00:00:00:00:00", 60.0)) + connect_task = asyncio.create_task(t.connect(ADDRESS)) while t._disconnected_event.is_set(): await asyncio.sleep(0) # wait until BleakClient.connect() returned await asyncio.sleep(0) # let start_notify await begin diff --git a/tests/test_smp_bumble_transport.py b/tests/test_smp_bumble_transport.py index 31d9692..a7b1ffd 100644 --- a/tests/test_smp_bumble_transport.py +++ b/tests/test_smp_bumble_transport.py @@ -4,13 +4,22 @@ import logging import os import tempfile +from contextlib import nullcontext from pathlib import Path -from typing import cast +from typing import Final, cast from unittest.mock import AsyncMock, MagicMock, patch import pytest - -from smpclient.transport import SMPTransportDisconnected +from bumble.hci import Address +from bumble.keys import PairingKeys + +from smpclient.transport import ( + Auto, + BufferSize, + GATTFragmentationStrategy, + SMPTransportDisconnected, + Unfragmented, +) from smpclient.transport.bumble import ( ATT_WRITE_OVERHEAD, SMP_CHARACTERISTIC_UUID, @@ -24,8 +33,15 @@ SMPBumbleTransportNotSMPServer, _DisconnectSentinel, _find_smp_characteristic, + bonded_devices, + clear_bond, + clear_bonds, +) +from smpclient.transport.bumble.device import ( + DEFAULT_HCI_TRANSPORT, + DEFAULT_HOST_ADDRESS, + DEFAULT_HOST_NAME, ) -from smpclient.transport.bumble.device import DEFAULT_HCI_TRANSPORT, DEFAULT_HOST_NAME from smpclient.transport.bumble.keystore import ( Custom, ExistingCustom, @@ -45,6 +61,12 @@ PairingSucceeded, PairingTimedOut, ) +from tests.support import advertise, negotiated + +pytestmark = pytest.mark.usefixtures("skip_negotiation") + +ADDRESS = "AA:BB:CC:DD:EE:FF" +"""A BD_ADDR; the bumble stack is mocked, so nothing is connected to.""" def test_smp_uuids_match_ble_transport() -> None: @@ -116,11 +138,13 @@ async def test_connect_while_connected_raises() -> None: t = SMPBumbleTransport() t._state = Connecting() with pytest.raises(SMPBumbleTransportException, match="Connecting"): - await t.connect("00:11:22:33:44:55", 5.0) + await t.connect(ADDRESS) -def _make_connected(max_write: int = 244) -> tuple[SMPBumbleTransport, MagicMock]: - t = SMPBumbleTransport() +def _make_connected( + max_write: int = 244, fragmentation_strategy: GATTFragmentationStrategy = Auto() +) -> tuple[SMPBumbleTransport, MagicMock]: + t = SMPBumbleTransport(fragmentation_strategy=fragmentation_strategy) smp_char = MagicMock() smp_char.write_value = AsyncMock() t._state = Connected( @@ -135,6 +159,30 @@ def _make_connected(max_write: int = 244) -> tuple[SMPBumbleTransport, MagicMock return t, smp_char +@pytest.mark.asyncio +async def test_auto_adopts_the_server_buffer() -> None: + t, _ = _make_connected(max_write=244) + assert t.max_unencoded_size == 244 + assert (await negotiated(t, 2048)).max_unencoded_size == 2048 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("buf_size, expected", [(2048, 244), (128, 128)]) +async def test_unfragmented_caps_at_the_write_size(buf_size: int, expected: int) -> None: + """One message per write: never more than one write, nor more than the server holds.""" + t, _ = _make_connected(max_write=244, fragmentation_strategy=Unfragmented()) + assert (await negotiated(t, buf_size)).max_unencoded_size == expected + + +@pytest.mark.asyncio +async def test_buffer_size_never_reads() -> None: + t, _ = _make_connected(max_write=244, fragmentation_strategy=BufferSize(512)) + with advertise(2048) as read: + await t.negotiate() + read.assert_not_awaited() + assert t.max_unencoded_size == 512 + + @pytest.mark.asyncio async def test_send_chunks_data_to_max_write() -> None: t, smp_char = _make_connected(max_write=4) @@ -375,6 +423,23 @@ def test_keystore_local_rejects_path_separators() -> None: resolve(Local("nested/bonds.json"), namespace="aa:bb:cc:dd:ee:ff") +@pytest.mark.asyncio +async def test_bond_functions_manage_the_hosts_bonds(tmp_path: Path) -> None: + keystore: Final = Custom(tmp_path / "bonds.json") + peers: Final = ("11:11:11:11:11:11", "22:22:22:22:22:22", "33:33:33:33:33:33") + for peer in peers: + await resolve(keystore, namespace=str(DEFAULT_HOST_ADDRESS)).update(peer, PairingKeys()) + + assert await bonded_devices(keystore=keystore) == peers + assert await bonded_devices(keystore=keystore, host_address=Address("F0:F1:F2:F3:F4:F5")) == () + + await clear_bond(peers[1], keystore=keystore) + assert await bonded_devices(keystore=keystore) == (peers[0], peers[2]) + + await clear_bonds(keystore=keystore) + assert await bonded_devices(keystore=keystore) == () + + def test_find_smp_characteristic_raises_when_service_missing() -> None: peer = MagicMock() peer.get_services_by_uuid.return_value = [] @@ -410,7 +475,7 @@ def test_disconnect_sentinel_is_namedtuple() -> None: class _MockBumbleEnvironment: - """Builds the mock bumble stack required by `SMPBumbleTransport.connect()`.""" + """Builds the mock bumble stack required by `SMPBumbleTransport.connect()` and `borrow()`.""" def __init__(self, *, with_bond: bool = False) -> None: self.transport = MagicMock() @@ -485,7 +550,7 @@ async def test_connect_transitions_to_connected_state( bumble_env: _MockBumbleEnvironment, ) -> None: t = SMPBumbleTransport() - await t.connect("AA:BB:CC:DD:EE:FF", 5.0) + await t.connect(ADDRESS) assert isinstance(t._state, Connected) assert t._state.max_write == 247 - ATT_WRITE_OVERHEAD bumble_env.device.power_on.assert_awaited_once() @@ -516,7 +581,7 @@ async def test_connect_proactively_encrypts_when_bonded( lambda _s, namespace: env.keystore, ) t = SMPBumbleTransport() - await t.connect("AA:BB:CC:DD:EE:FF", 5.0) + await t.connect(ADDRESS) env.connection.encrypt.assert_awaited_once() @@ -544,7 +609,7 @@ async def _connect_snapshotting(*args: object, **kwargs: object) -> MagicMock: bumble_env.device.connect = _connect_snapshotting t = SMPBumbleTransport(pair_on_connect=delegate) - await t.connect("AA:BB:CC:DD:EE:FF", 5.0) + await t.connect(ADDRESS) assert factory_set_at["value"], ( "pairing_config_factory must be set before device.connect() returns" ) @@ -585,7 +650,7 @@ async def _emit_security_request_after_connect(*args: object, **kwargs: object) bumble_env.device.connect = _emit_security_request_after_connect - await t.connect("AA:BB:CC:DD:EE:FF", 5.0) + await t.connect(ADDRESS) bumble_env.connection.pair.assert_awaited_once() assert isinstance(t._state, Connected) @@ -622,7 +687,7 @@ async def test_connect_failure_tears_down_partial_state( bumble_env.smp_char.subscribe.side_effect = RuntimeError("boom") t = SMPBumbleTransport() with pytest.raises(RuntimeError, match="boom"): - await t.connect("AA:BB:CC:DD:EE:FF", 5.0) + await t.connect(ADDRESS) assert isinstance(t._state, Disconnected) bumble_env.connection.disconnect.assert_awaited() bumble_env.device.power_off.assert_awaited() @@ -634,7 +699,7 @@ async def test_disconnect_owned_tears_down_everything( bumble_env: _MockBumbleEnvironment, ) -> None: t = SMPBumbleTransport() - await t.connect("AA:BB:CC:DD:EE:FF", 5.0) + await t.connect(ADDRESS) await t.disconnect() assert isinstance(t._state, Disconnected) bumble_env.smp_char.unsubscribe.assert_awaited() @@ -644,37 +709,83 @@ async def test_disconnect_owned_tears_down_everything( @pytest.mark.asyncio -async def test_use_connection_borrowed_only_unsubscribes_on_disconnect( +async def test_borrow_borrowed_only_unsubscribes_on_disconnect( bumble_env: _MockBumbleEnvironment, ) -> None: t = SMPBumbleTransport() - await t.use_connection(bumble_env.connection) + await t.borrow(bumble_env.connection) assert isinstance(t._state, ConnectedBorrowed) await t.disconnect() - bumble_env.smp_char.unsubscribe.assert_awaited() + bumble_env.smp_char.unsubscribe.assert_awaited_once_with(t._on_notification) bumble_env.connection.disconnect.assert_not_called() bumble_env.device.power_off.assert_not_called() bumble_env.transport.close.assert_not_called() @pytest.mark.asyncio -async def test_use_connection_skips_discover_when_services_present( +async def test_borrow_returns_the_connection_when_negotiation_fails( + bumble_env: _MockBumbleEnvironment, +) -> None: + t = SMPBumbleTransport() + + with ( + patch( + "smpclient._request.read_mcumgr_parameters", + AsyncMock(side_effect=asyncio.CancelledError), + ), + pytest.raises(asyncio.CancelledError), + ): + await t.borrow(bumble_env.connection) + + assert isinstance(t._state, Disconnected) + bumble_env.smp_char.unsubscribe.assert_awaited_once_with(t._on_notification) + bumble_env.connection.disconnect.assert_not_called() + + +@pytest.mark.asyncio +async def test_connect_tears_down_when_cancelled( + bumble_env: _MockBumbleEnvironment, caplog: pytest.LogCaptureFixture +) -> None: + connecting: Final = asyncio.Event() + + async def connect_until_cancelled(*_args: object, **_kwargs: object) -> MagicMock: + connecting.set() + await asyncio.Event().wait() + return bumble_env.connection + + bumble_env.device.connect = AsyncMock(side_effect=connect_until_cancelled) + t = SMPBumbleTransport() + + connect: Final = asyncio.create_task(t.connect(ADDRESS)) + await connecting.wait() + connect.cancel() + with pytest.raises(asyncio.CancelledError): + await connect + + assert isinstance(t._state, Disconnected) + bumble_env.device.power_off.assert_awaited_once() + bumble_env.transport.close.assert_awaited_once() + assert not [r for r in caplog.records if r.levelno >= logging.ERROR] + + +@pytest.mark.asyncio +async def test_borrow_skips_discover_when_services_present( bumble_env: _MockBumbleEnvironment, ) -> None: bumble_env.peer.services = [MagicMock()] t = SMPBumbleTransport() - await t.use_connection(bumble_env.connection, peer=bumble_env.peer) + await t.borrow(bumble_env.connection, peer=bumble_env.peer) bumble_env.peer.discover_all.assert_not_called() @pytest.mark.asyncio -async def test_use_connection_while_connected_raises( +async def test_borrow_while_connected_raises( bumble_env: _MockBumbleEnvironment, ) -> None: t = SMPBumbleTransport() - await t.connect("AA:BB:CC:DD:EE:FF", 5.0) + await t.connect(ADDRESS) with pytest.raises(SMPBumbleTransportException): - await t.use_connection(bumble_env.connection) + await t.borrow(bumble_env.connection) @pytest.mark.asyncio @@ -1055,11 +1166,14 @@ async def test_cli_echo_success(capsys: pytest.CaptureFixture[str]) -> None: response = MagicMock() response.r = "pong" client = MagicMock() - client.__aenter__ = AsyncMock(return_value=client) - client.__aexit__ = AsyncMock(return_value=None) client.request = AsyncMock(return_value=response) + transport = MagicMock() + transport.connected.return_value = nullcontext(transport) with ( + patch( + "smpclient.transport.bumble.__main__.SMPBumbleTransport", return_value=transport + ) as transport_class, patch("smpclient.transport.bumble.__main__.SMPClient", return_value=client), patch("smpclient.transport.bumble.__main__.success", return_value=True), patch("smpclient.transport.bumble.__main__.error", return_value=False), @@ -1068,6 +1182,8 @@ async def test_cli_echo_success(capsys: pytest.CaptureFixture[str]) -> None: _EchoArgs(hci="usb:0", address="AA:BB:CC:DD:EE:FF", message="ping", timeout=5.0) ) assert rc == 0 + transport_class.assert_called_once_with(hci="usb:0", connect_timeout_s=5.0) + transport.connected.assert_called_once_with("AA:BB:CC:DD:EE:FF") assert "pong" in capsys.readouterr().out @@ -1077,11 +1193,12 @@ async def test_cli_echo_returns_1_on_error(capsys: pytest.CaptureFixture[str]) - response = MagicMock() client = MagicMock() - client.__aenter__ = AsyncMock(return_value=client) - client.__aexit__ = AsyncMock(return_value=None) client.request = AsyncMock(return_value=response) + transport = MagicMock() + transport.connected.return_value = nullcontext(transport) with ( + patch("smpclient.transport.bumble.__main__.SMPBumbleTransport", return_value=transport), patch("smpclient.transport.bumble.__main__.SMPClient", return_value=client), patch("smpclient.transport.bumble.__main__.success", return_value=False), patch("smpclient.transport.bumble.__main__.error", return_value=True), @@ -1193,7 +1310,7 @@ def _post_pair_encrypted() -> None: bumble_env.connection.pair = AsyncMock(side_effect=_post_pair_encrypted) t = SMPBumbleTransport(pair_on_connect=NoInputNoOutput(), settle_s=0.0) - await t.connect("AA:BB:CC:DD:EE:FF", 5.0) + await t.connect(ADDRESS) bumble_env.connection.pair.assert_awaited_once() assert isinstance(t._state, Connected) @@ -1208,9 +1325,9 @@ async def test_resolve_target_raises_when_no_device_with_name( "smpclient.transport.bumble.scan_for_devices", AsyncMock(return_value=()), ) - t = SMPBumbleTransport() + t = SMPBumbleTransport(connect_timeout_s=0.1) with pytest.raises(SMPBumbleTransportDeviceNotFound): - await t.connect("UnknownName", 0.1) + await t.connect("UnknownName") # Suppress unused-imports warnings for symbols re-exported for downstream code. diff --git a/tests/test_smp_client.py b/tests/test_smp_client.py index 0570a3e..b12cd2c 100644 --- a/tests/test_smp_client.py +++ b/tests/test_smp_client.py @@ -39,6 +39,7 @@ ) from smpclient import SMPClient, error, error_v1, error_v2, success, wrapping_sequence +from smpclient import transport as smptransport from smpclient.exceptions import SMPBadSequence, SMPUploadError, SMPValidationException from smpclient.transport.serial import ( BufferParams, @@ -79,12 +80,8 @@ class SMPMockTransport: """Satisfies the `SMPTransport` `Protocol`.""" def __init__(self) -> None: - self.connect = AsyncMock() - self.disconnect = AsyncMock() self.send = AsyncMock() self.receive = AsyncMock() - self._smp_server_transport_buffer_size: int | None = None - self.initialize = AsyncMock() self._mtu = 0 self._max_unencoded_size = 0 self.sequence_offset = 0 @@ -126,26 +123,14 @@ def sent_frame(m: SMPMockTransport) -> Any: def test_constructor() -> None: m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) assert s._transport is m - assert s._address == "address" - - -@pytest.mark.asyncio -async def test_connect() -> None: - m = SMPMockTransport() - s = SMPClient(m, "address", 5.0) - s._initialize = AsyncMock() # type: ignore - await s.connect() - - m.connect.assert_awaited_once_with("address", 5.0) - s._initialize.assert_awaited_once_with(5.0) @pytest.mark.asyncio async def test_request() -> None: m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) req = ResetWriteRequest() m.receive.return_value = bytes(ResetWriteResponse().to_frame(sequence=0)) @@ -209,7 +194,7 @@ async def test_request() -> None: async def test_request_unparseable_frame() -> None: """A frame matching none of the Response/ErrorV1/ErrorV2 types raises with diagnostics.""" m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) req = ResetWriteRequest() # Same group, so the frame reaches the decoders -- but `r` is a field of none of @@ -238,7 +223,7 @@ def test_wrapping_sequence() -> None: async def test_injected_sequence() -> None: """The sequence space is injectable, so a test can pin what goes on the wire.""" m = SMPMockTransport() - s = SMPClient(m, "address", sequence=iter((7, 9))) + s = SMPClient(m, sequence=lambda: iter((7, 9))) m.receive.return_value = bytes(ResetWriteResponse().to_frame(sequence=0)) for expected in (7, 9): @@ -254,7 +239,7 @@ async def test_request_mismatched_group_propagates() -> None: instead of being collected as one more parse failure. """ m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) m.receive.return_value = bytes(ImageUploadWriteResponse(off=0).to_frame(sequence=0)) @@ -270,7 +255,7 @@ async def test_request_truncated_payload_is_diagnosed() -> None: decode chain catches the wider type. """ m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) truncated = b"\xbf\x61\x72" # an indefinite-length map that simply stops m.receive.return_value = ( @@ -295,7 +280,7 @@ async def test_request_truncated_payload_is_diagnosed() -> None: @pytest.mark.asyncio async def test_upload() -> None: m = SMPMockTransport() - s = SMPClient(m, "address", 2.5) + s = SMPClient(m, timeout_s=2.5) s.request = AsyncMock() # type: ignore @@ -368,7 +353,7 @@ async def test_upload_hello_world_bin( image = f.read() m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) accumulated_image = bytearray([]) @@ -406,9 +391,9 @@ async def test_upload_hello_world_bin_encoded( fragmentation_strategy=BufferParams( line_length=line_length, line_buffers=line_buffers, - ) + ), ) - s = SMPClient(m, "address") + s = SMPClient(m) # MTU is line_length * line_buffers, which may be <= max_smp_encoded_frame_size # due to integer division assert s._transport.mtu == line_length * line_buffers @@ -468,8 +453,8 @@ async def test_upload_hello_world_bin_raw(mtu: int) -> None: ) as f: image = f.read() - m = SMPSerialRawTransport(mtu=mtu) - s = SMPClient(m, "address") + m = SMPSerialRawTransport(fragmentation_strategy=smptransport.BufferSize(mtu)) + s = SMPClient(m) assert s._transport.mtu == mtu assert s._transport.max_unencoded_size == mtu, "The raw transport has no encoding overhead" @@ -509,7 +494,7 @@ async def mock_request( @pytest.mark.asyncio async def test_upload_file() -> None: m = SMPMockTransport() - s = SMPClient(m, "address", 2.5) + s = SMPClient(m, timeout_s=2.5) s.request = AsyncMock() # type: ignore @@ -581,7 +566,7 @@ async def test_file_upload_test_txt( data = f.read() m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) accumulated_data = bytearray([]) @@ -615,7 +600,7 @@ async def test_file_upload_test_255_bytes_file( data = f.read() m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) accumulated_data = bytearray([]) @@ -651,9 +636,9 @@ async def test_file_upload_test_encoded(max_smp_encoded_frame_size: int, line_bu fragmentation_strategy=BufferParams( line_length=line_length, line_buffers=line_buffers, - ) + ), ) - s = SMPClient(m, "address") + s = SMPClient(m) # MTU is line_length * line_buffers, which may be <= max_smp_encoded_frame_size # due to integer division assert s._transport.mtu == line_length * line_buffers @@ -707,7 +692,7 @@ async def mock_request( @pytest.mark.asyncio async def test_download_file() -> None: m = SMPMockTransport() - s = SMPClient(m, "address", 2.5) + s = SMPClient(m, timeout_s=2.5) s.request = AsyncMock() # type: ignore @@ -804,7 +789,7 @@ async def test_download_file() -> None: @pytest.mark.asyncio async def test_download_file_error_first() -> None: m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) s.request = AsyncMock() # type: ignore @@ -822,7 +807,7 @@ async def test_download_file_error_first() -> None: @pytest.mark.asyncio async def test_download_file_no_len_first() -> None: m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) s.request = AsyncMock() # type: ignore @@ -841,7 +826,7 @@ async def test_download_file_no_len_first() -> None: @pytest.mark.asyncio async def test_download_file_error_not_first() -> None: m = SMPMockTransport() - s = SMPClient(m, "address") + s = SMPClient(m) s.request = AsyncMock() # type: ignore @@ -877,10 +862,7 @@ def test_maximize_upload_packet_fills_decoded_buffer( the wire -- larger than the buffer, which the server decodes incrementally as the lines arrive. The unified generic handles both `ImageUploadWriteRequest` and `FileUploadRequest`. """ - client = SMPClient( - SMPSerialTransport(fragmentation_strategy=BufferSize(buf_size=buf_size)), - "address", - ) + client = SMPClient(SMPSerialTransport(fragmentation_strategy=BufferSize(buf_size=buf_size))) max_unencoded_size = client._transport.max_unencoded_size assert max_unencoded_size == buf_size - FRAME_OVERHEAD diff --git a/tests/test_smp_serial_raw_transport.py b/tests/test_smp_serial_raw_transport.py index 5703efd..6603d18 100644 --- a/tests/test_smp_serial_raw_transport.py +++ b/tests/test_smp_serial_raw_transport.py @@ -4,7 +4,7 @@ import asyncio from collections.abc import Generator -from typing import Any +from typing import Any, Final from unittest.mock import AsyncMock, MagicMock, PropertyMock, patch import pytest @@ -14,9 +14,15 @@ from smp.packet import CRC16_STRUCT, crc16_func from smpclient.exceptions import SMPClientException -from smpclient.transport import SMPTransportDisconnected +from smpclient.transport import BufferSize, SMPTransportDisconnected from smpclient.transport.serial import Cobs, SMPSerialRawTransport from smpclient.transport.serial.framing.cobs import cobs_encode +from tests.support import advertise, negotiated + +pytestmark = pytest.mark.usefixtures("skip_negotiation") + +PORT = "/dev/ttyUSB0" +"""A port name; `Serial` is mocked, so nothing is opened.""" @pytest.fixture(autouse=True) @@ -26,7 +32,7 @@ def mock_serial() -> Generator[None, Any, None]: def test_constructor() -> None: - t = SMPSerialRawTransport(mtu=512) + t = SMPSerialRawTransport(fragmentation_strategy=BufferSize(512)) assert t.mtu == 512 assert t.max_unencoded_size == 512 @@ -36,15 +42,31 @@ def test_constructor_defaults() -> None: assert t.mtu == 384 +@pytest.mark.asyncio +async def test_negotiate_with_auto() -> None: + """`Auto` adopts the server's buffer: the whole message rides in it, with no framing.""" + t = await negotiated(SMPSerialRawTransport(), 1024) + assert t.mtu == t.max_unencoded_size == 1024 + + +@pytest.mark.asyncio +async def test_negotiate_never_reads_for_buffer_size() -> None: + t = SMPSerialRawTransport(fragmentation_strategy=BufferSize(512)) + with advertise(1024) as read: + await t.negotiate() + read.assert_not_awaited() + assert t.max_unencoded_size == 512 + + @pytest.mark.asyncio async def test_connect_disconnect() -> None: ports: list[str] = ["COM2", "/dev/ttyACM0", "/dev/ttyUSB0"] - t = SMPSerialRawTransport() - t._conn.read_all = MagicMock(return_value=b"") # type: ignore - for p in ports: - await asyncio.wait_for(t.connect(p, 1.0), timeout=1.0) + t = SMPSerialRawTransport(connect_timeout_s=1.0) + t._conn.read_all = MagicMock(return_value=b"") # type: ignore + + await asyncio.wait_for(t.connect(p), timeout=1.0) t._conn.open.assert_called_once() # type: ignore assert t._conn.port == p @@ -57,11 +79,84 @@ async def test_connect_disconnect() -> None: @pytest.mark.asyncio async def test_connect_retries_until_timeout() -> None: - t = SMPSerialRawTransport() + t = SMPSerialRawTransport(connect_timeout_s=0.1) t._conn.open = MagicMock(side_effect=SerialException("nope")) # type: ignore with pytest.raises(TimeoutError): - await asyncio.wait_for(t.connect("/dev/ttyUSB0", 0.1), timeout=2.0) + await asyncio.wait_for(t.connect(PORT), timeout=2.0) + + +@pytest.mark.asyncio +async def test_connect_closes_the_port_when_the_flush_fails() -> None: + """The flush is outside the retry, which would reopen the open port until the timeout.""" + t = SMPSerialRawTransport() + t._conn.reset_input_buffer = MagicMock(side_effect=SerialException("flush")) # type: ignore + + with pytest.raises(SerialException): + await t.connect(PORT) + + t._conn.open.assert_called_once() # type: ignore + t._conn.close.assert_called_once() # type: ignore + + +@pytest.mark.asyncio +async def test_connect_closes_the_port_when_cancelled_while_negotiating() -> None: + t = SMPSerialRawTransport() + + with ( + patch( + "smpclient._request.read_mcumgr_parameters", + AsyncMock(side_effect=asyncio.CancelledError), + ), + pytest.raises(asyncio.CancelledError), + ): + await t.connect(PORT) + + t._conn.close.assert_called_once() # type: ignore + + +@pytest.mark.asyncio +async def test_borrowed_uses_the_port_and_leaves_it_open() -> None: + port: Final = MagicMock(out_waiting=0) + t = SMPSerialRawTransport() + r = EchoWriteRequest(d="Hello pytest!").to_frame(sequence=0) + + async with t.borrowed(port) as borrowed: + assert borrowed is t + await t.send(bytes(r)) + + port.write.assert_called_once_with(bytes(r)) + port.close.assert_not_called() + t._serial.open.assert_not_called() # type: ignore + t._serial.close.assert_not_called() # type: ignore + assert t._conn is t._serial + + +@pytest.mark.asyncio +async def test_borrow_negotiates_the_fragmentation_strategy() -> None: + t = SMPSerialRawTransport() + + with advertise(2048) as read_mcumgr_parameters: + await t.borrow(MagicMock()) + + read_mcumgr_parameters.assert_awaited_once() + assert t.max_unencoded_size == 2048 + + +@pytest.mark.asyncio +async def test_borrow_reverts_to_the_owned_port_when_negotiation_fails() -> None: + t = SMPSerialRawTransport() + + with ( + patch( + "smpclient._request.read_mcumgr_parameters", + AsyncMock(side_effect=asyncio.CancelledError), + ), + pytest.raises(asyncio.CancelledError), + ): + await t.borrow(MagicMock()) + + assert t._conn is t._serial @pytest.mark.asyncio @@ -92,7 +187,7 @@ async def test_send_waits_for_tx_drain() -> None: @pytest.mark.asyncio async def test_send_too_large_raises() -> None: - t = SMPSerialRawTransport(mtu=16) + t = SMPSerialRawTransport(fragmentation_strategy=BufferSize(16)) with pytest.raises(ValueError): await t.send(b"\x00" * 32) @@ -109,7 +204,7 @@ async def test_send_disconnected_raises() -> None: @pytest.mark.asyncio async def test_receive_single_packet() -> None: t = SMPSerialRawTransport() - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m = EchoWriteResponse(r="Hello pytest!").to_frame(sequence=0) t._conn.read_all = MagicMock(side_effect=[bytes(m)]) # type: ignore @@ -123,7 +218,7 @@ async def test_receive_single_packet() -> None: @pytest.mark.asyncio async def test_receive_fragmented() -> None: t = SMPSerialRawTransport() - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m = EchoWriteResponse(r="Hello pytest!").to_frame(sequence=0) fragments = [ @@ -143,7 +238,7 @@ async def test_receive_fragmented() -> None: @pytest.mark.asyncio async def test_receive_byte_at_a_time() -> None: t = SMPSerialRawTransport() - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m = EchoWriteResponse(r="Hi").to_frame(sequence=0) t._conn.read_all = MagicMock( # type: ignore @@ -159,7 +254,7 @@ async def test_receive_byte_at_a_time() -> None: @pytest.mark.asyncio async def test_receive_consecutive_messages() -> None: t = SMPSerialRawTransport() - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m1 = EchoWriteResponse(r="SMP Message 1").to_frame(sequence=0) m2 = EchoWriteResponse(r="SMP Message 2").to_frame(sequence=1) @@ -182,7 +277,7 @@ async def test_receive_overrun_raises() -> None: SMP is strictly request/response; the server should never send unsolicited bytes. """ t = SMPSerialRawTransport() - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m = EchoWriteResponse(r="Hello!").to_frame(sequence=0) t._conn.read_all = MagicMock(side_effect=[bytes(m) + b"\x00\x01\x02"]) # type: ignore @@ -196,7 +291,7 @@ async def test_receive_overrun_raises() -> None: @pytest.mark.asyncio async def test_receive_polls_when_nothing_available() -> None: t = SMPSerialRawTransport() - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m = EchoWriteResponse(r="ok").to_frame(sequence=0) t._conn.read_all = MagicMock(side_effect=[b"", b"", bytes(m)]) # type: ignore @@ -215,8 +310,8 @@ async def test_receive_oversized_header_raises() -> None: Defensive bound against noisy or corrupted UART traffic that would otherwise cause an unbounded wait. """ - t = SMPSerialRawTransport(mtu=64) - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + t = SMPSerialRawTransport(fragmentation_strategy=BufferSize(64)) + await t.connect(PORT) bogus_header = smphdr.Header( op=smphdr.OP.WRITE_RSP, @@ -273,7 +368,7 @@ async def test_send_with_cobs_framing_encodes() -> None: @pytest.mark.asyncio async def test_receive_with_cobs_framing_decodes() -> None: t = SMPSerialRawTransport(framing=Cobs()) - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m = EchoWriteResponse(r="Hello pytest!").to_frame(sequence=0) (wire,) = Cobs().encode(bytes(m)) @@ -287,7 +382,7 @@ async def test_receive_with_cobs_framing_decodes() -> None: @pytest.mark.asyncio async def test_receive_with_cobs_framing_fragmented() -> None: t = SMPSerialRawTransport(framing=Cobs()) - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m = EchoWriteResponse(r="fragment me across reads").to_frame(sequence=0) (wire,) = Cobs().encode(bytes(m)) @@ -305,7 +400,7 @@ async def test_receive_two_cobs_frames_in_one_read() -> None: The next receive returns it without consulting read_all again. """ t = SMPSerialRawTransport(framing=Cobs()) - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m1 = EchoWriteResponse(r="first").to_frame(sequence=0) m2 = EchoWriteResponse(r="second").to_frame(sequence=1) @@ -328,7 +423,7 @@ async def test_receive_cobs_framing_resyncs_past_corrupt_frame() -> None: corrupt frame would surface `dropped`, not `recovered`. """ t = SMPSerialRawTransport(framing=Cobs()) - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) dropped = EchoWriteResponse(r="dropped").to_frame(sequence=0) recovered = EchoWriteResponse(r="recovered").to_frame(sequence=1) @@ -352,7 +447,7 @@ async def test_receive_framed_yields_so_an_outer_timeout_can_fire() -> None: `asyncio.timeout` could never fire on a wrong-baud / wrong-protocol / noisy peer. """ t = SMPSerialRawTransport(framing=Cobs()) - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m = EchoWriteResponse(r="never valid").to_frame(sequence=0) corrupt = cobs_encode(bytes(m) + CRC16_STRUCT.pack(crc16_func(bytes(m)) ^ 0xFFFF)) + b"\x00" diff --git a/tests/test_smp_serial_transport.py b/tests/test_smp_serial_transport.py index b12ce9b..845087c 100644 --- a/tests/test_smp_serial_transport.py +++ b/tests/test_smp_serial_transport.py @@ -3,25 +3,32 @@ from __future__ import annotations import asyncio -import logging -import warnings -from collections.abc import Callable, Generator -from typing import Any, get_args +import inspect +from collections.abc import Generator +from typing import Any, Final, get_args from unittest.mock import AsyncMock, MagicMock, PropertyMock, patch import pytest +import serial from serial import SerialException +from serial.serialutil import SerialBase from smp import packet as smppacket from smp.os_management import EchoWriteRequest, EchoWriteResponse -from smpclient.transport import SMPTransportDisconnected +from smpclient.transport import Auto, SMPTransportDisconnected from smpclient.transport.serial import ( - Auto, BufferParams, BufferSize, - FragmentationStrategy, + SerialFragmentationStrategy, + SerialOptions, SMPSerialTransport, ) +from tests.support import advertise, negotiated + +pytestmark = pytest.mark.usefixtures("skip_negotiation") + +PORT = "/dev/ttyACM0" +"""A port name; `Serial` is mocked, so nothing is opened.""" FRAME_OVERHEAD = smppacket.FRAME_LENGTH_STRUCT.size + smppacket.CRC16_STRUCT.size """The SMP serial frame's 2-byte length + 2-byte CRC16 that share the decoded buffer.""" @@ -34,7 +41,7 @@ def mock_serial() -> Generator[None, Any, None]: def test_constructor() -> None: - # Test with Auto() (default): conservative 7.1.0-equivalent 128 * 2 budget pre-init + # Test with Auto() (default): conservative 128 * 2 budget pre-init t = SMPSerialTransport() assert t.mtu == 256 # 128 * 2, the conservative default before server params are read assert t._line_length == 128 @@ -57,15 +64,37 @@ def test_constructor() -> None: assert t.max_unencoded_size == 1024 - FRAME_OVERHEAD +def test_serial_options_lock_pyserial() -> None: + """`SerialOptions` is pyserial's settings, in order, with its defaults but the baudrate. + + The settings are `SerialBase`'s: on Windows, `serial.Serial.__init__` is `*args, **kwargs`. + """ + assert issubclass(serial.Serial, SerialBase) + pyserial_defaults: Final = { + name: parameter.default + for name, parameter in inspect.signature(SerialBase).parameters.items() + if name != "port" and parameter.kind is not inspect.Parameter.VAR_KEYWORD + } + assert tuple(pyserial_defaults) == SerialOptions._fields + assert {**pyserial_defaults, "baudrate": 115200} == SerialOptions()._asdict() + + +def test_options_configure_the_port() -> None: + options: Final = SerialOptions(baudrate=9600, rtscts=True, exclusive=True) + with patch("smpclient.transport.serial.common.Serial") as serial_class: + SMPSerialTransport(options=options) + serial_class.assert_called_once_with(**options._asdict()) + + @pytest.mark.asyncio async def test_connect_disconnect() -> None: ports: list[str] = ["COM2", "/dev/ttyACM0", "/dev/ttyUSB0"] - t = SMPSerialTransport() - t._conn.read_all = MagicMock(return_value=b"") # type: ignore - for p in ports: - await asyncio.wait_for(t.connect(p, 1.0), timeout=1.0) + t = SMPSerialTransport(connect_timeout_s=1.0) + t._conn.read_all = MagicMock(return_value=b"") # type: ignore + + await asyncio.wait_for(t.connect(p), timeout=1.0) t._conn.open.assert_called_once() # type: ignore assert t._conn.port == p @@ -97,6 +126,20 @@ async def test_send() -> None: assert p.call_count == 2 # called twice since out buffer was not drained on first call +@pytest.mark.asyncio +async def test_borrowed_receives_from_the_port_and_leaves_it_open() -> None: + m: Final = EchoWriteResponse(r="Hello pytest!").to_frame(sequence=0) + port: Final = MagicMock(read_all=MagicMock(side_effect=smppacket.encode(bytes(m), 8))) + t = SMPSerialTransport() + + async with t.borrowed(port): + assert await t.receive() == bytes(m) + + port.close.assert_not_called() + t._serial.open.assert_not_called() # type: ignore + assert t._conn is t._serial + + @pytest.mark.asyncio async def test_receive() -> None: t = SMPSerialTransport() @@ -120,7 +163,7 @@ async def test_receive() -> None: @pytest.mark.asyncio async def test_read_one_smp_packet() -> None: t = SMPSerialTransport() - await t.connect("COM2", timeout_s=1.0) + await t.connect(PORT) m1 = EchoWriteResponse(r="Hello pytest!").to_frame(sequence=0) m2 = EchoWriteResponse(r="Hello computer!").to_frame(sequence=1) @@ -169,7 +212,7 @@ async def test_send_and_receive() -> None: @pytest.mark.asyncio async def test_receive_timeout() -> None: - t = SMPSerialTransport(timeout=0.1) + t = SMPSerialTransport(options=SerialOptions(timeout=0.1)) t._read_one_smp_packet = AsyncMock(side_effect=TimeoutError) # type: ignore with pytest.raises(TimeoutError): @@ -179,7 +222,7 @@ async def test_receive_timeout() -> None: @pytest.mark.asyncio async def test_only_serial_data_no_smp() -> None: t = SMPSerialTransport() - await t.connect("/dev/ttyACM0", timeout_s=1.0) + await t.connect(PORT) t._conn.read_all = MagicMock( # type: ignore side_effect=[ @@ -216,7 +259,7 @@ async def test_only_serial_data_no_smp() -> None: @pytest.mark.asyncio async def test_only_smp_data_no_serial() -> None: t = SMPSerialTransport() - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m1 = EchoWriteResponse(r="SMP Message 1").to_frame(sequence=0) m2 = EchoWriteResponse(r="SMP Message 2").to_frame(sequence=1) @@ -240,7 +283,7 @@ async def test_only_smp_data_no_serial() -> None: @pytest.mark.asyncio async def test_serial_and_smp_data() -> None: t = SMPSerialTransport() - await t.connect("/dev/ttyUSB0", timeout_s=1.0) + await t.connect(PORT) m1 = EchoWriteResponse(r="SMP1").to_frame(sequence=0) m2 = EchoWriteResponse(r="SMP2").to_frame(sequence=1) @@ -284,24 +327,25 @@ async def test_serial_and_smp_data() -> None: @pytest.mark.asyncio async def test_not_connected_exception_handling() -> None: t = SMPSerialTransport() - t._conn.is_open = False + t._serial.is_open = False t._conn.read_all = MagicMock(side_effect=SerialException("Not connected")) # type: ignore with pytest.raises(SMPTransportDisconnected): await t.receive() -def test_initialize_with_auto() -> None: +@pytest.mark.asyncio +async def test_negotiate_with_auto() -> None: """Test that Auto mode updates parameters based on server's buffer size.""" t = SMPSerialTransport() # Uses Auto() by default - # Before initialize, uses the conservative 7.1.0-equivalent 128 * 2 defaults + # Before negotiating, uses the conservative 128 * 2 defaults assert t._line_length == 128 assert t._line_buffers == 2 assert t._max_smp_encoded_frame_size == 256 - # After initialize with server buffer size - t.initialize(512) + # After negotiating against the server buffer size + await negotiated(t, 512) assert t._line_length == 128 assert t._line_buffers == 512 // 128 # 4 assert t._max_smp_encoded_frame_size == 512 @@ -311,36 +355,34 @@ def test_initialize_with_auto() -> None: assert t.max_unencoded_size == 512 - FRAME_OVERHEAD -def test_initialize_with_buffer_params() -> None: +@pytest.mark.asyncio +async def test_negotiate_with_buffer_params() -> None: """Test that BufferParams mode doesn't change user-specified parameters.""" t = SMPSerialTransport(fragmentation_strategy=BufferParams(line_length=128, line_buffers=2)) - # Before initialize + # Before negotiating assert t._line_length == 128 assert t._line_buffers == 2 assert t._max_smp_encoded_frame_size == 256 # 128 * 2 - # After initialize - parameters should NOT change - t.initialize(512) + # After negotiating - parameters should NOT change + await negotiated(t, 512) assert t._line_length == 128 assert t._line_buffers == 2 assert t._max_smp_encoded_frame_size == 256 assert t.mtu == 256 -def test_initialize_with_buffer_params_warning(caplog: pytest.LogCaptureFixture) -> None: - """Test that a warning is logged when user's params exceed server buffer size.""" - t = SMPSerialTransport( - fragmentation_strategy=BufferParams( - line_length=128, - line_buffers=4, # 128 * 4 = 512 - ) - ) - - with caplog.at_level(logging.WARNING): - t.initialize(256) # Server buffer (256) is smaller than calculated size (512) - - assert any("exceeds server buffer size" in record.message for record in caplog.records) +@pytest.mark.asyncio +async def test_negotiate_never_reads_for_pinned_strategies() -> None: + """A pinned strategy knows its size, so negotiating never reads the server's params.""" + for strategy in (BufferParams(line_length=128, line_buffers=4), BufferSize(buf_size=1024)): + t = SMPSerialTransport(fragmentation_strategy=strategy) + max_unencoded_size = t.max_unencoded_size + with advertise(256) as read: + await t.negotiate() + read.assert_not_awaited() + assert t.max_unencoded_size == max_unencoded_size def test_buffer_size() -> None: @@ -352,10 +394,11 @@ def test_buffer_size() -> None: assert t.max_unencoded_size == buf_size - FRAME_OVERHEAD -def test_buffer_size_matches_initialized_auto() -> None: - """BufferSize(n) is equivalent to Auto initialized with buf_size n.""" +@pytest.mark.asyncio +async def test_buffer_size_matches_negotiated_auto() -> None: + """BufferSize(n) is equivalent to Auto negotiated against buf_size n.""" auto = SMPSerialTransport() - auto.initialize(1024) + await negotiated(auto, 1024) told = SMPSerialTransport(fragmentation_strategy=BufferSize(buf_size=1024)) assert told.max_unencoded_size == auto.max_unencoded_size == 1024 - FRAME_OVERHEAD @@ -370,19 +413,20 @@ def test_buffer_size_small_line_length() -> None: assert t.max_unencoded_size == 384 - FRAME_OVERHEAD -def test_line_buffers_never_misleading_zero() -> None: +@pytest.mark.asyncio +async def test_line_buffers_never_misleading_zero() -> None: """Sub-line-length decoded buffers report >= 1 line buffer, never a misleading 0.""" # BufferSize with a buffer smaller than one line still reports at least one line buffer. assert SMPSerialTransport(fragmentation_strategy=BufferSize(buf_size=96))._line_buffers == 1 - # Auto initialized against a sub-line-length server buffer, likewise. + # Auto negotiated against a sub-line-length server buffer, likewise. auto_small = SMPSerialTransport() - auto_small.initialize(96) + await negotiated(auto_small, 96) assert auto_small._line_buffers == 1 # A non-multiple server buffer floors to a sane count and still fills buf_size - overhead. auto_400 = SMPSerialTransport() - auto_400.initialize(400) + await negotiated(auto_400, 400) assert auto_400._line_buffers == 400 // 128 # 3 assert auto_400.max_unencoded_size == 400 - FRAME_OVERHEAD @@ -415,7 +459,7 @@ async def test_decoded_buffer_strategies_put_full_encoded_frame_on_the_wire() -> for buf_size, encoded_size in expected_encoded.items(): told = SMPSerialTransport(fragmentation_strategy=BufferSize(buf_size=buf_size)) auto = SMPSerialTransport() - auto.initialize(buf_size) + await negotiated(auto, buf_size) for t in (told, auto): on_wire = await _frame_on_the_wire(t, b"\x5a" * t.max_unencoded_size) @@ -423,157 +467,9 @@ async def test_decoded_buffer_strategies_put_full_encoded_frame_on_the_wire() -> assert len(on_wire) > buf_size # more encoded bytes on the wire than the buffer holds -def test_initialize_with_buffer_size_warning(caplog: pytest.LogCaptureFixture) -> None: - """A BufferSize larger than the server's advertised buffer warns; the manual size wins.""" - t = SMPSerialTransport(fragmentation_strategy=BufferSize(buf_size=1024)) - - with caplog.at_level(logging.WARNING): - t.initialize(512) - - assert any( - "exceeds the server's advertised buffer size" in record.message for record in caplog.records - ) - assert t.max_unencoded_size == 1024 - FRAME_OVERHEAD - - def test_fragmentation_strategy_alias() -> None: - """`FragmentationStrategy` is the union of the three strategy types.""" - assert set(get_args(FragmentationStrategy)) == {Auto, BufferSize, BufferParams} - - -@pytest.mark.parametrize( - "make, mtu, line_length, line_buffers", - [ - pytest.param( - lambda: SMPSerialTransport( - max_smp_encoded_frame_size=512, line_length=128, line_buffers=4 - ), - 512, - 128, - 4, - id="kw-frame-ll-lb", - ), - pytest.param( - lambda: SMPSerialTransport(line_length=64, line_buffers=4), - 256, # max_smp_encoded_frame_size defaults to the 7.1.0 256 - 64, - 4, - id="kw-ll-lb-defaults-frame-256", - ), - # 7.1.0 positional layout was (max_smp_encoded_frame_size, line_length, line_buffers), - # with the line_length=128, line_buffers=2 defaults. - pytest.param(lambda: SMPSerialTransport(256), 256, 128, 2, id="pos-frame"), - pytest.param(lambda: SMPSerialTransport(256, 128, 2), 256, 128, 2, id="pos-triple"), - # A frame size larger than line_length * line_buffers still drives mtu (as in 7.1.0), - # rather than being silently downgraded to the 128 * 2 == 256 budget. - pytest.param(lambda: SMPSerialTransport(512), 512, 128, 2, id="pos-frame-gt-budget"), - pytest.param( - lambda: SMPSerialTransport(max_smp_encoded_frame_size=1024), - 1024, - 128, - 2, - id="kw-frame-only-gt-budget", - ), - ], -) -def test_deprecated_params_reproduce_7_1_0( - make: Callable[[], SMPSerialTransport], mtu: int, line_length: int, line_buffers: int -) -> None: - """The deprecated 7.1.0 params still construct, warn, and keep 7.1.0 sizing. - - `mtu` is the explicit `max_smp_encoded_frame_size` (defaulting to the 7.1.0 256), - independent of `line_length * line_buffers` -- not silently downgraded to the budget. - """ - with pytest.warns(DeprecationWarning, match="fragmentation_strategy"): - t = make() - assert t.mtu == mtu - assert t._line_length == line_length - assert t._line_buffers == line_buffers - - -@pytest.mark.parametrize( - "frame_size, expected_mtu, expected_max_unencoded", - # Values from smpclient 7.1.0 (default line_length=128, line_buffers=2): - # mtu == max_smp_encoded_frame_size, max_unencoded == _base64_max(mtu) - framing(2). - [(256, 256, 169), (512, 512, 361), (1024, 1024, 745)], -) -def test_deprecated_frame_size_matches_7_1_0_throughput( - frame_size: int, expected_mtu: int, expected_max_unencoded: int -) -> None: - """A legacy `max_smp_encoded_frame_size` yields the exact 7.1.0 mtu/max_unencoded_size. - - Guards against the regression where the frame size was downgraded to 128 * 2 == 256 - (which halved, or worse, the per-request payload for upgraders). - """ - with pytest.warns(DeprecationWarning): - t = SMPSerialTransport(max_smp_encoded_frame_size=frame_size) - assert t.mtu == expected_mtu - assert t.max_unencoded_size == expected_max_unencoded - - -def test_deprecated_params_match_equivalent_buffer_params() -> None: - """A *consistent* deprecated call (frame == line_length*line_buffers) equals its BufferParams.""" - with pytest.warns(DeprecationWarning): - legacy = SMPSerialTransport(max_smp_encoded_frame_size=512, line_length=128, line_buffers=4) - modern = SMPSerialTransport( - fragmentation_strategy=BufferParams(line_length=128, line_buffers=4) - ) - - assert legacy.mtu == modern.mtu # 512 == 128 * 4 - assert legacy.max_unencoded_size == modern.max_unencoded_size - assert legacy._line_length == modern._line_length - assert legacy._line_buffers == modern._line_buffers - - -def test_deprecated_frame_size_mismatch_is_logged(caplog: pytest.LogCaptureFixture) -> None: - """A frame size disagreeing with line_length*line_buffers is logged but still drives mtu. - - 7.1.0 logged the mismatch (WARNING when greater, ERROR when smaller) and kept using the - explicit max_smp_encoded_frame_size; this reproduces that, rather than downgrading mtu. - """ - with caplog.at_level(logging.WARNING), pytest.warns(DeprecationWarning): - t = SMPSerialTransport(max_smp_encoded_frame_size=512, line_length=128, line_buffers=2) - assert any("is not equal to" in record.message for record in caplog.records) - assert t.mtu == 512 # the explicit frame size wins, as in 7.1.0 (not 128 * 2 == 256) - - caplog.clear() - with caplog.at_level(logging.ERROR), pytest.warns(DeprecationWarning): - t = SMPSerialTransport(max_smp_encoded_frame_size=64, line_length=128, line_buffers=2) - assert any( - record.levelno == logging.ERROR and "is less than" in record.message - for record in caplog.records - ) - assert t.mtu == 64 # still honored, as in 7.1.0 - - -@pytest.mark.parametrize( - "make", - [ - pytest.param(lambda: SMPSerialTransport(), id="auto-default"), - pytest.param(lambda: SMPSerialTransport(fragmentation_strategy=Auto()), id="auto-explicit"), - pytest.param(lambda: SMPSerialTransport(BufferSize(buf_size=1024)), id="buffersize"), - pytest.param( - lambda: SMPSerialTransport(BufferParams(line_length=128, line_buffers=4)), - id="bufferparams", - ), - ], -) -def test_modern_constructors_do_not_warn(make: Callable[[], SMPSerialTransport]) -> None: - """The modern fragmentation_strategy API must never emit a DeprecationWarning.""" - with warnings.catch_warnings(): - warnings.simplefilter("error", DeprecationWarning) - make() - - -def test_explicit_strategy_wins_over_stray_legacy_args(caplog: pytest.LogCaptureFixture) -> None: - """An explicit strategy is returned as-is (never the legacy path), but stray args are logged.""" - resolve = SMPSerialTransport._resolve_fragmentation_strategy - with warnings.catch_warnings(), caplog.at_level(logging.WARNING): - warnings.simplefilter("error", DeprecationWarning) # the explicit strategy must not warn - assert resolve(BufferSize(buf_size=1024), None, 64, None) == BufferSize(buf_size=1024) - assert resolve(Auto(), 999, 64, 8) == Auto() - # the silently-dropped legacy args are surfaced rather than ignored without a trace - assert any("ignoring deprecated" in record.message for record in caplog.records) + """`SerialFragmentationStrategy` is the union of the three strategy types.""" + assert set(get_args(SerialFragmentationStrategy)) == {Auto, BufferSize, BufferParams} @pytest.mark.parametrize( @@ -587,7 +483,7 @@ def test_explicit_strategy_wins_over_stray_legacy_args(caplog: pytest.LogCapture pytest.param(BufferParams(line_length=128, line_buffers=0), id="bufferparams-zero-buffers"), ], ) -def test_invalid_strategy_raises_value_error(strategy: FragmentationStrategy) -> None: +def test_invalid_strategy_raises_value_error(strategy: SerialFragmentationStrategy) -> None: """The modern API rejects sizes that would hang the encoder or yield a non-positive payload.""" with pytest.raises(ValueError): SMPSerialTransport(fragmentation_strategy=strategy) @@ -603,14 +499,15 @@ def test_invalid_strategy_raises_value_error(strategy: FragmentationStrategy) -> pytest.param(BufferParams(line_length=128, line_buffers=1), id="bufferparams"), ], ) -def test_valid_strategy_does_not_raise(strategy: FragmentationStrategy) -> None: +def test_valid_strategy_does_not_raise(strategy: SerialFragmentationStrategy) -> None: """Valid strategies construct and report a positive max_unencoded_size.""" t = SMPSerialTransport(fragmentation_strategy=strategy) assert t.max_unencoded_size > 0 -def test_auto_rejects_tiny_server_buffer() -> None: +@pytest.mark.asyncio +async def test_auto_rejects_tiny_server_buffer() -> None: """Auto raises if the server advertises a buffer too small to hold a framed message.""" t = SMPSerialTransport() with pytest.raises(ValueError, match="frame overhead"): - t.initialize(FRAME_OVERHEAD) # buf_size == overhead -> zero-byte payload + await negotiated(t, FRAME_OVERHEAD) # buf_size == overhead -> zero-byte payload diff --git a/tests/test_smp_udp_transport.py b/tests/test_smp_udp_transport.py index 5d74c9b..b3bd7ba 100644 --- a/tests/test_smp_udp_transport.py +++ b/tests/test_smp_udp_transport.py @@ -8,8 +8,15 @@ from smp.os_management import EchoWriteResponse from smpclient.exceptions import SMPClientException +from smpclient.transport import BufferSize from smpclient.transport._udp_client import Addr, UDPClient from smpclient.transport.udp import IPV4_UDP_OVERHEAD, IPV6_UDP_OVERHEAD, SMPUDPTransport +from tests.support import advertise, negotiated + +pytestmark = pytest.mark.usefixtures("skip_negotiation") + +ADDRESS = "192.168.0.1" +"""An address; the UDP client is mocked.""" def test_init() -> None: @@ -24,15 +31,27 @@ def test_init() -> None: @patch("smpclient.transport.udp.UDPClient", autospec=True) @pytest.mark.asyncio async def test_connect(_: MagicMock) -> None: - t = SMPUDPTransport() + t = SMPUDPTransport(connect_timeout_s=0.001) t._client = cast(MagicMock, t._client) # type: ignore # Mock _transport for IPv4/IPv6 detection t._client._transport = MagicMock() t._client._transport.get_extra_info.return_value = None - await t.connect("192.168.0.1", 0.001) - t._client.connect.assert_awaited_once_with(Addr(host="192.168.0.1", port=1337)) + await t.connect(ADDRESS) + t._client.connect.assert_awaited_once_with(Addr(host=ADDRESS, port=1337)) + + +@patch("smpclient.transport.udp.UDPClient", autospec=True) +@pytest.mark.asyncio +async def test_connect_port(_: MagicMock) -> None: + t = SMPUDPTransport() + t._client = cast(MagicMock, t._client) # type: ignore + t._client._transport = MagicMock() + t._client._transport.get_extra_info.return_value = None + + await t.connect(ADDRESS, 1338) + t._client.connect.assert_awaited_once_with(Addr(host=ADDRESS, port=1338)) @patch("smpclient.transport.udp.UDPClient", autospec=True) @@ -132,13 +151,36 @@ def test_max_unencoded_size_custom_mtu() -> None: assert t.max_unencoded_size == 484 +@pytest.mark.parametrize( + "buf_size, expected", + [(384, 384), (1472, 1472), (2048, 1472)], +) +@pytest.mark.asyncio +async def test_max_unencoded_size_capped_by_server_buffer(buf_size: int, expected: int) -> None: + """Zephyr copies each datagram into one `buf_size` buffer, so neither bound may be exceeded.""" + t = await negotiated(SMPUDPTransport(mtu=1500), buf_size) + assert t.max_unencoded_size == expected + + +@pytest.mark.asyncio +@pytest.mark.parametrize("buf_size, expected", [(384, 384), (1472, 1472), (2048, 1472)]) +async def test_buffer_size_is_capped_by_the_mss_and_never_reads( + buf_size: int, expected: int +) -> None: + t = SMPUDPTransport(mtu=1500, fragmentation_strategy=BufferSize(buf_size)) + with advertise(4096) as read: + await t.negotiate() + read.assert_not_awaited() + assert t.max_unencoded_size == expected + + @pytest.mark.asyncio async def test_ipv4_detection_real_socket() -> None: """Test IPv4 auto-detection with real socket connection.""" - t = SMPUDPTransport(mtu=1500) + t = SMPUDPTransport(mtu=1500, connect_timeout_s=1.0) # Create a real UDP connection to localhost IPv4 - await t.connect("127.0.0.1", 1.0) + await t.connect("127.0.0.1") assert t._is_ipv6 is False assert t.max_unencoded_size == 1500 - IPV4_UDP_OVERHEAD @@ -150,10 +192,10 @@ async def test_ipv4_detection_real_socket() -> None: @pytest.mark.asyncio async def test_ipv6_detection_real_socket() -> None: """Test IPv6 auto-detection with real socket connection.""" - t = SMPUDPTransport(mtu=1500) + t = SMPUDPTransport(mtu=1500, connect_timeout_s=1.0) # Create a real UDP connection to localhost IPv6 - await t.connect("::1", 1.0) + await t.connect("::1") assert t._is_ipv6 is True assert t.max_unencoded_size == 1500 - IPV6_UDP_OVERHEAD