From ac594c15d293eca755af32bbe1748bd5b1ac16d9 Mon Sep 17 00:00:00 2001 From: Ryan Summers Date: Wed, 28 Feb 2024 11:03:57 +0100 Subject: [PATCH 1/7] Propagating results outward instead of silently ignoring --- src/control_pipe.rs | 16 ++++++++-------- src/device.rs | 41 ++++++++++++++++++++++++++--------------- 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/src/control_pipe.rs b/src/control_pipe.rs index c3d2a56..b883b1a 100644 --- a/src/control_pipe.rs +++ b/src/control_pipe.rs @@ -129,18 +129,18 @@ impl ControlPipe<'_, B> { None } - pub fn handle_out(&mut self) -> Option { + pub fn handle_out(&mut self) -> Result> { match self.state { ControlState::DataOut(req) => { let i = self.i; let count = match self.ep_out.read(&mut self.buf[i..]) { Ok(count) => count, - Err(UsbError::WouldBlock) => return None, + Err(UsbError::WouldBlock) => return Ok(None), Err(_) => { // Failed to read or buffer overflow (overflow is only possible if the host // sends more data than it indicated in the SETUP request) self.set_error(); - return None; + return Ok(None); } }; @@ -154,7 +154,7 @@ impl ControlPipe<'_, B> { if self.i >= self.len { usb_debug!("Request OUT complete: {:?}", req); self.state = ControlState::CompleteOut; - return Some(req); + return Ok(Some(req)); } } // The host may terminate a DATA stage early by sending a zero-length status packet @@ -167,7 +167,7 @@ impl ControlPipe<'_, B> { "Control transfer completed. Current state: {:?}", self.state ); - let _ = self.ep_out.read(&mut []); + self.ep_out.read(&mut [])?; self.state = ControlState::Idle; } _ => { @@ -176,14 +176,14 @@ impl ControlPipe<'_, B> { "Discarding EP0 data due to unexpected state. Current state: {:?}", self.state ); - let _ = self.ep_out.read(&mut []); + self.ep_out.read(&mut [])?; // Unexpected OUT packet self.set_error() } } - None + Ok(None) } pub fn handle_in_complete(&mut self) -> bool { @@ -262,7 +262,7 @@ impl ControlPipe<'_, B> { } }; - let _ = self.ep_in.write(&[]); + self.ep_in.write(&[])?; self.state = ControlState::StatusIn; Ok(()) } diff --git a/src/device.rs b/src/device.rs index a2f24f9..cecb31c 100644 --- a/src/device.rs +++ b/src/device.rs @@ -182,6 +182,7 @@ impl UsbDevice<'_, B> { } _ => { self.bus.resume(); + self.device_state = self .suspended_device_state .expect("Unknown state before suspend"); @@ -213,14 +214,24 @@ impl UsbDevice<'_, B> { let req = if (ep_setup & 1) != 0 { self.control.handle_setup() } else if (ep_out & 1) != 0 { - self.control.handle_out() + match self.control.handle_out() { + Ok(req) => req, + Err(_err) => { + // TODO: Propagate error out of `poll()` + usb_debug!("Failed to handle EP0: {_err}"); + None + } + } } else { None }; match req { Some(req) if req.direction == UsbDirection::In => { - self.control_in(classes, req) + if let Err(_err) = self.control_in(classes, req) { + // TODO: Propagate error out of `poll()` + usb_debug!("Failed to handle control request: {_err}"); + } } Some(req) if req.direction == UsbDirection::Out => { self.control_out(classes, req) @@ -310,14 +321,14 @@ impl UsbDevice<'_, B> { false } - fn control_in(&mut self, classes: &mut ClassList<'_, B>, req: control::Request) { + fn control_in(&mut self, classes: &mut ClassList<'_, B>, req: control::Request) -> Result<()> { use crate::control::{Recipient, Request}; for cls in classes.iter_mut() { cls.control_in(ControlIn::new(&mut self.control, &req)); if !self.control.waiting_for_response() { - return; + return Ok(()); } } @@ -334,14 +345,14 @@ impl UsbDevice<'_, B> { 0x0000 }; - let _ = xfer.accept_with(&status.to_le_bytes()); + xfer.accept_with(&status.to_le_bytes())?; } (Recipient::Interface, Request::GET_STATUS) => { usb_trace!("Processing Interface::GetStatus"); let status: u16 = 0x0000; - let _ = xfer.accept_with(&status.to_le_bytes()); + xfer.accept_with(&status.to_le_bytes())?; } (Recipient::Endpoint, Request::GET_STATUS) => { @@ -354,7 +365,7 @@ impl UsbDevice<'_, B> { 0x0000 }; - let _ = xfer.accept_with(&status.to_le_bytes()); + xfer.accept_with(&status.to_le_bytes())?; } (Recipient::Device, Request::GET_DESCRIPTOR) => { @@ -369,15 +380,14 @@ impl UsbDevice<'_, B> { _ => CONFIGURATION_NONE, }; - let _ = xfer.accept_with(&config.to_le_bytes()); + xfer.accept_with(&config.to_le_bytes())?; } (Recipient::Interface, Request::GET_INTERFACE) => { usb_trace!("Processing Interface::GetInterface"); // Reject interface numbers bigger than 255 if req.index > core::u8::MAX.into() { - let _ = xfer.reject(); - return; + return xfer.reject(); } // Ask class implementations, whether they know the alternate setting @@ -385,23 +395,24 @@ impl UsbDevice<'_, B> { for cls in classes { if let Some(setting) = cls.get_alt_setting(InterfaceNumber(req.index as u8)) { - let _ = xfer.accept_with(&setting.to_le_bytes()); - return; + return xfer.accept_with(&setting.to_le_bytes()); } } // If no class returned an alternate setting, return the default value - let _ = xfer.accept_with(&DEFAULT_ALTERNATE_SETTING.to_le_bytes()); + xfer.accept_with(&DEFAULT_ALTERNATE_SETTING.to_le_bytes())?; } - _ => (), + _ => {} }; } if self.control.waiting_for_response() { usb_debug!("Rejecting control transfer because we were waiting for a response"); - let _ = self.control.reject(); + self.control.reject()?; } + + Ok(()) } fn control_out(&mut self, classes: &mut ClassList<'_, B>, req: control::Request) { From 49ceb08d9af1df1d4971d42062493582525db91e Mon Sep 17 00:00:00 2001 From: Ryan Summers Date: Wed, 28 Feb 2024 11:10:31 +0100 Subject: [PATCH 2/7] Cleaning up further result usage --- src/control_pipe.rs | 32 ++++++++++---------------------- src/device.rs | 12 +++++++++++- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/src/control_pipe.rs b/src/control_pipe.rs index b883b1a..0a1b7d3 100644 --- a/src/control_pipe.rs +++ b/src/control_pipe.rs @@ -186,18 +186,13 @@ impl ControlPipe<'_, B> { Ok(None) } - pub fn handle_in_complete(&mut self) -> bool { + pub fn handle_in_complete(&mut self) -> Result { match self.state { ControlState::DataIn => { - self.write_in_chunk(); + self.write_in_chunk()?; } ControlState::DataInZlp => { - if self.ep_in.write(&[]).is_err() { - // There isn't much we can do if the write fails, except to wait for another - // poll or for the host to resend the request. - return false; - } - + self.ep_in.write(&[])?; usb_trace!("wrote EP0: ZLP"); self.state = ControlState::DataInLast; } @@ -207,7 +202,7 @@ impl ControlPipe<'_, B> { } ControlState::StatusIn => { self.state = ControlState::Idle; - return true; + return Ok(true); } ControlState::Idle => { // If we received a message on EP0 while sending the last portion of an IN @@ -221,23 +216,14 @@ impl ControlPipe<'_, B> { } }; - false + Ok(false) } - fn write_in_chunk(&mut self) { + fn write_in_chunk(&mut self) -> Result<()> { let count = min(self.len - self.i, self.ep_in.max_packet_size() as usize); let buffer = self.static_in_buf.unwrap_or(&self.buf); - let count = match self.ep_in.write(&buffer[self.i..(self.i + count)]) { - Ok(c) => c, - // There isn't much we can do if the write fails, except to wait for another poll or for - // the host to resend the request. - Err(_err) => { - usb_debug!("Failed to write EP0: {:?}", _err); - return; - } - }; - + let count = self.ep_in.write(&buffer[self.i..(self.i + count)])?; usb_trace!("wrote EP0: {:?}", &buffer[self.i..(self.i + count)]); self.i += count; @@ -251,6 +237,8 @@ impl ControlPipe<'_, B> { ControlState::DataInLast }; } + + Ok(()) } pub fn accept_out(&mut self) -> Result<()> { @@ -304,7 +292,7 @@ impl ControlPipe<'_, B> { self.len = min(data_len, req.length as usize); self.i = 0; self.state = ControlState::DataIn; - self.write_in_chunk(); + self.write_in_chunk()?; Ok(()) } diff --git a/src/device.rs b/src/device.rs index cecb31c..db6122b 100644 --- a/src/device.rs +++ b/src/device.rs @@ -243,7 +243,17 @@ impl UsbDevice<'_, B> { // phases of the control transfer. If we just got a SETUP packet or // an OUT token, we can safely ignore the IN-COMPLETE indication and // continue with the next transfer. - let completed = self.control.handle_in_complete(); + let completed = match self.control.handle_in_complete() { + Ok(completed) => completed, + Err(_err) => { + // TODO: Propagate this out of `poll()` + usb_debug!( + "Failed to process control-input complete: {}", + _err + ); + false + } + }; if !B::QUIRK_SET_ADDRESS_BEFORE_STATUS && completed From 8318f0cea51a3fa206d5eb76a63f4030a66e5d5e Mon Sep 17 00:00:00 2001 From: Ryan Summers Date: Wed, 28 Feb 2024 11:16:46 +0100 Subject: [PATCH 3/7] Fixing build --- src/control_pipe.rs | 3 ++- src/device.rs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/control_pipe.rs b/src/control_pipe.rs index 0a1b7d3..f72cab6 100644 --- a/src/control_pipe.rs +++ b/src/control_pipe.rs @@ -136,10 +136,11 @@ impl ControlPipe<'_, B> { let count = match self.ep_out.read(&mut self.buf[i..]) { Ok(count) => count, Err(UsbError::WouldBlock) => return Ok(None), - Err(_) => { + Err(_err) => { // Failed to read or buffer overflow (overflow is only possible if the host // sends more data than it indicated in the SETUP request) self.set_error(); + usb_debug!("Failed EP0 read: {:?}", _err); return Ok(None); } }; diff --git a/src/device.rs b/src/device.rs index db6122b..9bff183 100644 --- a/src/device.rs +++ b/src/device.rs @@ -218,7 +218,7 @@ impl UsbDevice<'_, B> { Ok(req) => req, Err(_err) => { // TODO: Propagate error out of `poll()` - usb_debug!("Failed to handle EP0: {_err}"); + usb_debug!("Failed to handle EP0: {}", _err); None } } @@ -230,7 +230,7 @@ impl UsbDevice<'_, B> { Some(req) if req.direction == UsbDirection::In => { if let Err(_err) = self.control_in(classes, req) { // TODO: Propagate error out of `poll()` - usb_debug!("Failed to handle control request: {_err}"); + usb_debug!("Failed to handle control request: {}", _err); } } Some(req) if req.direction == UsbDirection::Out => { From 5fe6f6ad00ad7d690bcbb1611bdae9a2e693d4d1 Mon Sep 17 00:00:00 2001 From: Ryan Summers Date: Wed, 28 Feb 2024 11:18:55 +0100 Subject: [PATCH 4/7] Fixing log order --- src/control_pipe.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/control_pipe.rs b/src/control_pipe.rs index f72cab6..1dd66ca 100644 --- a/src/control_pipe.rs +++ b/src/control_pipe.rs @@ -139,8 +139,8 @@ impl ControlPipe<'_, B> { Err(_err) => { // Failed to read or buffer overflow (overflow is only possible if the host // sends more data than it indicated in the SETUP request) - self.set_error(); usb_debug!("Failed EP0 read: {:?}", _err); + self.set_error(); return Ok(None); } }; From deec99cb691a1cdd0bf9fc5405301452b0964f96 Mon Sep 17 00:00:00 2001 From: Ryan Summers Date: Wed, 28 Feb 2024 11:20:34 +0100 Subject: [PATCH 5/7] Fixing log format --- src/device.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/device.rs b/src/device.rs index 9bff183..3ac1eb1 100644 --- a/src/device.rs +++ b/src/device.rs @@ -218,7 +218,7 @@ impl UsbDevice<'_, B> { Ok(req) => req, Err(_err) => { // TODO: Propagate error out of `poll()` - usb_debug!("Failed to handle EP0: {}", _err); + usb_debug!("Failed to handle EP0: {:?}", _err); None } } @@ -230,7 +230,7 @@ impl UsbDevice<'_, B> { Some(req) if req.direction == UsbDirection::In => { if let Err(_err) = self.control_in(classes, req) { // TODO: Propagate error out of `poll()` - usb_debug!("Failed to handle control request: {}", _err); + usb_debug!("Failed to handle control request: {:?}", _err); } } Some(req) if req.direction == UsbDirection::Out => { @@ -248,7 +248,7 @@ impl UsbDevice<'_, B> { Err(_err) => { // TODO: Propagate this out of `poll()` usb_debug!( - "Failed to process control-input complete: {}", + "Failed to process control-input complete: {:?}", _err ); false From c82290cabc1b05780a807240d62b7032a321f032 Mon Sep 17 00:00:00 2001 From: Ryan Summers Date: Wed, 28 Feb 2024 11:25:37 +0100 Subject: [PATCH 6/7] Using proper log macro --- src/macros.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/macros.rs b/src/macros.rs index e6edd32..ff96a6d 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -1,7 +1,7 @@ #[cfg(all(feature = "log", not(feature = "defmt")))] macro_rules! usb_log { (trace, $($arg:expr),*) => { log::trace!($($arg),*) }; - (debug, $($arg:expr),*) => { log::trace!($($arg),*) }; + (debug, $($arg:expr),*) => { log::debug!($($arg),*) }; } #[cfg(feature = "defmt")] From beb6b9ee93c8885692d4a04bd2dc22a743242ac4 Mon Sep 17 00:00:00 2001 From: Ryan Summers Date: Wed, 28 Feb 2024 12:06:02 +0100 Subject: [PATCH 7/7] Cleaning up more result propagation --- src/device.rs | 85 +++++++++++++++++++++++++++++---------------------- 1 file changed, 49 insertions(+), 36 deletions(-) diff --git a/src/device.rs b/src/device.rs index 3ac1eb1..e5b49df 100644 --- a/src/device.rs +++ b/src/device.rs @@ -230,11 +230,14 @@ impl UsbDevice<'_, B> { Some(req) if req.direction == UsbDirection::In => { if let Err(_err) = self.control_in(classes, req) { // TODO: Propagate error out of `poll()` - usb_debug!("Failed to handle control request: {:?}", _err); + usb_debug!("Failed to handle input control request: {:?}", _err); } } Some(req) if req.direction == UsbDirection::Out => { - self.control_out(classes, req) + if let Err(_err) = self.control_out(classes, req) { + // TODO: Propagate error out of `poll()` + usb_debug!("Failed to handle output control request: {:?}", _err); + } } None if ((ep_in_complete & 1) != 0) => { @@ -380,7 +383,7 @@ impl UsbDevice<'_, B> { (Recipient::Device, Request::GET_DESCRIPTOR) => { usb_trace!("Processing Device::GetDescriptor"); - UsbDevice::get_descriptor(&self.config, classes, xfer) + UsbDevice::get_descriptor(&self.config, classes, xfer)?; } (Recipient::Device, Request::GET_CONFIGURATION) => { @@ -425,14 +428,14 @@ impl UsbDevice<'_, B> { Ok(()) } - fn control_out(&mut self, classes: &mut ClassList<'_, B>, req: control::Request) { + fn control_out(&mut self, classes: &mut ClassList<'_, B>, req: control::Request) -> Result<()> { use crate::control::{Recipient, Request}; for cls in classes.iter_mut() { cls.control_out(ControlOut::new(&mut self.control, &req)); if !self.control.waiting_for_response() { - return; + return Ok(()); } } @@ -451,14 +454,14 @@ impl UsbDevice<'_, B> { ) => { usb_debug!("Remote wakeup disabled"); self.remote_wakeup_enabled = false; - let _ = xfer.accept(); + xfer.accept()?; } (Recipient::Endpoint, Request::CLEAR_FEATURE, Request::FEATURE_ENDPOINT_HALT) => { usb_debug!("EP{} halt removed", req.index & 0x8f); self.bus .set_stalled(((req.index as u8) & 0x8f).into(), false); - let _ = xfer.accept(); + xfer.accept()?; } ( @@ -468,14 +471,14 @@ impl UsbDevice<'_, B> { ) => { usb_debug!("Remote wakeup enabled"); self.remote_wakeup_enabled = true; - let _ = xfer.accept(); + xfer.accept()?; } (Recipient::Endpoint, Request::SET_FEATURE, Request::FEATURE_ENDPOINT_HALT) => { usb_debug!("EP{} halted", req.index & 0x8f); self.bus .set_stalled(((req.index as u8) & 0x8f).into(), true); - let _ = xfer.accept(); + xfer.accept()?; } (Recipient::Device, Request::SET_ADDRESS, 1..=127) => { @@ -486,24 +489,24 @@ impl UsbDevice<'_, B> { } else { self.pending_address = req.value as u8; } - let _ = xfer.accept(); + xfer.accept()?; } (Recipient::Device, Request::SET_CONFIGURATION, CONFIGURATION_VALUE_U16) => { usb_debug!("Device configured"); self.device_state = UsbDeviceState::Configured; - let _ = xfer.accept(); + xfer.accept()?; } (Recipient::Device, Request::SET_CONFIGURATION, CONFIGURATION_NONE_U16) => { usb_debug!("Device deconfigured"); match self.device_state { UsbDeviceState::Default => { - let _ = xfer.accept(); + xfer.accept()?; } _ => { self.device_state = UsbDeviceState::Addressed; - let _ = xfer.accept(); + xfer.accept()?; } } } @@ -511,43 +514,49 @@ impl UsbDevice<'_, B> { (Recipient::Interface, Request::SET_INTERFACE, alt_setting) => { // Reject interface numbers and alt settings bigger than 255 if req.index > core::u8::MAX.into() || alt_setting > core::u8::MAX.into() { - let _ = xfer.reject(); - return; + xfer.reject()?; + return Ok(()); } // Ask class implementations, whether they accept the alternate interface setting. for cls in classes { if cls.set_alt_setting(InterfaceNumber(req.index as u8), alt_setting as u8) { - let _ = xfer.accept(); - return; + xfer.accept()?; + return Ok(()); } } // Default behaviour, if no class implementation accepted the alternate setting. if alt_setting == DEFAULT_ALTERNATE_SETTING_U16 { usb_debug!("Accepting unused alternate settings"); - let _ = xfer.accept(); + xfer.accept()?; } else { usb_debug!("Rejecting unused alternate settings"); - let _ = xfer.reject(); + xfer.reject()?; } } _ => { - let _ = xfer.reject(); - return; + xfer.reject()?; + return Ok(()); } } } if self.control.waiting_for_response() { usb_debug!("Rejecting control transfer due to waiting response"); - let _ = self.control.reject(); + self.control.reject()?; } + + Ok(()) } - fn get_descriptor(config: &Config, classes: &mut ClassList<'_, B>, xfer: ControlIn) { + fn get_descriptor( + config: &Config, + classes: &mut ClassList<'_, B>, + xfer: ControlIn, + ) -> Result<()> { let req = *xfer.request(); let (dtype, index) = req.descriptor_type_index(); @@ -555,12 +564,14 @@ impl UsbDevice<'_, B> { fn accept_writer( xfer: ControlIn, f: impl FnOnce(&mut DescriptorWriter) -> Result<()>, - ) { - let _ = xfer.accept(|buf| { + ) -> Result<()> { + xfer.accept(|buf| { let mut writer = DescriptorWriter::new(buf); f(&mut writer)?; Ok(writer.position()) - }); + })?; + + Ok(()) } match dtype { @@ -575,9 +586,9 @@ impl UsbDevice<'_, B> { bw.end_bos(); Ok(()) - }), + })?, - descriptor_type::DEVICE => accept_writer(xfer, |w| w.device(config)), + descriptor_type::DEVICE => accept_writer(xfer, |w| w.device(config))?, descriptor_type::CONFIGURATION => accept_writer(xfer, |w| { w.configuration(config)?; @@ -590,7 +601,7 @@ impl UsbDevice<'_, B> { w.end_configuration(); Ok(()) - }), + })?, descriptor_type::STRING => match index { // first STRING Request @@ -608,7 +619,7 @@ impl UsbDevice<'_, B> { descriptor_type::STRING, &lang_id_bytes[..config.string_descriptors.len() * 2], ) - }) + })?; } // rest STRING Requests @@ -623,8 +634,8 @@ impl UsbDevice<'_, B> { .iter() .find(|lang| lang.id == lang_id) else { - xfer.reject().ok(); - return; + xfer.reject()?; + return Ok(()); }; match index { @@ -643,17 +654,19 @@ impl UsbDevice<'_, B> { }; if let Some(string_descriptor) = string { - accept_writer(xfer, |w| w.string(string_descriptor)); + accept_writer(xfer, |w| w.string(string_descriptor))?; } else { - let _ = xfer.reject(); + xfer.reject()?; } } }, _ => { - let _ = xfer.reject(); + xfer.reject()?; } - } + }; + + Ok(()) } fn reset(&mut self, classes: &mut ClassList<'_, B>) {