From ce9f133d258fe173211063b06c87c7c5d1384986 Mon Sep 17 00:00:00 2001 From: Nathan Conrad Date: Sat, 21 Sep 2019 12:00:04 -0400 Subject: [PATCH 1/9] FSDEV: go to NAK when unstalling; on reset, set EP0 to NAK, prioritize reset interrupt, fix small typos. --- src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c | 51 +++++++++++-------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c b/src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c index 9bd4d2362..00f5b1239 100644 --- a/src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c +++ b/src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c @@ -331,7 +331,9 @@ static const tusb_desc_endpoint_t ep0IN_desc = .bEndpointAddress = 0x80 }; +#if defined(__GNUC__) && (__GNUC__ >= 7) #pragma GCC diagnostic pop +#endif static void dcd_handle_bus_reset(void) { @@ -349,7 +351,6 @@ static void dcd_handle_bus_reset(void) dcd_edpt_open (0, &ep0IN_desc); newDADDR = 0u; USB->DADDR = USB_DADDR_EF; // Set enable flag, and leaving the device address as zero. - pcd_set_ep_rx_status(USB, 0, USB_EP_RX_VALID); // And start accepting SETUP on EP0 } // FIXME: Defined to return uint16 so that ASSERT can be used, even though a return value is not needed. @@ -518,8 +519,22 @@ static uint16_t dcd_ep_ctr_handler(void) static void dcd_fs_irqHandler(void) { - uint16_t int_status = USB->ISTR; - // unused IRQs: (USB_ISTR_PMAOVR | USB_ISTR_ERR | USB_ISTR_WKUP | USB_ISTR_SUSP | USB_ISTR_ESOF | USB_ISTR_L1REQ ) + uint32_t int_status = USB->ISTR; + //const uint32_t handled_ints = USB_ISTR_CTR | USB_ISTR_RESET | USB_ISTR_WKUP + // | USB_ISTR_SUSP | USB_ISTR_SOF | USB_ISTR_ESOF; + // unused IRQs: (USB_ISTR_PMAOVR | USB_ISTR_ERR | USB_ISTR_L1REQ ) + + // The ST driver loops here on the CTR bit, but that loop has been moved into the + // dcd_ep_ctr_handler(), so less need to loop here. The other interrupts shouldn't + // be triggered repeatedly. + + if(int_status & USB_ISTR_RESET) { + // USBRST is start of reset. + reg16_clear_bits(&USB->ISTR, USB_ISTR_RESET); + dcd_handle_bus_reset(); + dcd_event_bus_signal(0, DCD_EVENT_BUS_RESET, true); + return; // Don't do the rest of the things here; perhaps they've been cleared? + } if (int_status & USB_ISTR_CTR) { @@ -528,12 +543,7 @@ static void dcd_fs_irqHandler(void) { dcd_ep_ctr_handler(); reg16_clear_bits(&USB->ISTR, USB_ISTR_CTR); } - if(int_status & USB_ISTR_RESET) { - // USBRST is start of reset. - reg16_clear_bits(&USB->ISTR, USB_ISTR_RESET); - dcd_handle_bus_reset(); - dcd_event_bus_signal(0, DCD_EVENT_BUS_RESET, true); - } + if (int_status & USB_ISTR_WKUP) { reg16_clear_bits(&USB->CNTR, USB_CNTR_LPMODE); @@ -544,6 +554,9 @@ static void dcd_fs_irqHandler(void) { if (int_status & USB_ISTR_SUSP) { + /* Suspend is asserted for both suspend and unplug events. without Vbus monitoring, + * these events cannot be differentiated, so we only trigger suspend. */ + /* Force low-power mode in the macrocell */ USB->CNTR |= USB_CNTR_FSUSP; USB->CNTR |= USB_CNTR_LPMODE; @@ -696,25 +709,19 @@ void dcd_edpt_stall (uint8_t rhport, uint8_t ep_addr) { (void)rhport; - if (ep_addr == 0) { // CTRL EP0 (OUT for setup) - pcd_set_ep_tx_status(USB,ep_addr, USB_EP_TX_STALL); + if (ep_addr & 0x80) + { // IN + pcd_set_ep_tx_status(USB, ep_addr & 0x7F, USB_EP_TX_STALL); } - - if (ep_addr & 0x80) { // IN - ep_addr &= 0x7F; - pcd_set_ep_tx_status(USB,ep_addr, USB_EP_TX_STALL); - } else { // OUT - pcd_set_ep_rx_status(USB,ep_addr, USB_EP_RX_STALL); + else + { // OUT + pcd_set_ep_rx_status(USB, ep_addr, USB_EP_RX_STALL); } } void dcd_edpt_clear_stall (uint8_t rhport, uint8_t ep_addr) { (void)rhport; - if (ep_addr == 0) - { - pcd_set_ep_tx_status(USB,ep_addr, USB_EP_TX_NAK); - } if (ep_addr & 0x80) { // IN @@ -730,7 +737,7 @@ void dcd_edpt_clear_stall (uint8_t rhport, uint8_t ep_addr) /* Reset to DATA0 if clearing stall condition. */ pcd_clear_rx_dtog(USB,ep_addr); - pcd_set_ep_rx_status(USB,ep_addr, USB_EP_RX_VALID); + pcd_set_ep_rx_status(USB,ep_addr, USB_EP_RX_NAK); } } From b6590490a8e2ad3debac4508f65af001dc1753af Mon Sep 17 00:00:00 2001 From: Nathan Conrad Date: Sat, 21 Sep 2019 12:00:36 -0400 Subject: [PATCH 2/9] USB Control: Pointer arithmetic on void* is forbiden --- src/device/usbd_control.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/device/usbd_control.c b/src/device/usbd_control.c index 4ec432185..eecc95edb 100644 --- a/src/device/usbd_control.c +++ b/src/device/usbd_control.c @@ -122,7 +122,7 @@ bool usbd_control_xfer_cb (uint8_t rhport, uint8_t ep_addr, xfer_result_t result } _control_state.total_transferred += xferred_bytes; - _control_state.buffer += xferred_bytes; + _control_state.buffer = ((uint8_t*)_control_state.buffer) + xferred_bytes; if ( _control_state.total_len == _control_state.total_transferred || xferred_bytes < CFG_TUD_ENDOINT0_SIZE ) { From aebecf169abff62b022da2c0e70f7416d62c91ef Mon Sep 17 00:00:00 2001 From: Nathan Conrad Date: Sat, 21 Sep 2019 12:02:06 -0400 Subject: [PATCH 3/9] Reorder handling of EP control requests, --- src/device/usbd.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/device/usbd.c b/src/device/usbd.c index 0366d1ece..146da5420 100644 --- a/src/device/usbd.c +++ b/src/device/usbd.c @@ -499,10 +499,6 @@ static bool process_control_request(uint8_t rhport, tusb_control_request_t const uint8_t const drv_id = _usbd_dev.ep2drv[ep_num][ep_dir]; TU_ASSERT(drv_id < USBD_CLASS_DRIVER_COUNT); - // Some classes such as TMC needs to clear/re-init its buffer when receiving CLEAR_FEATURE request - // We will forward all request targeted endpoint to its class driver - // - For non-standard request: driver can ACK or Stall the request by return true/false - // - For standard request: usbd decide the ACK stage regardless of driver return value bool ret = false; if ( TUSB_REQ_TYPE_STANDARD != p_request->bmRequestType_bit.type ) @@ -511,12 +507,6 @@ static bool process_control_request(uint8_t rhport, tusb_control_request_t const usbd_control_set_complete_callback(usbd_class_drivers[drv_id].control_complete); } - // Invoke class driver first if available - if ( usbd_class_drivers[drv_id].control_request ) - { - ret = usbd_class_drivers[drv_id].control_request(rhport, p_request); - } - // Then handle if it is standard request if ( TUSB_REQ_TYPE_STANDARD == p_request->bmRequestType_bit.type ) { @@ -552,7 +542,17 @@ static bool process_control_request(uint8_t rhport, tusb_control_request_t const default: TU_BREAKPOINT(); return false; } } + // Some classes such as TMC needs to clear/re-init its buffer when receiving CLEAR_FEATURE request + // We will forward all request targeted endpoint to its class driver + // For class-type requests: must (call tud_control_status(); return true) or (return false) + // For std-type requests: non-std requests codes are already discarded. + // must not call tud_control_status(), and return value will have no effect + // Invoke class driver last, so that EP is already stalled + if ( usbd_class_drivers[drv_id].control_request ) + { + ret = ret | usbd_class_drivers[drv_id].control_request(rhport, p_request); + } return ret; } break; From 55abb3d71763037ffcc4a55c5ecd72c1ef8e47b5 Mon Sep 17 00:00:00 2001 From: Nathan Conrad Date: Sat, 21 Sep 2019 12:02:52 -0400 Subject: [PATCH 4/9] Calling EP open with bad parameters should be considered a bug. --- src/device/usbd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/device/usbd.c b/src/device/usbd.c index 146da5420..67c5bdebc 100644 --- a/src/device/usbd.c +++ b/src/device/usbd.c @@ -804,7 +804,7 @@ bool usbd_open_edpt_pair(uint8_t rhport, uint8_t const* p_desc, uint8_t ep_count { tusb_desc_endpoint_t const * desc_ep = (tusb_desc_endpoint_t const *) p_desc; - TU_VERIFY(TUSB_DESC_ENDPOINT == desc_ep->bDescriptorType && xfer_type == desc_ep->bmAttributes.xfer); + TU_ASSERT(TUSB_DESC_ENDPOINT == desc_ep->bDescriptorType && xfer_type == desc_ep->bmAttributes.xfer); TU_ASSERT(dcd_edpt_open(rhport, desc_ep)); if ( tu_edpt_dir(desc_ep->bEndpointAddress) == TUSB_DIR_IN ) From be28a05409f80f9fe983306b1fc60e1ea30da0bc Mon Sep 17 00:00:00 2001 From: Nathan Conrad Date: Sat, 21 Sep 2019 12:03:36 -0400 Subject: [PATCH 5/9] Make type casting explicit --- src/device/usbd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/device/usbd.c b/src/device/usbd.c index 67c5bdebc..b7a44d24e 100644 --- a/src/device/usbd.c +++ b/src/device/usbd.c @@ -633,7 +633,7 @@ static void mark_interface_endpoint(uint8_t ep2drv[8][2], uint8_t const* p_desc, ep2drv[tu_edpt_number(ep_addr)][tu_edpt_dir(ep_addr)] = driver_id; } - len += tu_desc_len(p_desc); + len = (uint16_t)(len + tu_desc_len(p_desc)); p_desc = tu_desc_next(p_desc); } } From a187f0268c9c42bfbfb22338692dfffed3f9bac5 Mon Sep 17 00:00:00 2001 From: Nathan Conrad Date: Sat, 21 Sep 2019 12:04:49 -0400 Subject: [PATCH 6/9] When unstalling, EP must be marked as not busy. Also, mark EP as busy when stalling as xfer requests should fail. --- src/device/usbd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/device/usbd.c b/src/device/usbd.c index b7a44d24e..e0148dc97 100644 --- a/src/device/usbd.c +++ b/src/device/usbd.c @@ -870,6 +870,7 @@ void usbd_edpt_stall(uint8_t rhport, uint8_t ep_addr) dcd_edpt_stall(rhport, ep_addr); _usbd_dev.ep_stall_map[dir] = (uint8_t) tu_bit_set(_usbd_dev.ep_stall_map[dir], epnum); + _usbd_dev.ep_busy_map[dir] = (uint8_t) tu_bit_set(_usbd_dev.ep_busy_map[dir], epnum); } void usbd_edpt_clear_stall(uint8_t rhport, uint8_t ep_addr) @@ -878,6 +879,7 @@ void usbd_edpt_clear_stall(uint8_t rhport, uint8_t ep_addr) uint8_t const dir = tu_edpt_dir(ep_addr); dcd_edpt_clear_stall(rhport, ep_addr); + _usbd_dev.ep_busy_map[dir] = (uint8_t) tu_bit_clear(_usbd_dev.ep_busy_map[dir], epnum); _usbd_dev.ep_stall_map[dir] = (uint8_t) tu_bit_clear(_usbd_dev.ep_stall_map[dir], epnum); } From 37b52e354fe4089be04e167e2a5483ae31383274 Mon Sep 17 00:00:00 2001 From: Nathan Conrad Date: Sat, 21 Sep 2019 12:17:17 -0400 Subject: [PATCH 7/9] Correct wording of comment on handling EP requests. --- src/device/usbd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/device/usbd.c b/src/device/usbd.c index e0148dc97..6401a2d9e 100644 --- a/src/device/usbd.c +++ b/src/device/usbd.c @@ -545,9 +545,9 @@ static bool process_control_request(uint8_t rhport, tusb_control_request_t const // Some classes such as TMC needs to clear/re-init its buffer when receiving CLEAR_FEATURE request // We will forward all request targeted endpoint to its class driver // For class-type requests: must (call tud_control_status(); return true) or (return false) - // For std-type requests: non-std requests codes are already discarded. + // For std-type requests: non-std request codes are already discarded. // must not call tud_control_status(), and return value will have no effect - // Invoke class driver last, so that EP is already stalled + // class driver is invoked last, so that EP already has EP stall cleared (in event of clear feature EP halt) if ( usbd_class_drivers[drv_id].control_request ) { From fbb8520acdb7137ef695c03b05e1a3d6c4d33ba2 Mon Sep 17 00:00:00 2001 From: Nathan Conrad Date: Sat, 21 Sep 2019 14:34:29 -0400 Subject: [PATCH 8/9] logical or. --- src/device/usbd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/device/usbd.c b/src/device/usbd.c index 6401a2d9e..fb28ff0e6 100644 --- a/src/device/usbd.c +++ b/src/device/usbd.c @@ -551,7 +551,7 @@ static bool process_control_request(uint8_t rhport, tusb_control_request_t const if ( usbd_class_drivers[drv_id].control_request ) { - ret = ret | usbd_class_drivers[drv_id].control_request(rhport, p_request); + ret = ret || usbd_class_drivers[drv_id].control_request(rhport, p_request); } return ret; } From 9498adef71fe2816d336061b38ed801dc980a8bb Mon Sep 17 00:00:00 2001 From: Nathan Conrad Date: Sat, 21 Sep 2019 19:29:57 -0400 Subject: [PATCH 9/9] Changing the bitwise to a logical OR created a huge hard to find bug. It shortcutted the call to the class function, so, lets use shortcutting anyway? --- src/device/usbd.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/device/usbd.c b/src/device/usbd.c index fb28ff0e6..50964bdee 100644 --- a/src/device/usbd.c +++ b/src/device/usbd.c @@ -549,9 +549,10 @@ static bool process_control_request(uint8_t rhport, tusb_control_request_t const // must not call tud_control_status(), and return value will have no effect // class driver is invoked last, so that EP already has EP stall cleared (in event of clear feature EP halt) - if ( usbd_class_drivers[drv_id].control_request ) + if ( usbd_class_drivers[drv_id].control_request && + usbd_class_drivers[drv_id].control_request(rhport, p_request)) { - ret = ret || usbd_class_drivers[drv_id].control_request(rhport, p_request); + ret = true; } return ret; }