Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ jobs:
python-version: ["3.10", "3.11", "3.12", "3.13", "3.14"]
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v6
with:
fetch-depth: 0

- uses: astral-sh/setup-uv@v6
- uses: astral-sh/setup-uv@v8.1.0

- run: uv run --python ${{ matrix.python-version }} task lint

Expand All @@ -28,7 +28,7 @@ jobs:

- run: uv build

- uses: actions/upload-artifact@v4
- uses: actions/upload-artifact@v7
with:
name: dist-${{ matrix.os }}-${{ matrix.python-version }}
path: |
Expand Down Expand Up @@ -80,11 +80,11 @@ jobs:
expect-hci-firmware: pass
name: transport-extras (${{ matrix.name }})
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v6
with:
fetch-depth: 0

- uses: astral-sh/setup-uv@v6
- uses: astral-sh/setup-uv@v8.1.0

- run: uv venv && uv pip install .${{ matrix.extras }}

Expand Down Expand Up @@ -126,8 +126,33 @@ jobs:
coverage:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@v6

- uses: astral-sh/setup-uv@v6
- uses: astral-sh/setup-uv@v8.1.0

- run: uv run task coverage

integration:
name: integration (native_sim + qemu)
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
with:
fetch-depth: 0

- uses: astral-sh/setup-uv@v8.1.0

- run: |
sudo dpkg --add-architecture i386
sudo apt-get update
sudo apt-get install -y --no-install-recommends qemu-system-arm libc6:i386

- run: uv run task test-integration

- name: Upload integration test logs
if: always()
uses: actions/upload-artifact@v7
with:
path: integration-tests.log
archive: false
if-no-files-found: warn
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ __pycache__
dist
site
coverage.xml
integration-tests.log
.claude/
flash.bin
14 changes: 8 additions & 6 deletions examples/usb/upgrade.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,10 @@ async def main() -> None:
print("Connecting to SMP DUT...", end="", flush=True)
async with SMPClient(
SMPSerialTransport(
max_smp_encoded_frame_size=max_smp_encoded_frame_size,
line_length=line_length,
line_buffers=line_buffers,
fragmentation_strategy=SMPSerialTransport.BufferParams(
line_length=line_length,
line_buffers=line_buffers,
)
),
port_a.device,
) as client:
Expand Down Expand Up @@ -187,9 +188,10 @@ async def ensure_request(request: SMPRequest[TRep, TEr1, TEr2]) -> TRep:
print("Connecting to B SMP DUT...", end="", flush=True)
async with SMPClient(
SMPSerialTransport(
max_smp_encoded_frame_size=max_smp_encoded_frame_size,
line_length=line_length,
line_buffers=line_buffers,
fragmentation_strategy=SMPSerialTransport.BufferParams(
line_length=line_length,
line_buffers=line_buffers,
)
),
port_b.device,
) as client:
Expand Down
9 changes: 7 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,9 @@ format = "ruff format ."
lint = "ruff check . && pydoclint src/smpclient"
fix = "ruff check --fix . && ruff format ."
typecheck = "mypy ."
test = "pytest --maxfail=1"
coverage = "pytest --cov --cov-report=xml --cov-report=term-missing"
test = "pytest --maxfail=1 -m 'not integration'"
test-integration = "pytest tests/integration --log-file=integration-tests.log --log-file-level=DEBUG --log-cli-level=INFO -o log_cli=true"
coverage = "pytest --cov --cov-report=xml --cov-report=term-missing -m 'not integration'"
all = "task format && task lint && task typecheck && task test"
matrix = """
UV_PROJECT_ENVIRONMENT=.venv-3.10 uv run --python 3.10 task all &&
Expand Down Expand Up @@ -118,10 +119,14 @@ check-yield-types = false
[tool.pytest.ini_options]
norecursedirs = ["dutfirmware/*", ".claude/*"]
filterwarnings = ["ignore:The --rsyncdir:DeprecationWarning"]
markers = [
"integration: end-to-end tests against real SMP servers (Linux only); see tests/integration/",
]

[tool.coverage.run]
source = ["smpclient", "tests"]
branch = true
omit = ["tests/integration/*"]

[tool.coverage.report]
fail_under = 91
Expand Down
107 changes: 84 additions & 23 deletions src/smpclient/transport/serial.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@
import math
import time
from enum import IntEnum, unique
from functools import cached_property
from typing import Final
from typing import Final, NamedTuple

try:
from serial import Serial, SerialException
Expand Down Expand Up @@ -59,11 +58,29 @@ class BufferState(IntEnum):
`_buffer` is being parsed as serial data.
"""

