From 5b47e732415b84ac0f8af01ddc7c1dfcb7256dff Mon Sep 17 00:00:00 2001 From: Ian Rees Date: Tue, 6 Aug 2024 15:32:13 +1200 Subject: [PATCH 1/7] Refactor tests to take rusb r/t device handle Need to support a test that hard resets the device and ensures that it re-enumerates. The current model supplies a device handle to the test case, but this case will require a handle to the rusb context so that it can determine when the device has enumerated. --- tests/test_class_host/device.rs | 149 +++++++++++++++++++++++++++----- tests/test_class_host/main.rs | 48 +--------- tests/test_class_host/tests.rs | 116 ++++++++++++++++++++++--- 3 files changed, 232 insertions(+), 81 deletions(-) diff --git a/tests/test_class_host/device.rs b/tests/test_class_host/device.rs index d204985..61e2816 100644 --- a/tests/test_class_host/device.rs +++ b/tests/test_class_host/device.rs @@ -1,7 +1,11 @@ use rusb::{ConfigDescriptor, Context, DeviceDescriptor, DeviceHandle, Language, UsbContext as _}; +use std::thread; use std::time::Duration; +use usb_device::device::CONFIGURATION_VALUE; use usb_device::test_class; +const TEST_INTERFACE: u8 = 0; + pub const TIMEOUT: Duration = Duration::from_secs(1); pub const EN_US: u16 = 0x0409; @@ -18,6 +22,7 @@ impl DeviceHandles { pub fn is_high_speed(&self) -> bool { self.handle.device().speed() == rusb::Speed::High } + /// Returns the max packet size for the `TestClass` bulk endpoint(s). pub fn bulk_max_packet_size(&self) -> u16 { self.config_descriptor @@ -34,6 +39,40 @@ impl DeviceHandles { .next() .expect("TestClass has at least one bulk endpoint") } + + /// Puts the device in a consistent state for running a test + pub fn pre_test(&mut self) -> rusb::Result<()> { + let res = self.reset(); + if let Err(err) = res { + println!("Failed to reset the device: {}", err); + return res; + } + + let res = self.set_active_configuration(CONFIGURATION_VALUE); + if let Err(err) = res { + println!("Failed to set active configuration: {}", err); + return res; + } + + let res = self.claim_interface(TEST_INTERFACE); + if let Err(err) = res { + println!("Failed to claim interface: {}", err); + return res; + } + + Ok(()) + } + + /// Cleanup the device following a test + pub fn post_test(&mut self) -> rusb::Result<()> { + let res = self.release_interface(TEST_INTERFACE); + if let Err(err) = res { + println!("Failed to release interface: {}", err); + return res; + } + + Ok(()) + } } impl ::std::ops::Deref for DeviceHandles { @@ -50,38 +89,102 @@ impl ::std::ops::DerefMut for DeviceHandles { } } -pub fn open_device(ctx: &Context) -> rusb::Result { - for device in ctx.devices()?.iter() { - let device_descriptor = device.device_descriptor()?; +pub struct UsbContext { + /// rusb Context handle + inner: Context, + device: Option, +} - if !(device_descriptor.vendor_id() == test_class::VID - && device_descriptor.product_id() == test_class::PID) - { - continue; - } +impl UsbContext { + pub fn new() -> rusb::Result { + let inner = rusb::Context::new()?; - let mut handle = device.open()?; + Ok(Self { + inner, + device: None, + }) + } + + /// Attempt to open the test device once + fn try_open_device(&self) -> rusb::Result { + for device in self.inner.devices()?.iter() { + let device_descriptor = device.device_descriptor()?; + + if !(device_descriptor.vendor_id() == test_class::VID + && device_descriptor.product_id() == test_class::PID) + { + continue; + } + + let mut handle = device.open()?; + + let langs = handle.read_languages(TIMEOUT)?; + if langs.is_empty() || langs[0].lang_id() != EN_US { + continue; + } + + let prod = handle.read_product_string(langs[0], &device_descriptor, TIMEOUT)?; + + if prod == test_class::PRODUCT { + handle.reset()?; + + let config_descriptor = device.config_descriptor(0)?; - let langs = handle.read_languages(TIMEOUT)?; - if langs.is_empty() || langs[0].lang_id() != EN_US { - continue; + return Ok(DeviceHandles { + device_descriptor, + config_descriptor, + handle, + en_us: langs[0], + }); + } } - let prod = handle.read_product_string(langs[0], &device_descriptor, TIMEOUT)?; + Err(rusb::Error::NoDevice) + } - if prod == test_class::PRODUCT { - handle.reset()?; + /// Look for the device for about 5 seconds in case it hasn't finished enumerating yet + pub fn open_device(&mut self) -> rusb::Result<&mut DeviceHandles> { + if self.device.is_none() { + for _ in 0..50 { + if let Ok(dev) = self.try_open_device() { + self.device = Some(dev); + break; + } + thread::sleep(Duration::from_millis(100)); + } + } - let config_descriptor = device.config_descriptor(0)?; + match self.device.as_mut() { + Some(device) => Ok(device), + None => Err(rusb::Error::NoDevice), + } + } - return Ok(DeviceHandles { - device_descriptor, - config_descriptor, - handle, - en_us: langs[0], - }); + /// Attempts to open (if necessary) and (re-)initialize a device for a test + pub fn device_for_test(&mut self) -> rusb::Result<&mut DeviceHandles> { + let dev = match self.open_device() { + Ok(dev) => dev, + Err(err) => { + println!("Did not find a TestClass device. Make sure the device is correctly programmed and plugged in. Last error: {}", err); + return Err(err); + } + }; + + match dev.pre_test() { + Ok(()) => Ok(dev), + Err(err) => { + println!("Failed to prepare for test: {}", err); + Err(err) + } } } - Err(rusb::Error::NoDevice) + /// Releases resources that might have been used in a test + pub fn cleanup_after_test(&mut self) -> rusb::Result<()> { + if let Some(dev) = &mut self.device { + dev.post_test() + } else { + Ok(()) + } + } } diff --git a/tests/test_class_host/main.rs b/tests/test_class_host/main.rs index 9cab6cb..bc7e40c 100644 --- a/tests/test_class_host/main.rs +++ b/tests/test_class_host/main.rs @@ -6,14 +6,11 @@ mod device; /// cannot be run in parallel. mod tests; -use crate::device::open_device; +use crate::device::UsbContext; use crate::tests::{get_tests, TestFn}; use std::io::prelude::*; use std::io::stdout; use std::panic; -use std::thread; -use std::time::Duration; -use usb_device::device::CONFIGURATION_VALUE; fn main() { let tests = get_tests(); @@ -21,51 +18,15 @@ fn main() { } fn run_tests(tests: &[(&str, TestFn)]) { - const INTERFACE: u8 = 0; - println!("test_class_host starting"); println!("looking for device..."); - let ctx = rusb::Context::new().expect("create libusb context"); - - // Look for the device for about 5 seconds in case it hasn't finished enumerating yet - let mut dev = Err(rusb::Error::NoDevice); - for _ in 0..50 { - dev = open_device(&ctx); - if dev.is_ok() { - break; - } - - thread::sleep(Duration::from_millis(100)); - } - - let mut dev = match dev { - Ok(d) => d, - Err(err) => { - println!("Did not find a TestClass device. Make sure the device is correctly programmed and plugged in. Last error: {}", err); - return; - } - }; + let mut ctx = UsbContext::new().expect("create libusb context"); println!("\nrunning {} tests", tests.len()); let mut success = 0; for (name, test) in tests { - if let Err(err) = dev.reset() { - println!("Failed to reset the device: {}", err); - return; - } - - if let Err(err) = dev.set_active_configuration(CONFIGURATION_VALUE) { - println!("Failed to set active configuration: {}", err); - return; - } - - if let Err(err) = dev.claim_interface(INTERFACE) { - println!("Failed to claim interface: {}", err); - return; - } - print!("test {} ... ", name); let _ = stdout().flush(); @@ -75,15 +36,14 @@ fn run_tests(tests: &[(&str, TestFn)]) { let hook = panic::take_hook(); panic::set_hook(Box::new(|_| {})); let res = panic::catch_unwind(panic::AssertUnwindSafe(|| { - test(&mut dev, &mut out); + test(&mut ctx, &mut out); })); panic::set_hook(hook); res }; - dev.release_interface(INTERFACE) - .expect("failed to release interface"); + ctx.cleanup_after_test().expect("post test cleanup failed"); if let Err(err) = res { let err = if let Some(err) = err.downcast_ref::<&'static str>() { diff --git a/tests/test_class_host/tests.rs b/tests/test_class_host/tests.rs index f745a1d..6e5b86a 100644 --- a/tests/test_class_host/tests.rs +++ b/tests/test_class_host/tests.rs @@ -6,7 +6,7 @@ use std::fmt::Write; use std::time::{Duration, Instant}; use usb_device::test_class; -pub type TestFn = fn(&mut DeviceHandles, &mut String) -> (); +pub type TestFn = fn(&mut UsbContext, &mut String) -> (); const BENCH_TIMEOUT: Duration = Duration::from_secs(10); @@ -16,7 +16,7 @@ macro_rules! tests { let mut tests: Vec<(&'static str, TestFn)> = Vec::new(); $( - fn $name($dev: &mut DeviceHandles, $out: &mut String) { + fn $name($dev: &mut UsbContext, $out: &mut String) { $body } @@ -30,7 +30,15 @@ macro_rules! tests { tests! { -fn control_request(dev, _out) { +fn control_request(context, _out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + let mut rng = rand::thread_rng(); let value: u16 = rng.gen(); @@ -63,7 +71,15 @@ fn control_request(dev, _out) { assert_eq!(&response, &expected); } -fn control_data(dev, _out) { +fn control_data(context, _out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + for len in &[0, 7, 8, 9, 15, 16, 17] { let data = random_data(*len); @@ -87,7 +103,15 @@ fn control_data(dev, _out) { } } -fn control_data_static(dev, _out) { +fn control_data_static(context, _out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + let mut response = [0u8; 257]; assert_eq!( @@ -100,7 +124,15 @@ fn control_data_static(dev, _out) { assert_eq!(&response[..], test_class::LONG_DATA); } -fn control_error(dev, _out) { +fn control_error(context, _out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + let res = dev.write_control( request_type(Direction::Out, RequestType::Vendor, Recipient::Device), test_class::REQ_UNKNOWN, 0, 0, @@ -111,7 +143,15 @@ fn control_error(dev, _out) { } } -fn string_descriptors(dev, _out) { +fn string_descriptors(context, _out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + assert_eq!( dev.read_product_string(dev.en_us, &dev.device_descriptor, TIMEOUT) .expect("read product string"), @@ -133,7 +173,15 @@ fn string_descriptors(dev, _out) { test_class::CUSTOM_STRING); } -fn interface_descriptor(dev, _out) { +fn interface_descriptor(context, _out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + let iface = dev.config_descriptor .interfaces() .find(|i| i.number() == 0) @@ -163,7 +211,15 @@ fn interface_descriptor(dev, _out) { test_class::INTERFACE_STRING); } -fn iso_endpoint_descriptors(dev, _out) { +fn iso_endpoint_descriptors(context, _out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + // Tests that an isochronous endpoint descriptor is present in the first // alternate setting, but not in the default setting. let iface = dev.config_descriptor @@ -197,7 +253,15 @@ fn iso_endpoint_descriptors(dev, _out) { assert!(iso_ep_count > 0, "At least one isochronous endpoint is expected"); } -fn bulk_loopback(dev, _out) { +fn bulk_loopback(context, _out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + let mut lens = vec![0, 1, 2, 32, 63, 64, 65, 127, 128, 129]; if dev.is_high_speed() { lens.extend([255, 256, 257, 511, 512, 513, 1023, 1024, 1025]); @@ -235,7 +299,15 @@ fn bulk_loopback(dev, _out) { } } -fn interrupt_loopback(dev, _out) { +fn interrupt_loopback(context, _out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + for len in &[0, 1, 2, 15, 31] { let data = random_data(*len); @@ -259,7 +331,15 @@ fn interrupt_loopback(dev, _out) { } } -fn bench_bulk_write(dev, out) { +fn bench_bulk_write(context, out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + run_bench(dev, out, |data| { assert_eq!( dev.write_bulk(0x01, data, BENCH_TIMEOUT) @@ -269,7 +349,15 @@ fn bench_bulk_write(dev, out) { }); } -fn bench_bulk_read(dev, out) { +fn bench_bulk_read(context, out) { + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + run_bench(dev, out, |data| { assert_eq!( dev.read_bulk(0x81, data, BENCH_TIMEOUT) @@ -279,7 +367,7 @@ fn bench_bulk_read(dev, out) { }); } -} +} // end tests! { fn run_bench(dev: &DeviceHandles, out: &mut String, f: impl Fn(&mut [u8])) { const TRANSFER_BYTES: usize = 64 * 1024; From 0e4f77d263e2bea01544fec3dc1181300a5ce256 Mon Sep 17 00:00:00 2001 From: Ian Rees Date: Fri, 9 Aug 2024 19:58:40 +1200 Subject: [PATCH 2/7] Add interface for hard resetting USB test class --- src/test_class.rs | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/test_class.rs b/src/test_class.rs index 3f2d077..4292a85 100644 --- a/src/test_class.rs +++ b/src/test_class.rs @@ -5,6 +5,7 @@ use crate::device::{StringDescriptors, UsbDevice, UsbDeviceBuilder, UsbVidPid}; use crate::Result; use core::cell::UnsafeCell; use core::cmp; +use core::marker::PhantomData; #[cfg(feature = "test-class-high-speed")] mod sizes { @@ -24,9 +25,14 @@ mod sizes { static mut CONTROL_BUFFER: UnsafeCell<[u8; 256]> = UnsafeCell::new([0; 256]); +pub trait HardwareSupport { + /// Hard reset the test device + fn hard_reset() -> !; +} + /// Test USB class for testing USB driver implementations. Supports various endpoint types and /// requests for testing USB peripheral drivers on actual hardware. -pub struct TestClass<'a, B: UsbBus> { +pub struct TestClass<'a, B: UsbBus, H: HardwareSupport> { custom_string: StringIndex, interface_string: StringIndex, iface: InterfaceNumber, @@ -45,6 +51,7 @@ pub struct TestClass<'a, B: UsbBus> { expect_bulk_out: bool, expect_interrupt_in_complete: bool, expect_interrupt_out: bool, + hardware: PhantomData, } pub const VID: u16 = 0x16c0; @@ -60,13 +67,14 @@ pub const REQ_READ_BUFFER: u8 = 2; pub const REQ_WRITE_BUFFER: u8 = 3; pub const REQ_SET_BENCH_ENABLED: u8 = 4; pub const REQ_READ_LONG_DATA: u8 = 5; +pub const REQ_HARD_RESET: u8 = 6; pub const REQ_UNKNOWN: u8 = 42; pub const LONG_DATA: &[u8] = &[0x17; 257]; -impl TestClass<'_, B> { +impl TestClass<'_, B, H> { /// Creates a new TestClass. - pub fn new(alloc: &UsbBusAllocator) -> TestClass<'_, B> { + pub fn new(alloc: &UsbBusAllocator) -> TestClass<'_, B, H> { TestClass { custom_string: alloc.string(), interface_string: alloc.string(), @@ -91,6 +99,7 @@ impl TestClass<'_, B> { expect_bulk_out: false, expect_interrupt_in_complete: false, expect_interrupt_out: false, + hardware: PhantomData, } } @@ -214,7 +223,7 @@ impl TestClass<'_, B> { } } -impl UsbClass for TestClass<'_, B> { +impl UsbClass for TestClass<'_, B, H> { fn reset(&mut self) { self.len = 0; self.i = 0; @@ -335,6 +344,9 @@ impl UsbClass for TestClass<'_, B> { xfer.accept() .expect("control_out REQ_SET_BENCH_ENABLED failed"); } + REQ_HARD_RESET => { + H::hard_reset(); + } _ => xfer.reject().expect("control_out reject failed"), } } From 30df4a3df96b50669a2e33757eb32c5ee67809ab Mon Sep 17 00:00:00 2001 From: Ian Rees Date: Fri, 9 Aug 2024 20:00:40 +1200 Subject: [PATCH 3/7] Minor refactor in test class host This reduces log noise for tests that reset the device under test --- tests/test_class_host/device.rs | 8 +------- tests/test_class_host/main.rs | 5 ++++- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/test_class_host/device.rs b/tests/test_class_host/device.rs index 61e2816..d4c9798 100644 --- a/tests/test_class_host/device.rs +++ b/tests/test_class_host/device.rs @@ -65,13 +65,7 @@ impl DeviceHandles { /// Cleanup the device following a test pub fn post_test(&mut self) -> rusb::Result<()> { - let res = self.release_interface(TEST_INTERFACE); - if let Err(err) = res { - println!("Failed to release interface: {}", err); - return res; - } - - Ok(()) + self.release_interface(TEST_INTERFACE) } } diff --git a/tests/test_class_host/main.rs b/tests/test_class_host/main.rs index bc7e40c..9df633d 100644 --- a/tests/test_class_host/main.rs +++ b/tests/test_class_host/main.rs @@ -43,7 +43,10 @@ fn run_tests(tests: &[(&str, TestFn)]) { res }; - ctx.cleanup_after_test().expect("post test cleanup failed"); + if let Err(err) = ctx.cleanup_after_test() { + println!("Failed to release interface: {}", err); + panic!("post test cleanup failed"); + } if let Err(err) = res { let err = if let Some(err) = err.downcast_ref::<&'static str>() { From d8d7e0ca8d69c1a9dddea97000b53abb32445628 Mon Sep 17 00:00:00 2001 From: Ian Rees Date: Fri, 9 Aug 2024 20:01:53 +1200 Subject: [PATCH 4/7] Add test_class_host reopen_device() Needed for testing enumeration time --- tests/test_class_host/device.rs | 46 ++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/tests/test_class_host/device.rs b/tests/test_class_host/device.rs index d4c9798..a67857b 100644 --- a/tests/test_class_host/device.rs +++ b/tests/test_class_host/device.rs @@ -1,6 +1,6 @@ use rusb::{ConfigDescriptor, Context, DeviceDescriptor, DeviceHandle, Language, UsbContext as _}; use std::thread; -use std::time::Duration; +use std::time::{Duration, Instant}; use usb_device::device::CONFIGURATION_VALUE; use usb_device::test_class; @@ -100,7 +100,7 @@ impl UsbContext { } /// Attempt to open the test device once - fn try_open_device(&self) -> rusb::Result { + fn open_device_immediate(&self) -> rusb::Result { for device in self.inner.devices()?.iter() { let device_descriptor = device.device_descriptor()?; @@ -136,15 +136,31 @@ impl UsbContext { Err(rusb::Error::NoDevice) } - /// Look for the device for about 5 seconds in case it hasn't finished enumerating yet - pub fn open_device(&mut self) -> rusb::Result<&mut DeviceHandles> { + /// Look for the device, retry until timeout expires + pub fn open_device(&mut self, timeout: Option) -> rusb::Result<&mut DeviceHandles> { if self.device.is_none() { - for _ in 0..50 { - if let Ok(dev) = self.try_open_device() { - self.device = Some(dev); - break; + match timeout { + Some(timeout) => { + let deadline = Instant::now() + timeout; + loop { + if let Ok(dev) = self.open_device_immediate() { + self.device = Some(dev); + break; + } + let now = Instant::now(); + if now >= deadline { + break; + } else { + let dur = Duration::from_millis(100).min(deadline - now); + thread::sleep(dur); + } + } + } + None => { + if let Ok(dev) = self.open_device_immediate() { + self.device = Some(dev); + } } - thread::sleep(Duration::from_millis(100)); } } @@ -154,9 +170,19 @@ impl UsbContext { } } + /// Closes device if it was open (handling errors), attempts to reopen + pub fn reopen_device(&mut self, timeout: Option) -> rusb::Result<&mut DeviceHandles> { + // This is expected to fail in tests where device was asked to reset + let _ = self.cleanup_after_test(); + + self.device = None; + + self.open_device(timeout) + } + /// Attempts to open (if necessary) and (re-)initialize a device for a test pub fn device_for_test(&mut self) -> rusb::Result<&mut DeviceHandles> { - let dev = match self.open_device() { + let dev = match self.open_device(Some(Duration::from_secs(5))) { Ok(dev) => dev, Err(err) => { println!("Did not find a TestClass device. Make sure the device is correctly programmed and plugged in. Last error: {}", err); From 5f6e49b35119222bd666242f48c4e2cbf7b91c77 Mon Sep 17 00:00:00 2001 From: Ian Rees Date: Fri, 9 Aug 2024 20:02:06 +1200 Subject: [PATCH 5/7] Add test case for prompt enumeration --- CHANGELOG.md | 1 + tests/test_class_host/tests.rs | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aeb10f9..28f53d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added +* Test case that hard-resets the device and measures the time it takes to re-enumerate. * `DummyUsbBus` without functionality to allow examples that actually compile (but not run). * Extended `UsbRev` enum with variants for USB 1.0 and 1.1. diff --git a/tests/test_class_host/tests.rs b/tests/test_class_host/tests.rs index 6e5b86a..7ac65b8 100644 --- a/tests/test_class_host/tests.rs +++ b/tests/test_class_host/tests.rs @@ -3,6 +3,7 @@ use rand::prelude::*; use rusb::{request_type, Direction, Recipient, RequestType, TransferType}; use std::cmp::max; use std::fmt::Write; +use std::thread; use std::time::{Duration, Instant}; use usb_device::test_class; @@ -367,6 +368,62 @@ fn bench_bulk_read(context, out) { }); } +fn enumerates_promptly(context, out) { + // Time measured between issuance of the hard reset command, to the device + // successfully enumerating. This approach might need to be improved on, + // since the time might depend on USB host implementation details, + // bootloaders in the device, etc. + const MAX_TIME: Duration = Duration::from_secs(2); + + let dev = match context.device_for_test() { + Ok(dev) => dev, + Err(err) => { + assert!(false, "Failed to prepare for test: {}", err); + return; + } + }; + + // Ensure we've got a device by doing a dummy read + let mut response = [0u8; 8]; + dev.read_control( + request_type(Direction::In, RequestType::Vendor, Recipient::Device), + test_class::REQ_READ_BUFFER, 0, 0, &mut response, TIMEOUT) + .expect("control read"); + + // Since the write_control() may have a timeout, measure from the point when + // the request is sent + let reset_started = Instant::now(); + + // This is expected to fail since the device immediately restarts + let res = dev.write_control( + request_type(Direction::Out, RequestType::Vendor, Recipient::Device), + test_class::REQ_HARD_RESET, 0, 0, &[], TIMEOUT); + + if res.is_ok() { + panic!("Hard reset request succeeded, implies the device didn't reset"); + } + + loop { + thread::sleep(Duration::from_millis(100)); + + if reset_started.elapsed() > MAX_TIME { + eprintln!(" Didn't enumerate in {:?}", MAX_TIME); + // Note this hoses the rest of the test suite, since the loop in + // main.rs expects tests to leave the test interface claimed. + panic!("Enumeration timed out"); + } + if context.reopen_device(None).is_ok() { + let enumeration_duration = reset_started.elapsed(); + + if let Ok(_) = context.device_for_test() { + writeln!(out, " enumerated in {:?}", enumeration_duration).expect("write failed"); + } + + break; + } + } +} + } // end tests! { fn run_bench(dev: &DeviceHandles, out: &mut String, f: impl Fn(&mut [u8])) { From 3e20a0db077f983f461b687a2527ab4504593bff Mon Sep 17 00:00:00 2001 From: Ian Rees Date: Sat, 10 Aug 2024 14:35:48 +1200 Subject: [PATCH 6/7] cargo doc warning cleanup --- src/class.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/class.rs b/src/class.rs index 4d70c8e..9052de8 100644 --- a/src/class.rs +++ b/src/class.rs @@ -8,8 +8,7 @@ use crate::{Result, UsbError}; /// A trait for implementing USB classes. /// -/// All methods are optional callbacks that will be called by -/// [UsbBus::poll](crate::bus::UsbBus::poll) +/// All methods are optional callbacks that will be called by [UsbBus::poll] pub trait UsbClass { /// Called when a GET_DESCRIPTOR request is received for a configuration descriptor. When /// called, the implementation should write its interface, endpoint and any extra class From 8de75199dd979711e24821943c566a5d119ab233 Mon Sep 17 00:00:00 2001 From: Ian Rees Date: Sat, 10 Aug 2024 14:49:54 +1200 Subject: [PATCH 7/7] Separate read_setup() from read() This change aims to resolve issues where an expected OUT Zero Length Packet (ZLP) was overwritten by a subsequent SETUP transaction. The USB spec requires that devices accept SETUP transactions, this behaviour is implemented directly in some USB 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. --- CHANGELOG.md | 1 + src/bus.rs | 26 ++++++++++++++++++++++++-- src/control_pipe.rs | 12 ++++++++++-- src/dummy.rs | 8 ++++++++ src/endpoint.rs | 37 +++++++++++++++++++++++++++++++++---- src/lib.rs | 4 ++++ 6 files changed, 80 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 28f53d4..1c819f3 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/src/bus.rs b/src/bus.rs index 5c6af03..62a987a 100644 --- a/src/bus.rs +++ b/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; - /// Reads a single packet of data from the specified endpoint and returns the actual length of - /// the packet. + /// Reads a single packet of OUT 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; + /// 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; + /// 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); diff --git a/src/control_pipe.rs b/src/control_pipe.rs index 8e2482a..dbf8b73 100644 --- a/src/control_pipe.rs +++ b/src/control_pipe.rs @@ -65,7 +65,7 @@ impl ControlPipe<'_, B> { } pub fn handle_setup(&mut self) -> Option { - 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 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; } _ => { diff --git a/src/dummy.rs b/src/dummy.rs index b8224e5..27497af 100644 --- a/src/dummy.rs +++ b/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 { + unimplemented!() + } + fn reset(&self) { unimplemented!() } diff --git a/src/endpoint.rs b/src/endpoint.rs index 2b31eec..2e8668a 100644 --- a/src/endpoint.rs +++ b/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 Endpoint<'_, B, In> { } impl 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 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 { 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 { + // 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. diff --git a/src/lib.rs b/src/lib.rs index ad009fb..5a144c8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -258,6 +258,10 @@ fn _ensure_sync() { Err(UsbError::InvalidEndpoint) } + fn read_setup(&self, _ep_addr: EndpointAddress, _buf: &mut [u8]) -> Result { + Err(UsbError::InvalidEndpoint) + } + fn set_stalled(&self, _ep_addr: EndpointAddress, _stalled: bool) {} fn is_stalled(&self, _ep_addr: EndpointAddress) -> bool { false