rust-embedded-community / usb-device

Experimental device-side USB framework for microcontrollers in Rust.
MIT License
413 stars 77 forks source link

Constructing the control pipe from a user-provided buffer #144

Closed ryan-summers closed 3 months ago

ryan-summers commented 4 months ago

This PR updates the UsbDevice to use a buffer for the control pipe that is provided by the user. This fixes #31.

This is a breaking API change so would require an eventual 0.4.0 release, which would be a good time to additionally propagate errors outwards etc. in the API.

This closes #62 by making control buffers confiugrable by the user.

ryan-summers commented 4 months ago

Fixed CI and reverted this to a draft - I'd like to release before we make breaking API changes here.

vitalyvb commented 3 months ago

Perhaps it would be nice to have a check that the buffer is at least as large as max_packet_size_0. Something like

diff --git a/src/device_builder.rs b/src/device_builder.rs
index c32c03d..d582737 100644
--- a/src/device_builder.rs
+++ b/src/device_builder.rs
@@ -33,6 +33,8 @@ pub enum BuilderError {
     InvalidPacketSize,
     /// Configuration specifies higher USB power draw than allowed
     PowerTooHigh,
+    /// Control buffer is smaller than max_packet_size_0
+    SmallControlBuffer,
 }

 /// Provides basic string descriptors about the device, including the manufacturer, product name,
@@ -110,8 +112,12 @@ impl<'a, B: UsbBus> UsbDeviceBuilder<'a, B> {
     }

     /// Creates the [`UsbDevice`] instance with the configuration in this builder.
-    pub fn build(self) -> UsbDevice<'a, B> {
-        UsbDevice::build(self.alloc, self.config, self.control_buffer)
+    pub fn build(self) -> Result<UsbDevice<'a, B>, BuilderError> {
+        if self.control_buffer.len() < self.config.max_packet_size_0 as usize {
+            Err(BuilderError::SmallControlBuffer)
+        } else {
+            Ok(UsbDevice::build(self.alloc, self.config, self.control_buffer))
+        }
     }

     builder_fields! {
diff --git a/src/test_class.rs b/src/test_class.rs
index 70dbb9c..e08736a 100644
--- a/src/test_class.rs
+++ b/src/test_class.rs
@@ -96,7 +96,7 @@ impl<B: UsbBus> TestClass<'_, B> {

     /// Convenience method to create a UsbDevice that is configured correctly for TestClass.
     pub fn make_device<'a>(&self, usb_bus: &'a UsbBusAllocator<B>) -> UsbDevice<'a, B> {
-        self.make_device_builder(usb_bus).build()
+        self.make_device_builder(usb_bus).build().unwrap()
     }

     /// Convenience method to create a UsbDeviceBuilder that is configured correctly for TestClass.

Or just a

diff --git a/src/device_builder.rs b/src/device_builder.rs
index c32c03d..02320e3 100644
--- a/src/device_builder.rs
+++ b/src/device_builder.rs
@@ -111,6 +111,9 @@ impl<'a, B: UsbBus> UsbDeviceBuilder<'a, B> {

     /// Creates the [`UsbDevice`] instance with the configuration in this builder.
     pub fn build(self) -> UsbDevice<'a, B> {
+        if self.control_buffer.len() < self.config.max_packet_size_0 as usize {
+            panic!("control_buffer is smaller than max_packet_size_0");
+        }
         UsbDevice::build(self.alloc, self.config, self.control_buffer)
     }
ryan-summers commented 3 months ago

@vitalyvb Great idea on checking the control buffer against the max packet size. I've added these checks to the builder.