Description
A code comment in serial.rs (line 169) explains why this duplication exists:
NOTE: The mctp-rs crate does not appear to support serializing requests and deserializing responses (only the opposite), so we have to do manual serialization until that is changed.
That comment is no longer accurate. MctpPacketContext::serialize_packet supports the host→EC direction and MctpPacketContext::deserialize_packet supports the EC→host direction.
The cost of the duplication is structural fragility: every time the wire format changes in embedded-services, ec-test-lib needs a parallel manual update. The most recent example is embedded-services#852, which made SmbusEspiMedium spec-compliant (emit + verify SMBus PEC byte). ec-test-lib did not emit PEC, so a manual fix was needed — see #92 for that change.
Proposed shape
Prerequisite (embedded-services, separate cross-org PR)
This repo's scope
Rewrite ec/test-lib/src/serial.rs::send to:
- Define a host-side
HostRequest enum (Battery / Thermal / TimeAlarm) and HostResponse enum. Impl MctpMessageTrait<'_> for both, mirroring the EC-side enums the macro generates.
- Construct an
OdpHeader { message_type: Request, service, message_id }.
- Use
MctpPacketContext::<SmbusEspiMedium>::new(...).serialize_packet(reply_context, (header, host_request)), iterate the returned SerializePacketState, write each packet to the UART.
- On RX: read bytes, call
SmbusEspiMedium::frame_complete(buf) to detect complete frames, then MctpPacketContext::deserialize_packet(frame) → Option<MctpMessage> (handles SOM/EOM/reassembly internally). On Some, call MctpMessage::parse_as::<HostResponse>().
- Delete
prepend_headers, append_cmd, Destination, all 8 hand-defined wire constants, the manual PEC compute/verify, and the multi-packet SOM/EOM loop.
Scope estimate
ec/test-lib/Cargo.toml: add mctp-rs as a direct dependency (currently transitive via embedded-services); drop smbus-pec (becomes mctp-rs transitive again).
ec/test-lib/src/serial.rs: replace ~80 LOC of hand-rolled framing with ~120 LOC of explicit HostRequest/HostResponse enums + MctpMessageTrait impls + new send(). Net ~+27 LOC but no implicit-flag-bit math, no hand-defined offsets, no parallel-source wire format.
- No public API change to
ec-test-cli (same CLI surface, same on-wire bytes).
Out of scope
- Adding new media to
ec-test-cli (e.g. --source dsp0253-serial using MctpSerialMedium). That becomes trivial once send() is parameterized over M: MctpMedium, but is a separate enhancement.
- Reorganizing the
Acpi (Windows IOCTL) or Mock data sources — they don't construct MCTP packets.
Acceptance
cargo build --release -p ec-test-cli and cargo clippy -p ec-test-cli -- -D warnings clean.
odp-embedded-controller's scripts/integration-test.sh continues to pass after EC_TEST_CLI_REV is bumped past the merge commit — same 18 commands (thermal × 8, battery × 3, rtc × 7) return valid responses.
- Wire-format bytes are byte-identical to pre-refactor when
cmd_code < 0x8000 (verifiable by capturing UART traffic with socat).
- The stale code comment at
serial.rs:169 is deleted.
Bonus payoff
Once parameterized over M: MctpMedium, a follow-up can add --source dsp0253-serial using MctpSerialMedium, letting ec-test-cli talk to dev-qemu configured with DSP0253 serial transport.
Sequencing
- PR on
embedded-services exposing OdpHeader + friends — merge first.
- This repo's PR consuming the newly-exposed types — merge second.
odp-embedded-controller bumps EC_TEST_CLI_REV.
This lands independently of and after #92 (the PEC fix).
Description
A code comment in
serial.rs(line 169) explains why this duplication exists:That comment is no longer accurate.
MctpPacketContext::serialize_packetsupports the host→EC direction andMctpPacketContext::deserialize_packetsupports the EC→host direction.The cost of the duplication is structural fragility: every time the wire format changes in
embedded-services,ec-test-libneeds a parallel manual update. The most recent example isembedded-services#852, which madeSmbusEspiMediumspec-compliant (emit + verify SMBus PEC byte).ec-test-libdid not emit PEC, so a manual fix was needed — see #92 for that change.Proposed shape
Prerequisite (
embedded-services, separate cross-org PR)This repo's scope
Rewrite
ec/test-lib/src/serial.rs::sendto:HostRequestenum (Battery / Thermal / TimeAlarm) andHostResponseenum. ImplMctpMessageTrait<'_>for both, mirroring the EC-side enums the macro generates.OdpHeader { message_type: Request, service, message_id }.MctpPacketContext::<SmbusEspiMedium>::new(...).serialize_packet(reply_context, (header, host_request)), iterate the returnedSerializePacketState, write each packet to the UART.SmbusEspiMedium::frame_complete(buf)to detect complete frames, thenMctpPacketContext::deserialize_packet(frame)→Option<MctpMessage>(handles SOM/EOM/reassembly internally). OnSome, callMctpMessage::parse_as::<HostResponse>().prepend_headers,append_cmd,Destination, all 8 hand-defined wire constants, the manual PEC compute/verify, and the multi-packet SOM/EOM loop.Scope estimate
ec/test-lib/Cargo.toml: addmctp-rsas a direct dependency (currently transitive viaembedded-services); dropsmbus-pec(becomesmctp-rstransitive again).ec/test-lib/src/serial.rs: replace ~80 LOC of hand-rolled framing with ~120 LOC of explicitHostRequest/HostResponseenums +MctpMessageTraitimpls + newsend(). Net ~+27 LOC but no implicit-flag-bit math, no hand-defined offsets, no parallel-source wire format.ec-test-cli(same CLI surface, same on-wire bytes).Out of scope
ec-test-cli(e.g.--source dsp0253-serialusingMctpSerialMedium). That becomes trivial oncesend()is parameterized overM: MctpMedium, but is a separate enhancement.Acpi(Windows IOCTL) orMockdata sources — they don't construct MCTP packets.Acceptance
cargo build --release -p ec-test-cliandcargo clippy -p ec-test-cli -- -D warningsclean.odp-embedded-controller'sscripts/integration-test.shcontinues to pass afterEC_TEST_CLI_REVis bumped past the merge commit — same 18 commands (thermal × 8, battery × 3, rtc × 7) return valid responses.cmd_code < 0x8000(verifiable by capturing UART traffic withsocat).serial.rs:169is deleted.Bonus payoff
Once parameterized over
M: MctpMedium, a follow-up can add--source dsp0253-serialusingMctpSerialMedium, lettingec-test-clitalk todev-qemuconfigured with DSP0253 serial transport.Sequencing
embedded-servicesexposingOdpHeader+ friends — merge first.odp-embedded-controllerbumpsEC_TEST_CLI_REV.This lands independently of and after #92 (the PEC fix).