Design Review: `i2c-api` Wire Protocol Module (`wire.rs`)

Date: 2026-02-15
Module: services/i2c/api/src/wire.rs
Reviewer: AI-assisted review
Status: Review complete — actionable findings

1. Purpose & Scope

wire.rs defines the binary IPC wire protocol for I2C operations. It provides:

Operation code enum (I2cOp)
Request header (8 bytes) and response header (4 bytes)
Manual little-endian encoding/decoding (no external dependencies)
Free-standing encode/decode helper functions
no_std compatible, zero-copy decode

Wire Layout

Request (8 bytes header + payload):
┌────┬─────┬──────┬─────┬──────────┬──────────┐
│ op │ bus │ addr │ res │ write_len│ read_len │  + [write data]
│ 1B │ 1B  │ 1B   │ 1B  │  2B LE   │  2B LE   │
└────┴─────┴──────┴─────┴──────────┴──────────┘

Response (4 bytes header + payload):
┌──────┬─────┬──────────┐
│ code │ res │ data_len │  + [read data]
│ 1B   │ 1B  │  2B LE   │
└──────┴─────┴──────────┘

2. What's Done Well

Aspect	Detail
Compact layout	8-byte request / 4-byte response headers with explicit reserved bytes for alignment — efficient for IPC on constrained targets
`const fn` constructors	`I2cRequestHeader::write()`, `::read()`, etc. allow compile-time header construction for static request tables
Explicit LE encoding	Uses `to_le_bytes()` / `from_le_bytes()` throughout — no reliance on platform endianness
Zero-copy decode	`get_response_data()` / `get_request_payload()` return `&[u8]` slices into the caller's buffer with correct lifetime annotations (`'a`)
Good test coverage	Roundtrip tests for both header types, encode helpers, and `I2cOp::from_u8` edge cases
Clear documentation	ASCII wire diagram in module doc; doc comments on all public items
No `unsafe`	Entirely safe Rust

3. Findings

3.1 HIGH — Raw `u8` Fields Bypass the Type System

Location: I2cRequestHeader (lines 80–90), I2cResponseHeader (lines 175–182)

Problem: The header structs store op: u8, address: u8, and code: u8 rather than the validated domain types I2cOp, I2cAddress, and ResponseCode that the crate already defines. This means:

A caller can construct I2cRequestHeader { op: 255, .. } — an invalid opcode with no compile-time or runtime error.
The address field completely bypasses I2cAddress validation (reserved-range checks, 7-bit range enforcement).
I2cResponseHeader.code can hold values not in ResponseCode; the response_code() method silently maps unknowns to ServerError.
The operation() method returns Option<I2cOp>, proving the struct itself doesn't guarantee validity.

Recommendation: Store the typed enums/newtypes in the struct fields. Convert to/from u8 only inside to_bytes() / from_bytes():

pub struct I2cRequestHeader {
    pub op: I2cOp,           // was u8
    pub bus: BusIndex,       // was u8
    pub address: I2cAddress, // was u8
    pub write_len: u16,
    pub read_len: u16,
}

impl I2cRequestHeader {
    pub fn from_bytes(bytes: &[u8]) -> Result<Self, WireError> {
        // ...
        let op = I2cOp::from_u8(bytes[0])
            .ok_or(WireError::InvalidOpcode(bytes[0]))?;
        // ...
    }
}

This makes illegal states unrepresentable at the struct level.

3.2 HIGH — Silent `as u16` Truncation

Location: Lines 280, 312, 370 — data.len() as u16

Problem: If data.len() > 65535, the cast silently wraps. The header will advertise a truncated length while the buffer contains more data, causing the receiver to misinterpret the payload boundary.

While unlikely on a small embedded target, this is a correctness bug — it violates the principle that encoding should either succeed faithfully or fail.

Recommendation (option A — fail-safe):

let write_len = u16::try_from(data.len()).ok()?;

Recommendation (option B — debug guard):

debug_assert!(data.len() <= u16::MAX as usize);

3.3 MEDIUM — `MAX_PAYLOAD_SIZE` Declared But Never Enforced

Location: Line 255 — pub const MAX_PAYLOAD_SIZE: usize = 256;

Problem: The constant is defined and documented but none of the encode_* functions check against it. A caller can encode a payload larger than 256 bytes; the serialization will succeed, but the receiver's fixed-size buffer will overflow or reject it.

Recommendation: Add validation at the top of each encode helper:

pub fn encode_write_request(buf: &mut [u8], bus: u8, address: u8, data: &[u8]) -> Option<usize> {
    if data.len() > MAX_PAYLOAD_SIZE {
        return None; // or Err(WireError::PayloadTooLarge)
    }
    // ...
}

3.4 MEDIUM — `Option` Return Type Loses Error Information

Location: All encode/decode functions

Problem: Every function returns Option<...>. Callers cannot distinguish between:

Buffer too small
Payload exceeds MAX_PAYLOAD_SIZE
Invalid opcode on decode
u16 length overflow

This makes debugging and logging difficult in production.

Recommendation: Introduce a dedicated error enum:

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum WireError {
    /// Output buffer too small for the encoded message.
    BufferTooSmall,
    /// Payload exceeds MAX_PAYLOAD_SIZE.
    PayloadTooLarge,
    /// Unrecognized operation code during decode.
    InvalidOpcode(u8),
    /// Unrecognized response code during decode.
    InvalidResponseCode(u8),
    /// Input buffer too short to contain a complete header.
    Truncated,
}

Change return types from Option<usize> to Result<usize, WireError>.

3.5 MEDIUM — Encode Helpers Accept Raw `u8` Parameters

Location: Lines 268–330 — encode_write_request(), encode_read_request(), etc.

Problem: The free-standing helpers take bus: u8, address: u8 as raw bytes. This defeats the type safety provided by BusIndex and I2cAddress elsewhere in the crate. A caller can easily swap the two arguments without a compiler warning.

Recommendation: Accept the domain types:

pub fn encode_write_request(
    buf: &mut [u8],
    bus: BusIndex,
    address: I2cAddress,
    data: &[u8],
) -> Result<usize, WireError> { ... }

3.6 LOW — Redundant Free-Standing Wrapper Functions

Location: Lines 331–358 — decode_response_header(), decode_request_header(), get_response_data(), get_request_payload()

Problem: These are trivial one-line delegates to existing methods:

pub fn decode_response_header(buf: &[u8]) -> Option<I2cResponseHeader> {
    I2cResponseHeader::from_bytes(buf)
}

They add API surface without adding value. Callers can (and some already do) call the methods directly.

Recommendation: Remove these wrappers. If a flat-function facade is desired, mark them #[inline] and add a doc note explaining why they exist.

3.7 LOW — Missing Encode Helpers for Declared Operations

Location: I2cOp enum vs. encode helpers

Problem: I2cOp declares seven variants, but encode helpers exist for only four:

Op	Constructor	Encode helper
`Write`	✅	✅ `encode_write_request`
`Read`	✅	✅ `encode_read_request`
`WriteRead`	✅	✅ `encode_write_read_request`
`Transaction`	❌	❌
`Probe`	✅	✅ `encode_probe_request`
`ConfigureSpeed`	❌	❌
`RecoverBus`	✅	❌

Callers targeting Transaction, ConfigureSpeed, or RecoverBus must manually construct headers, which is error-prone and inconsistent.

Recommendation: Either add the missing helpers or document that these ops are not yet supported/implemented.

3.8 LOW — Reserved Byte Handling Is Asymmetric

Location: from_bytes() implementations

Problem: On decode, the reserved byte (bytes[3] in request, bytes[1] in response) is silently ignored. This is correct today, but if a future protocol version uses these bytes, old decoders will silently accept new-format messages without detecting the version mismatch.

Recommendation: Consider asserting bytes[reserved] == 0 during decode, or reserving one bit as a version flag for forward compatibility.

4. Security Considerations

No unsafe code — all operations are safe.
No panics on malformed input — all decode paths return Option/None for undersized buffers.
Bounds-checked slicing — all buf[..] accesses are preceded by length checks.
Risk: The silent truncation (finding 3.2) could theoretically be used to craft a message where the header length disagrees with actual data, but this requires the sender to be compromised, which is outside the I2C IPC threat model.

5. Prioritized Action Items

#	Priority	Finding	Effort
1	High	3.1 — Use typed enums/newtypes in header structs	Medium — requires updating all call sites
2	High	3.2 — Replace `as u16` with `try_from`	Small — mechanical change
3	Medium	3.3 — Enforce `MAX_PAYLOAD_SIZE` in encode functions	Small
4	Medium	3.4 — Introduce `WireError` enum	Medium — touches return types
5	Medium	3.5 — Accept `BusIndex`/`I2cAddress` in helpers	Small — depends on 3.1
6	Low	3.6 — Remove redundant wrappers	Trivial
7	Low	3.7 — Add missing encode helpers	Small
8	Low	3.8 — Validate reserved bytes	Trivial

6. Conclusion

The module is functional, safe, well-documented, and appropriate for its no_std IPC context. The primary weakness is that the wire layer bypasses the type safety the rest of the crate carefully establishes — accepting and storing raw u8 where validated domain types exist. Fixing findings 3.1 and 3.2 would bring the wire protocol in line with the crate's overall design philosophy of making illegal states unrepresentable. The remaining findings are incremental improvements.

Design Review: i2c-api Wire Protocol Module (wire.rs)