usbh always retry control transfer (max 3) in case of XFER_RESULT_FAILED.

added tuh_connected()

6 files changed, 125 insertions, 109 deletions

diff --git a/src/common/tusb_types.h b/src/common/tusb_types.h
index 9e4d9380c..42ce5fc12 100644
--- a/src/common/tusb_types.h
+++ b/src/common/tusb_types.h
@@ -302,9 +302,9 @@ typedef enum {
 
 enum {
   CONTROL_STAGE_IDLE = 0,
-  CONTROL_STAGE_SETUP,
-  CONTROL_STAGE_DATA,
-  CONTROL_STAGE_ACK
+  CONTROL_STAGE_SETUP, // 1
+  CONTROL_STAGE_DATA,  // 2
+  CONTROL_STAGE_ACK    // 3
 };
 
 enum {
diff --git a/src/host/hub.c b/src/host/hub.c
index 611bf71be..1d3e9ae1d 100644
--- a/src/host/hub.c
+++ b/src/host/hub.c
@@ -120,12 +120,9 @@ bool hub_port_clear_feature(uint8_t hub_addr, uint8_t hub_port, uint8_t feature,
 }
 
 bool hub_port_set_feature(uint8_t hub_addr, uint8_t hub_port, uint8_t feature,
-                          tuh_xfer_cb_t complete_cb, uintptr_t user_data)
-{
-  tusb_control_request_t const request =
-  {
-    .bmRequestType_bit =
-    {
+                          tuh_xfer_cb_t complete_cb, uintptr_t user_data) {
+  tusb_control_request_t const request = {
+    .bmRequestType_bit = {
       .recipient = (hub_port == 0) ? TUSB_REQ_RCPT_DEVICE : TUSB_REQ_RCPT_OTHER,
       .type      = TUSB_REQ_TYPE_CLASS,
       .direction = TUSB_DIR_OUT
@@ -136,8 +133,7 @@ bool hub_port_set_feature(uint8_t hub_addr, uint8_t hub_port, uint8_t feature,
     .wLength  = 0
   };
 
-  tuh_xfer_t xfer =
-  {
+  tuh_xfer_t xfer = {
     .daddr       = hub_addr,
     .ep_addr     = 0,
     .setup       = &request,
@@ -152,12 +148,9 @@ bool hub_port_set_feature(uint8_t hub_addr, uint8_t hub_port, uint8_t feature,
 }
 
 bool hub_port_get_status(uint8_t hub_addr, uint8_t hub_port, void* resp,
-                         tuh_xfer_cb_t complete_cb, uintptr_t user_data)
-{
-  tusb_control_request_t const request =
-  {
-    .bmRequestType_bit =
-    {
+                         tuh_xfer_cb_t complete_cb, uintptr_t user_data) {
+  tusb_control_request_t const request = {
+    .bmRequestType_bit = {
       .recipient = (hub_port == 0) ? TUSB_REQ_RCPT_DEVICE : TUSB_REQ_RCPT_OTHER,
       .type      = TUSB_REQ_TYPE_CLASS,
       .direction = TUSB_DIR_IN
@@ -168,8 +161,7 @@ bool hub_port_get_status(uint8_t hub_addr, uint8_t hub_port, void* resp,
     .wLength  = 4
   };
 
-  tuh_xfer_t xfer =
-  {
+  tuh_xfer_t xfer = {
     .daddr       = hub_addr,
     .ep_addr     = 0,
     .setup       = &request,
@@ -228,9 +220,9 @@ void hub_close(uint8_t dev_addr) {
   }
 }
 
-bool hub_edpt_status_xfer(uint8_t dev_addr) {
-  hub_interface_t* hub_itf = get_hub_itf(dev_addr);
-  return usbh_edpt_xfer(dev_addr, hub_itf->ep_in, &hub_itf->status_change, 1);
+bool hub_edpt_status_xfer(uint8_t daddr) {
+  hub_interface_t* hub_itf = get_hub_itf(daddr);
+  return usbh_edpt_xfer(daddr, hub_itf->ep_in, &hub_itf->status_change, 1);
 }
 
 //--------------------------------------------------------------------+
@@ -292,15 +284,15 @@ static void config_port_power_complete (tuh_xfer_t* xfer) {
   uint8_t const daddr = xfer->daddr;
   hub_interface_t* p_hub = get_hub_itf(daddr);
 
-  if (xfer->setup->wIndex == p_hub->bNbrPorts)
-  {
+  if (xfer->setup->wIndex == p_hub->bNbrPorts) {
     // All ports are power -> queue notification status endpoint and
     // complete the SET CONFIGURATION
-    TU_ASSERT( usbh_edpt_xfer(daddr, p_hub->ep_in, &p_hub->status_change, 1), );
-
+    if (!hub_edpt_status_xfer(daddr)) {
+      TU_MESS_FAILED();
+      TU_BREAKPOINT();
+    }
     usbh_driver_set_config_complete(daddr, p_hub->itf_num);
-  }else
-  {
+  } else {
     // power next port
     uint8_t const hub_port = (uint8_t) (xfer->setup->wIndex + 1);
     hub_port_set_feature(daddr, hub_port, HUB_FEATURE_PORT_POWER, config_port_power_complete, 0);
@@ -310,7 +302,6 @@ static void config_port_power_complete (tuh_xfer_t* xfer) {
 //--------------------------------------------------------------------+
 // Connection Changes
 //--------------------------------------------------------------------+
-
 static void hub_port_get_status_complete (tuh_xfer_t* xfer);
 static void hub_get_status_complete (tuh_xfer_t* xfer);
 static void connection_clear_conn_change_complete (tuh_xfer_t* xfer);
@@ -389,46 +380,35 @@ static void hub_get_status_complete (tuh_xfer_t* xfer)
   }
 }
 
-static void hub_port_get_status_complete (tuh_xfer_t* xfer)
-{
+static void hub_port_get_status_complete(tuh_xfer_t *xfer) {
   TU_ASSERT(xfer->result == XFER_RESULT_SUCCESS, );
 
   uint8_t const daddr = xfer->daddr;
-  hub_interface_t* p_hub = get_hub_itf(daddr);
+  hub_interface_t *p_hub = get_hub_itf(daddr);
   uint8_t const port_num = (uint8_t) tu_le16toh(xfer->setup->wIndex);
 
   // Connection change
-  if (p_hub->port_status.change.connection)
-  {
+  if (p_hub->port_status.change.connection) {
     // Port is powered and enabled
     //TU_VERIFY(port_status.status_current.port_power && port_status.status_current.port_enable, );
 
     // Acknowledge Port Connection Change
     hub_port_clear_feature(daddr, port_num, HUB_FEATURE_PORT_CONNECTION_CHANGE, connection_clear_conn_change_complete, 0);
-  }else
-  {
+  } else {
     // Clear other port status change interrupts. TODO Not currently handled - just cleared.
-    if (p_hub->port_status.change.port_enable)
-    {
+    if (p_hub->port_status.change.port_enable) {
       hub_port_clear_feature(daddr, port_num, HUB_FEATURE_PORT_ENABLE_CHANGE, hub_clear_feature_complete_stub, 0);
-    }
-    else if (p_hub->port_status.change.suspend)
-    {
+    } else if (p_hub->port_status.change.suspend) {
       hub_port_clear_feature(daddr, port_num, HUB_FEATURE_PORT_SUSPEND_CHANGE, hub_clear_feature_complete_stub, 0);
-    }
-    else if (p_hub->port_status.change.over_current)
-    {
+    } else if (p_hub->port_status.change.over_current) {
       hub_port_clear_feature(daddr, port_num, HUB_FEATURE_PORT_OVER_CURRENT_CHANGE, hub_clear_feature_complete_stub, 0);
-    }
-    else if (p_hub->port_status.change.reset)
-    {
+    } else if (p_hub->port_status.change.reset) {
       hub_port_clear_feature(daddr, port_num, HUB_FEATURE_PORT_RESET_CHANGE, hub_clear_feature_complete_stub, 0);
     }
-    // Other changes are: L1 state
-    // TODO clear change
+    else {
+      // Other changes are: L1 state
+      // TODO clear change
 
-    else
-    {
       // prepare for next hub status
       // TODO continue with status_change, or maybe we can do it again with status
       hub_edpt_status_xfer(daddr);
diff --git a/src/host/hub.h b/src/host/hub.h
index 59097fb0a..3b19e9838 100644
--- a/src/host/hub.h
+++ b/src/host/hub.h
@@ -184,7 +184,7 @@ bool hub_port_get_status(uint8_t hub_addr, uint8_t hub_port, void *resp,
                          tuh_xfer_cb_t complete_cb, uintptr_t user_data);
 
 // Get status from Interrupt endpoint
-bool hub_edpt_status_xfer(uint8_t dev_addr);
+bool hub_edpt_status_xfer(uint8_t daddr);
 
 // Reset a port
 TU_ATTR_ALWAYS_INLINE static inline
diff --git a/src/host/usbh.c b/src/host/usbh.c
index 5f4f17167..ac920f533 100644
--- a/src/host/usbh.c
+++ b/src/host/usbh.c
@@ -44,6 +44,10 @@
   #define CFG_TUH_INTERFACE_MAX   8
 #endif
 
+enum {
+  USBH_CONTROL_RETRY_MAX = 3,
+};
+
 //--------------------------------------------------------------------+
 // Weak stubs: invoked if no strong implementation is available
 //--------------------------------------------------------------------+
@@ -272,9 +276,10 @@ static struct {
   tuh_xfer_cb_t complete_cb;
   uintptr_t user_data;
 
-  uint8_t daddr;
   volatile uint8_t stage;
+  uint8_t daddr;
   volatile uint16_t actual_len;
+  uint8_t failed_count;
 } _ctrl_xfer;
 
 typedef struct {
@@ -285,7 +290,6 @@ typedef struct {
 CFG_TUH_MEM_SECTION static usbh_epbuf_t _usbh_epbuf;
 
 //------------- Helper Function -------------//
-
 TU_ATTR_ALWAYS_INLINE static inline usbh_device_t* get_device(uint8_t dev_addr) {
   TU_VERIFY(dev_addr > 0 && dev_addr <= TOTAL_DEVICES, NULL);
   return &_usbh_devices[dev_addr-1];
@@ -312,6 +316,15 @@ bool tuh_mounted(uint8_t dev_addr) {
   return dev->configured;
 }
 
+bool tuh_connected(uint8_t daddr) {
+  if (daddr == 0) {
+    return _dev0.enumerating; // dev0 is connected if still enumerating
+  } else {
+    const usbh_device_t* dev = get_device(daddr);
+    return dev && dev->connected;
+  }
+}
+
 bool tuh_vid_pid_get(uint8_t dev_addr, uint16_t *vid, uint16_t *pid) {
   *vid = *pid = 0;
 
@@ -421,7 +434,9 @@ bool tuh_rhport_init(uint8_t rhport, const tusb_rhport_init_t* rh_init) {
 }
 
 bool tuh_deinit(uint8_t rhport) {
-  if (!tuh_rhport_is_active(rhport)) return true;
+  if (!tuh_rhport_is_active(rhport)) {
+    return true;
+  }
 
   // deinit host controller
   hcd_int_disable(rhport);
@@ -482,7 +497,9 @@ void tuh_task_ext(uint32_t timeout_ms, bool in_isr) {
   (void) in_isr; // not implemented yet
 
   // Skip if stack is not initialized
-  if (!tuh_inited()) return;
+  if (!tuh_inited()) {
+    return;
+  }
 
   // Loop until there is no more events in the queue
   while (1) {
@@ -616,17 +633,9 @@ static void _control_blocking_complete_cb(tuh_xfer_t* xfer) {
 
 // TODO timeout_ms is not supported yet
 bool tuh_control_xfer (tuh_xfer_t* xfer) {
-  // EP0 with setup packet
-  TU_VERIFY(xfer->ep_addr == 0 && xfer->setup);
-
-  // Check if device is still connected (enumerating for dev0)
+  TU_VERIFY(xfer->ep_addr == 0 && xfer->setup); // EP0 with setup packet
   const uint8_t daddr = xfer->daddr;
-  if (daddr == 0) {
-    TU_VERIFY(_dev0.enumerating);
-  } else {
-    const usbh_device_t* dev = get_device(daddr);
-    TU_VERIFY(dev && dev->connected);
-  }
+  TU_VERIFY(tuh_connected(daddr)); // Check if device is still connected (enumerating for dev0)
 
   // pre-check to help reducing mutex lock
   TU_VERIFY(_ctrl_xfer.stage == CONTROL_STAGE_IDLE);
@@ -634,14 +643,15 @@ bool tuh_control_xfer (tuh_xfer_t* xfer) {
 
   bool const is_idle = (_ctrl_xfer.stage == CONTROL_STAGE_IDLE);
   if (is_idle) {
-    _ctrl_xfer.stage       = CONTROL_STAGE_SETUP;
-    _ctrl_xfer.daddr       = daddr;
-    _ctrl_xfer.actual_len  = 0;
+    _ctrl_xfer.stage        = CONTROL_STAGE_SETUP;
+    _ctrl_xfer.daddr        = daddr;
+    _ctrl_xfer.actual_len   = 0;
+    _ctrl_xfer.failed_count = 0;
 
-    _ctrl_xfer.buffer      = xfer->buffer;
-    _ctrl_xfer.complete_cb = xfer->complete_cb;
-    _ctrl_xfer.user_data   = xfer->user_data;
-    _usbh_epbuf.request    = (*xfer->setup);
+    _ctrl_xfer.buffer       = xfer->buffer;
+    _ctrl_xfer.complete_cb  = xfer->complete_cb;
+    _ctrl_xfer.user_data    = xfer->user_data;
+    _usbh_epbuf.request     = (*xfer->setup);
   }
 
   (void) osal_mutex_unlock(_usbh_mutex);
@@ -655,7 +665,7 @@ bool tuh_control_xfer (tuh_xfer_t* xfer) {
   TU_LOG_BUF_USBH(xfer->setup, 8);
 
   if (xfer->complete_cb) {
-    TU_ASSERT( hcd_setup_send(rhport, daddr, (uint8_t const*) &_usbh_epbuf.request) );
+    TU_ASSERT(hcd_setup_send(rhport, daddr, (uint8_t const *) &_usbh_epbuf.request));
   }else {
     // blocking if complete callback is not provided
     // change callback to internal blocking, and result as user argument
@@ -665,7 +675,7 @@ bool tuh_control_xfer (tuh_xfer_t* xfer) {
     _ctrl_xfer.user_data   = (uintptr_t) &result;
     _ctrl_xfer.complete_cb = _control_blocking_complete_cb;
 
-    TU_ASSERT( hcd_setup_send(rhport, daddr, (uint8_t*) &_usbh_epbuf.request) );
+    TU_ASSERT(hcd_setup_send(rhport, daddr, (uint8_t *) &_usbh_epbuf.request));
 
     while (result == XFER_RESULT_INVALID) {
       // Note: this can be called within an callback ie. part of tuh_task()
@@ -722,28 +732,46 @@ static bool usbh_control_xfer_cb (uint8_t daddr, uint8_t ep_addr, xfer_result_t
   const uint8_t rhport = usbh_get_rhport(daddr);
   tusb_control_request_t const * request = &_usbh_epbuf.request;
 
-  if (XFER_RESULT_SUCCESS != result) {
-    TU_LOG_USBH("[%u:%u] Control %s, xferred_bytes = %" PRIu32 "\r\n", rhport, daddr, result == XFER_RESULT_STALLED ? "STALLED" : "FAILED", xferred_bytes);
-    TU_LOG_BUF_USBH(request, 8);
+  switch (result) {
+    case XFER_RESULT_STALLED:
+      TU_LOG_USBH("[%u:%u] Control STALLED, xferred_bytes = %" PRIu32 "\r\n", rhport, daddr, xferred_bytes);
+      TU_LOG_BUF_USBH(request, 8);
+      _control_xfer_complete(daddr, result);
+    break;
 
-    // terminate transfer if any stage failed
-    _control_xfer_complete(daddr, result);
-  }else {
-    switch(_ctrl_xfer.stage) {
-      case CONTROL_STAGE_SETUP:
-        if (request->wLength) {
-          // DATA stage: initial data toggle is always 1
-          _set_control_xfer_stage(CONTROL_STAGE_DATA);
-          TU_ASSERT( hcd_edpt_xfer(rhport, daddr, tu_edpt_addr(0, request->bmRequestType_bit.direction), _ctrl_xfer.buffer, request->wLength) );
-          return true;
-        }
+    case XFER_RESULT_FAILED:
+      if (tuh_connected(daddr) && _ctrl_xfer.failed_count < USBH_CONTROL_RETRY_MAX) {
+        TU_LOG_USBH("[%u:%u] Control FAILED %u/%u, retrying\r\n", rhport, daddr, _ctrl_xfer.failed_count+1, USBH_CONTROL_RETRY_MAX);
+        (void) osal_mutex_lock(_usbh_mutex, OSAL_TIMEOUT_WAIT_FOREVER);
+        _ctrl_xfer.stage = CONTROL_STAGE_SETUP;
+        _ctrl_xfer.failed_count++;
+        _ctrl_xfer.actual_len = 0; // reset actual_len
+        (void) osal_mutex_unlock(_usbh_mutex);
+
+        TU_ASSERT(hcd_setup_send(rhport, daddr, (uint8_t const *) request));
+      } else {
+        TU_LOG_USBH("[%u:%u] Control FAILED, xferred_bytes = %" PRIu32 "\r\n", rhport, daddr, xferred_bytes);
+        TU_LOG_BUF_USBH(request, 8);
+        _control_xfer_complete(daddr, result);
+      }
+    break;
+
+    case XFER_RESULT_SUCCESS:
+      switch(_ctrl_xfer.stage) {
+        case CONTROL_STAGE_SETUP:
+          if (request->wLength) {
+            // DATA stage: initial data toggle is always 1
+            _set_control_xfer_stage(CONTROL_STAGE_DATA);
+            TU_ASSERT( hcd_edpt_xfer(rhport, daddr, tu_edpt_addr(0, request->bmRequestType_bit.direction), _ctrl_xfer.buffer, request->wLength) );
+            return true;
+          }
         TU_ATTR_FALLTHROUGH;
 
-      case CONTROL_STAGE_DATA:
-        if (request->wLength) {
-          TU_LOG_USBH("[%u:%u] Control data:\r\n", rhport, daddr);
-          TU_LOG_MEM_USBH(_ctrl_xfer.buffer, xferred_bytes, 2);
-        }
+        case CONTROL_STAGE_DATA:
+          if (request->wLength) {
+            TU_LOG_USBH("[%u:%u] Control data:\r\n", rhport, daddr);
+            TU_LOG_MEM_USBH(_ctrl_xfer.buffer, xferred_bytes, 2);
+          }
 
         _ctrl_xfer.actual_len = (uint16_t) xferred_bytes;
 
@@ -752,23 +780,26 @@ static bool usbh_control_xfer_cb (uint8_t daddr, uint8_t ep_addr, xfer_result_t
         TU_ASSERT( hcd_edpt_xfer(rhport, daddr, tu_edpt_addr(0, 1 - request->bmRequestType_bit.direction), NULL, 0) );
         break;
 
-      case CONTROL_STAGE_ACK: {
-        // Abort all pending transfers if SET_CONFIGURATION request
-        // NOTE: should we force closing all non-control endpoints in the future?
-        if (request->bRequest == TUSB_REQ_SET_CONFIGURATION && request->bmRequestType == 0x00) {
-          for(uint8_t epnum=1; epnum<CFG_TUH_ENDPOINT_MAX; epnum++) {
-            for(uint8_t dir=0; dir<2; dir++) {
-              tuh_edpt_abort_xfer(daddr, tu_edpt_addr(epnum, dir));
+        case CONTROL_STAGE_ACK: {
+          // Abort all pending transfers if SET_CONFIGURATION request
+          // NOTE: should we force closing all non-control endpoints in the future?
+          if (request->bRequest == TUSB_REQ_SET_CONFIGURATION && request->bmRequestType == 0x00) {
+            for(uint8_t epnum=1; epnum<CFG_TUH_ENDPOINT_MAX; epnum++) {
+              for(uint8_t dir=0; dir<2; dir++) {
+                tuh_edpt_abort_xfer(daddr, tu_edpt_addr(epnum, dir));
+              }
             }
           }
+
+          _control_xfer_complete(daddr, result);
+          break;
         }
 
-        _control_xfer_complete(daddr, result);
-        break;
+        default: return false; // unsupported stage
       }
+      break;
 
-      default: return false;
-    }
+    default: return false; // unsupported result
   }
 
   return true;
@@ -1769,8 +1800,9 @@ static void enum_full_complete(void) {
   _dev0.enumerating = 0;
 
 #if CFG_TUH_HUB
-  // get next hub status
-  if (_dev0.hub_addr) hub_edpt_status_xfer(_dev0.hub_addr);
+  if (_dev0.hub_addr) {
+    hub_edpt_status_xfer(_dev0.hub_addr); // get next hub status
+  }
 #endif
 
 }
diff --git a/src/host/usbh.h b/src/host/usbh.h
index 72c237573..c8b137173 100644
--- a/src/host/usbh.h
+++ b/src/host/usbh.h
@@ -183,6 +183,10 @@ tusb_speed_t tuh_speed_get(uint8_t daddr);
 // Check if device is connected and configured
 bool tuh_mounted(uint8_t daddr);
 
+// Check if device is connected which mean device has at least 1 successful transfer
+// Note: It may not be addressed/configured/mounted yet
+bool tuh_connected(uint8_t daddr);
+
 // Check if device is suspended
 TU_ATTR_ALWAYS_INLINE static inline
 bool tuh_suspended(uint8_t daddr) {
diff --git a/src/host/usbh_pvt.h b/src/host/usbh_pvt.h
index 95de915e9..d27c4baa2 100644
--- a/src/host/usbh_pvt.h
+++ b/src/host/usbh_pvt.h
@@ -83,8 +83,8 @@ void usbh_defer_func(osal_task_func_t func, void *param, bool in_isr);
 bool usbh_edpt_xfer_with_callback(uint8_t dev_addr, uint8_t ep_addr, uint8_t * buffer, uint16_t total_bytes,
                                   tuh_xfer_cb_t complete_cb, uintptr_t user_data);
 
-TU_ATTR_ALWAYS_INLINE
-static inline bool usbh_edpt_xfer(uint8_t dev_addr, uint8_t ep_addr, uint8_t * buffer, uint16_t total_bytes) {
+TU_ATTR_ALWAYS_INLINE static inline
+bool usbh_edpt_xfer(uint8_t dev_addr, uint8_t ep_addr, uint8_t * buffer, uint16_t total_bytes) {
   return usbh_edpt_xfer_with_callback(dev_addr, ep_addr, buffer, total_bytes, NULL, 0);
 }