class Auto(NamedTuple):
"""Automatically determine buffer parameters from the SMP server.

On connect, queries the server's MCUMGR_PARAM for `buf_size`
(CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE) and calculates:
- line_length: 128 (standard MTU for uart/usb/shell)
- line_buffers: buf_size / line_length

Falls back to BufferParams() if server doesn't support MCUMGR_PARAM.
"""

class BufferParams(NamedTuple):
"""Buffer parameters for the serial transport."""

line_length: int = 128
"""The maximum SMP packet size."""

line_buffers: int = 1
"""The number of line buffers in the serial buffer."""

def __init__( # noqa: DOC301
self,
max_smp_encoded_frame_size: int = 256,
line_length: int = 128,
line_buffers: int = 2,
fragmentation_strategy: Auto | BufferParams = Auto(),
baudrate: int = 115200,
bytesize: int = 8,
parity: str = "N",
Expand All @@ -79,11 +96,10 @@ def __init__( # noqa: DOC301
"""Initialize the serial transport.

Args:
max_smp_encoded_frame_size: The maximum size of an encoded SMP
frame. The SMP server needs to have a buffer large enough to
receive the encoded frame packets and to store the decoded frame.
line_length: The maximum SMP packet size.
line_buffers: The number of line buffers in the serial buffer.
fragmentation_strategy: The fragmentation strategy to use. Either
`SMPSerialTransport.Auto()` to automatically determine buffer
parameters from the SMP server, or `SMPSerialTransport.BufferParams`
to manually specify buffer parameters.
baudrate: The baudrate of the serial connection. OK to ignore for
USB CDC ACM.
bytesize: The number of data bits.
Expand All @@ -98,18 +114,7 @@ def __init__( # noqa: DOC301
exclusive: The exclusive access timeout.

"""
if max_smp_encoded_frame_size < line_length * line_buffers:
logger.error(
f"{max_smp_encoded_frame_size=} is less than {line_length=} * {line_buffers=}!"
)
elif max_smp_encoded_frame_size != line_length * line_buffers:
logger.warning(
f"{max_smp_encoded_frame_size=} is not equal to {line_length=} * {line_buffers=}!"
)

self._max_smp_encoded_frame_size: Final = max_smp_encoded_frame_size
self._line_length: Final = line_length
self._line_buffers: Final = line_buffers
self._fragmentation_strategy: Final = fragmentation_strategy
self._conn: Final = Serial(
baudrate=baudrate,
bytesize=bytesize,
Expand Down Expand Up @@ -142,6 +147,62 @@ def _reset_state(self) -> None:
self._buffer = bytearray([])
self._buffer_state = SMPSerialTransport.BufferState.SERIAL

@property
def _line_length(self) -> int:
"""The maximum SMP packet size."""
if isinstance(self._fragmentation_strategy, SMPSerialTransport.Auto):
return self.BufferParams().line_length
else:
return self._fragmentation_strategy.line_length

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think that we dropped 3.9 support, so this should be replaced with exhaustive match case.


@property
def _line_buffers(self) -> int:
"""The number of line buffers."""
if isinstance(self._fragmentation_strategy, SMPSerialTransport.Auto):
if self._smp_server_transport_buffer_size is not None:
return self._smp_server_transport_buffer_size // self.BufferParams().line_length
return self.BufferParams().line_buffers
else:
return self._fragmentation_strategy.line_buffers

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch on the floor-division inconsistency — fixed in a714fc8, though the empirical result is the opposite of an overflow.

I probed the real servers (native_sim / QEMU / mps2): buf_size is CONFIG_MCUMGR_TRANSPORT_NETBUF_SIZE, which bounds the decoded SMP serial frame uint16 length | message | uint16 CRC16 reassembled into one netbuf — the base64/line framing is stripped before the netbuf. So the limit is simply buf_size - 4 (2-byte length + 2-byte CRC16), and buf_count is irrelevant to a single serial message (one netbuf per message).

Boundary, confirmed at three sizes (client allowed to send past the old cap):

buf_size buf_size - 4 buf_size - 3
96 accepted dropped
256 accepted dropped
384 accepted dropped

So the old base64_max(buf_size) - per_line_framing * (buf_size // line_length) was actually ~25-30% conservative (e.g. buf384 → 255 instead of 380), not over-permissive — and the floor-division line count you flagged fed that wrong framing term.

The fix drops the line-count dependency in Auto: max_unencoded_size = buf_size - (FRAME_LENGTH + CRC16). BufferParams mode (the encoded line-buffer model, e.g. MCUboot's 128×8 UART buffer) is unchanged. Locked by tests: a unit assert (buf_size - 4), an integration assert across the buffer matrix, and test_max_payload_roundtrip actually round-tripping a max-size message (including the sub-line-length buf96) against the real fixtures.


@property
def _max_smp_encoded_frame_size(self) -> int:
"""The maximum encoded frame size (line_length * line_buffers)."""
if isinstance(self._fragmentation_strategy, SMPSerialTransport.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
else:
return (
self._fragmentation_strategy.line_length * self._fragmentation_strategy.line_buffers
)

@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
"""
super().initialize(smp_server_transport_buffer_size)

if isinstance(self._fragmentation_strategy, SMPSerialTransport.Auto):
logger.info(
f"Auto-configured from server: {self._line_length=}, "
f"{self._line_buffers=}, mtu={self._max_smp_encoded_frame_size}"
)
else:
# Validate user's BufferParams against server capabilities
calculated_size = (
self._fragmentation_strategy.line_length * self._fragmentation_strategy.line_buffers
)
if calculated_size > smp_server_transport_buffer_size:
logger.warning(
f"BufferParams (line_length={self._fragmentation_strategy.line_length} * "
f"line_buffers={self._fragmentation_strategy.line_buffers} = {calculated_size}) " # noqa: E501
f"exceeds server buffer size ({smp_server_transport_buffer_size})"
)

@override
async def connect(self, address: str, timeout_s: float) -> None:
self._reset_state()
Expand Down Expand Up @@ -353,7 +414,7 @@ def mtu(self) -> int:
return self._max_smp_encoded_frame_size

@override
@cached_property
@property
def max_unencoded_size(self) -> int:
"""The serial transport encodes each packet instead of sending SMP messages as raw bytes."""
# For each packet, AKA line_buffer, include the cost of the base64
Expand Down
35 changes: 35 additions & 0 deletions tests/fixtures/smp-server/SHA256SUMS
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
ea06893a7884a9e855ae09e592cbb9e5064fc9e3ab68c70ac238700b3c7ae510 manifest.json
ded3da56c3ff6d85d1ec80359c31e8d2b58d7987d0b71759678dedb20b0d0449 zephyr_4.4.0_smp_server_2ad0d6bf_mps2_an385_serial.elf
b17a5dbbd22e00e6e7b081e4e7e0996d8bfb4f87c1de245b96800a6e96b54d32 zephyr_4.4.0_smp_server_2ad0d6bf_mps2_an385_serial_recovery.hex
e31c5be010e916bfa1187feb0ef0d7bcd3fd1c209eea9b26b4fc2b87d086fcc5 zephyr_4.4.0_smp_server_2ad0d6bf_mps2_an385_serial_recovery.signed.bin
54ddb241d86cd2b370ccb8147c09bccde2894929f1943fa3b047a9cfcd3aa605 zephyr_4.4.0_smp_server_2ad0d6bf_mps2_an385_shell.elf
0e7954777e82c7177de74613bdb1547f2426f6d04b0701ecbc2c5d7da9365c62 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial.exe
129b43ca8ed6bde8f50decfdeb6e7da0240e246aa4f9a19fae65601e44061eb4 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_bigrx.exe
19787661a316346863f764f8207ce397fe0f0d1365704c14781fc0c6864a17b7 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_buf1024.exe
c7135af5c29f5a90f74577d2fafb302f07102865ef1932e8618f4087c7a54f45 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_buf128.exe
dc105ccbffc7572fa08ce76ff910a775d0fd36e4f0fa18acfaa93da6323c4503 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_buf2048.exe
20da65e7d19438bb149f5f802903b5fd7d090e9d93b77b71cc394e178303b0c7 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_buf256.exe
4e4684c543a48bfa06b117396ea31ef99596af56bfbcc2e8a3b5925c9a87c85a zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_buf400.exe
8e059883f00525426887e8e8755bc8ad0c34cd2594117b1833dc16f725c89541 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_buf512.exe
3f5dc33e9ba02b9e97025edea90f3aa630924908c5a974df4af68f59e698da59 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_buf96.exe
7dc763219a94bd3870b19aba67ca2cfcfc959bd94848ff98f3f45d1dcef7f7cf zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_fs.exe
61b816356fa9d0bfed635d3b1108e1fd65c32fd17886877446794efda1177ee4 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_line512.exe
3edf11b30d1ef42feb23ca022f3bc9fdb3cae21b4ca5629316c4b7ba0db18a4a zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_noparams.exe
c1f969fd23401cd2d0ae6a0c9a9ab25e4e2beb30c3c78a60edc8d6d32a9d8028 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_serial_raw.exe
9248791d2cc80a31501b6ac22f30ef81eb8c43f8a4d134885f41af983ccc36f4 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_shell.exe
554ae77bdf6025b2877fbc998680d08c79a3f12b3aff76f3fc7b7cb67e1e8270 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_udp.exe
4a5d38516fa8367a894b7faf1d39d1607b962aae4f3bacb9019b13c0dd25e585 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_udp6.exe
5efb203b9cd8a4a9116ffe4d940d1bb1b6f4210479cc32ae0c62996bee073afd zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_udp_buf1024.exe
e135910bc35393aa5dc8923ca5fafbd0ca4daffa8acadbe28704e318d2356fea zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_udp_buf2048.exe
3e4c64a4d713507903de807f5a547f0cc1757bbb14db901ad25f4e3cd7b189b3 zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_udp_buf256.exe
4289d6bf82292aa36cf291ea3ea660a18c9b8f367febcfc9defa4f3c0c823a4e zephyr_4.4.0_smp_server_2ad0d6bf_native_sim_udp_fs.exe
a714eb5caf48f324fd51367eca27eb01e9f202e8f01178b556e578e942277aff zephyr_4.4.0_smp_server_2ad0d6bf_nrf52840dk_ble.merged.hex
736eae2bb5d8d875eec0964ccb23db911aa53aa1eaedab1a81569074ad13847e zephyr_4.4.0_smp_server_2ad0d6bf_nrf52840dk_ble.signed.bin
de27f6cf50a92e2513cc4af00f5197f050d4ad00451bee26d373119e33358fb3 zephyr_4.4.0_smp_server_2ad0d6bf_nrf52840dk_serial.merged.hex
54959d5f5a960e94f1bc3dd0c3e521d5dabd4986dce0b291261d6c01104366ac zephyr_4.4.0_smp_server_2ad0d6bf_nrf52840dk_serial.signed.bin
e3e6c5c51bceaddc5f5e378b9032fc9c3583eb0b15b59fc7128962a92034a45e zephyr_4.4.0_smp_server_2ad0d6bf_nrf52840dk_serial_recovery.merged.hex
452c0289779d4a7d95f2d348ac47e254fb49087d4f718a5a0d3e2146f3a54637 zephyr_4.4.0_smp_server_2ad0d6bf_nrf52840dk_serial_recovery.signed.bin
9c085e0bb54210f22ff9121323eec40cb7dcd8317a5a191937261efb947843b4 zephyr_4.4.0_smp_server_2ad0d6bf_qemu_cortex_m0_serial.merged.hex
2ebb423a6fbffacb2508e476e8ff36eca89b9dc4fa788ad90fe5616738533692 zephyr_4.4.0_smp_server_2ad0d6bf_qemu_cortex_m0_serial.signed.bin
a8c21413eef8b2572f088a7728ca1a6da4ce3297c1306205c4b3c45230cfec1a zephyr_4.4.0_smp_server_2ad0d6bf_qemu_cortex_m0_serial_buf256.hex
0b8ed3fb9533df117c4e5fb27f1e16352fef6b90225ae8b2fd7dcd1468bbc1e4 zephyr_4.4.0_smp_server_2ad0d6bf_qemu_cortex_m0_serial_buf512.hex
18 changes: 18 additions & 0 deletions tests/fixtures/smp-server/VERSION
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Vendored SMP server fixtures
#
# Prebuilt Zephyr SMP servers used by the integration suite (tests/integration/).
# Source: https://github.com/intercreate/smp-server-fixtures
#
# Pinned release (permanent, per-commit — NOT the rolling `latest` tag, which
# can drift / be replaced):
# tag: 2ad0d6bf https://github.com/intercreate/smp-server-fixtures/releases/tag/2ad0d6bf
# zephyr: 4.4.0
#
# The integration harness builds its fixture registry from the vendored
# `manifest.json` (also from this release) and verifies each artifact against
# `SHA256SUMS` before launch; see tests/integration/servers.py. Only the subset
# of artifacts actually committed here become live fixtures — the manifest lists
# the full release so adding one is just `gh release download <tag> --pattern ...`.
#
# To bump: re-download the artifacts + SHA256SUMS + manifest.json from a newer
# per-commit release and update the `tag` above.
Loading
Loading