Add SMBus PEC byte to ec-test-lib serial transport#92
Merged
Conversation
The serial transport in ec-test-lib currently sends MCTP frames without the trailing SMBus PEC (Packet Error Code) byte that the SMBus protocol specifies as the trailing byte of each frame. embedded-services' uart-service historically compensated for this on the device side by stripping the PEC byte on TX and skipping the PEC byte on RX. embedded-services PR OpenDevicePartnership/embedded-services#852 makes uart-service strictly spec-compliant: SmbusEspiMedium now emits a PEC byte on every TX frame and requires a PEC byte on every RX frame. Without a matching change on the host side, `ec-test-cli --source serial` against any post-#852 uart-service consumer (dev-imxrt, dev-mcxa, dev-npcx, dev-qemu) hangs waiting for a PEC byte the EC never sends, and the extra PEC byte sent by ec-test-cli corrupts the EC's next request. This change adds PEC computation on TX (one byte appended after the existing frame) and verification on RX (one byte read + compared against `smbus_pec::pec()` over the received frame), using the `smbus_pec` crate already in the transitive dependency graph. Coordination note: this PR and embedded-services #852 must land in tight sequence. After #852 merges and `odp-embedded-controller` bumps its `embedded-services` pin past it, `EC_TEST_CLI_REV` in `.github/workflows/check.yml` must be bumped past this commit at the same time, otherwise the `integration-test:` CI job added by OpenDevicePartnership/odp-embedded-controller#14 will fail. Validated end-to-end against `odp-embedded-controller`'s `scripts/integration-test.sh` running dev-qemu built against embedded-services with PR #852 applied: all 18 commands (thermal x8, battery x3, rtc x7) succeed. Assisted-by: GitHub Copilot CLI:claude-opus-4.7-1m-internal
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates ec-test-lib’s serial transport to be SMBus-spec compliant by adding Packet Error Code (PEC) handling on both transmit (append) and receive (verify), aligning host behavior with updated uart-service expectations.
Changes:
- Add the
smbus-peccrate as a direct dependency for PEC calculation. - Append a computed PEC byte after each transmitted SMBus-framed request.
- Read and validate the trailing PEC byte on each received response packet, returning a protocol error on mismatch.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ec/test-lib/src/serial.rs | Compute and append PEC on TX; read and verify PEC on RX for each packet. |
| ec/test-lib/Cargo.toml | Add smbus-pec = "1.0.1" dependency used by the serial transport. |
| ec/Cargo.lock | Lockfile update to include smbus-pec in the resolved dependency graph. |
RobertZ2011
approved these changes
May 20, 2026
kurtjd
approved these changes
May 20, 2026
Member
There was a problem hiding this comment.
Looks good, thanks!
So should we wait for OpenDevicePartnership/embedded-services#852 to be merged, then odp-embedded-controller-updated, before we merge this?
philgweber
approved these changes
May 20, 2026
tullom
approved these changes
May 20, 2026
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.
Summary
Adds SMBus PEC (Packet Error Code) byte computation on TX and verification on RX to
ec-test-lib's serial transport, bringing it in line with the SMBus spec.Motivation
embedded-servicesPR OpenDevicePartnership/embedded-services#852 makesuart-service'sSmbusEspiMediumstrictly spec-compliant: it now emits a PEC byte on every TX frame and requires one on every RX frame. Without a matching host-side change,ec-test-cli --source serialagainst any post-#852uart-serviceconsumer hangs — the EC waits for a PEC byte the host never sends, and the host waits for a PEC byte the EC includes but the host doesn't expect.Changes
ec/test-lib/Cargo.toml: addsmbus-pec = "1.0.1"direct dependencyec/test-lib/src/serial.rs:smbus_pec::pec()byte after the existing frame writesmbus_pec::pec()over the received frame; returnError::Protocolon mismatchValidation
Test procedure
The goal was to validate that
ec-test-cli --source serialworks correctly against adev-qemuEC firmware built with the post-#852embedded-services(which emits and requires PEC bytes).Environment:
odp-platform-qemu-sbsadevcontainer (providesqemu-system-riscv32v10.0.0).Step 1 — Build
dev-qemuwith post-#852embedded-services:In
odp-embedded-controller, temporarily patchedplatform/dev-qemu/Cargo.tomlto:uart-serviceto thedymk/phase-34-uart-service-genericbranch ofdymk/embedded-services(which contains PR #852's changes: genericService<R, M>, spec-compliantSmbusEspiMediumwith PEC emit/verify,frame_complete()requiring PEC byte)[patch]section redirecting allembedded-servicescrates to the same fork branch (required for trait coherence sinceplatform-commonalso depends onembedded-services)MctpSerialMedium(Phase 35's DSP0253 choice) toService::default_smbusespi(relay)so the EC speaks the SmbusEspi wire format thatec-test-cliusesBuilt inside the devcontainer:
Step 2 — Build
ec-test-cliwith this PR's PEC changes:In
odp-platform-common/ec, applied this PR's changes totest-lib/Cargo.tomlandtest-lib/src/serial.rs, plus a temporary[patch]block inec/Cargo.tomlredirectingembedded-servicesto the samedymkfork branch (so theSerializableMessagetrait + message types match betweenec-test-clianddev-qemu):cd ec cargo build --release -p ec-test-cliThe
[patch]block is test-only — it is NOT part of this PR. In production,ec-test-cli's message struct shapes come from the upstreamv0.2.0branch which is wire-compatible.Step 3 — Run the integration test:
Copied the built
ec-test-clibinary into the devcontainer's PATH, then ranodp-embedded-controller's existingscripts/integration-test.sh(added by OpenDevicePartnership/odp-embedded-controller#14). This script:dev-qemuwithcargo run-headless(QEMU with-serial pty)/dev/pts/Npathec-test-cli --port /dev/pts/N <service> <command>invocationsAll 18 commands returned valid, non-error responses.
Step 4 — Verified the FAILURE case without this PR's changes:
Also ran the same test with an UNPATCHED
ec-test-cli(no PEC changes) against the same post-#852dev-qemu. Result: immediate timeout on the first command (thermal get-temperature), confirming the PEC mismatch is the root cause:Step 5 — Cleanup:
All temporary patches (
[patch]blocks, medium swap indev-qemu/src/main.rs,ec-test-clibinary in devcontainer) were reverted. Both repos' working trees verified clean.Coordination note
This PR and
embedded-services#852must land in tight sequence. After #852 merges andodp-embedded-controllerbumps itsembedded-servicespin past it,EC_TEST_CLI_REVin.github/workflows/check.ymlmust be bumped past this commit at the same time — otherwise theintegration-test:CI job will fail.Follow-up
A structural refactor to use
mctp-rsdirectly inec-test-lib::serial(eliminating the remaining hand-rolled MCTP framing) is planned as a separate followup PR.