From ce3bdca6b90a88b67b679b5d8dfecb42e65a98f1 Mon Sep 17 00:00:00 2001 From: Dylan Knutson Date: Fri, 15 May 2026 15:21:21 -0700 Subject: [PATCH 1/6] feat(mctp-rs)!: add MctpMedium::frame_complete trait method Add a new method `frame_complete(&self, buf: &[u8]) -> Result, 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) 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 #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. --- mctp-rs/src/lib.rs | 11 ++++- mctp-rs/src/mctp_packet_context.rs | 2 +- mctp-rs/src/medium/mod.rs | 15 +++++- mctp-rs/src/medium/serial.rs | 77 ++++++++++++++++++++++++++++++ mctp-rs/src/medium/smbus_espi.rs | 71 +++++++++++++++++++++++++++ mctp-rs/src/test_util.rs | 9 ++++ 6 files changed, 180 insertions(+), 5 deletions(-) diff --git a/mctp-rs/src/lib.rs b/mctp-rs/src/lib.rs index 7fa1809b..9aa58577 100644 --- a/mctp-rs/src/lib.rs +++ b/mctp-rs/src/lib.rs @@ -17,7 +17,8 @@ //! # impl MctpMedium for MyMedium { type Frame=MyMediumFrame; type Error=&'static str; type ReplyContext=(); type Encoding=PassthroughEncoding; //! # fn max_message_body_size(&self)->usize{self.mtu} //! # fn deserialize<'b>(&self,p:&'b [u8])->MctpPacketResult<(Self::Frame,EncodingDecoder<'b,Self::Encoding>),Self>{Ok((MyMediumFrame{packet_size:p.len()},EncodingDecoder::new(p)))} -//! # fn serialize<'b,F>(&self,_:Self::ReplyContext,b:&'b mut [u8],w:F)->MctpPacketResult<&'b [u8],Self> where F: for<'a> FnOnce(&mut EncodingEncoder<'a,Self::Encoding>)->MctpPacketResult<(),Self>{let n={let mut e=EncodingEncoder::::new(b);w(&mut e)?;e.wire_position()};Ok(&b[..n])}} +//! # fn serialize<'b,F>(&self,_:Self::ReplyContext,b:&'b mut [u8],w:F)->MctpPacketResult<&'b [u8],Self> where F: for<'a> FnOnce(&mut EncodingEncoder<'a,Self::Encoding>)->MctpPacketResult<(),Self>{let n={let mut e=EncodingEncoder::::new(b);w(&mut e)?;e.wire_position()};Ok(&b[..n])} +//! # fn frame_complete(&self,b:&[u8])->MctpPacketResult,Self>{if b.is_empty(){Ok(None)}else{Ok(Some(b.len()))}}} //! # impl MctpMediumFrame for MyMediumFrame { fn packet_size(&self)->usize{self.packet_size} fn reply_context(&self)->(){()}} //! let mut assembly_buffer = [0u8; 1024]; //! let medium = MyMedium { mtu: 256 }; @@ -61,7 +62,8 @@ //! # #[derive(Debug, Clone, Copy)] struct MyMediumFrame { packet_size: usize } //! # impl MctpMedium for MyMedium { type Frame=MyMediumFrame; type Error=&'static str; type ReplyContext=(); type Encoding=PassthroughEncoding; fn max_message_body_size(&self)->usize{self.mtu} //! # fn deserialize<'b>(&self,p:&'b [u8])->MctpPacketResult<(Self::Frame,EncodingDecoder<'b,Self::Encoding>),Self>{Ok((MyMediumFrame{packet_size:p.len()},EncodingDecoder::new(p)))} -//! # fn serialize<'b,F>(&self,_:Self::ReplyContext,b:&'b mut [u8],w:F)->MctpPacketResult<&'b [u8],Self> where F: for<'a> FnOnce(&mut EncodingEncoder<'a,Self::Encoding>)->MctpPacketResult<(),Self>{let n={let mut e=EncodingEncoder::::new(b);w(&mut e)?;e.wire_position()};Ok(&b[..n])}} +//! # fn serialize<'b,F>(&self,_:Self::ReplyContext,b:&'b mut [u8],w:F)->MctpPacketResult<&'b [u8],Self> where F: for<'a> FnOnce(&mut EncodingEncoder<'a,Self::Encoding>)->MctpPacketResult<(),Self>{let n={let mut e=EncodingEncoder::::new(b);w(&mut e)?;e.wire_position()};Ok(&b[..n])} +//! # fn frame_complete(&self,b:&[u8])->MctpPacketResult,Self>{if b.is_empty(){Ok(None)}else{Ok(Some(b.len()))}}} //! # impl MctpMediumFrame for MyMediumFrame { fn packet_size(&self)->usize{self.packet_size} fn reply_context(&self)->(){()}} //! let mut buf = [0u8; 1024]; //! let mut ctx = MctpPacketContext::new(MyMedium { mtu: 64 }, &mut buf); @@ -147,6 +149,11 @@ //! }; //! Ok(&buffer[..written]) //! } +//! +//! fn frame_complete(&self, buf: &[u8]) -> MctpPacketResult, Self> { +//! // Trivial: treat any non-empty buffer as a complete frame. +//! if buf.is_empty() { Ok(None) } else { Ok(Some(buf.len())) } +//! } //! } //! //! impl MctpMediumFrame for MyMediumFrame { diff --git a/mctp-rs/src/mctp_packet_context.rs b/mctp-rs/src/mctp_packet_context.rs index 893b333c..5ed02498 100644 --- a/mctp-rs/src/mctp_packet_context.rs +++ b/mctp-rs/src/mctp_packet_context.rs @@ -12,7 +12,7 @@ use crate::{ /// Represents the state needed to construct a repsonse to a request: /// the MCTP transport source/destination, the sequence number to use for /// the reply, and the medium-specific context that came with the request. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct MctpReplyContext { pub destination_endpoint_id: EndpointId, diff --git a/mctp-rs/src/medium/mod.rs b/mctp-rs/src/medium/mod.rs index 693deda4..e650fed9 100644 --- a/mctp-rs/src/medium/mod.rs +++ b/mctp-rs/src/medium/mod.rs @@ -20,8 +20,8 @@ pub trait MctpMedium: Sized { type ReplyContext: core::fmt::Debug + Copy + Clone + PartialEq + Eq; /// the byte-stuffing transform used by this medium when (de)serializing - /// wire bytes. Stateless — see [`crate::buffer_encoding`]. Most media - /// use [`PassthroughEncoding`](crate::buffer_encoding::PassthroughEncoding) + /// wire bytes. Stateless — see [`BufferEncoding`](crate::BufferEncoding). + /// Most media use [`PassthroughEncoding`](crate::PassthroughEncoding) /// (no transform); media that need byte-stuffing (e.g., DSP0253 serial) /// supply their own impl. type Encoding: BufferEncoding; @@ -55,6 +55,17 @@ pub trait MctpMedium: Sized { ) -> MctpPacketResult<&'buf [u8], Self> where F: for<'a> FnOnce(&mut EncodingEncoder<'a, Self::Encoding>) -> MctpPacketResult<(), Self>; + + /// Returns `Ok(Some(len))` when `buf` contains a complete medium-framed + /// packet starting at `buf[0]`, where `len` is the total byte count of + /// that packet. Returns `Ok(None)` when `buf` has a partial frame + /// (caller should read more bytes and retry). Returns `Err(...)` when + /// `buf` is malformed. + /// + /// Used by generic consumers (e.g., `uart-service::Service`) + /// to assemble packets from byte streams without knowing medium + /// framing details. + fn frame_complete(&self, buf: &[u8]) -> MctpPacketResult, Self>; } pub trait MctpMediumFrame: Clone + Copy { diff --git a/mctp-rs/src/medium/serial.rs b/mctp-rs/src/medium/serial.rs index 9075ba1f..c1f80684 100644 --- a/mctp-rs/src/medium/serial.rs +++ b/mctp-rs/src/medium/serial.rs @@ -293,6 +293,27 @@ impl MctpMedium for MctpSerialMedium { Ok(&buffer[..end_pos + 1]) } + + fn frame_complete(&self, buf: &[u8]) -> MctpPacketResult, Self> { + // This medium's `serialize` emits `[revision, byte_count, ...stuffed body, + // fcs_msb, fcs_lsb, 0x7E]` — only an END flag, no leading START flag. + // + // The `body_start` check below is forward-defensive: DSP0253 variants are + // permitted to emit a leading 0x7E flag, and this lets `frame_complete` + // accept that case too (skip the leading flag, then find the next 0x7E). + // Either pattern returns the full frame length to the caller. + if buf.is_empty() { + return Ok(None); + } + let body_start = if buf[0] == END_FLAG { 1 } else { 0 }; + if body_start >= buf.len() { + return Ok(None); + } + match buf[body_start..].iter().position(|&b| b == END_FLAG) { + Some(idx) => Ok(Some(body_start + idx + 1)), + None => Ok(None), + } + } } #[cfg(test)] @@ -713,4 +734,60 @@ mod medium_tests { assert!(packet_count >= 2, "expected multi-packet split, got {packet_count}"); assert_eq!(total_decoded_body, payload.len()); } + + // ----- frame_complete tests ----- + + #[test] + fn frame_complete_empty_buf_returns_none() { + assert_eq!(MctpSerialMedium.frame_complete(&[]).unwrap(), None); + } + + #[test] + fn frame_complete_no_end_flag_returns_none() { + // Bytes present but no 0x7E anywhere → partial frame + let buf = [0x01, 0x05, 0xAA, 0xBB, 0xCC]; + assert_eq!(MctpSerialMedium.frame_complete(&buf).unwrap(), None); + } + + #[test] + fn frame_complete_only_end_flag_returns_none() { + // A lone 0x7E is treated as a leading flag (skipped); after + // skipping there are no bytes left, so partial. + let buf = [END_FLAG]; + assert_eq!(MctpSerialMedium.frame_complete(&buf).unwrap(), None); + } + + #[test] + fn frame_complete_no_leading_flag_finds_end_flag() { + // [ revision, byte_count, ...body, fcs_msb, fcs_lsb, 0x7E ] + // matches what MctpSerialMedium::serialize emits — no leading flag. + let buf = [SERIAL_REVISION, 0x03, 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, END_FLAG]; + assert_eq!(MctpSerialMedium.frame_complete(&buf).unwrap(), Some(8)); + } + + #[test] + fn frame_complete_with_leading_flag_skips_and_finds_end_flag() { + // Forward-defensive: some DSP0253 variants emit a leading START + // flag. frame_complete should skip the leading 0x7E and find the + // trailing one, returning the FULL length (including leading flag). + let buf = [END_FLAG, SERIAL_REVISION, 0x02, 0xAA, 0xBB, 0xCC, 0xDD, END_FLAG]; + assert_eq!(MctpSerialMedium.frame_complete(&buf).unwrap(), Some(8)); + } + + #[test] + fn frame_complete_extra_bytes_after_end_flag_returns_first_frame_len() { + // First frame is 6 bytes (incl. END flag); 2 trailing bytes belong + // to the next frame (caller's problem). + let buf = [SERIAL_REVISION, 0x01, 0xAA, 0xBB, 0xCC, END_FLAG, 0x99, 0x88]; + assert_eq!(MctpSerialMedium.frame_complete(&buf).unwrap(), Some(6)); + } + + #[test] + fn frame_complete_back_to_back_after_leading_flag() { + // Edge case: leading flag, then payload, then end flag, then more + // bytes. Should return length of first frame INCLUDING the leading + // flag; trailing bytes belong to a subsequent frame. + let buf = [END_FLAG, SERIAL_REVISION, 0x01, 0xAA, 0xBB, END_FLAG, 0x77]; + assert_eq!(MctpSerialMedium.frame_complete(&buf).unwrap(), Some(6)); + } } diff --git a/mctp-rs/src/medium/smbus_espi.rs b/mctp-rs/src/medium/smbus_espi.rs index eeeb5979..958cf409 100644 --- a/mctp-rs/src/medium/smbus_espi.rs +++ b/mctp-rs/src/medium/smbus_espi.rs @@ -106,6 +106,18 @@ impl MctpMedium for SmbusEspiMedium { fn max_message_body_size(&self) -> usize { 32 } + + fn frame_complete(&self, buf: &[u8]) -> MctpPacketResult, Self> { + // SmbusEspi framing: [dst_addr | src_addr | byte_count | cmd_code] + // [ body bytes (byte_count) ] [ PEC byte ] + // Total: 4 + byte_count + 1 = 5 + byte_count + if buf.len() < 4 { + return Ok(None); + } + let byte_count = buf[2] as usize; + let total = 4 + byte_count + 1; + if buf.len() < total { Ok(None) } else { Ok(Some(total)) } + } } #[repr(u8)] @@ -845,4 +857,63 @@ mod tests { MctpPacketError::SerializeError("encode error") ); } + + // ----- frame_complete tests ----- + + #[test] + fn frame_complete_empty_buf_returns_none() { + assert_eq!(SmbusEspiMedium.frame_complete(&[]).unwrap(), None); + } + + #[test] + fn frame_complete_partial_header_returns_none() { + // SmbusEspi header is 4 bytes (dst, src, byte_count, cmd_code). + // Any byte count from 1 to 3 means we don't know byte_count yet. + for n in 1..4 { + let buf: Vec = (0..n).map(|_| 0u8).collect(); + assert_eq!( + SmbusEspiMedium.frame_complete(&buf).unwrap(), + None, + "partial header ({} bytes) should be incomplete", + n + ); + } + } + + #[test] + fn frame_complete_exact_frame_returns_total_len() { + // header (4) + body (byte_count = 3) + PEC (1) = 8 bytes + let buf: [u8; 8] = [0x20, 0x10, 0x03, 0x0F, 0xAA, 0xBB, 0xCC, 0xDD]; + assert_eq!(SmbusEspiMedium.frame_complete(&buf).unwrap(), Some(8)); + } + + #[test] + fn frame_complete_short_of_body_returns_none() { + // byte_count says 5, but we only have 4 header + 3 body bytes (no PEC yet) + let buf: [u8; 7] = [0x20, 0x10, 0x05, 0x0F, 0xAA, 0xBB, 0xCC]; + assert_eq!(SmbusEspiMedium.frame_complete(&buf).unwrap(), None); + } + + #[test] + fn frame_complete_short_of_pec_returns_none() { + // byte_count = 2 → expects 4 + 2 + 1 = 7 bytes; we have 6 + let buf: [u8; 6] = [0x20, 0x10, 0x02, 0x0F, 0xAA, 0xBB]; + assert_eq!(SmbusEspiMedium.frame_complete(&buf).unwrap(), None); + } + + #[test] + fn frame_complete_extra_bytes_after_frame_returns_first_frame_len() { + // First frame: 4 + 1 + 1 = 6 bytes. Buffer has 8 bytes (2 trailing). + // frame_complete reports the length of the FIRST frame; trailing + // bytes are the caller's problem (e.g., next iteration of the loop). + let buf: [u8; 8] = [0x20, 0x10, 0x01, 0x0F, 0xAA, 0xBB, 0xCC, 0xDD]; + assert_eq!(SmbusEspiMedium.frame_complete(&buf).unwrap(), Some(6)); + } + + #[test] + fn frame_complete_zero_byte_count_returns_5_bytes() { + // Edge case: byte_count = 0 means 4 header + 0 body + 1 PEC = 5 bytes + let buf: [u8; 5] = [0x20, 0x10, 0x00, 0x0F, 0xCC]; + assert_eq!(SmbusEspiMedium.frame_complete(&buf).unwrap(), Some(5)); + } } diff --git a/mctp-rs/src/test_util.rs b/mctp-rs/src/test_util.rs index e328a0c9..f78be575 100644 --- a/mctp-rs/src/test_util.rs +++ b/mctp-rs/src/test_util.rs @@ -62,6 +62,15 @@ impl MctpMedium for TestMedium { fn max_message_body_size(&self) -> usize { self.mtu } + fn frame_complete(&self, buf: &[u8]) -> MctpPacketResult, Self> { + // TestMedium frames are length-bounded by the buffer passed in; + // for tests we treat any non-empty buffer as a complete frame. + if buf.len() < self.header.len() + self.trailer.len() { + Ok(None) + } else { + Ok(Some(buf.len())) + } + } fn serialize<'buf, F>( &self, _: Self::ReplyContext, From 6ca096085d9d71cdab43cc54aed94825816e978b Mon Sep 17 00:00:00 2001 From: Dylan Knutson Date: Fri, 15 May 2026 15:27:34 -0700 Subject: [PATCH 2/6] refactor(uart-service)!: make Service generic over MctpMedium; delete fake-PEC strips MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make `embedded-services::uart-service::Service` → `Service`. 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 = Service;` - `impl DefaultService { 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` generic over the medium. The `MctpReplyContext` struct now derives `Clone, Copy` (additive — required for storing in `Service`). --- espi-service/src/espi_service.rs | 11 +-- uart-service/src/lib.rs | 148 +++++++++++++++++++++---------- uart-service/src/task.rs | 7 +- 3 files changed, 105 insertions(+), 61 deletions(-) diff --git a/espi-service/src/espi_service.rs b/espi-service/src/espi_service.rs index fdbdc1de..f47ab3fd 100644 --- a/espi-service/src/espi_service.rs +++ b/espi-service/src/espi_service.rs @@ -163,13 +163,6 @@ impl<'hw, RelayHandler: embedded_services::relay::mctp::RelayHandler> ServiceInn let src_slice = unsafe { slice::from_raw_parts(port_event.base_addr as *const u8, port_event.length) }; - // TODO: This is a workaround because mctp_rs expects a PEC byte, so we hardcode a 0 at the end. - // We should add functionality to mctp_rs to disable PEC. - let mut with_pec = [0u8; 100]; - with_pec[..src_slice.len()].copy_from_slice(src_slice); - with_pec[src_slice.len()] = 0; - let with_pec = &with_pec[..=src_slice.len()]; - #[cfg(feature = "defmt")] // Required because without defmt, there is no implementation of UpperHex for [u8] embedded_services::debug!("OOB message: {:02X}", &src_slice[0..]); @@ -179,7 +172,7 @@ impl<'hw, RelayHandler: embedded_services::relay::mctp::RelayHandler> ServiceInn assembly_buf.as_mut_slice(), ); - match mctp_ctx.deserialize_packet(with_pec) { + match mctp_ctx.deserialize_packet(src_slice) { Ok(Some(message)) => { trace!("MCTP packet successfully deserialized"); match message.parse_as::() { @@ -302,8 +295,6 @@ impl<'hw, RelayHandler: embedded_services::relay::mctp::RelayHandler> ServiceInn error!("serialize_packet_from_subsystem: {:?}", e); Error::Serialize })?; - // Last byte is PEC, ignore for now - let packet = &packet[..packet.len() - 1]; trace!("Sending MCTP response: {:?}", packet); self.write_to_hw(espi, packet).map_err(|e| { diff --git a/uart-service/src/lib.rs b/uart-service/src/lib.rs index 9f31f986..e3ecffe6 100644 --- a/uart-service/src/lib.rs +++ b/uart-service/src/lib.rs @@ -1,8 +1,9 @@ //! uart-service //! -//! To keep things consistent with eSPI service, this also uses the `SmbusEspiMedium` (though not -//! strictly necessary, this helps minimize code changes on the host side when swicthing between -//! eSPI or UART). +//! UART transport for MCTP packets, generic over [`mctp_rs::MctpMedium`]. +//! Use [`DefaultService`] for the SmbusEspi-medium baseline; use +//! [`Service::new`] directly with another medium (e.g. DSP0253 serial) +//! for non-SmbusEspi callers. //! //! Revisit: Will also need to consider how to handle notifications (likely need to have user //! provide GPIO pin we can use). @@ -16,14 +17,12 @@ use embedded_io_async::Write as UartWrite; use embedded_services::GlobalRawMutex; use embedded_services::relay::mctp::{RelayHandler, RelayHeader, RelayResponse}; use embedded_services::trace; -use mctp_rs::smbus_espi::SmbusEspiMedium; -use mctp_rs::smbus_espi::SmbusEspiReplyContext; +use mctp_rs::MctpMedium; +use mctp_rs::smbus_espi::{SmbusEspiMedium, SmbusEspiReplyContext}; // Should be as large as the largest possible MCTP packet and its metadata. const BUF_SIZE: usize = 256; const HOST_TX_QUEUE_SIZE: usize = 5; -const SMBUS_HEADER_SIZE: usize = 4; -const SMBUS_LEN_IDX: usize = 2; #[derive(Clone)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] @@ -34,48 +33,64 @@ pub(crate) struct HostResultMessage { #[derive(Debug, Clone, Copy)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] -pub enum Error { +pub enum Error { /// Comms error. Comms, /// UART error. Uart, /// MCTP serialization error. - Mctp(mctp_rs::MctpPacketError), + Mctp(mctp_rs::MctpPacketError), /// Other serialization error. Serialize(&'static str), - /// Index/slice error. - IndexSlice, /// Buffer error. Buffer(embedded_services::buffer::Error), } -pub struct Service { +/// UART-driven MCTP relay service, generic over the medium `M`. +/// +/// # `M: Copy` bound +/// +/// The `Copy` bound on `M` is required because [`MctpPacketContext`] +/// takes the medium by value, and `Service` needs to construct a fresh +/// `MctpPacketContext` on each request and each response (the +/// `MctpPacketContext` borrows the assembly buffer for the duration of +/// one packet, so it must be re-created per round-trip). Storing `M` by +/// value and copying it into each new context is the simplest shape; +/// it's free for both shipped media (`SmbusEspiMedium`, `MctpSerialMedium` +/// are zero-sized types). A future medium with internal state that +/// cannot be `Copy` would need either an `&'_ M`-based redesign of +/// `MctpPacketContext` or an interior-mutability wrapper. +/// +/// [`MctpPacketContext`]: mctp_rs::MctpPacketContext +pub struct Service { host_tx_queue: Channel, HOST_TX_QUEUE_SIZE>, relay_handler: R, + medium: M, + reply_context: mctp_rs::MctpReplyContext, } -impl Service { - pub fn new(relay_handler: R) -> Result { +impl Service { + pub fn new(relay_handler: R, medium: M, reply_context: mctp_rs::MctpReplyContext) -> Result> { Ok(Self { host_tx_queue: Channel::new(), relay_handler, + medium, + reply_context, }) } - async fn process_response(&self, uart: &mut T, response: HostResultMessage) -> Result<(), Error> { + async fn process_response( + &self, + uart: &mut T, + response: HostResultMessage, + ) -> Result<(), Error> { let mut assembly_buf = [0u8; BUF_SIZE]; - let mut mctp_ctx = mctp_rs::MctpPacketContext::new(SmbusEspiMedium, &mut assembly_buf); - - let reply_context: mctp_rs::MctpReplyContext = mctp_rs::MctpReplyContext { - source_endpoint_id: mctp_rs::EndpointId::Id(0x80), - destination_endpoint_id: mctp_rs::EndpointId::Id(response.handler_service_id.into()), - packet_sequence_number: mctp_rs::MctpSequenceNumber::new(0), - message_tag: mctp_rs::MctpMessageTag::try_from(3).map_err(Error::Serialize)?, - medium_context: SmbusEspiReplyContext { - destination_slave_address: 1, - source_slave_address: 0, - }, // Medium-specific context - }; + let mut mctp_ctx = mctp_rs::MctpPacketContext::::new(self.medium, &mut assembly_buf); + + // Start from the stored reply_context, override the per-response + // destination_endpoint_id from the responding service. + let mut reply_context = self.reply_context; + reply_context.destination_endpoint_id = mctp_rs::EndpointId::Id(response.handler_service_id.into()); let header = response.message.create_header(&response.handler_service_id); let mut packet_state = mctp_ctx @@ -84,37 +99,47 @@ impl Service { while let Some(packet_result) = packet_state.next() { let packet = packet_result.map_err(Error::Mctp)?; - // Last byte is PEC, ignore for now - let packet = packet.get(..packet.len() - 1).ok_or(Error::IndexSlice)?; - // Then actually send the response packet (which includes 4-byte SMBUS header containing payload size) + // Then actually send the response packet (medium framing already applied) uart.write_all(packet).await.map_err(|_| Error::Uart)?; } Ok(()) } - async fn wait_for_request(&self, uart: &mut T) -> Result<(), Error> { + async fn wait_for_request(&self, uart: &mut T) -> Result<(), Error> { + // Incremental read loop: read bytes, ask the medium whether the + // assembled prefix is a complete frame, repeat until it is. + let mut buf = [0u8; BUF_SIZE]; + let mut filled = 0usize; + let packet_len = loop { + let dst = buf.get_mut(filled..).ok_or(Error::Serialize("buffer overrun"))?; + if dst.is_empty() { + return Err(Error::Serialize("frame exceeds BUF_SIZE")); + } + let n = uart.read(dst).await.map_err(|_| Error::Uart)?; + if n == 0 { + return Err(Error::Comms); + } + filled += n; + match self + .medium + .frame_complete(buf.get(..filled).ok_or(Error::Serialize("buffer overrun"))?) + .map_err(Error::Mctp)? + { + Some(len) => break len, + None => continue, + } + }; + let mut assembly_buf = [0u8; BUF_SIZE]; - let mut mctp_ctx = mctp_rs::MctpPacketContext::::new(SmbusEspiMedium, &mut assembly_buf); - - // First wait for SMBUS header, which tells us how big the incoming packet is - let mut buf = [0; BUF_SIZE]; - uart.read_exact(buf.get_mut(..SMBUS_HEADER_SIZE).ok_or(Error::IndexSlice)?) - .await - .map_err(|_| Error::Uart)?; - - // Then wait until we've received the full payload - let len = *buf.get(SMBUS_LEN_IDX).ok_or(Error::IndexSlice)? as usize; - uart.read_exact( - buf.get_mut(SMBUS_HEADER_SIZE..SMBUS_HEADER_SIZE + len) - .ok_or(Error::IndexSlice)?, - ) - .await - .map_err(|_| Error::Uart)?; + let mut mctp_ctx = mctp_rs::MctpPacketContext::::new(self.medium, &mut assembly_buf); let message = mctp_ctx - .deserialize_packet(&buf) + .deserialize_packet( + buf.get(..packet_len) + .ok_or(Error::Serialize("frame exceeds BUF_SIZE"))?, + ) .map_err(Error::Mctp)? .ok_or(Error::Serialize("Partial message not supported"))?; @@ -136,3 +161,30 @@ impl Service { self.host_tx_queue.receive().await } } + +/// Backwards-compatible alias for SmbusEspi-medium services. +pub type DefaultService = Service; + +impl DefaultService { + /// Constructor for SmbusEspi-medium services. Hardcodes the + /// reply-context addressing used by the existing SmbusEspi + /// consumers (destination_slave_address: 1, source_slave_address: 0). + /// The `destination_endpoint_id` is overridden per-response inside + /// `process_response`, so the value passed here is a placeholder. + pub fn default_smbusespi(relay_handler: R) -> Result> { + Self::new( + relay_handler, + SmbusEspiMedium, + mctp_rs::MctpReplyContext { + source_endpoint_id: mctp_rs::EndpointId::Id(0x80), + destination_endpoint_id: mctp_rs::EndpointId::Id(0), + packet_sequence_number: mctp_rs::MctpSequenceNumber::new(0), + message_tag: mctp_rs::MctpMessageTag::try_from(3).map_err(Error::Serialize)?, + medium_context: SmbusEspiReplyContext { + destination_slave_address: 1, + source_slave_address: 0, + }, + }, + ) + } +} diff --git a/uart-service/src/task.rs b/uart-service/src/task.rs index c5f7d59e..351b64ce 100644 --- a/uart-service/src/task.rs +++ b/uart-service/src/task.rs @@ -1,13 +1,14 @@ -use crate::{Error, Service}; +use crate::{DefaultService, Error}; use embedded_io_async::Read as UartRead; use embedded_io_async::Write as UartWrite; use embedded_services::error; use embedded_services::relay::mctp::RelayHandler; +use mctp_rs::smbus_espi::SmbusEspiMedium; pub async fn uart_service( - uart_service: &Service, + uart_service: &DefaultService, mut uart: T, -) -> Result { +) -> Result> { // Note: eSPI service uses `select!` to seemingly allow asyncrhonous `responses` from services, // but there are concerns around async cancellation here at least for UART service. // From d341a2a2161a2519e3a95de82b136b7ef74fa081 Mon Sep 17 00:00:00 2001 From: Dylan Knutson Date: Fri, 15 May 2026 18:36:41 -0700 Subject: [PATCH 3/6] refactor(uart-service): make task::uart_service generic over MctpMedium The previous commit kept `task::uart_service` hardcoded to `&DefaultService` and `Error` because no in-tree consumer needed the generic form. A downstream consumer that instantiates `Service` cannot be passed where `&DefaultService` 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` argument. --- uart-service/src/task.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/uart-service/src/task.rs b/uart-service/src/task.rs index 351b64ce..02653cb0 100644 --- a/uart-service/src/task.rs +++ b/uart-service/src/task.rs @@ -1,14 +1,14 @@ -use crate::{DefaultService, Error}; +use crate::{Error, Service}; use embedded_io_async::Read as UartRead; use embedded_io_async::Write as UartWrite; use embedded_services::error; use embedded_services::relay::mctp::RelayHandler; -use mctp_rs::smbus_espi::SmbusEspiMedium; +use mctp_rs::MctpMedium; -pub async fn uart_service( - uart_service: &DefaultService, +pub async fn uart_service( + uart_service: &Service, mut uart: T, -) -> Result> { +) -> Result> { // Note: eSPI service uses `select!` to seemingly allow asyncrhonous `responses` from services, // but there are concerns around async cancellation here at least for UART service. // From 79cbfcefe49569c0771488f5216a8473e652d6dc Mon Sep 17 00:00:00 2001 From: Dylan Knutson Date: Fri, 15 May 2026 18:39:53 -0700 Subject: [PATCH 4/6] fix(uart-service): emit per-variant static log messages in generic task loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After the genericization, naïve `error!("... {:?}", e)` no longer compiles because `Error: 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` 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. --- uart-service/src/task.rs | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/uart-service/src/task.rs b/uart-service/src/task.rs index 02653cb0..2b3188df 100644 --- a/uart-service/src/task.rs +++ b/uart-service/src/task.rs @@ -16,12 +16,40 @@ pub async fn uart_service` only implements `defmt::Format` +/// when `M::Error` does too, and that bound can't be expressed as a +/// 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(e: &Error) { + match e { + Error::Comms => error!("uart-service request: comms error (uart returned 0 bytes)"), + Error::Uart => error!("uart-service request: uart I/O error"), + Error::Mctp(_) => error!("uart-service request: mctp framing/decode error"), + Error::Serialize(s) => error!("uart-service request: serialize error: {}", s), + Error::Buffer(_) => error!("uart-service request: buffer error"), + } +} + +/// Emit a per-variant static log message for a `process_response` error. +fn log_response_error(e: &Error) { + match e { + Error::Comms => error!("uart-service response: comms error"), + Error::Uart => error!("uart-service response: uart I/O error"), + Error::Mctp(_) => error!("uart-service response: mctp framing/encode error"), + Error::Serialize(s) => error!("uart-service response: serialize error: {}", s), + Error::Buffer(_) => error!("uart-service response: buffer error"), + } +} From bfb1fb30b1af05e13f72e8788ec702f023379362 Mon Sep 17 00:00:00 2001 From: Dylan Knutson Date: Sat, 16 May 2026 19:06:53 -0700 Subject: [PATCH 5/6] feat(mctp-rs): add MctpMessageBuffer::body() + message_type() accessors 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. --- mctp-rs/src/lib.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/mctp-rs/src/lib.rs b/mctp-rs/src/lib.rs index 9aa58577..705bcf6d 100644 --- a/mctp-rs/src/lib.rs +++ b/mctp-rs/src/lib.rs @@ -211,6 +211,22 @@ pub struct MctpMessageBuffer<'buffer> { rest: &'buffer [u8], } +impl<'buffer> MctpMessageBuffer<'buffer> { + /// The raw message body bytes (after the message-type byte). + /// + /// Use when the caller wants to parse the body without going through + /// `MctpMessage::parse_as` (e.g., when the body is a small fixed-layout + /// payload and a typed `MctpMessageTrait` impl would be overkill). + pub fn body(&self) -> &'buffer [u8] { + self.rest + } + + /// The MCTP message-type byte (e.g., `0x7E` for VendorDefinedPci). + pub fn message_type(&self) -> u8 { + self.message_type + } +} + impl<'buffer, M: MctpMedium> MctpMessage<'buffer, M> { pub fn can_parse_as>(&self) -> bool { self.message_buffer.message_type == P::MESSAGE_TYPE From 3ed520aa7bab17f1065ad5e1a60823b1e52ac120 Mon Sep 17 00:00:00 2001 From: Dylan Knutson Date: Wed, 20 May 2026 15:18:40 -0700 Subject: [PATCH 6/6] refactor(uart-service): consolidate log helpers into single log_error fn Merge log_request_error and log_response_error into a single log_error(direction, e) function that takes a &str direction parameter. defmt supports &str as a format argument, so the per-variant match two near-identical functions. Assisted-by: GitHub Copilot CLI:claude-opus-4.6-1m-internal --- uart-service/src/task.rs | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/uart-service/src/task.rs b/uart-service/src/task.rs index 2b3188df..06a2b8c0 100644 --- a/uart-service/src/task.rs +++ b/uart-service/src/task.rs @@ -16,40 +16,22 @@ pub async fn uart_service` only implements `defmt::Format` -/// when `M::Error` does too, and that bound can't be expressed as a -/// 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(e: &Error) { +fn log_error(direction: &str, e: &Error) { match e { - Error::Comms => error!("uart-service request: comms error (uart returned 0 bytes)"), - Error::Uart => error!("uart-service request: uart I/O error"), - Error::Mctp(_) => error!("uart-service request: mctp framing/decode error"), - Error::Serialize(s) => error!("uart-service request: serialize error: {}", s), - Error::Buffer(_) => error!("uart-service request: buffer error"), - } -} - -/// Emit a per-variant static log message for a `process_response` error. -fn log_response_error(e: &Error) { - match e { - Error::Comms => error!("uart-service response: comms error"), - Error::Uart => error!("uart-service response: uart I/O error"), - Error::Mctp(_) => error!("uart-service response: mctp framing/encode error"), - Error::Serialize(s) => error!("uart-service response: serialize error: {}", s), - Error::Buffer(_) => error!("uart-service response: buffer error"), + Error::Comms => error!("uart-service {}: comms error", direction), + Error::Uart => error!("uart-service {}: uart I/O error", direction), + Error::Mctp(_) => error!("uart-service {}: mctp error", direction), + Error::Serialize(s) => error!("uart-service {}: serialize error: {}", direction, s), + Error::Buffer(_) => error!("uart-service {}: buffer error", direction), } }