Merge #721

721: USB HID: simplify API r=Dirbaio a=Dirbaio

Following the discussion from #720  and [matrix](https://matrix.to/#/!YoLPkieCYHGzdjUhOK:matrix.org/$PcPr8E_JbodEPuUUKI2PzIC9sx7nF3y0kV2T5O4UWj8?via=matrix.org&via=converser.eu&via=braun-odw.eu), this is a second take on simplifying the HID API.

Split into a separate PR so it can be reviewed separately.

See individual commit messages for details.

cc `@alexmoon` 

Co-authored-by: Dario Nieuwenhuis <[email protected]>

4 files changed, 178 insertions, 164 deletions

diff --git a/embassy-usb-hid/src/lib.rs b/embassy-usb-hid/src/lib.rs
index e870becf5..abcd6b5d9 100644
--- a/embassy-usb-hid/src/lib.rs
+++ b/embassy-usb-hid/src/lib.rs
@@ -41,6 +41,24 @@ const HID_REQ_SET_REPORT: u8 = 0x09;
 const HID_REQ_GET_PROTOCOL: u8 = 0x03;
 const HID_REQ_SET_PROTOCOL: u8 = 0x0b;
 
+pub struct Config<'d> {
+    /// HID report descriptor.
+    pub report_descriptor: &'d [u8],
+
+    /// Handler for control requests.
+    pub request_handler: Option<&'d dyn RequestHandler>,
+
+    /// Configures how frequently the host should poll for reading/writing HID reports.
+    ///
+    /// A lower value means better throughput & latency, at the expense
+    /// of CPU on the device & bandwidth on the bus. A value of 10 is reasonable for
+    /// high performance uses, and a value of 255 is good for best-effort usecases.
+    pub poll_ms: u8,
+
+    /// Max packet size for both the IN and OUT endpoints.
+    pub max_packet_size: u16,
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
 pub enum ReportId {
@@ -60,12 +78,12 @@ impl ReportId {
     }
 }
 
-pub struct State<'a, const IN_N: usize, const OUT_N: usize> {
-    control: MaybeUninit<Control<'a>>,
+pub struct State<'d> {
+    control: MaybeUninit<Control<'d>>,
     out_report_offset: AtomicUsize,
 }
 
