breaking: header and smp_data in Frame (related to #45) - #46
Draft
JPHutchins wants to merge 2 commits into
Draft
JPHutchins wants to merge 2 commits into
JPHutchins wants to merge 2 commits into
Conversation
Flattening of SMP Data fields into the _MessageBase is replaced by use of a Frame[T]: Frame(Header, T) where T is the generic SMPData. The _MessageBase class is removed. Commands must inherit from SMPData instead. to_frame() is available on SMPData instances to create a Frame[T]. Frames can be serialized like _MessageBase, though BYTES is removed (bytes() remains).
There was a problem hiding this comment.
Pull Request Overview
This PR implements a breaking change that restructures SMP message handling by introducing a Frame[T] generic type that separates header and SMP data. The _MessageBase class is removed and replaced with SMPData as the base class for commands, which can be converted to frames using the to_frame() method.
Key changes:
- Replace
_MessageBasewithSMPDatabase class andFrame[T]structure - Remove
BYTESproperty in favor ofbytes()method - Update all test files to use new
to_frame()method and access data viasmp_dataattribute
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| smp/message.py | Core implementation of new Frame[T] and SMPData classes |
| smp/init.py | Updated documentation and examples for new API |
| tests/helpers.py | Refactored test helpers to support new Frame structure |
| tests/test_*.py | Updated tests to use new Frame API and data access patterns |
| tests/user/test_intercreate.py | Updated intercreate tests for new Frame structure |
| tests/binary_regressions/test_binary_lock.py | Updated binary regression tests |
|
|
||
| def assert_response(r: smpimg.ImageStatesReadResponse) -> None: | ||
| d = cast(dict, cbor2.loads(r.BYTES[8:])) | ||
| d = cast(dict, cbor2.loads(bytes(r))) |
There was a problem hiding this comment.
The assert_response function is trying to load CBOR data from bytes(r) where r is an ImageStatesReadResponse (SMPData), but bytes(r) returns the CBOR-encoded data. The function should be extracting the CBOR payload from a frame, not the raw SMPData.
Suggested change
| d = cast(dict, cbor2.loads(bytes(r))) | |
| d = cast(dict, cbor2.loads(r.to_frame().payload)) |
| command_id: Any, | ||
| data: Dict[str, Any], | ||
| nested_model: Type[BaseModel] | None = None, | ||
| group_id: smphdr.GroupId = None, # type: ignore[assignment] |
There was a problem hiding this comment.
The parameter group_id has a type annotation of smphdr.GroupId but defaults to None. This creates a type inconsistency. Consider using smphdr.GroupId | None = None or making it a required parameter.
Suggested change
| group_id: smphdr.GroupId = None, # type: ignore[assignment] | |
| group_id: smphdr.GroupId | None = None, |
JPHutchins
force-pushed
the
breaking/fix/45/remove-frame-flattening
branch
from
August 4, 2025 02:54
8393404 to
e3af3db
Compare
|
I don't really have the time to review the PR in more detail, but some quick feedback is that I find the name "frame" here misleading, as in my head it refers to an SMP serial packet frame and not the SMP message itself. |
Contributor
Me too
But Zephyr says so 😜 |
|
@sgfeniex, thanks for pointing that out, I really don't know how I missed it 👀 I guess I was focused on the transport layer and the names "message", "packet" and "frame" got mixed up. I still think "SMP message" is a better name for the header + data, but the Zephyr documentation is as official as it gets, so frame it is 😃 |
This was referenced Jul 14, 2026
JPHutchins
added a commit
that referenced
this pull request
Jul 16, 2026
Replace the flattened pydantic message model with composition: an SMP
message is a `Frame[T] = (Header, Data[T])`, where a `Data` payload carries
no header until `to_frame()` wraps it — mirroring the Rust
`Message<T>{Header, SMPData<T>}`. De/serialization moves from pydantic + raw
`cbor2` to `msgspec.Struct` + `msgspec-cbor`.
Part of the `screaming-goblin` breaking effort (intercreate/smpmgr#103).
## Model
- `Data` is the CBOR payload; `Frame[T]` is `(Header, Data)`.
- `data.to_frame(*, version, flags, sequence) -> Frame[T]` synthesizes the
header; `bytes(frame)` is header + canonical CBOR.
- `Msg.loads(wire) -> Frame[T]` and `Msg.load(header, mapping) -> Frame[T]`
are the decode entry points. Encoding uses
`msgspec_cbor.encode(order="canonical")` (length-first, the wire order);
decoding uses `msgspec_cbor.decode`.
- Header/group-id classvars (`_OP`, `_GROUP_ID`, `_COMMAND_ID`, `_FLAGS`)
stay on `Data` subclasses.
## Decode boundary
- `loads()` validates `header.length == len(payload)` (`SMPMalformed`) and
`header.group_id == cls._GROUP_ID` (`SMPMismatchedGroupId`) before
decoding.
- msgspec cannot decode a union of more than one int-like type, so group ids
(`GroupId | UserGroupId | int`) and boot mode (`BootMode | int`) decode as
`int` and resolve to their real member types afterward. These per-message
`_convert_mapping` hooks route through msgspec (`_validate_mapping` +
`msgspec.convert`) so a malformed payload raises `msgspec.ValidationError`,
never a bare `KeyError`/`TypeError`.
- Value invariants run on construction via `__post_init__` (e.g.
`ResetWriteRequest` rejects a `boot_mode` outside `[0, 255]`); msgspec
type-checks field values at the decode boundary, which is where untrusted
wire bytes arrive.
## Groups
All groups ported (image, os, file, enumeration, settings, shell,
statistics, zephyr, error, user/intercreate). Notably in `os_management`:
`tasks` stays a sum type (`TaskStatistics | TaskStatisticsZephyr`,
discriminated structurally since the wire carries no tag); memory pools are a
bare dynamic map (`dict[str, MemoryPoolStatistics]`); the `no-downgrade`
field alias is preserved. `ErrorV2` recovers its rc enum by searching
`__orig_bases__` for the `ErrorV2` origin.
## Behavior changes
- `omit_defaults=True`: a field explicitly set to its default is no longer
emitted on the wire (pydantic's `exclude_unset` emitted it). Benign for
MCUmgr (absent == default), but explicit defaults no longer round-trip on
the byte stream.
- `boot_mode` out of range raises `ValueError` (was pydantic
`ValidationError`); the bootloader `response` decodes to the typed
`MCUbootModeQueryResponse` (the `Any` arm is gone).
- `pydantic` is removed from `[project].dependencies`; `pip install smp` no
longer pulls it.
## Verification
- Byte-exact: all 27,435 locked binary regressions pass unchanged
(`tests/binary_regressions`).
- Import speed (#26): cold import 312 ms -> 87 ms (3.6x); pydantic is off the
`image_management` path.
- `camas matrix` green on Python 3.10-3.14 (format, lint, mypy, pyright, full
test) and coverage 100%.
- Per-file `_do_test` duplication (#5) replaced by one composition-based
helper, `tests/helpers.py::assert_frame`.
Addresses #45, #26, #5; supersedes the pydantic-only attempt in #46. Targets
`screaming-goblin`, not the default branch — closing keywords belong on the
eventual `screaming-goblin` -> `main` PR.
Co-Authored-By: claude-opus-4-8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Eq2MMZAtCoSitXj3GQ5Lcz
JPHutchins
added a commit
that referenced
this pull request
Jul 16, 2026
Replace the flattened pydantic message model with composition: an SMP
message is a `Frame[T] = (Header, Data[T])`, where a `Data` payload carries
no header until `to_frame()` wraps it — mirroring the Rust
`Message<T>{Header, SMPData<T>}`. De/serialization moves from pydantic + raw
`cbor2` to `msgspec.Struct` + `msgspec-cbor`.
Part of the `screaming-goblin` breaking effort (intercreate/smpmgr#103).
## Model
- `Data` is the CBOR payload; `Frame[T]` is `(Header, Data)`.
- `data.to_frame(*, version, flags, sequence) -> Frame[T]` synthesizes the
header; `bytes(frame)` is header + canonical CBOR.
- `Msg.loads(wire) -> Frame[T]` and `Msg.load(header, mapping) -> Frame[T]`
are the decode entry points. Encoding uses
`msgspec_cbor.encode(order="canonical")` (length-first, the wire order);
decoding uses `msgspec_cbor.decode`.
- Header/group-id classvars (`_OP`, `_GROUP_ID`, `_COMMAND_ID`, `_FLAGS`)
stay on `Data` subclasses.
## Decode boundary
- `loads()` validates `header.length == len(payload)` (`SMPMalformed`) and
`header.group_id == cls._GROUP_ID` (`SMPMismatchedGroupId`) before
decoding.
- msgspec cannot decode a union of more than one int-like type, so group ids
(`GroupId | UserGroupId | int`) and boot mode (`BootMode | int`) decode as
`int` and resolve to their real member types afterward. These per-message
`_convert_mapping` hooks route through msgspec (`_validate_mapping` +
`msgspec.convert`) so a malformed payload raises `msgspec.ValidationError`,
never a bare `KeyError`/`TypeError`.
- Value invariants run on construction via `__post_init__` (e.g.
`ResetWriteRequest` rejects a `boot_mode` outside `[0, 255]`); msgspec
type-checks field values at the decode boundary, which is where untrusted
wire bytes arrive.
## Groups
All groups ported (image, os, file, enumeration, settings, shell,
statistics, zephyr, error, user/intercreate). Notably in `os_management`:
`tasks` stays a sum type (`TaskStatistics | TaskStatisticsZephyr`,
discriminated structurally since the wire carries no tag); memory pools are a
bare dynamic map (`dict[str, MemoryPoolStatistics]`); the `no-downgrade`
field alias is preserved. `ErrorV2` recovers its rc enum by searching
`__orig_bases__` for the `ErrorV2` origin.
## Behavior changes
- `omit_defaults=True`: a field explicitly set to its default is no longer
emitted on the wire (pydantic's `exclude_unset` emitted it). Benign for
MCUmgr (absent == default), but explicit defaults no longer round-trip on
the byte stream.
- `boot_mode` out of range raises `ValueError` (was pydantic
`ValidationError`); the bootloader `response` decodes to the typed
`MCUbootModeQueryResponse` (the `Any` arm is gone).
- `pydantic` is removed from `[project].dependencies`; `pip install smp` no
longer pulls it.
## Verification
- Byte-exact: all 27,435 locked binary regressions pass unchanged
(`tests/binary_regressions`).
- Import speed (#26): cold import 312 ms -> 87 ms (3.6x); pydantic is off the
`image_management` path.
- `camas matrix` green on Python 3.10-3.14 (format, lint, mypy, pyright, full
test) and coverage 100%.
- Per-file `_do_test` duplication (#5) replaced by one composition-based
helper, `tests/helpers.py::assert_frame`.
Addresses #45, #26, #5; supersedes the pydantic-only attempt in #46. Targets
`screaming-goblin`, not the default branch — closing keywords belong on the
eventual `screaming-goblin` -> `main` PR.
Co-Authored-By: claude-opus-4-8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Eq2MMZAtCoSitXj3GQ5Lcz
JPHutchins
added a commit
that referenced
this pull request
Jul 17, 2026
Replace the flattened pydantic message model with composition: an SMP
message is a `Frame[T] = (Header, Data[T])`, where a `Data` payload carries
no header until `to_frame()` wraps it — mirroring the Rust
`Message<T>{Header, SMPData<T>}`. De/serialization moves from pydantic + raw
`cbor2` to `msgspec.Struct` + `msgspec-cbor`.
Part of the `screaming-goblin` breaking effort (intercreate/smpmgr#103).
## Model
- `Data` is the CBOR payload; `Frame[T]` is `(Header, Data)`.
- `data.to_frame(*, version, flags, sequence) -> Frame[T]` synthesizes the
header; `bytes(frame)` is header + canonical CBOR.
- `Msg.loads(wire) -> Frame[T]` and `Msg.load(header, mapping) -> Frame[T]`
are the decode entry points. Encoding uses
`msgspec_cbor.encode(order="canonical")` (length-first, the wire order);
decoding uses `msgspec_cbor.decode`.
- Header/group-id classvars (`_OP`, `_GROUP_ID`, `_COMMAND_ID`, `_FLAGS`)
stay on `Data` subclasses.
## Decode boundary
- `loads()` validates `header.length == len(payload)` (`SMPMalformed`) and
`header.group_id == cls._GROUP_ID` (`SMPMismatchedGroupId`) before
decoding.
- msgspec cannot decode a union of more than one int-like type, so group ids
(`GroupId | UserGroupId | int`) and boot mode (`BootMode | int`) decode as
`int` and resolve to their real member types afterward. These per-message
`_convert_mapping` hooks route through msgspec (`_validate_mapping` +
`msgspec.convert`) so a malformed payload raises `msgspec.ValidationError`,
never a bare `KeyError`/`TypeError`.
- Value invariants run on construction via `__post_init__` (e.g.
`ResetWriteRequest` rejects a `boot_mode` outside `[0, 255]`); msgspec
type-checks field values at the decode boundary, which is where untrusted
wire bytes arrive.
## Groups
All groups ported (image, os, file, enumeration, settings, shell,
statistics, zephyr, error, user/intercreate). Notably in `os_management`:
`tasks` stays a sum type (`TaskStatistics | TaskStatisticsZephyr`,
discriminated structurally since the wire carries no tag); memory pools are a
bare dynamic map (`dict[str, MemoryPoolStatistics]`); the `no-downgrade`
field alias is preserved. `ErrorV2` recovers its rc enum by searching
`__orig_bases__` for the `ErrorV2` origin.
## Behavior changes
- `omit_defaults=True`: a field explicitly set to its default is no longer
emitted on the wire (pydantic's `exclude_unset` emitted it). Benign for
MCUmgr (absent == default), but explicit defaults no longer round-trip on
the byte stream.
- `boot_mode` out of range raises `ValueError` (was pydantic
`ValidationError`); the bootloader `response` decodes to the typed
`MCUbootModeQueryResponse` (the `Any` arm is gone).
- `pydantic` is removed from `[project].dependencies`; `pip install smp` no
longer pulls it.
## Verification
- Byte-exact: all 27,435 locked binary regressions pass unchanged
(`tests/binary_regressions`).
- Import speed (#26): cold import 312 ms -> 87 ms (3.6x); pydantic is off the
`image_management` path.
- `camas matrix` green on Python 3.10-3.14 (format, lint, mypy, pyright, full
test) and coverage 100%.
- Per-file `_do_test` duplication (#5) replaced by one composition-based
helper, `tests/helpers.py::assert_frame`.
Addresses #45, #26, #5; supersedes the pydantic-only attempt in #46. Targets
`screaming-goblin`, not the default branch — closing keywords belong on the
eventual `screaming-goblin` -> `main` PR.
Co-Authored-By: claude-opus-4-8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Eq2MMZAtCoSitXj3GQ5Lcz
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Flattening of SMP Data fields into the _MessageBase is replaced by use of a Frame[T]: Frame(Header, T) where T is the generic SMPData.
The _MessageBase class is removed. Commands must
inherit from SMPData instead. to_frame() is available on SMPData instances to create a Frame[T]. Frames
can be serialized like _MessageBase, though BYTES
is removed (bytes() remains).