fix(sp-mctp-framer): close v1.3 review findings + inline tests + test-fixtures feature#67
Closed
dymk wants to merge 5 commits into
Closed
fix(sp-mctp-framer): close v1.3 review findings + inline tests + test-fixtures feature#67dymk wants to merge 5 commits into
dymk wants to merge 5 commits into
Conversation
Add explicit `channel = "1.92.0"` to the workspace rust-toolchain.toml and `rust-version = "1.92"` to [workspace.package]. Wire each member crate's [package] to opt in via `rust-version.workspace = true` (Cargo's [workspace.package] does NOT auto-inherit; explicit per-member opt-in is required, mirroring `version.workspace = true` / `edition.workspace = true`). Members opted in: dev-hooks, ec-service-lib, espi-device, espi-device-stub, platform/ihv1-sp, platform/qemu-sp, aarch64-haf. odp-ffa is left untouched — it pre-pins `rust-version = "1.75"` directly; that pre-existing decision is preserved and noted in the phase SUMMARY. Devcontainer + CI already run 1.92.0; this locks that floor declaratively against silent regression below mctp-rs's edition-2024 1.85 requirement. Per planning Phase 13 D-1, D-2, D-3. Closes/absorbs backlog 999.1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds mctp-rs at the EC-matching git pin (rev 3d941ba5205ca7781bf37e3dc7c5dfdc99a082d6) to [workspace.dependencies] for SP↔EC MCTP framing interop. Pin is byte-for-byte identical to EC's mod/ec/platform/dev-qemu/Cargo.lock (verified by Phase 12 spike, see .planning/phases/12-.../12-VERDICT.md section (a)). Bumps num_enum floor from 0.7.3 to 0.7.4 in workspace deps; resolver locks 0.7.6 (latest matching ^0.7.4) — same minor mctp-rs requires transitively. Direct workspace consumers via num_enum.workspace = true (ec-service-lib, espi-device, odp-ffa) all rebuild cleanly for aarch64-unknown-none-softfloat. Devcontainer `make all` end-to-end green. Note: mctp-rs is declared in [workspace.dependencies] but does not yet appear in Cargo.lock — cargo only adds workspace deps to the lockfile once a member consumes them. The lockfile entry materializes in the next commit (planning Plan 15-02) when sp-mctp-framer declares mctp-rs.workspace = true. Per planning Phase 15 D-1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds sp-mctp-framer as a workspace member, consuming the mctp-rs
workspace dep added in the previous commit. The crate is no_std with
no heap on the runtime path (Box/Vec/String absent from src/lib.rs);
only test code uses pretty_assertions for diff readability.
Public API:
- encode_battery_request(buf, battery_id) -> Result<usize, FramerError>
- decode_battery_response(buf) -> Result<BatteryResponse, FramerError>
Validated by host-side cargo test golden-bytes round-trip against the
Phase 12 spike captures (TX 18B + RX 13B). Encoder produces the
captured 18-byte sequence byte-for-byte; decoder parses the captured
13-byte response into structured fields. See
.planning/phases/12-spike-mctp-uart-ping/12-VERDICT.md §(c) for
provenance.
Source-confirmed constants (the planning artifact had three swap-
direction errors; all caught and corrected against upstream source):
1. SMBUS slave addresses: dst=1, src=0 (matching uart-service:75-76).
mctp-rs SmbusEspiMedium::serialize swaps the reply-context fields,
so reply_ctx.{dst,src}=(1,0) lands as (0,1) on wire — combined
with the wire byte-order convention this produces the captured
`00 0f 0e 03` SmbusEspi header (where 0x0f is the MCTP command
code, NOT a slave address as the plan mistakenly assumed).
2. MCTP endpoint IDs: source=0x08(BATTERY), destination=0x80(SP).
mctp-rs serialize.rs:62-63 swaps these too — to put dst_eid=0x08
on wire we set source=0x08.
3. ODP relay-header bit-extraction (from
embedded-services::relay::mctp bitfield!):
is_request: raw bit 25
service_id: raw bits 23..16
is_error: raw bit 15
message_id: raw bits 14..0 (15-bit field, widened from u8 to u16)
Decoder appends a placeholder PEC byte before delegating to
mctp-rs::SmbusEspiMedium::deserialize (which requires but does not
validate the trailing PEC). Encoder drops the trailing PEC from
mctp-rs's output to match uart-service's on-wire form. Both
documented inline.
Closes Phase 15 MF-01, MF-02, MF-03.
Per planning Phase 15 D-1, D-2.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-fixtures feature Address 9 v1.3 review findings on the framer crate: - M2: decode_battery_response now hard-errors (Truncated) on a body slice that under-runs the SmbusEspi-claimed byte_count, instead of silently returning an empty body. - F-3: oversized byte_count beyond the input buffer is mapped to BufTooSmall rather than the (more confusing) Truncated. - L-doc-3: body header bytes are now derived from odp_header.to_u32().to_be_bytes() at runtime instead of a hard-coded literal, so the encode path can never drift from the header struct. - M-doc-2: rename OdpRelayBodyOwned -> OdpRelayBodyDecode (the type is not actually owned; it borrows from the caller's RX buffer). - M5: introduce a 'test-fixtures' Cargo feature that re-exports the Phase 12 captured wire bytes (PHASE_12_TX_18B / PHASE_12_RX_13B) as pub constants. Replaces the bit-identical duplicate previously living in the OUTER mctp_ping.rs. - T-1, T-2, T-7, T-10, F-3 tests: inline 'mod tests' into lib.rs and add coverage for the truncation, oversize, partial-body, encode/ decode roundtrip, and 17/18-byte boundary cases. Also rewrite Cargo.toml to use the workspace pretty_assertions, declare peer-crate metadata (repository / readme / keywords), and gate the new fixtures behind the test-fixtures feature. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4 tasks
Cargo Vet Audit Failed
If the unvetted dependencies are not neededPlease modify Cargo.toml file to avoid including the dependencies. If the unvetted dependencies are neededPost a new comment with the questionnaire below to the PR to help the auditors vet the dependencies. Copy and paste the questionnaire as a new comment and provide your answers:1. What crates (with version) need to be audited? 2. How many of the crates are version updates vs new dependencies? 3. To confirm none of the already included crates serve your needs, please provide a brief description of the purpose of the new crates. 4. Any extra notes to the auditors to help with their audits. |
Rename and generalize the public API so the framer is no longer
hard-coded to Battery::GetSta:
encode_battery_request(buf, battery_id)
→ encode_request(buf, service_id, message_id, payload)
decode_battery_response(buf) -> BatteryResponse
→ decode_response(buf) -> OdpRelayResponse
The framer now takes per-service identity from its caller (service_id
and message_id) and an arbitrary-length payload slice, and prepends the
4-byte ODP relay header echo internally. The previously-hard-coded
constants (BATTERY_SVC_EID, BATTERY_GETSTA_MSG_ID, BATTERY_SVC_ID) are
removed; battery-specific values now live in the consumer
(`mctp_ping.rs` in the platform crate).
Implementation notes:
* Internal `OdpRelayBody` is now `&'a [u8]` instead of `[u8; 5]`, so
payload length is dynamic. Caller-side payload bound is
`MAX_PAYLOAD_LEN` (publicly exposed); over-bound payloads surface as
`BufTooSmall` (configuration error) not `EncodeFailed` (mctp-rs
rejection) — new `encode_oversize_payload_errors_as_buf_too_small`
test pins this.
* Min output buffer size is now derived per-payload (was const 18).
* SCRATCH_SIZE / FRAME_OVERHEAD constants extracted so the buffer-sizing
math is auditable in one place.
* `test_fixtures::PHASE_12_TX_18B` / `PHASE_12_RX_13B` retain their
battery-specific names — they ARE captured battery traffic and serve
as the wire-format reference for both encode and decode validation.
* Inline tests parameterize on local `BATTERY_SVC_ID` /
`BATTERY_GETSTA_MSG_ID` constants so they continue to validate against
the captured fixtures while exercising the generic API surface.
Test results:
cargo test -p sp-mctp-framer --target x86_64-unknown-linux-gnu
→ 9 passed (was 8; new test covers oversize payload path)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes 9 v1.3 review findings on the
sp-mctp-framercrate (used byodp-platform-qemu-sbsaSP↔EC MCTP-over-eSPI transport), then generalizes the public API so the framer is no longer hard-coded toBattery::GetSta. All changes are framer-internal — the consumer change is one commit on the companion PR.Changes
Generic API (top commit)
encode_battery_request(buf, battery_id)→encode_request(buf, service_id, message_id, payload).decode_battery_response(buf) -> BatteryResponse→decode_response(buf) -> OdpRelayResponse.OdpRelayBodynow borrows a variable-length payload slice instead of owning a fixed[u8; 5].pub const MAX_PAYLOAD_LENexposes the upper bound; over-bound payloads surface asBufTooSmall(configuration error) rather thanEncodeFailed(mctp-rs rejection).BATTERY_SVC_EID,BATTERY_GETSTA_MSG_ID,BATTERY_SVC_ID) removed from the framer; they now live in the consumer where they belong.Correctness (review-fix commit)
decode_response(formerlydecode_battery_response) now hard-errorsTruncatedwhen the body slice under-runs the SmbusEspi-claimedbyte_count, instead of silently returning an empty body.byte_countbeyond the input buffer is mapped toBufTooSmallrather than the more-confusingTruncated.odp_header.to_u32().to_be_bytes()instead of a hard-coded literal, so the encode path can never drift from the header struct.API hygiene
OdpRelayBodyOwned→OdpRelayBodyDecode(the type isn't owned; it borrows from the caller's RX buffer).test-fixturesCargo feature re-exports the captured wire-byte fixtures (PHASE_12_TX_18B/PHASE_12_RX_13B) aspubconstants. Replaces a bit-identical duplicate previously living in the consumer crate. The fixtures intentionally retain their battery-specific names — they ARE captured battery traffic and serve as the wire-format reference.Tests + meta
mod testsintolib.rscovering truncation, oversize, partial-body, encode/decode round-trip, the 17/18-byte boundary, and the new oversize-payload path.pretty_assertions, peer-crate metadata (repository/readme/keywords), README.Test plan
The downstream end-to-end test (
odp-platform-qemu-sbsamake -C e2e-tests test-serial) was also re-run against an earlier commit on this branch and producedMCTP_PING_OK service_id=8 msg_id=1 is_error=1. Will be re-verified against the post-refactor tip before the companion PR is marked ready-for-review.Dependency
This is the leaf PR. A companion PR will follow against
OpenDevicePartnership/odp-platform-qemu-sbsathat bumps this submodule and adopts the new generic API surface.