From a435d78cf78deb1a93682d9ff2632706eaa1b951 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Wed, 30 Mar 2022 02:01:09 +0200 Subject: [PATCH] usb: cleanup and simplify error handling. --- embassy-nrf/src/usb.rs | 18 ++---- embassy-usb-serial/src/lib.rs | 117 ++++++++++++++-------------------- embassy-usb/src/builder.rs | 6 +- embassy-usb/src/descriptor.rs | 69 ++++++++------------ embassy-usb/src/lib.rs | 12 ++-- 5 files changed, 91 insertions(+), 131 deletions(-) diff --git a/embassy-nrf/src/usb.rs b/embassy-nrf/src/usb.rs index d9524675d..1057d880c 100644 --- a/embassy-nrf/src/usb.rs +++ b/embassy-nrf/src/usb.rs @@ -403,11 +403,9 @@ unsafe fn read_dma(i: usize, buf: &mut [u8]) -> Result(i: usize, buf: &[u8]) -> Result<(), WriteError> { +unsafe fn write_dma(i: usize, buf: &[u8]) { let regs = T::regs(); - if buf.len() > 64 { - return Err(WriteError::BufferOverflow); - } + assert!(buf.len() <= 64); let mut ram_buf: MaybeUninit<[u8; 64]> = MaybeUninit::uninit(); let ptr = if !slice_in_ram(buf) { @@ -441,8 +439,6 @@ unsafe fn write_dma(i: usize, buf: &[u8]) -> Result<(), WriteError> regs.tasks_startepin[i].write(|w| w.bits(1)); while regs.events_endepin[i].read().bits() == 0 {} dma_end(); - - Ok(()) } impl<'d, T: Instance> driver::EndpointOut for Endpoint<'d, T, Out> { @@ -497,6 +493,8 @@ impl<'d, T: Instance> driver::EndpointIn for Endpoint<'d, T, In> { READY_ENDPOINTS.fetch_and(!(1 << i), Ordering::AcqRel); unsafe { write_dma::(i, buf) } + + Ok(()) } } } @@ -535,9 +533,7 @@ impl<'d, T: Instance> ControlPipe<'d, T> { async fn write(&mut self, buf: &[u8], last_chunk: bool) { let regs = T::regs(); regs.events_ep0datadone.reset(); - unsafe { - write_dma::(0, buf).unwrap(); - } + unsafe { write_dma::(0, buf) } regs.shorts .modify(|_, w| w.ep0datadone_ep0status().bit(last_chunk)); @@ -616,7 +612,7 @@ impl<'d, T: Instance> driver::ControlPipe for ControlPipe<'d, T> { fn data_out<'a>(&'a mut self, buf: &'a mut [u8]) -> Self::DataOutFuture<'a> { async move { - let req = self.request.unwrap(); + let req = unwrap!(self.request); assert!(req.direction == UsbDirection::Out); assert!(req.length > 0); @@ -649,7 +645,7 @@ impl<'d, T: Instance> driver::ControlPipe for ControlPipe<'d, T> { debug!("control in accept {:x}", buf); #[cfg(not(feature = "defmt"))] debug!("control in accept {:x?}", buf); - let req = self.request.unwrap(); + let req = unwrap!(self.request); assert!(req.direction == UsbDirection::In); let req_len = usize::from(req.length); diff --git a/embassy-usb-serial/src/lib.rs b/embassy-usb-serial/src/lib.rs index ce1fb4775..a30cdd678 100644 --- a/embassy-usb-serial/src/lib.rs +++ b/embassy-usb-serial/src/lib.rs @@ -184,78 +184,57 @@ impl<'d, D: Driver<'d>> CdcAcmClass<'d, D> { let read_ep = builder.alloc_bulk_endpoint_out(max_packet_size); let write_ep = builder.alloc_bulk_endpoint_in(max_packet_size); - builder - .config_descriptor - .iad( - comm_if, - 2, - USB_CLASS_CDC, - CDC_SUBCLASS_ACM, - CDC_PROTOCOL_NONE, - ) - .unwrap(); + builder.config_descriptor.iad( + comm_if, + 2, + USB_CLASS_CDC, + CDC_SUBCLASS_ACM, + CDC_PROTOCOL_NONE, + ); + builder.config_descriptor.interface( + comm_if, + USB_CLASS_CDC, + CDC_SUBCLASS_ACM, + CDC_PROTOCOL_NONE, + ); + builder.config_descriptor.write( + CS_INTERFACE, + &[ + CDC_TYPE_HEADER, // bDescriptorSubtype + 0x10, + 0x01, // bcdCDC (1.10) + ], + ); + builder.config_descriptor.write( + CS_INTERFACE, + &[ + CDC_TYPE_ACM, // bDescriptorSubtype + 0x00, // bmCapabilities + ], + ); + builder.config_descriptor.write( + CS_INTERFACE, + &[ + CDC_TYPE_UNION, // bDescriptorSubtype + comm_if.into(), // bControlInterface + data_if.into(), // bSubordinateInterface + ], + ); + builder.config_descriptor.write( + CS_INTERFACE, + &[ + CDC_TYPE_CALL_MANAGEMENT, // bDescriptorSubtype + 0x00, // bmCapabilities + data_if.into(), // bDataInterface + ], + ); + builder.config_descriptor.endpoint(comm_ep.info()); builder .config_descriptor - .interface(comm_if, USB_CLASS_CDC, CDC_SUBCLASS_ACM, CDC_PROTOCOL_NONE) - .unwrap(); - - builder - .config_descriptor - .write( - CS_INTERFACE, - &[ - CDC_TYPE_HEADER, // bDescriptorSubtype - 0x10, - 0x01, // bcdCDC (1.10) - ], - ) - .unwrap(); - - builder - .config_descriptor - .write( - CS_INTERFACE, - &[ - CDC_TYPE_ACM, // bDescriptorSubtype - 0x00, // bmCapabilities - ], - ) - .unwrap(); - - builder - .config_descriptor - .write( - CS_INTERFACE, - &[ - CDC_TYPE_UNION, // bDescriptorSubtype - comm_if.into(), // bControlInterface - data_if.into(), // bSubordinateInterface - ], - ) - .unwrap(); - - builder - .config_descriptor - .write( - CS_INTERFACE, - &[ - CDC_TYPE_CALL_MANAGEMENT, // bDescriptorSubtype - 0x00, // bmCapabilities - data_if.into(), // bDataInterface - ], - ) - .unwrap(); - - builder.config_descriptor.endpoint(comm_ep.info()).unwrap(); - - builder - .config_descriptor - .interface(data_if, USB_CLASS_CDC_DATA, 0x00, 0x00) - .unwrap(); - - builder.config_descriptor.endpoint(write_ep.info()).unwrap(); - builder.config_descriptor.endpoint(read_ep.info()).unwrap(); + .interface(data_if, USB_CLASS_CDC_DATA, 0x00, 0x00); + builder.config_descriptor.endpoint(write_ep.info()); + builder.config_descriptor.endpoint(read_ep.info()); CdcAcmClass { comm_ep, diff --git a/embassy-usb/src/builder.rs b/embassy-usb/src/builder.rs index 0c118b782..c8a9db7a4 100644 --- a/embassy-usb/src/builder.rs +++ b/embassy-usb/src/builder.rs @@ -168,9 +168,9 @@ impl<'d, D: Driver<'d>> UsbDeviceBuilder<'d, D> { let mut config_descriptor = DescriptorWriter::new(config_descriptor_buf); let mut bos_descriptor = BosWriter::new(DescriptorWriter::new(bos_descriptor_buf)); - device_descriptor.device(&config).unwrap(); - config_descriptor.configuration(&config).unwrap(); - bos_descriptor.bos().unwrap(); + device_descriptor.device(&config); + config_descriptor.configuration(&config); + bos_descriptor.bos(); UsbDeviceBuilder { bus, diff --git a/embassy-usb/src/descriptor.rs b/embassy-usb/src/descriptor.rs index 5f8b0d560..ff971e127 100644 --- a/embassy-usb/src/descriptor.rs +++ b/embassy-usb/src/descriptor.rs @@ -1,13 +1,6 @@ use super::builder::Config; use super::{types::*, CONFIGURATION_VALUE, DEFAULT_ALTERNATE_SETTING}; -#[derive(Debug, PartialEq, Eq, Clone, Copy)] -#[cfg_attr(feature = "defmt", derive(defmt::Format))] -pub enum Error { - BufferFull, - InvalidState, -} - /// Standard descriptor types #[allow(missing_docs)] pub mod descriptor_type { @@ -69,11 +62,11 @@ impl<'a> DescriptorWriter<'a> { } /// Writes an arbitrary (usually class-specific) descriptor. - pub fn write(&mut self, descriptor_type: u8, descriptor: &[u8]) -> Result<(), Error> { + pub fn write(&mut self, descriptor_type: u8, descriptor: &[u8]) { let length = descriptor.len(); if (self.position + 2 + length) > self.buf.len() || (length + 2) > 255 { - return Err(Error::BufferFull); + panic!("Descriptor buffer full"); } self.buf[self.position] = (length + 2) as u8; @@ -84,11 +77,9 @@ impl<'a> DescriptorWriter<'a> { self.buf[start..start + length].copy_from_slice(descriptor); self.position = start + length; - - Ok(()) } - pub(crate) fn device(&mut self, config: &Config) -> Result<(), Error> { + pub(crate) fn device(&mut self, config: &Config) { self.write( descriptor_type::DEVICE, &[ @@ -112,7 +103,7 @@ impl<'a> DescriptorWriter<'a> { ) } - pub(crate) fn configuration(&mut self, config: &Config) -> Result<(), Error> { + pub(crate) fn configuration(&mut self, config: &Config) { self.num_interfaces_mark = Some(self.position + 4); self.write_iads = config.composite_with_iads; @@ -168,9 +159,9 @@ impl<'a> DescriptorWriter<'a> { function_class: u8, function_sub_class: u8, function_protocol: u8, - ) -> Result<(), Error> { + ) { if !self.write_iads { - return Ok(()); + return; } self.write( @@ -183,9 +174,7 @@ impl<'a> DescriptorWriter<'a> { function_protocol, 0, ], - )?; - - Ok(()) + ); } /// Writes a interface descriptor. @@ -204,7 +193,7 @@ impl<'a> DescriptorWriter<'a> { interface_class: u8, interface_sub_class: u8, interface_protocol: u8, - ) -> Result<(), Error> { + ) { self.interface_alt( number, DEFAULT_ALTERNATE_SETTING, @@ -237,11 +226,13 @@ impl<'a> DescriptorWriter<'a> { interface_sub_class: u8, interface_protocol: u8, interface_string: Option, - ) -> Result<(), Error> { + ) { if alternate_setting == DEFAULT_ALTERNATE_SETTING { match self.num_interfaces_mark { Some(mark) => self.buf[mark] += 1, - None => return Err(Error::InvalidState), + None => { + panic!("you can only call `interface/interface_alt` after `configuration`.") + } }; } @@ -260,9 +251,7 @@ impl<'a> DescriptorWriter<'a> { interface_protocol, // bInterfaceProtocol str_index, // iInterface ], - )?; - - Ok(()) + ); } /// Writes an endpoint descriptor. @@ -271,10 +260,10 @@ impl<'a> DescriptorWriter<'a> { /// /// * `endpoint` - Endpoint previously allocated with /// [`UsbDeviceBuilder`](crate::bus::UsbDeviceBuilder). - pub fn endpoint(&mut self, endpoint: &EndpointInfo) -> Result<(), Error> { + pub fn endpoint(&mut self, endpoint: &EndpointInfo) { match self.num_endpoints_mark { Some(mark) => self.buf[mark] += 1, - None => return Err(Error::InvalidState), + None => panic!("you can only call `endpoint` after `interface/interface_alt`."), }; self.write( @@ -286,17 +275,15 @@ impl<'a> DescriptorWriter<'a> { (endpoint.max_packet_size >> 8) as u8, // wMaxPacketSize endpoint.interval, // bInterval ], - )?; - - Ok(()) + ); } /// Writes a string descriptor. - pub(crate) fn string(&mut self, string: &str) -> Result<(), Error> { + pub(crate) fn string(&mut self, string: &str) { let mut pos = self.position; if pos + 2 > self.buf.len() { - return Err(Error::BufferFull); + panic!("Descriptor buffer full"); } self.buf[pos] = 0; // length placeholder @@ -306,7 +293,7 @@ impl<'a> DescriptorWriter<'a> { for c in string.encode_utf16() { if pos >= self.buf.len() { - return Err(Error::BufferFull); + panic!("Descriptor buffer full"); } self.buf[pos..pos + 2].copy_from_slice(&c.to_le_bytes()); @@ -316,8 +303,6 @@ impl<'a> DescriptorWriter<'a> { self.buf[self.position] = (pos - self.position) as u8; self.position = pos; - - Ok(()) } } @@ -335,7 +320,7 @@ impl<'a> BosWriter<'a> { } } - pub(crate) fn bos(&mut self) -> Result<(), Error> { + pub(crate) fn bos(&mut self) { self.num_caps_mark = Some(self.writer.position + 4); self.writer.write( descriptor_type::BOS, @@ -343,11 +328,9 @@ impl<'a> BosWriter<'a> { 0x00, 0x00, // wTotalLength 0x00, // bNumDeviceCaps ], - )?; + ); - self.capability(capability_type::USB_2_0_EXTENSION, &[0; 4])?; - - Ok(()) + self.capability(capability_type::USB_2_0_EXTENSION, &[0; 4]); } /// Writes capability descriptor to a BOS @@ -356,17 +339,17 @@ impl<'a> BosWriter<'a> { /// /// * `capability_type` - Type of a capability /// * `data` - Binary data of the descriptor - pub fn capability(&mut self, capability_type: u8, data: &[u8]) -> Result<(), Error> { + pub fn capability(&mut self, capability_type: u8, data: &[u8]) { match self.num_caps_mark { Some(mark) => self.writer.buf[mark] += 1, - None => return Err(Error::InvalidState), + None => panic!("called `capability` not between `bos` and `end_bos`."), } let mut start = self.writer.position; let blen = data.len(); if (start + blen + 3) > self.writer.buf.len() || (blen + 3) > 255 { - return Err(Error::BufferFull); + panic!("Descriptor buffer full"); } self.writer.buf[start] = (blen + 3) as u8; @@ -376,8 +359,6 @@ impl<'a> BosWriter<'a> { start += 3; self.writer.buf[start..start + blen].copy_from_slice(data); self.writer.position = start + blen; - - Ok(()) } pub(crate) fn end_bos(&mut self) { diff --git a/embassy-usb/src/lib.rs b/embassy-usb/src/lib.rs index 2287267b6..32b67a766 100644 --- a/embassy-usb/src/lib.rs +++ b/embassy-usb/src/lib.rs @@ -158,7 +158,13 @@ impl<'d, D: Driver<'d>> UsbDevice<'d, D> { // If the request has a data state, we must read it. let data = if req.length > 0 { - let size = self.control.data_out(self.control_buf).await.unwrap(); + let size = match self.control.data_out(self.control_buf).await { + Ok(size) => size, + Err(_) => { + warn!("usb: failed to read CONTROL OUT data stage."); + return; + } + }; &self.control_buf[0..size] } else { &[] @@ -311,7 +317,6 @@ impl<'d, D: Driver<'d>> UsbDevice<'d, D> { if index == 0 { self.control_in_accept_writer(req, |w| { w.write(descriptor_type::STRING, &lang_id::ENGLISH_US.to_le_bytes()) - .unwrap(); }) .await } else { @@ -328,8 +333,7 @@ impl<'d, D: Driver<'d>> UsbDevice<'d, D> { }; if let Some(s) = s { - self.control_in_accept_writer(req, |w| w.string(s).unwrap()) - .await; + self.control_in_accept_writer(req, |w| w.string(s)).await; } else { self.control.reject() }