Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Mar 12, 2024

Conversation

ryan-summers
Copy link
Member

@ryan-summers ryan-summers commented Feb 28, 2024

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.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Could you have a look at the CI?

@ryan-summers ryan-summers marked this pull request as draft February 29, 2024 10:43
@ryan-summers
Copy link
Member Author

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

@ryan-summers ryan-summers marked this pull request as ready for review March 6, 2024 10:22
@vitalyvb
Copy link
Contributor

vitalyvb commented Mar 7, 2024

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)
     }
 

src/test_class.rs Outdated Show resolved Hide resolved
@ryan-summers
Copy link
Member Author

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

Copy link
Contributor

@vitalyvb vitalyvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

@ryan-summers ryan-summers merged commit fddbdbf into master Mar 12, 2024
3 checks passed
@ryan-summers ryan-summers deleted the feature/control-buffer-args branch March 12, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the control transfer buffer size actually configurable
3 participants