Make uart-service::Service generic over MctpMedium; delete fake-PEC strips#852
Make uart-service::Service generic over MctpMedium; delete fake-PEC strips#852dymk wants to merge 5 commits into
Conversation
dymk
force-pushed
the
dymk/phase-34-uart-service-generic
branch
from
7ee94ae to
c5a4fb7
Compare
dymk
requested review from
jerrysxie,
kurtjd,
philgweber,
tullom and
williampMSFT
There was a problem hiding this comment.
Pull request overview
This PR makes uart-service usable with multiple MCTP media by parameterizing the service over mctp_rs::MctpMedium, and adds a new MctpMedium::frame_complete() hook so byte-stream transports can detect medium frame boundaries generically. It also removes SMBus PEC-related workarounds in uart-service and espi-service now that PEC is handled correctly by SmbusEspiMedium’s (de)serialization.
Changes:
- Add
MctpMedium::frame_complete(buf) -> Result<Option<usize>, _>and implement it forSmbusEspiMedium,MctpSerialMedium, andTestMedium. - Refactor
uart-service::ServicetoService<R, M>with aDefaultService<R> = Service<R, SmbusEspiMedium>convenience alias/constructor. - Add
MctpMessageBuffer::body()andmessage_type()accessors and remove fake PEC stripping inuart-serviceandespi-service.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| uart-service/src/task.rs | Updates UART task signature for generic Service<R, M> and adjusts error logging. |
| uart-service/src/lib.rs | Makes UART service generic over MctpMedium, adds framing-based RX loop, introduces DefaultService. |
| mctp-rs/src/test_util.rs | Implements frame_complete() for the test medium. |
| mctp-rs/src/medium/smbus_espi.rs | Implements frame_complete() for SMBus/eSPI framing using the header byte_count. |
| mctp-rs/src/medium/serial.rs | Implements frame_complete() for DSP0253 serial by scanning for the end flag. |
| mctp-rs/src/medium/mod.rs | Extends the MctpMedium trait with the new frame_complete() requirement and docs. |
| mctp-rs/src/mctp_packet_context.rs | Makes MctpReplyContext Clone + Copy to support storage in uart-service::Service. |
| mctp-rs/src/lib.rs | Updates docs for new trait method and adds MctpMessageBuffer accessors. |
| espi-service/src/espi_service.rs | Removes fake PEC injection/stripping so packets are handled as-is by the medium. |
kurtjd
left a comment
kurtjd
left a comment
There was a problem hiding this comment.
Looks good, and you said there will be a follow up PR in odp-embedded-controller, but has this been validated with the ec-test-tui/cli using the serial source (e.g.: https://github.com/OpenDevicePartnership/odp-platform-common/blob/main/ec/test-lib/src/serial.rs)?
If it doesn't work and require changes, you will need a simultaneous PR put up over there to make sure they agree. I would like to have PRs up here, in odp-emebdded-controller, and in odp-platform-common to make sure verything is updated simultaneously.
dymk
force-pushed
the
dymk/phase-34-uart-service-generic
branch
2 times, most recently
from
3023f6c to
876878c
Compare
dymk
force-pushed
the
dymk/phase-34-uart-service-generic
branch
3 times, most recently
from
ec56d26 to
f8a6988
Compare
This doesn't address my concern. The ec-tets-tui/cli currently manually push bytes over serial in a way that should agree with what the uart-service is expecting. I just wanted to ensure that the Also, the followup PR shouldn't just be scoped to dev-qemu. All the dev-platforms instantiate the uart-service, and the ec-test-tui is in fact expected to talk to all of them over the |
|
@kurtjd re: breaking the TUI app - I'll put up a PR to make the changes there as well to fix the smbus/mctp workaround. Does it support being ran outside of a WinVOS image and talking to a virtual serial port? If so it'd be nice to have it as part of the E2E tests that run in CI. |
Yes, the ec-test-app can be run from any host machine with the I recently integrated it into CI on the odp-embedded-controller side here: OpenDevicePartnership/odp-embedded-controller#14 |
Add a new method `frame_complete(&self, buf: &[u8]) -> Result<Option<usize>, Self::Error>` to the `MctpMedium` trait that returns `Ok(Some(len))` when buf contains a complete medium-framed packet starting at buf[0], `Ok(None)` when buf has a partial packet, `Err(...)` when buf is malformed. This lets generic consumers (uart-service::Service<R, M>) read packets from byte streams without knowing the framing details: - `SmbusEspiMedium::frame_complete` reads the 4-byte SMBUS header, parses the byte_count field, and returns `Ok(Some(4 + byte_count + 1))` when the full body+PEC has been received. Length-prefix framing. - `MctpSerialMedium::frame_complete` scans buf for the 0x7E end-flag (skipping a leading flag if present) and returns the position +1. DSP0253 sentinel framing. BREAKING CHANGE: this is a new required trait method (no default impl). Both shipped media (SmbusEspi + Serial) are updated in this same commit so the change is non-breaking at the workspace level. Out-of-tree implementations of `MctpMedium` (none known) will need to add a `frame_complete` impl. Coordinated with the uart-service genericization commit later in this PR. Also fixes a pre-existing rustdoc intra-doc-link in `MctpMedium::Encoding`'s doc comment (introduced by the in-tree mctp-rs port in upstream PR OpenDevicePartnership#844) that points at the now-private `crate::buffer_encoding` module. Re-points the link to the public `crate::BufferEncoding` and `crate::PassthroughEncoding` re-exports. Surfaced as a CI failure on the nightly/doc job on this PR's first push; trivial to fix here vs as a separate upstream PR.
…delete fake-PEC strips
Make `embedded-services::uart-service::Service<R: RelayHandler>` →
`Service<R: RelayHandler, M: MctpMedium + Copy>`. Move the SmbusEspi-specific
reply-context construction out of `process_response` into a new
`Service::new(relay, medium, reply_context)` constructor signature, storing
`medium` and `reply_context` as fields. Replace the inlined SMBUS-header
parse in `wait_for_request` with a generic incremental-read loop that calls
`self.medium.frame_complete(...)` (added in the previous commit).
Backwards-compat:
- `pub type DefaultService<R> = Service<R, SmbusEspiMedium>;`
- `impl<R: RelayHandler> DefaultService<R> { pub fn default_smbusespi(relay) }`
constructor helper that hardcodes the existing SmbusEspi reply context
(`SmbusEspiReplyContext { destination_slave_address: 1, source_slave_address: 0 }`).
Existing in-tree consumers update their import to `DefaultService` AND
change `Service::new(relay).unwrap()` →
`DefaultService::default_smbusespi(relay).unwrap()` (1-2 lines each).
Those consumer updates land in a separate downstream PR.
Also DELETE the 2 fake-PEC strip workarounds (no longer needed):
- `uart-service/src/lib.rs` `// Last byte is PEC, ignore for now`
- `espi-service/src/espi_service.rs` `// TODO: workaround
because mctp_rs expects a PEC byte, so we hardcode a 0`
- `espi-service/src/espi_service.rs` `// Last byte is PEC, ignore for now`
Under a serial medium (DSP0253) PEC is irrelevant — DSP0253 uses
FCS-16, not PEC. Under SmbusEspiMedium, the workarounds were
SmbusEspi-shaped accidents that are obviated by `frame_complete`-driven
framing.
BREAKING CHANGE: `Service::new` signature changed from `new(relay)`
to `new(relay, medium, reply_context)`. The
`DefaultService::default_smbusespi(relay)` constructor is the migration
path for SmbusEspi callers. The `Error` enum is now `Error<M>` generic
over the medium. The `MctpReplyContext<M>` struct now derives
`Clone, Copy` (additive — required for storing in `Service`).
The previous commit kept `task::uart_service` hardcoded to `&DefaultService<R>` and `Error<SmbusEspiMedium>` because no in-tree consumer needed the generic form. A downstream consumer that instantiates `Service<R, MctpSerialMedium>` cannot be passed where `&DefaultService<R>` is required, and the loop body's helper methods (`wait_for_request`, `wait_for_response`, `process_response`) are crate-private so the consumer cannot inline its own wrapper. Make the function generic over `M: MctpMedium + Copy` to match `Service`'s existing bound. Pure type-signature change; loop body is byte-identical. Existing SmbusEspiMedium callers (e.g. `DefaultService::default_smbusespi(relay)`) continue to work via inference because `M` is inferred from the `&Service<R, M>` argument.
…sk loop
After the genericization, naïve `error!("... {:?}", e)` no longer
compiles because `Error<M>: defmt::Format` requires `M::Error:
defmt::Format`, and that bound cannot be expressed as a
feature-gated where-clause on stable Rust (rust-lang/rust#115590).
Restore per-variant diagnostic signal without the format-trait
bound by matching on the `Error<M>` discriminant and emitting a
fixed static string per variant. Six variants — Comms, Uart, Mctp,
Serialize, Buffer — each get an unambiguous log line, with
`Serialize(s)` interpolating its `&'static str` payload.
Also splits the loop into separate `log_request_error` and
`log_response_error` helpers so the request vs response site is
clear in the log output.
Expose the message body slice and message_type byte on MctpMessageBuffer so callers can read raw bytes without going through MctpMessage::parse_as. Use case: a consumer wants to parse a small fixed-layout payload (e.g., 4 little-endian u32 fields for ACPI BST) without defining a full MctpMessageTrait impl just to access self.message_buffer.rest. Pure additive: no public API removed; existing parse_as / can_parse_as paths unchanged.
dymk
force-pushed
the
dymk/phase-34-uart-service-generic
branch
from
f8a6988 to
bfb1fb3
Compare
| /// feature-gated where-clause on stable Rust (rust-lang/rust#115590). | ||
| /// Matching on the variant + emitting a fixed string preserves the | ||
| /// per-variant signal without that bound. | ||
| fn log_request_error<M: MctpMedium>(e: &Error<M>) { |
There was a problem hiding this comment.
Looks good and seems like everything is in place to ensure nothing breaks.
Minor nit: I think it's better to have a single:
fn log_error<M: MctpMedium>(msg: &str, e: &Error<M>)
Then just call it like:
log_error("uart-service request:", &e)
log_error("uart-service response:", &e)
Make
uart-service::Servicegeneric overMctpMedium; delete fake-PEC stripsSummary
Makes
uart-service::Servicegenuinely generic overmctp_rs::MctpMedium, so the same service implementation works for both the existing SmbusEspi medium and the DSP0253-serial medium. Adds one new method to theMctpMediumtrait —frame_complete(buf) -> Result<Option<usize>, ...>— that lets generic byte-stream consumers detect frame boundaries without knowing the medium's framing details. Deletes three fake-PEC-strip workarounds that were SmbusEspi-shaped accidents (the real PEC byte is already handled bySmbusEspiMedium'sserialize/deserializevia thesmbus_peccrate).Also adds a small additive
MctpMessageBuffer::body()+message_type()accessor pair so callers can read raw body bytes without going throughMctpMessage::parse_as. Useful for a downstream SP-side service that parses a small fixed-layout payload and doesn't want to define a fullMctpMessageTraitimpl just to access the body slice.API changes
MctpMediumtrait gains a requiredframe_complete(buf) -> Result<Option<usize>, ...>method. Both shipped media (SmbusEspiMedium,MctpSerialMedium) ship impls in this PR. Out-of-tree implementors (none known) need to add aframe_completeimpl.uart-service::Service::new(relay)is nowService::<R, M>::new(relay, medium, reply_context). TheErrorenum is nowError<M: MctpMedium>to surface medium-specific MCTP errors. Existing SmbusEspi callers migrate via a one-line swap:MctpReplyContext<M>now derivesClone, Copy(additive — required for storing it as a field onService).What's added
MctpMedium::frame_complete(buf) -> Result<Option<usize>, Self::Error>— sync, byte-stream-agnostic frame-completeness predicate. Supports both length-prefix framing (SmbusEspi: 4-byte header →byte_count→ body + PEC) and sentinel framing (DSP0253: scan for0x7Eend-flag).Service<R, M: MctpMedium + Copy>— genuinely generic over medium.Service::new(relay, medium, reply_context)takes medium-specific addressing as a constructor arg.DefaultService<R> = Service<R, SmbusEspiMedium>type alias +DefaultService::default_smbusespi(relay)constructor that bakes in the existingSmbusEspiReplyContext { destination_slave_address: 1, source_slave_address: 0 }.MctpMessageBuffer::body() -> &[u8]+MctpMessageBuffer::message_type() -> u8accessors (additive; no existing API removed).What's removed
uart-service/src/lib.rs—// Last byte is PEC, ignore for nowstrip. Obviated byframe_complete-driven framing (the medium reports the exact frame length, including the PEC byte;MctpPacketContext::deserialize_packetconsumes the PEC correctly).espi-service/src/espi_service.rs—// TODO: workaround because mctp_rs expects a PEC byte, so we hardcode a 0block.espi-service/src/espi_service.rs—// Last byte is PEC, ignore for nowstrip inserialize_packet_from_subsystem.SMBUS_HEADER_SIZEandSMBUS_LEN_IDXinuart-service/src/lib.rs.Consumer migrations (NOT in this PR)
The three in-tree consumers (
dev-qemu,dev-imxrt,dev-npcx) live in a separate repo and update theirService::new(relay)call sites toDefaultService::default_smbusespi(relay)in a follow-up PR there.Test results
cargo test --workspaceconsumer pass count is unchanged (no new tests added by this PR; theframe_completeimpls are covered indirectly by existing media round-trip tests).