ec: test-lib: Add HID/windows source

[kurtjd]

This adds a new windows source which is designed to talk to Windows class interface drivers. Right now, it only talks to a HIDTime.sys stub (see: OpenDevicePartnership/odp-windows-drivers#4) and thermal and battery fall back on mocks. Note: This was originally called the hid source but after a discussion offline I agreed it makes sense to make the name more generic since really it talks to anything that conforms to the Windows class interface, which might not necessarily be HID behind the scenes.

A couple notes:
IOCTL_GET_WAKE_ALARM_SYSTEM_POWERSTATE (mentioned in linked issue) doesn't seem to correspond to any of the RtcSource traits so it is currently unused. Additionally, get/set wake status don't seem to have corresponding IOCTLs so they currently just return stub values directly in the implementation.

Also, renamed the local source back to acpi since the windows source is local as well (to avoid confusion). So the acpi source goes through the traditional FFA path whereas the windows source talks directly to all Windows class interface drivers.

Tested by injecting this along with the HIDTime stub driver from the linked PR above into a VHDX file and booting it in our platform qemu repo. The app successfully gets the stubbed values back from the driver.

Depends on #161 (to resolve the cargo deny issue).

Resolves OpenDevicePartnership/odp-platform-qemu-arm-virt#44
Resolves https://github.com/OpenDevicePartnership/pfd-devops/issues/29

[Copilot]

Pull request overview

Adds a new Windows HID-backed data source to ec-test-lib and exposes it through the ec-test-cli and ec-test-tui frontends, while renaming the prior “local” Windows source to the more specific acpi.

Changes:

• Introduce ec_test_lib::hid implementing RtcSource via Windows device-interface enumeration + IOCTLs, with thermal/battery temporarily backed by Mock.
• Rename the Windows “local” source selection to acpi and add a new hid source option in both CLI and TUI.
• Update docs and Windows feature flags to reflect the new source and required drivers.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
ec/test-tui/src/source.rs	Adds Windows hid source selection and renames local → acpi.
ec/test-tui/src/main.rs	Updates SourceKind variants/display for acpi + new hid.
ec/test-tui/README.md	Documents acpi and hid source requirements and usage.
ec/test-lib/src/lib.rs	Exposes new Windows-only hid module.
ec/test-lib/src/hid.rs	Implements new HID source (device path resolution + IOCTL-based RTC).
ec/test-lib/README.md	Documents hid as a Windows-only transport and clarifies acpi description.
ec/test-lib/Cargo.toml	Enables windows crate Win32_System_Power feature required by HID source.
ec/test-cli/src/main.rs	Adds dispatch for hid and renames local → acpi.
ec/test-cli/src/cli.rs	Updates SourceKind variants/display for acpi + new hid.
ec/test-cli/README.md	Documents acpi and hid source requirements and usage.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

ec/test-lib/src/hid.rs:56

• get_device_path allocates a Vec<u8> and then casts/dereferences it as SP_DEVICE_INTERFACE_DETAIL_DATA_W (and later casts into *const u16). Because Vec<u8> has alignment 1, this can be misaligned for both the struct and the UTF-16 DevicePath, which is undefined behavior in Rust and can crash on some architectures. Allocate an aligned buffer (or align within a larger allocation) before writing/reading typed fields and building the PCWSTR.

    let mut buffer = vec![0u8; required as usize];
    let detail = buffer.as_mut_ptr() as *mut SP_DEVICE_INTERFACE_DETAIL_DATA_W;
    unsafe {
        (*detail).cbSize = std::mem::size_of::<SP_DEVICE_INTERFACE_DETAIL_DATA_W>() as u32;
    }

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

ec/test-lib/src/windows.rs:245

• Similar to get_timer_value, the IOCTL response's TimerIdentifier is not validated. A mismatched identifier would currently be ignored and could surface an incorrect policy for a different timer.

    fn get_expired_timer_wake_policy(&self, timer_id: AcpiTimerId) -> Result<AlarmExpiredWakePolicy, Error> {
        // WAKE_ALARM_INFORMATION carries the policy seconds in `Timeout`.
        let mut input = [0u8; 8];
        input[0..4].copy_from_slice(&u32::from(timer_id).to_le_bytes());
        let mut out = [0u8; 8];
        self.time_alarm.ioctl(IOCTL_GET_WAKE_ALARM_POLICY, &input, &mut out)?;
        let policy = u32::from_le_bytes(out[4..8].try_into().map_err(|_| Error::InvalidData)?);
        Ok(AlarmExpiredWakePolicy(policy))

ec/test-lib/src/windows.rs:228

• The IOCTL response is treated as valid as long as it returns 8 bytes, but the first 4 bytes (TimerIdentifier) are not validated against the requested timer_id. If the driver returns a mismatched/garbled buffer, this will silently read a timeout for the wrong timer. Consider validating out[0..4] and returning InvalidData on mismatch.

This issue also appears on line 238 of the same file.

        // WAKE_ALARM_INFORMATION { TimerIdentifier: u32, Timeout: u32 }
        let mut input = [0u8; 8];
        input[0..4].copy_from_slice(&u32::from(timer_id).to_le_bytes());
        let mut out = [0u8; 8];
        self.time_alarm.ioctl(IOCTL_GET_WAKE_ALARM_VALUE, &input, &mut out)?;
        let seconds = u32::from_le_bytes(out[4..8].try_into().map_err(|_| Error::InvalidData)?);

[kurtjd]

After talking with @philgweber offline, agreed it makes sense to name this new source more generically since it talks to anything that conforms to the Windows class interface (so the driver behind the scenes might be HID-based, might not). So updated the PR description to match that. This means it can talk to a HID driver that conforms to this interface or a direct ACPI driver that also conforms.

If someone has a better name than windows for the source I'm open to suggestions (naming things is hard).

[williampMSFT]

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

ec/test-cli/src/cli.rs:53

• The PR description says this source was renamed to a more generic windows source because it can talk to any Windows class interface driver (not necessarily HID). However, the CLI still exposes the user-facing name hid, which seems to conflict with that stated intent and may be misleading as more class drivers are added. Consider renaming the variant/value to windows (and keeping HID as an internal implementation detail), or updating the PR description if hid is the intended public name.

    /// Real hardware via the local OS ACPI interface (Windows).
    #[cfg(target_os = "windows")]
    #[default]
    Acpi,
    /// Real hardware via the HID class drivers (Windows).
    #[cfg(target_os = "windows")]
    Hid,

ec/test-lib/src/hid.rs:93

• windows::core::Error::code().0 is an HRESULT, but Error::Io and its Display output describe it as a Win32 error code. This makes failures harder to interpret (HRESULT vs GetLastError). Either store a HRESULT type or update the wording to explicitly say HRESULT (optionally also decoding the underlying Win32 code when applicable).

    /// A Win32 call failed with the given error code.
    Io(i32),
    /// The device returned a malformed or unexpected buffer.
    InvalidData,
}

impl std::fmt::Display for Error {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            Self::DeviceNotFound => write!(f, "HID device not found"),
            Self::Io(code) => write!(f, "Win32 error {code:#x}"),
            Self::InvalidData => write!(f, "Invalid data"),

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

ec/test-lib/src/windows.rs:50

• SetupDiGetDeviceInterfaceDetailW’s initial “size query” call intentionally fails, but the current code discards the error entirely. If the call fails for another reason (bad GUID, permissions, etc.) and required stays 0, this path will incorrectly return DeviceNotFound and lose the real HRESULT, making failures harder to diagnose.

    // First call reports the required buffer size (fails with ERROR_INSUFFICIENT_BUFFER).
    let mut required = 0u32;
    let _ = unsafe {
        SetupDiGetDeviceInterfaceDetailW(device_info_set, &interface_data, None, 0, Some(&mut required), None)
    };

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

ec/test-lib/src/windows.rs:218

• timestamp.as_bytes() returns a byte slice (see acpi.rs), so passing &input makes this an &&[u8]. Pass the slice directly into ioctl.

    fn set_real_time(&self, timestamp: AcpiTimestamp) -> Result<(), Error> {
        let input = timestamp.as_bytes();
        self.time_alarm.ioctl(IOCTL_ACPI_SET_REAL_TIME, &input, &mut [])?;
        Ok(())

ec/test-lib/src/windows.rs:18

• Because this file defines the ec_test_lib::windows module, paths starting with windows::... resolve to the current module, not the external windows crate from dependencies. These imports should be written as absolute ::windows::... paths (or the dependency should be renamed) to avoid name shadowing.

use windows::Win32::Devices::DeviceAndDriverInstallation::*;
use windows::Win32::Foundation::*;
use windows::Win32::Storage::FileSystem::*;
use windows::Win32::System::IO::DeviceIoControl;
use windows::Win32::System::Power::{

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (3)

ec/test-lib/src/windows.rs:273

• get_capabilities() advertises get_wake_status_supported based on the driver-reported capability, but this source’s get_wake_status()/clear_wake_status() implementations are currently stubs that always return "not expired" / success. This can mislead callers (TUI + test-script) into believing wake-status is supported when it isn’t actually implemented. Consider reporting this capability as unsupported until there’s a real IOCTL-backed implementation.

        caps.set_ac_wake_implemented(raw.AcWakeSupported.into());
        caps.set_dc_wake_implemented(raw.DcWakeSupported.into());
        caps.set_realtime_implemented(raw.RealTimeFeaturesSupported.into());
        caps.set_realtime_accuracy_in_milliseconds(raw.RealTimeResolution == AcpiTimeResolutionMilliseconds);
        caps.set_get_wake_status_supported(raw.S4S5WakeStatusSupported.into());
        caps.set_ac_s4_wake_supported(raw.S4AcWakeSupported.into());

ec/test-lib/src/windows.rs:50

• get_device_path() currently collapses all SetupDiEnumDeviceInterfaces failures into DeviceNotFound, and it ignores the error from the first SetupDiGetDeviceInterfaceDetailW sizing call. If either call fails for reasons other than "no interface" / ERROR_INSUFFICIENT_BUFFER, the user will get a misleading DeviceNotFound instead of the real HRESULT, which makes diagnosing driver/permission issues much harder.

    unsafe { SetupDiEnumDeviceInterfaces(device_info_set, None, interface_guid, 0, &mut interface_data) }
        .map_err(|_| Error::DeviceNotFound)?;

    // First call reports the required buffer size (fails with ERROR_INSUFFICIENT_BUFFER).
    let mut required = 0u32;

ec/test-cli/README.md:27

• Docs were updated to rename the Windows ACPI backend from local to acpi, but ec/test-script/README.md still shows ec-test-cli --source local ... (line 31). This will break copy/paste for script runners on Windows after this PR.

ec-test-cli --source <mock|serial|acpi|windows> [OPTIONS] <COMMAND>

• --source — The data source to use. Accepts mock, serial, or (Windows only) acpi and windows. Defaults to serial on Linux and acpi on Windows.

</details>

[philgweber]

For Thermal/MPTF we can talk to Billy about using the Signal IO directly or at what level we want to expose the interface to the OS can hopefully match exactly for ACPI and HID.

[philgweber]

We may find this requirement is a bit too strict. There may be cases for some generic IOCTL's we send a request and response may be variable length. For HID we always know the exact response length, but may need to loosen this requirement if we run into an issue.

[kurtjd]

Sounds good, will keep that in mind. Thanks!

[philgweber]

There are many other battery methods, I assume this is just here for a basic reference so Ratatui doesn't crash?

[kurtjd]

Yes I implement BatterySource and ThermalSource traits here as mock for now just so the app doesn't panic/crash. These will be changed once we have HIDBattery/HIDThermal drivers to match HIDTime.