Separate read_setup() from read()

This change was made in response to an issue found on the Microchip
SAMD21, where an expected OUT Zero Length Packet (ZLP) was sometimes
overwritten by a subsequent SETUP transaction.  The USB spec requires
that devices accept SETUP transactions, and in the SAMD parts this
behaviour is implemented in hardware.  To allow the control pipe
implementation to distinguish between OUT data and SETUP data, the
read() method was changed to return an error if SETUP data was present,
and a new read_setup() was added that expects SETUP data not OUT data.

Author: ianrrees

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ### Changed
 
+* [breaking] Bus API now has separate `read()` and `read_setup()` methods for OUT and SETUP data.
 * [breaking] The control pipe is now provided in the `UsbDeviceBuilder` API to allow for user-provided control
 pipes. This makes it so that control pipes have configurable sizing.
 * Don't require UsbBus to be Sync. If a UsbBus is not Sync, it can still be used to make a UsbDevice, but that UsbDevice will not be Sync (ensuring soundness).

src/bus.rs

@@ -81,8 +81,8 @@ pub trait UsbBus: Sized {
     /// Implementations may also return other errors if applicable.
     fn write(&self, ep_addr: EndpointAddress, buf: &[u8]) -> Result<usize>;
 
-    /// Reads a single packet of data from the specified endpoint and returns the actual length of
-    /// the packet.
+    /// Reads a single packet of non-SETUP data from the specified endpoint and returns the length
+    /// of the packet.
     ///
     /// This should also clear any NAK flags and prepare the endpoint to receive the next packet.
     ///
@@ -97,10 +97,32 @@ pub trait UsbBus: Sized {
     ///   fit in `buf`. This is generally an error in the class implementation, because the class
     ///   should use a buffer that is large enough for the `max_packet_size` it specified when
     ///   allocating the endpoint.
+    /// * [`InvalidState`](crate::UsbError::InvalidState) - The received packet is a SETUP
+    ///   transaction, and needs to be read through [`read_setup()`](UsbBus::read_setup()) instead.
     ///
     /// Implementations may also return other errors if applicable.
     fn read(&self, ep_addr: EndpointAddress, buf: &mut [u8]) -> Result<usize>;
 
+    /// Reads a packet of SETUP data from the specified endpoint, returns the length of the packet
+    ///
+    /// This is a distinct method from [`read()`](UsbBus::read()) because the USB spec states that
+    /// the function (device) must accept the SETUP transaction, which in some implementations can
+    /// result in a SETUP transaction overwriting data from an earlier OUT transaction.  Separate
+    /// read methods allow the control pipe implementation to detect this overwriting.
+    ///
+    /// # Errors
+    ///
+    /// * [`InvalidEndpoint`](crate::UsbError::InvalidEndpoint) - The `ep_addr` does not point to a
+    ///   valid endpoint that was previously allocated with [`UsbBus::alloc_ep`].
+    /// * [`WouldBlock`](crate::UsbError::WouldBlock) - There is no SETUP packet to be read. Note
+    ///   that this is different from a received zero-length packet, which is valid in USB. A
+    ///   zero-length packet will return `Ok(0)`.
+    /// * [`BufferOverflow`](crate::UsbError::BufferOverflow) - The received packet is too long to
+    ///   fit in `buf`. This is generally an error in the class implementation, because the class
+    ///   should use a buffer that is large enough for the `max_packet_size` it specified when
+    ///   allocating the endpoint.
+    fn read_setup(&self, ep_addr: EndpointAddress, buf: &mut [u8]) -> Result<usize>;
+
     /// Sets or clears the STALL condition for an endpoint. If the endpoint is an OUT endpoint, it
     /// should be prepared to receive data again.
     fn set_stalled(&self, ep_addr: EndpointAddress, stalled: bool);

src/control_pipe.rs

@@ -65,7 +65,7 @@ impl<B: UsbBus> ControlPipe<'_, B> {
     }
 
     pub fn handle_setup(&mut self) -> Option<Request> {
-        let count = match self.ep_out.read(&mut self.buf[..]) {
+        let count = match self.ep_out.read_setup(&mut self.buf[..]) {
             Ok(count) => {
                 usb_trace!("Read {} bytes on EP0-OUT: {:?}", count, &self.buf[..count]);
                 count
@@ -165,7 +165,15 @@ impl<B: UsbBus> ControlPipe<'_, B> {
                     "Control transfer completed. Current state: {:?}",
                     self.state
                 );
-                self.ep_out.read(&mut [])?;
+                match self.ep_out.read(&mut []) {
+                    Ok(_) => {}
+                    Err(UsbError::InvalidState) => {
+                        // Host sent a new SETUP transaction, which may have overwritten the ZLP
+                    }
+                    Err(err) => {
+                        return Err(err);
+                    }
+                }
                 self.state = ControlState::Idle;
             }
             _ => {

src/dummy.rs

@@ -56,6 +56,14 @@ impl UsbBus for DummyUsbBus {
         unimplemented!()
     }
 
+    fn read_setup(
+        &self,
+        ep_addr: crate::class_prelude::EndpointAddress,
+        buf: &mut [u8],
+    ) -> crate::Result<usize> {
+        unimplemented!()
+    }
+
     fn reset(&self) {
         unimplemented!()
     }

src/endpoint.rs

@@ -1,5 +1,5 @@
 use crate::bus::UsbBus;
-use crate::{Result, UsbDirection};
+use crate::{Result, UsbDirection, UsbError};
 use core::marker::PhantomData;
 use portable_atomic::{AtomicPtr, Ordering};
 
@@ -197,9 +197,10 @@ impl<B: UsbBus> Endpoint<'_, B, In> {
 }
 
 impl<B: UsbBus> Endpoint<'_, B, Out> {
-    /// Reads a single packet of data from the specified endpoint and returns the actual length of
-    /// the packet. The buffer should be large enough to fit at least as many bytes as the
-    /// `max_packet_size` specified when allocating the endpoint.
+    /// Reads a single packet of data and returns the length of the packet
+    ///
+    /// The buffer should be large enough to fit at least as many bytes as the `max_packet_size`
+    /// specified when allocating the endpoint.
     ///
     /// # Errors
     ///
@@ -211,9 +212,37 @@ impl<B: UsbBus> Endpoint<'_, B, Out> {
     ///   USB. A zero-length packet will return `Ok(0)`.
     /// * [`BufferOverflow`](crate::UsbError::BufferOverflow) - The received packet is too long to
     ///   fit in `data`. This is generally an error in the class implementation.
+    /// * [`InvalidState`](crate::UsbError::InvalidState) - The received packet is a SETUP
+    ///   transaction, and needs to be read through [`read_setup()`](Endpoint::read_setup())
+    ///   instead.
     pub fn read(&self, data: &mut [u8]) -> Result<usize> {
         self.bus().read(self.address, data)
     }
+
+    /// Reads a single packet of SETUP data and returns the length of the packet
+    ///
+    /// The buffer should be large enough to fit at least as many bytes as the `max_packet_size`
+    /// specified when allocating the endpoint.  See [`UsbBus::read_setup()`] for rationale for two
+    /// distinct read methods.
+    ///
+    /// # Errors
+    ///
+    /// Note: USB bus implementation errors are directly passed through, so be prepared to handle
+    /// other errors as well.
+    ///
+    /// * [`WouldBlock`](crate::UsbError::WouldBlock) - There is no packet to be read. Note that
+    ///   this is different from a received zero-length packet, which is valid and significant in
+    ///   USB. A zero-length packet will return `Ok(0)`.
+    /// * [`BufferOverflow`](crate::UsbError::BufferOverflow) - The received packet is too long to
+    ///   fit in `data`. This is generally an error in the class implementation.
+    pub fn read_setup(&self, data: &mut [u8]) -> Result<usize> {
+        // SETUP transactions can only occur on control endpoints
+        if self.ep_type != EndpointType::Control {
+            Err(UsbError::InvalidEndpoint)
+        } else {
+            self.bus().read_setup(self.address, data)
+        }
+    }
 }
 
 /// Type-safe endpoint address.

src/lib.rs

@@ -258,6 +258,10 @@ fn _ensure_sync() {
             Err(UsbError::InvalidEndpoint)
         }
 
+        fn read_setup(&self, _ep_addr: EndpointAddress, _buf: &mut [u8]) -> Result<usize> {
+            Err(UsbError::InvalidEndpoint)
+        }
+
         fn set_stalled(&self, _ep_addr: EndpointAddress, _stalled: bool) {}
         fn is_stalled(&self, _ep_addr: EndpointAddress) -> bool {
             false