Merge pull request #175 from pigrew/clear_feature_ordering

Clear feature ordering

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,18 @@ 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 request codes are already discarded.
+      //                           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 &&
+           usbd_class_drivers[drv_id].control_request(rhport, p_request))
+      {
+        ret = true;
+      }
       return ret;
     }
     break;
@@ -633,7 +634,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);
   }
 }
@@ -804,7 +805,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 )
@@ -870,6 +871,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 +880,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);
 }
 

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

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;
+  uint32_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 )
+  //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)
+  if (ep_addr & 0x80)
-    pcd_set_ep_tx_status(USB,ep_addr, USB_EP_TX_STALL);
+  { // IN
+    pcd_set_ep_tx_status(USB, ep_addr & 0x7F, USB_EP_TX_STALL);
   }
-
+  else
-  if (ep_addr & 0x80) { // IN
+  { // OUT
-    ep_addr &= 0x7F;
+    pcd_set_ep_rx_status(USB, ep_addr, USB_EP_RX_STALL);
-    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);
   }
 }
 
 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);
   }
 }