[kurtjd]

Provides a basic way for now for the test-cli app to exercise eSPI/PCC (see: OpenDevicePartnership/odp-embedded-controller#48).

The PING command just tests simple command + payload (and response) via PCC type 3. It calls into the ectest driver (just like the other commands do when acpi is source) but the driver then uses the native Windows PCC interface, which knows how to talk to the PCCT provided by firmware (well, QEMU provides it in our case). This then gets translated to eSPI peripheral channel requests voer the virtual eSPI bus to the EC which updates "shared" memory with a PONG reply and notifies the host.

The PROBE command is just to see some information about PCC capabilities on the host.

[felipebalbi]

I think we should tighten up the types before adding more commands here. The wire format needs integers and byte arrays, but the rest of the code doesn't need to think in those terms.

The TryFrom implementations are a start, but we're still exposing a message that's "valid only on success" alongside a public integer that every caller has to interpret. Same issue with probe metadata after a failed query. Let's express those relationships in the types instead of relying on comments and checks in the CLI.

Something like the (incomplete) sample below:

use windows::Win32::Foundation::NTSTATUS;

enum Error {
    InvalidLength,
    UnsupportedVersion,
    MessageTooLarge,
    Native(NTSTATUS),
    // ...
}

// Private storage; fallible construction establishes the size bound.
struct Message {
    bytes: [u8; MESSAGE_SIZE],
    len: usize,
}

struct ExecuteRequest {
    command: u8,
    message: Message,
}

impl ExecuteRequest {
    fn encode(&self) -> [u8; EXECUTE_REQUEST_SIZE] {
        // The message already fits. Encoding cannot fail.
        todo!()
    }
}

struct ExecuteResponse {
    message: [u8; MESSAGE_SIZE],
}

fn parse_execute(bytes: &[u8]) -> Result<ExecuteResponse, Error> {
    // Check the wire layout/version and decode the native status once.
    let (status, message) = decode_wire_response(bytes)?;

    if status.is_err() {
        return Err(Error::Native(status));
    }

    // A failed execution cannot produce this value.
    Ok(ExecuteResponse { message })
}

decode_wire_response() is omitted here. It would return the existing NTSTATUS, not a bare i32. No need to invent another representation for Windows status codes.

Above that, I'd give Ping and Pong their own types, with encoding and parsing at the edges. Checking that a parsed pong's sequence matches the outstanding ping is still necessary; interpreting the wire format shouldn't be the CLI's job.

Also, let's use an Error enum rather than static strings and preserve those errors through the public API instead of collapsing them all into InvalidData. Render the text when presenting the error to the user.

In summary, I'm suggesting that we parse once at the boundary, then carry the invariant in the type. The point isn't to replace every integer with a wrapper. It's to stop passing around states that the next caller has to remember not to use :)

[felipebalbi]

can we give the ping request and pong response proper types? I'd rather have Ping { sequence } with an infallible encoder, and parse the reply into a Pong before checking that it belongs to this request. The copy here is fixed-size, so I'm not claiming it is expensive, but the protocol is currently just byte arrays assembled and interpreted by convention.

[felipebalbi]

let's use an Error enum instead of &'static str here and in the other encode/parse functions. Invalid length, unsupported version, reserved fields and unexpected metadata are different failures. Callers should be able to match on them without comparing error messages. We can render the text when presenting the error to the user.

[felipebalbi]

why expose this as an i32? Same question for query_status. The windows crate already provides NTSTATUS, named status constants and success/failure helpers. Let's convert at the boundary instead of making the CLI interpret signed integers.

Keep the raw code for unknown statuses so we don't lose diagnostics. uefi-rs does something similar with its status type. The CLI shouldn't need to know that a negative integer means failure.

[felipebalbi]

"valid only on success" should be expressed by the type, not just the comment. Can we return a result or option where only the successful variant carries the message? As written, every caller has to remember to check another public field before using this one. Same issue with the probe metadata on a failed query. TryFrom is a start, but the parsed value should carry the invariant forward. Parse, don't validate :)

[felipebalbi]

can we establish the message-size invariant when constructing the request instead? A bounded message type with private storage would let this encoder accept only requests that fit, making encoding infallible. Keep the generic bridge generic if arbitrary command bytes are intentional; the ping-specific types can live above it. The point is to check once at construction, not keep passing an unrestricted slice around.

[felipebalbi]

let's not throw away the parse error here. Same for probe() and the request encoder above. An oversized request from the caller is not the same failure as a malformed response from the driver, and collapsing everything into InvalidData makes both harder to diagnose. Please preserve the typed errors through the public API.