-impl<'a, const IN_N: usize, const OUT_N: usize> State<'a, IN_N, OUT_N> {
+impl<'d> State<'d> {
     pub fn new() -> Self {
         State {
             control: MaybeUninit::uninit(),
@@ -74,107 +92,128 @@ impl<'a, const IN_N: usize, const OUT_N: usize> State<'a, IN_N, OUT_N> {
     }
 }
 
-pub struct HidClass<'d, D: Driver<'d>, T, const IN_N: usize> {
-    input: ReportWriter<'d, D, IN_N>,
-    output: T,
+pub struct HidReaderWriter<'d, D: Driver<'d>, const READ_N: usize, const WRITE_N: usize> {
+    reader: HidReader<'d, D, READ_N>,
+    writer: HidWriter<'d, D, WRITE_N>,
 }
 
-impl<'d, D: Driver<'d>, const IN_N: usize> HidClass<'d, D, (), IN_N> {
-    /// Creates a new HidClass.
-    ///
-    /// poll_ms configures how frequently the host should poll for reading/writing
-    /// HID reports. A lower value means better throughput & latency, at the expense
-    /// of CPU on the device & bandwidth on the bus. A value of 10 is reasonable for
-    /// high performance uses, and a value of 255 is good for best-effort usecases.
-    ///
-    /// This allocates an IN endpoint only.
-    pub fn new<const OUT_N: usize>(
-        builder: &mut UsbDeviceBuilder<'d, D>,
-        state: &'d mut State<'d, IN_N, OUT_N>,
-        report_descriptor: &'static [u8],
-        request_handler: Option<&'d dyn RequestHandler>,
-        poll_ms: u8,
-        max_packet_size: u16,
-    ) -> Self {
-        let ep_in = builder.alloc_interrupt_endpoint_in(max_packet_size, poll_ms);
-        let control = state.control.write(Control::new(
-            report_descriptor,
-            request_handler,
-            &state.out_report_offset,
-        ));
-        control.build(builder, None, &ep_in);
-
-        Self {
-            input: ReportWriter { ep_in },
-            output: (),
-        }
+fn build<'d, D: Driver<'d>>(
+    builder: &mut UsbDeviceBuilder<'d, D>,
+    state: &'d mut State<'d>,
+    config: Config<'d>,
+    with_out_endpoint: bool,
+) -> (Option<D::EndpointOut>, D::EndpointIn, &'d AtomicUsize) {
+    let control = state.control.write(Control::new(
+        config.report_descriptor,
+        config.request_handler,
+        &state.out_report_offset,
+    ));
+
+    let len = config.report_descriptor.len();
+    let if_num = builder.alloc_interface_with_handler(control);
+    let ep_in = builder.alloc_interrupt_endpoint_in(config.max_packet_size, config.poll_ms);
+    let ep_out = if with_out_endpoint {
+        Some(builder.alloc_interrupt_endpoint_out(config.max_packet_size, config.poll_ms))
+    } else {
+        None
+    };
+
+    builder.config_descriptor.interface(
+        if_num,
+        USB_CLASS_HID,
+        USB_SUBCLASS_NONE,
+        USB_PROTOCOL_NONE,
+    );
+
+    // HID descriptor
+    builder.config_descriptor.write(
+        HID_DESC_DESCTYPE_HID,
+        &[
+            // HID Class spec version
+            HID_DESC_SPEC_1_10[0],
+            HID_DESC_SPEC_1_10[1],
+            // Country code not supported
+            HID_DESC_COUNTRY_UNSPEC,
+            // Number of following descriptors
+            1,
+            // We have a HID report descriptor the host should read
+            HID_DESC_DESCTYPE_HID_REPORT,
+            // HID report descriptor size,
+            (len & 0xFF) as u8,
+            (len >> 8 & 0xFF) as u8,
+        ],
+    );
+
+    builder.config_descriptor.endpoint(ep_in.info());
+    if let Some(ep_out) = &ep_out {
+        builder.config_descriptor.endpoint(ep_out.info());
     }
-}
 
-impl<'d, D: Driver<'d>, T, const IN_N: usize> HidClass<'d, D, T, IN_N> {
-    /// Gets the [`ReportWriter`] for input reports.
-    ///
-    /// **Note:** If the `HidClass` was created with [`new_ep_out()`](Self::new_ep_out)
-    /// this writer will be useless as no endpoint is availabe to send reports.
-    pub fn input(&mut self) -> &mut ReportWriter<'d, D, IN_N> {
-        &mut self.input
-    }
+    (ep_out, ep_in, &state.out_report_offset)
 }
 
-impl<'d, D: Driver<'d>, const IN_N: usize, const OUT_N: usize>
-    HidClass<'d, D, ReportReader<'d, D, OUT_N>, IN_N>
+impl<'d, D: Driver<'d>, const READ_N: usize, const WRITE_N: usize>
+    HidReaderWriter<'d, D, READ_N, WRITE_N>
 {
-    /// Creates a new HidClass.
+    /// Creates a new HidReaderWriter.
     ///
-    /// poll_ms configures how frequently the host should poll for reading/writing
-    /// HID reports. A lower value means better throughput & latency, at the expense
-    /// of CPU on the device & bandwidth on the bus. A value of 10 is reasonable for
-    /// high performance uses, and a value of 255 is good for best-effort usecases.
+    /// This will allocate one IN and one OUT endpoints. If you only need writing (sending)
+    /// HID reports, consider using [`HidWriter::new`] instead, which allocates an IN endpoint only.
     ///
-    /// This allocates two endpoints (IN and OUT).
-    pub fn with_output_ep(
+    pub fn new(
         builder: &mut UsbDeviceBuilder<'d, D>,
-        state: &'d mut State<'d, IN_N, OUT_N>,
-        report_descriptor: &'static [u8],
-        request_handler: Option<&'d dyn RequestHandler>,
-        poll_ms: u8,
-        max_packet_size: u16,
+        state: &'d mut State<'d>,
+        config: Config<'d>,
     ) -> Self {
-        let ep_out = builder.alloc_interrupt_endpoint_out(max_packet_size, poll_ms);
-        let ep_in = builder.alloc_interrupt_endpoint_in(max_packet_size, poll_ms);
-
-        let control = state.control.write(Control::new(
-            report_descriptor,
-            request_handler,
-            &state.out_report_offset,
-        ));
-        control.build(builder, Some(&ep_out), &ep_in);
+        let (ep_out, ep_in, offset) = build(builder, state, config, true);
 
         Self {
-            input: ReportWriter { ep_in },
-            output: ReportReader {
-                ep_out,
-                offset: &state.out_report_offset,
+            reader: HidReader {
+                ep_out: ep_out.unwrap(),
+                offset,
             },
+            writer: HidWriter { ep_in },
         }
     }
 
-    /// Gets the [`ReportReader`] for output reports.
-    pub fn output(&mut self) -> &mut ReportReader<'d, D, OUT_N> {
-        &mut self.output
+    /// Splits into seperate readers/writers for input and output reports.
+    pub fn split(self) -> (HidReader<'d, D, READ_N>, HidWriter<'d, D, WRITE_N>) {
+        (self.reader, self.writer)
+    }
+
+    /// Waits for both IN and OUT endpoints to be enabled.
+    pub async fn ready(&mut self) -> () {
+        self.reader.ready().await;
+        self.writer.ready().await;
     }
 
-    /// Splits this `HidClass` into seperate readers/writers for input and output reports.
-    pub fn split(self) -> (ReportWriter<'d, D, IN_N>, ReportReader<'d, D, OUT_N>) {
-        (self.input, self.output)
+    /// Writes an input report by serializing the given report structure.
+    #[cfg(feature = "usbd-hid")]
+    pub async fn write_serialize<IR: AsInputReport>(
+        &mut self,
+        r: &IR,
+    ) -> Result<(), EndpointError> {
+        self.writer.write_serialize(r).await
+    }
+
+    /// Writes `report` to its interrupt endpoint.
+    pub async fn write(&mut self, report: &[u8]) -> Result<(), EndpointError> {
+        self.writer.write(report).await
+    }
+
+    /// Reads an output report from the Interrupt Out pipe.
+    ///
+    /// See [`HidReader::read`].
+    pub async fn read(&mut self, buf: &mut [u8]) -> Result<usize, ReadError> {
+        self.reader.read(buf).await
     }
 }
 
-pub struct ReportWriter<'d, D: Driver<'d>, const N: usize> {
+pub struct HidWriter<'d, D: Driver<'d>, const N: usize> {
     ep_in: D::EndpointIn,
 }
 
-pub struct ReportReader<'d, D: Driver<'d>, const N: usize> {
+pub struct HidReader<'d, D: Driver<'d>, const N: usize> {
     ep_out: D::EndpointOut,
     offset: &'d AtomicUsize,
 }
@@ -197,17 +236,39 @@ impl From<embassy_usb::driver::EndpointError> for ReadError {
     }
 }
 
-impl<'d, D: Driver<'d>, const N: usize> ReportWriter<'d, D, N> {
+impl<'d, D: Driver<'d>, const N: usize> HidWriter<'d, D, N> {
+    /// Creates a new HidWriter.
+    ///
+    /// This will allocate one IN endpoint only, so the host won't be able to send
+    /// reports to us. If you need that, consider using [`HidReaderWriter::new`] instead.
+    ///
+    /// poll_ms configures how frequently the host should poll for reading/writing
+    /// HID reports. A lower value means better throughput & latency, at the expense
+    /// of CPU on the device & bandwidth on the bus. A value of 10 is reasonable for
+    /// high performance uses, and a value of 255 is good for best-effort usecases.
+    pub fn new(
+        builder: &mut UsbDeviceBuilder<'d, D>,
+        state: &'d mut State<'d>,
+        config: Config<'d>,
+    ) -> Self {
+        let (ep_out, ep_in, _offset) = build(builder, state, config, false);
+
+        assert!(ep_out.is_none());
+
+        Self { ep_in }
+    }
+
     /// Waits for the interrupt in endpoint to be enabled.
     pub async fn ready(&mut self) -> () {
         self.ep_in.wait_enabled().await
     }
 
-    /// Tries to write an input report by serializing the given report structure.
-    ///
-    /// Panics if no endpoint is available.
+    /// Writes an input report by serializing the given report structure.
     #[cfg(feature = "usbd-hid")]
-    pub async fn serialize<IR: AsInputReport>(&mut self, r: &IR) -> Result<(), EndpointError> {
+    pub async fn write_serialize<IR: AsInputReport>(
+        &mut self,
+        r: &IR,
+    ) -> Result<(), EndpointError> {
         let mut buf: [u8; N] = [0; N];
         let size = match serialize(&mut buf, r) {
             Ok(size) => size,
@@ -217,8 +278,6 @@ impl<'d, D: Driver<'d>, const N: usize> ReportWriter<'d, D, N> {
     }
 
     /// Writes `report` to its interrupt endpoint.
-    ///
-    /// Panics if no endpoint is available.
     pub async fn write(&mut self, report: &[u8]) -> Result<(), EndpointError> {
         assert!(report.len() <= N);
 
@@ -236,16 +295,13 @@ impl<'d, D: Driver<'d>, const N: usize> ReportWriter<'d, D, N> {
     }
 }
 
-impl<'d, D: Driver<'d>, const N: usize> ReportReader<'d, D, N> {
+impl<'d, D: Driver<'d>, const N: usize> HidReader<'d, D, N> {
     /// Waits for the interrupt out endpoint to be enabled.
     pub async fn ready(&mut self) -> () {
         self.ep_out.wait_enabled().await
     }
 
-    /// Starts a task to deliver output reports from the Interrupt Out pipe to
-    /// `handler`.
-    ///
-    /// Terminates when the interface becomes disabled.
+    /// Delivers output reports from the Interrupt Out pipe to `handler`.
     ///
     /// If `use_report_ids` is true, the first byte of the report will be used as
     /// the `ReportId` value. Otherwise the `ReportId` value will be 0.
@@ -355,17 +411,17 @@ pub trait RequestHandler {
 }
 
 struct Control<'d> {
-    report_descriptor: &'static [u8],
+    report_descriptor: &'d [u8],
     request_handler: Option<&'d dyn RequestHandler>,
     out_report_offset: &'d AtomicUsize,
     hid_descriptor: [u8; 9],
 }
 
-impl<'a> Control<'a> {
+impl<'d> Control<'d> {
     fn new(
-        report_descriptor: &'static [u8],
-        request_handler: Option<&'a dyn RequestHandler>,
-        out_report_offset: &'a AtomicUsize,
+        report_descriptor: &'d [u8],
+        request_handler: Option<&'d dyn RequestHandler>,
+        out_report_offset: &'d AtomicUsize,
     ) -> Self {
         Control {
             report_descriptor,
@@ -391,47 +447,6 @@ impl<'a> Control<'a> {
             ],
         }
     }
-
-    fn build<'d, D: Driver<'d>>(
-        &'d mut self,
-        builder: &mut UsbDeviceBuilder<'d, D>,
-        ep_out: Option<&D::EndpointOut>,
-        ep_in: &D::EndpointIn,
-    ) {
-        let len = self.report_descriptor.len();
-        let if_num = builder.alloc_interface_with_handler(self);
-
-        builder.config_descriptor.interface(
-            if_num,
-            USB_CLASS_HID,
-            USB_SUBCLASS_NONE,
-            USB_PROTOCOL_NONE,
-        );
-
-        // HID descriptor
-        builder.config_descriptor.write(
-            HID_DESC_DESCTYPE_HID,
-            &[
-                // HID Class spec version
-                HID_DESC_SPEC_1_10[0],
-                HID_DESC_SPEC_1_10[1],
-                // Country code not supported
-                HID_DESC_COUNTRY_UNSPEC,
-                // Number of following descriptors
-                1,
-                // We have a HID report descriptor the host should read
-                HID_DESC_DESCTYPE_HID_REPORT,
-                // HID report descriptor size,
-                (len & 0xFF) as u8,
-                (len >> 8 & 0xFF) as u8,
-            ],
-        );
-
-        builder.config_descriptor.endpoint(ep_in.info());
-        if let Some(ep) = ep_out {
-            builder.config_descriptor.endpoint(ep.info());
-        }
-    }
 }
 
 impl<'d> ControlHandler for Control<'d> {
diff --git a/embassy-usb/src/control.rs b/embassy-usb/src/control.rs
index a613f1145..9300d8c4c 100644
--- a/embassy-usb/src/control.rs
+++ b/embassy-usb/src/control.rs
@@ -271,9 +271,9 @@ impl<C: driver::ControlPipe> ControlPipe<C> {
 
             let res = &buf[0..total];
             #[cfg(feature = "defmt")]
-            trace!("  control out data: {:02x}", buf);
+            trace!("  control out data: {:02x}", res);
             #[cfg(not(feature = "defmt"))]
-            trace!("  control out data: {:02x?}", buf);
+            trace!("  control out data: {:02x?}", res);
 
             Ok((res, StatusStage {}))
         }
diff --git a/examples/nrf/src/bin/usb_hid_keyboard.rs b/examples/nrf/src/bin/usb_hid_keyboard.rs
index 5f03f5126..a2d78b08e 100644
--- a/examples/nrf/src/bin/usb_hid_keyboard.rs
+++ b/examples/nrf/src/bin/usb_hid_keyboard.rs
@@ -18,7 +18,7 @@ use embassy_nrf::usb::Driver;
 use embassy_nrf::Peripherals;
 use embassy_usb::control::OutResponse;
 use embassy_usb::{Config, DeviceStateHandler, UsbDeviceBuilder};
-use embassy_usb_hid::{HidClass, ReportId, RequestHandler, State};
+use embassy_usb_hid::{HidReaderWriter, ReportId, RequestHandler, State};
 use futures::future::join;
 use usbd_hid::descriptor::{KeyboardReport, SerializedDescriptor};
 
@@ -75,7 +75,7 @@ async fn main(_spawner: Spawner, p: Peripherals) {
     let request_handler = MyRequestHandler {};
     let device_state_handler = MyDeviceStateHandler::new();
 
-    let mut state = State::<8, 1>::new();
+    let mut state = State::new();
 
     let mut builder = UsbDeviceBuilder::new(
         driver,
@@ -88,14 +88,13 @@ async fn main(_spawner: Spawner, p: Peripherals) {
     );
 
     // Create classes on the builder.
-    let hid = HidClass::with_output_ep(
-        &mut builder,
-        &mut state,
-        KeyboardReport::desc(),
-        Some(&request_handler),
-        60,
-        64,
-    );
+    let config = embassy_usb_hid::Config {
+        report_descriptor: KeyboardReport::desc(),
+        request_handler: Some(&request_handler),
+        poll_ms: 60,
+        max_packet_size: 64,
+    };
+    let hid = HidReaderWriter::<_, 1, 8>::new(&mut builder, &mut state, config);
 
     // Build the builder.
     let mut usb = builder.build();
@@ -135,7 +134,7 @@ async fn main(_spawner: Spawner, p: Peripherals) {
 
     let mut button = Input::new(p.P0_11.degrade(), Pull::Up);
 
-    let (mut hid_in, hid_out) = hid.split();
+    let (reader, mut writer) = hid.split();
 
     // Do stuff with the class!
     let in_fut = async {
@@ -153,7 +152,7 @@ async fn main(_spawner: Spawner, p: Peripherals) {
                     modifier: 0,
                     reserved: 0,
                 };
-                match hid_in.serialize(&report).await {
+                match writer.write_serialize(&report).await {
                     Ok(()) => {}
                     Err(e) => warn!("Failed to send report: {:?}", e),
                 };
@@ -167,7 +166,7 @@ async fn main(_spawner: Spawner, p: Peripherals) {
                 modifier: 0,
                 reserved: 0,
             };
-            match hid_in.serialize(&report).await {
+            match writer.write_serialize(&report).await {
                 Ok(()) => {}
                 Err(e) => warn!("Failed to send report: {:?}", e),
             };
@@ -175,7 +174,7 @@ async fn main(_spawner: Spawner, p: Peripherals) {
     };
 
     let out_fut = async {
-        hid_out.run(false, &request_handler).await;
+        reader.run(false, &request_handler).await;
     };
 
     let power_irq = interrupt::take!(POWER_CLOCK);
diff --git a/examples/nrf/src/bin/usb_hid_mouse.rs b/examples/nrf/src/bin/usb_hid_mouse.rs
index fe27e76fb..1e98dd1ae 100644
--- a/examples/nrf/src/bin/usb_hid_mouse.rs
+++ b/examples/nrf/src/bin/usb_hid_mouse.rs
@@ -13,7 +13,7 @@ use embassy_nrf::usb::Driver;
 use embassy_nrf::Peripherals;
 use embassy_usb::control::OutResponse;
 use embassy_usb::{Config, UsbDeviceBuilder};
-use embassy_usb_hid::{HidClass, ReportId, RequestHandler, State};
+use embassy_usb_hid::{HidWriter, ReportId, RequestHandler, State};
 use futures::future::join;
 use usbd_hid::descriptor::{MouseReport, SerializedDescriptor};
 
@@ -52,7 +52,7 @@ async fn main(_spawner: Spawner, p: Peripherals) {
     let mut control_buf = [0; 16];
     let request_handler = MyRequestHandler {};
 
-    let mut control = State::<5, 0>::new();
+    let mut state = State::new();
 
     let mut builder = UsbDeviceBuilder::new(
         driver,
@@ -65,14 +65,14 @@ async fn main(_spawner: Spawner, p: Peripherals) {
     );
 
     // Create classes on the builder.
-    let mut hid = HidClass::new(
-        &mut builder,
-        &mut control,
-        MouseReport::desc(),
-        Some(&request_handler),
-        60,
-        8,
-    );
+    let config = embassy_usb_hid::Config {
+        report_descriptor: MouseReport::desc(),
+        request_handler: Some(&request_handler),
+        poll_ms: 60,
+        max_packet_size: 8,
+    };
+
+    let mut writer = HidWriter::<_, 5>::new(&mut builder, &mut state, config);
 
     // Build the builder.
     let mut usb = builder.build();
@@ -94,7 +94,7 @@ async fn main(_spawner: Spawner, p: Peripherals) {
                 wheel: 0,
                 pan: 0,
             };
-            match hid.input().serialize(&report).await {
+            match writer.write_serialize(&report).await {
                 Ok(()) => {}
                 Err(e) => warn!("Failed to send report: {:?}", e),
             }