usb: Hub Driver and USBH recover port automatically after disconnection

This commit updates the Hub Driver and USBH as follows:
- Updated communication mechanism between Hub Driver and USBH
    - USBH notifies the Hub Driver via Hub request callback (usbh_hub_req_cb_t)
    - Hub Driver notifies the USBH via usbh_hub_pass_event()
- Hub Driver now defers all event handling to hub_process.
- Hub Driver and USBH will now automatically recover the port after a disconnected
    device has been closed.
- Fixed incorrect assert in usbh_dev_close()

components/usb/hub.c

@@ -42,9 +42,11 @@ implement the bare minimum to control the root HCD port.
 #define ENUM_FULL_SPEED_MPS                         64      //Worst case MPS for the default endpoint of a full-speed device
 #define ENUM_LANGID                                 0x409   //Current enumeration only supports English (United States) string descriptors
 
+//Hub driver action flags. LISTED IN THE ORDER THEY SHOULD BE HANDLED IN within hub_process(). Some actions are mutually exclusive
 #define HUB_DRIVER_FLAG_ACTION_ROOT_EVENT           0x01
-#define HUB_DRIVER_FLAG_ACTION_ENUM_EVENT           0x02
+#define HUB_DRIVER_FLAG_ACTION_PORT_DISABLE         0x02
 #define HUB_DRIVER_FLAG_ACTION_PORT_RECOVER         0x04
+#define HUB_DRIVER_FLAG_ACTION_ENUM_EVENT           0x08
 
 /**
  * @brief Hub driver states
@@ -54,7 +56,8 @@ implement the bare minimum to control the root HCD port.
 typedef enum {
     HUB_DRIVER_STATE_INSTALLED,             /**< Hub driver is installed. Root port is not powered */
     HUB_DRIVER_STATE_ROOT_POWERED,          /**< Root port is powered, is not connected */
-    HUB_DRIVER_STATE_ROOT_ENUMERATING,      /**< A device has connected to the root port and is undergoing enumeration */
+    HUB_DRIVER_STATE_ROOT_ENUM,             /**< A device has connected to the root port and is undergoing enumeration */
+    HUB_DRIVER_STATE_ROOT_ENUM_FAILED,      /**< Enumeration of a connect device has failed. Waiting for that device to disconnect */
     HUB_DRIVER_STATE_ROOT_ACTIVE,           /**< The connected device is enumerated */
     HUB_DRIVER_STATE_ROOT_RECOVERY,         /**< Root port encountered an error and needs to be recovered */
 } hub_driver_state_t;
@@ -170,10 +173,10 @@ typedef struct {
             uint32_t val;
         } flags;
         hub_driver_state_t driver_state;
-        usb_device_handle_t root_dev_hdl;   //Indicates if an enumerated device is connected to the root port
     } dynamic;
     //Single thread members don't require a critical section so long as they are never accessed from multiple threads
     struct {
+        usb_device_handle_t root_dev_hdl;   //Indicates if an enumerated device is connected to the root port
         enum_ctrl_t enum_ctrl;
     } single_thread;
     //Constant members do no change after installation thus do not require a critical section
@@ -210,8 +213,47 @@ const char *HUB_DRIVER_TAG = "HUB";
 
 // ------------------------------------------------- Forward Declare ---------------------------------------------------
 
+/**
+ * @brief HCD port callback for the root port
+ *
+ * - This callback is called from the context of the HCD, so any event handling should be deferred to hub_process()
+ * - Under the current HCD implementation, this callback should only be ever be called in an ISR
+ * - This callback needs to call the notification to ensure hub_process() gets a chance to run
+ *
+ * @param port_hdl HCD port handle
+ * @param port_event HCD port event
+ * @param user_arg Callback argument
+ * @param in_isr Whether callback is in an ISR context
+ * @return Whether a yield is required
+ */
+static bool root_port_callback(hcd_port_handle_t port_hdl, hcd_port_event_t port_event, void *user_arg, bool in_isr);
+
+/**
+ * @brief HCD pipe callback for the default pipe of the device under enumeration
+ *
+ * - This callback is called from the context of the HCD, so any event handling should be deferred to hub_process()
+ * - This callback needs to call the notification to ensure hub_process() gets a chance to run
+ *
+ * @param pipe_hdl HCD pipe handle
+ * @param pipe_event Pipe event
+ * @param user_arg Callback argument
+ * @param in_isr Whether callback is in an ISR context
+ * @return Whether a yield is required
+ */
 static bool enum_dflt_pipe_callback(hcd_pipe_handle_t pipe_hdl, hcd_pipe_event_t pipe_event, void *user_arg, bool in_isr);
 
+/**
+ * @brief USBH Hub driver request callback
+ *
+ * - This callback is called from the context of the USBH, so so any event handling should be deferred to hub_process()
+ * - This callback needs to call the notification to ensure hub_process() gets a chance to run
+ *
+ * @param port_hdl HCD port handle
+ * @param hub_req Hub driver request
+ * @param arg Callback argument
+ */
+static void usbh_hub_req_callback(hcd_port_handle_t port_hdl, usbh_hub_req_t hub_req, void *arg);
+
 // ------------------------------------------------- Enum Functions ----------------------------------------------------
 
 static bool enum_stage_start(enum_ctrl_t *enum_ctrl)
@@ -539,9 +581,9 @@ static void enum_stage_cleanup(enum_ctrl_t *enum_ctrl)
 {
     //We currently only support a single device connected to the root port. Move the device handle from enum to root
     HUB_DRIVER_ENTER_CRITICAL();
-    p_hub_driver_obj->dynamic.root_dev_hdl = enum_ctrl->dev_hdl;
     p_hub_driver_obj->dynamic.driver_state = HUB_DRIVER_STATE_ROOT_ACTIVE;
     HUB_DRIVER_EXIT_CRITICAL();
+    p_hub_driver_obj->single_thread.root_dev_hdl = enum_ctrl->dev_hdl;
     usb_device_handle_t dev_hdl = enum_ctrl->dev_hdl;
     //Clear values in enum_ctrl
     enum_ctrl->dev_hdl = NULL;
@@ -554,11 +596,11 @@ static void enum_stage_cleanup_failed(enum_ctrl_t *enum_ctrl)
 {
     //Enumeration failed. Clear the enum device handle and pipe
     if (enum_ctrl->dev_hdl) {
-        //Halt, flush, and dequeue enum default pipe
+        //If enumeration failed due to a port event, we need to Halt, flush, and dequeue enum default pipe in case there
+        //was an in-flight URB.
         ESP_ERROR_CHECK(hcd_pipe_command(enum_ctrl->pipe, HCD_PIPE_CMD_HALT));
         ESP_ERROR_CHECK(hcd_pipe_command(enum_ctrl->pipe, HCD_PIPE_CMD_FLUSH));
-        urb_t *dequeued_enum_urb = hcd_urb_dequeue(enum_ctrl->pipe);
-        assert(dequeued_enum_urb == enum_ctrl->urb);
+        hcd_urb_dequeue(enum_ctrl->pipe);   //This could return NULL if there
         ESP_ERROR_CHECK(usbh_hub_enum_failed(enum_ctrl->dev_hdl)); //Free the underlying device object first before recovering the port
     }
     //Clear values in enum_ctrl
@@ -568,6 +610,9 @@ static void enum_stage_cleanup_failed(enum_ctrl_t *enum_ctrl)
     //Enum could have failed due to a port error. If so, we need to trigger a port recovery
     if (p_hub_driver_obj->dynamic.driver_state == HUB_DRIVER_STATE_ROOT_RECOVERY) {
         p_hub_driver_obj->dynamic.flags.actions |= HUB_DRIVER_FLAG_ACTION_PORT_RECOVER;
+    } else {
+        //Otherwise, we move to the enum failed state and wait for the device to disconnect
+        p_hub_driver_obj->dynamic.driver_state = HUB_DRIVER_STATE_ROOT_ENUM_FAILED;
     }
     HUB_DRIVER_EXIT_CRITICAL();
 }
@@ -670,35 +715,27 @@ static bool enum_dflt_pipe_callback(hcd_pipe_handle_t pipe_hdl, hcd_pipe_event_t
     return p_hub_driver_obj->constant.notif_cb(USB_NOTIF_SOURCE_HUB, in_isr, p_hub_driver_obj->constant.notif_cb_arg);
 }
 
-static void usbh_hub_callback(hcd_port_handle_t port_hdl, usbh_hub_event_t hub_event, void *arg)
+static void usbh_hub_req_callback(hcd_port_handle_t port_hdl, usbh_hub_req_t hub_req, void *arg)
 {
-    //We currently only support the root port
+    //We currently only support the root port, so the port_hdl should match the root port
     assert(port_hdl == p_hub_driver_obj->constant.root_port_hdl);
+
     HUB_DRIVER_ENTER_CRITICAL();
-    //Any hub_event results in whatever device connected to the root port to no longer be valid. We clear root_dev_hdl here.
-    usb_device_handle_t dev_hdl = p_hub_driver_obj->dynamic.root_dev_hdl;
-    p_hub_driver_obj->dynamic.root_dev_hdl = NULL;
-    assert(dev_hdl);
-    bool call_port_disable = false;
-    switch (hub_event) {
-        case USBH_HUB_EVENT_CLEANUP_PORT:
-            //After USBH has cleaned up gone device. The port can be recovered.
-            p_hub_driver_obj->dynamic.flags.actions |= HUB_DRIVER_FLAG_ACTION_PORT_RECOVER;
+    switch (hub_req) {
+        case USBH_HUB_REQ_PORT_DISABLE:
+            p_hub_driver_obj->dynamic.flags.actions |= HUB_DRIVER_FLAG_ACTION_PORT_DISABLE;
             break;
-        case USBH_HUB_EVENT_DISABLE_PORT:
-            p_hub_driver_obj->dynamic.driver_state = HUB_DRIVER_STATE_ROOT_POWERED;
-            call_port_disable = true;
+        case USBH_HUB_REQ_PORT_RECOVER:
+            p_hub_driver_obj->dynamic.flags.actions |= HUB_DRIVER_FLAG_ACTION_PORT_RECOVER;
             break;
         default:
-            abort();    //Should never occur
+            //Should never occur
+            abort();
             break;
     }
     HUB_DRIVER_EXIT_CRITICAL();
-    if (call_port_disable) {
-        ESP_LOGD(HUB_DRIVER_TAG, "Disabling root port");
-        hcd_port_command(port_hdl, HCD_PORT_CMD_DISABLE);
-        ESP_ERROR_CHECK(usbh_hub_dev_port_disabled(dev_hdl));
-    }
+
+    p_hub_driver_obj->constant.notif_cb(USB_NOTIF_SOURCE_HUB, false, p_hub_driver_obj->constant.notif_cb_arg);
 }
 
 // ---------------------- Handlers -------------------------
@@ -716,7 +753,7 @@ static void root_port_handle_events(hcd_port_handle_t root_port_hdl)
                 //Start enumeration
                 HUB_DRIVER_ENTER_CRITICAL();
                 p_hub_driver_obj->dynamic.flags.actions |= HUB_DRIVER_FLAG_ACTION_ENUM_EVENT;
-                p_hub_driver_obj->dynamic.driver_state = HUB_DRIVER_STATE_ROOT_ENUMERATING;
+                p_hub_driver_obj->dynamic.driver_state = HUB_DRIVER_STATE_ROOT_ENUM;
                 HUB_DRIVER_EXIT_CRITICAL();
                 p_hub_driver_obj->single_thread.enum_ctrl.stage = ENUM_STAGE_START;
             } else {
@@ -727,21 +764,22 @@ static void root_port_handle_events(hcd_port_handle_t root_port_hdl)
         case HCD_PORT_EVENT_DISCONNECTION:
         case HCD_PORT_EVENT_ERROR:
         case HCD_PORT_EVENT_OVERCURRENT: {
-            usb_device_handle_t dev_hdl = NULL;
+            bool pass_event_to_usbh = false;
             HUB_DRIVER_ENTER_CRITICAL();
             switch (p_hub_driver_obj->dynamic.driver_state) {
-                case HUB_DRIVER_STATE_ROOT_POWERED:
-                    //This occurred before enumeration. Therefore, there's no device and we can go straight to port recovery
-                    p_hub_driver_obj->dynamic.flags.actions |= HUB_DRIVER_FLAG_ACTION_ENUM_EVENT;
+                case HUB_DRIVER_STATE_ROOT_POWERED: //This occurred before enumeration
+                case HUB_DRIVER_STATE_ROOT_ENUM_FAILED: //This occurred after a failed enumeration.
+                    //Therefore, there's no device and we can go straight to port recovery
+                    p_hub_driver_obj->dynamic.flags.actions |= HUB_DRIVER_FLAG_ACTION_PORT_RECOVER;
                     break;
-                case HUB_DRIVER_STATE_ROOT_ENUMERATING:
+                case HUB_DRIVER_STATE_ROOT_ENUM:
                     //This occurred during enumeration. Therefore, we need to recover the failed enumeration
                     p_hub_driver_obj->dynamic.flags.actions |= HUB_DRIVER_FLAG_ACTION_ENUM_EVENT;
                     p_hub_driver_obj->single_thread.enum_ctrl.stage = ENUM_STAGE_CLEANUP_FAILED;
                     break;
                 case HUB_DRIVER_STATE_ROOT_ACTIVE:
                     //There was an enumerated device. We need to indicate to USBH that the device is gone
-                    dev_hdl = p_hub_driver_obj->dynamic.root_dev_hdl;
+                    pass_event_to_usbh = true;
                     break;
                 default:
                     abort();    //Should never occur
@@ -749,8 +787,9 @@ static void root_port_handle_events(hcd_port_handle_t root_port_hdl)
             }
             p_hub_driver_obj->dynamic.driver_state = HUB_DRIVER_STATE_ROOT_RECOVERY;
             HUB_DRIVER_EXIT_CRITICAL();
-            if (dev_hdl) {
-                ESP_ERROR_CHECK(usbh_hub_mark_dev_gone(dev_hdl));
+            if (pass_event_to_usbh) {
+                assert(p_hub_driver_obj->single_thread.root_dev_hdl);
+                ESP_ERROR_CHECK(usbh_hub_pass_event(p_hub_driver_obj->single_thread.root_dev_hdl, USBH_HUB_EVENT_PORT_ERROR));
             }
             break;
         }
@@ -868,7 +907,7 @@ esp_err_t hub_install(hub_config_t *hub_config)
     p_hub_driver_obj = hub_driver_obj;
     HUB_DRIVER_EXIT_CRITICAL();
     //Indicate to USBH that the hub is installed
-    ESP_ERROR_CHECK(usbh_hub_is_installed(usbh_hub_callback, NULL));
+    ESP_ERROR_CHECK(usbh_hub_is_installed(usbh_hub_req_callback, NULL));
     ret = ESP_OK;
     return ret;
 
@@ -937,12 +976,22 @@ esp_err_t hub_process(void)
     HUB_DRIVER_EXIT_CRITICAL();
 
     while (action_flags) {
+        /*
+        Mutually exclude Root event and Port disable:
+        If a device was waiting for its port to be disabled, and a port error occurs in that time, the root event
+        handler will send a USBH_HUB_EVENT_PORT_ERROR to the USBH already, thus freeing the device and canceling the
+        waiting of port disable.
+        */
         if (action_flags & HUB_DRIVER_FLAG_ACTION_ROOT_EVENT) {
             root_port_handle_events(p_hub_driver_obj->constant.root_port_hdl);
+        } else if (action_flags & HUB_DRIVER_FLAG_ACTION_PORT_DISABLE) {
+            ESP_LOGD(HUB_DRIVER_TAG, "Disabling root port");
+            hcd_port_command(p_hub_driver_obj->constant.root_port_hdl, HCD_PORT_CMD_DISABLE);
+            ESP_ERROR_CHECK(usbh_hub_pass_event(p_hub_driver_obj->single_thread.root_dev_hdl, USBH_HUB_EVENT_PORT_DISABLED));
+            //The root port has been disabled, so the root_dev_hdl is no longer valid
+            p_hub_driver_obj->single_thread.root_dev_hdl = NULL;
         }
-        if (action_flags & HUB_DRIVER_FLAG_ACTION_ENUM_EVENT) {
-            enum_handle_events();
-        }
+
         if (action_flags & HUB_DRIVER_FLAG_ACTION_PORT_RECOVER) {
             ESP_LOGD(HUB_DRIVER_TAG, "Recovering root port");
             ESP_ERROR_CHECK(hcd_port_recover(p_hub_driver_obj->constant.root_port_hdl));
@@ -950,7 +999,14 @@ esp_err_t hub_process(void)
             HUB_DRIVER_ENTER_CRITICAL();
             p_hub_driver_obj->dynamic.driver_state = HUB_DRIVER_STATE_ROOT_POWERED;
             HUB_DRIVER_EXIT_CRITICAL();
+            //USBH requesting a port recovery means the device has already been freed. Clear root_dev_hdl
+            p_hub_driver_obj->single_thread.root_dev_hdl = NULL;
+        }
+
+        if (action_flags & HUB_DRIVER_FLAG_ACTION_ENUM_EVENT) {
+            enum_handle_events();
         }
+
         HUB_DRIVER_ENTER_CRITICAL();
         action_flags = p_hub_driver_obj->dynamic.flags.actions;
         p_hub_driver_obj->dynamic.flags.actions = 0;

components/usb/private_include/usbh.h

@@ -23,14 +23,44 @@ extern "C" {
 // ----------------------- Events --------------------------
 
 typedef enum {
-    USBH_EVENT_DEV_NEW,                 /**< A new device has been enumerated and added to the device pool */
-    USBH_EVENT_DEV_GONE,                /**< A device is gone. Clients should close the device */
-    USBH_EVENT_DEV_ALL_FREE,            /**< All devices have been freed */
+    USBH_EVENT_DEV_NEW,             /**< A new device has been enumerated and added to the device pool */
+    USBH_EVENT_DEV_GONE,            /**< A device is gone. Clients should close the device */
+    USBH_EVENT_DEV_ALL_FREE,        /**< All devices have been freed */
 } usbh_event_t;
 
+/**
+ * @brief Hub driver requests
+ *
+ * Various requests of the Hub driver that the USBH can make.
+ */
+typedef enum {
+    USBH_HUB_REQ_PORT_DISABLE,      /**< Request that the Hub driver disable a particular port (occurs after a device
+                                         has been freed). Hub driver should respond with a USBH_HUB_EVENT_PORT_DISABLED */
+    USBH_HUB_REQ_PORT_RECOVER,      /**< Request that the Hub driver recovers a particular port (occurs after a gone
+                                         device has been freed). */
+} usbh_hub_req_t;
+
+/**
+ * @brief Hub driver events for the USBH
+ *
+ * These events as passed by the Hub driver to the USBH via usbh_hub_pass_event()
+ *
+ * USBH_HUB_EVENT_PORT_ERROR:
+ * - The port has encountered an error (such as a sudden disconnection). The device connected to that port is no longer valid.
+ * - The USBH should:
+ *      - Trigger a USBH_EVENT_DEV_GONE
+ *      - Prevent further transfers to the device
+ *      - Trigger the device's cleanup if it is already closed
+ *      - When the last client closes the device via usbh_dev_close(), free the device object and issue a USBH_HUB_REQ_PORT_RECOVER request
+ *
+ * USBH_HUB_EVENT_PORT_DISABLED:
+ * - A previous USBH_HUB_REQ_PORT_DISABLE has completed.
+ * - The USBH should free the device object
+ */
 typedef enum {
-    USBH_HUB_EVENT_CLEANUP_PORT,        /**< Indicate to the Hub driver that it should clean up the port of a device (occurs after a gone device has been freed) */
-    USBH_HUB_EVENT_DISABLE_PORT,        /**< Indicate to the Hub driver that it should disable the port of a device (occurs after a device has been freed) */
+    USBH_HUB_EVENT_PORT_ERROR,      /**< The port has encountered an error (such as a sudden disconnection). The device
+                                         connected to that port should be marked gone. */
+    USBH_HUB_EVENT_PORT_DISABLED,   /**< Previous USBH_HUB_REQ_PORT_DISABLE request completed */
 } usbh_hub_event_t;
 
 // ---------------------- Callbacks ------------------------
@@ -51,9 +81,11 @@ typedef void (*usbh_event_cb_t)(usb_device_handle_t dev_hdl, usbh_event_t usbh_e
 
 /**
  * @brief Callback used by the USBH to request actions from the Hub driver
- * @note This callback is called from within usbh_process()
+ *
+ * The Hub Request Callback allows the USBH to request the Hub actions on a particular port. Conversely, the Hub driver
+ * will indicate completion of some of these requests to the USBH via the usbh_hub_event() funtion.
  */
-typedef void (*usbh_hub_cb_t)(hcd_port_handle_t port_hdl, usbh_hub_event_t hub_event, void *arg);
+typedef void (*usbh_hub_req_cb_t)(hcd_port_handle_t port_hdl, usbh_hub_req_t hub_req, void *arg);
 
 // ----------------------- Objects -------------------------
 
@@ -289,11 +321,11 @@ esp_err_t usbh_ep_get_context(usb_device_handle_t dev_hdl, uint8_t bEndpointAddr
  * - This should only be called after the USBH has already be installed
  *
  * @note Hub Driver only
- * @param[in] hub_callback Hub callback
+ * @param[in] hub_req_callback Hub request callback
  * @param[in] callback_arg Callback argument
  * @return esp_err_t
  */
-esp_err_t usbh_hub_is_installed(usbh_hub_cb_t hub_callback, void *callback_arg);
+esp_err_t usbh_hub_is_installed(usbh_hub_req_cb_t hub_req_callback, void *callback_arg);
 
 /**
  * @brief Indicates to USBH the start of enumeration for a device
@@ -313,35 +345,13 @@ esp_err_t usbh_hub_is_installed(usbh_hub_cb_t hub_callback, void *callback_arg);
 esp_err_t usbh_hub_add_dev(hcd_port_handle_t port_hdl, usb_speed_t dev_speed, usb_device_handle_t *new_dev_hdl, hcd_pipe_handle_t *default_pipe_hdl);
 
 /**
- * @brief Indicate that a device is gone
+ * @brief Indicates to the USBH that a hub event has occurred for a particular device
  *
- * This Hub driver must call this function to indicate that a device is gone. A device is gone when:
- * - It suddenly disconnects
- * - Its upstream port or device has an error or is also gone.
- * Marking a device as gone will:
- * - Trigger a USBH_EVENT_DEV_GONE
- * - Prevent further transfers to the device
- * - Trigger the device's cleanup if it is already closed
- * - When the last client closes the device via usbh_dev_close(), the device's resources will be cleaned up
- *
- * @note Hub Driver only
- * @param[in] dev_hdl Device handle
- * @return esp_err_t
- */
-esp_err_t usbh_hub_mark_dev_gone(usb_device_handle_t dev_hdl);
-
-/**
- * @brief Indicate that a device's port has been disabled
- *
- * - The Hub driver must call this function once it has disabled the port of a particular device
- * - The Hub driver disables a device's port when requested by the USBH via the USBH_HUB_EVENT_DISABLE_PORT
- * - This function will trigger the device's cleanup.
- *
- * @note Hub Driver only
- * @param[in] dev_hdl Device handle
+ * @param dev_hdl Device handle
+ * @param hub_event Hub event
  * @return esp_err_t
  */
-esp_err_t usbh_hub_dev_port_disabled(usb_device_handle_t dev_hdl);
+esp_err_t usbh_hub_pass_event(usb_device_handle_t dev_hdl, usbh_hub_event_t hub_event);
 
 // ----------------- Enumeration Related -------------------
 

components/usb/test/usb_host/msc_client_async_dconn.c

@@ -36,6 +36,8 @@ Implementation of an asynchronous MSC client used for USB Host disconnection tes
     - Deregister MSC client
 */
 
+#define TEST_DCONN_ITERATIONS   3
+
 typedef enum {
     TEST_STAGE_WAIT_CONN,
     TEST_STAGE_DEV_OPEN,
@@ -155,6 +157,7 @@ void msc_client_async_dconn_task(void *arg)
 
     bool exit_loop = false;
     bool skip_event_handling = false;
+    int dconn_iter = 0;
     while (!exit_loop) {
         if (!skip_event_handling) {
             TEST_ASSERT_EQUAL(ESP_OK, usb_host_client_handle_events(msc_obj.client_hdl, portMAX_DELAY));
@@ -166,6 +169,10 @@ void msc_client_async_dconn_task(void *arg)
         msc_obj.cur_stage = msc_obj.next_stage;
 
         switch (msc_obj.cur_stage) {
+            case TEST_STAGE_WAIT_CONN: {
+                //Nothing to do while waiting for connection
+                break;
+            }
             case TEST_STAGE_DEV_OPEN: {
                 ESP_LOGD(MSC_CLIENT_TAG, "Open");
                 //Open the device
@@ -227,7 +234,15 @@ void msc_client_async_dconn_task(void *arg)
                 ESP_LOGD(MSC_CLIENT_TAG, "Close");
                 TEST_ASSERT_EQUAL(ESP_OK, usb_host_interface_release(msc_obj.client_hdl, msc_obj.dev_hdl, MOCK_MSC_SCSI_INTF_NUMBER));
                 TEST_ASSERT_EQUAL(ESP_OK, usb_host_device_close(msc_obj.client_hdl, msc_obj.dev_hdl));
-                exit_loop = true;
+                dconn_iter++;
+                if (dconn_iter < TEST_DCONN_ITERATIONS) {
+                    //Start the next test iteration by going back to TEST_STAGE_WAIT_CONN and reenabling connections
+                    msc_obj.next_stage = TEST_STAGE_WAIT_CONN;
+                    skip_event_handling = true; //Need to execute TEST_STAGE_WAIT_CONN
+                    test_usb_set_phy_state(true, 0);
+                } else {
+                    exit_loop = true;
+                }
                 break;
             }
             default:

components/usb/test/usb_host/msc_client_async_enum.c

@@ -28,9 +28,12 @@ Implementation of an asynchronous MSC client used for USB Host enumeration test.
     - Check the device and configuration descriptor of the device
     - Check the device's information
     - Close device
+    - Repeat for multiple iterations from waiting connection by forcing a disconnection
     - Deregister MSC client
 */
 
+#define TEST_ENUM_ITERATIONS   3
+
 typedef enum {
     TEST_STAGE_WAIT_CONN,
     TEST_STAGE_DEV_OPEN,
@@ -90,6 +93,7 @@ void msc_client_async_enum_task(void *arg)
 
     bool exit_loop = false;
     bool skip_event_handling = false;
+    int enum_iter = 0;
     while (!exit_loop) {
         if (!skip_event_handling) {
             TEST_ASSERT_EQUAL(ESP_OK, usb_host_client_handle_events(msc_obj.client_hdl, portMAX_DELAY));
@@ -101,6 +105,10 @@ void msc_client_async_enum_task(void *arg)
         msc_obj.cur_stage = msc_obj.next_stage;
 
         switch (msc_obj.cur_stage) {
+            case TEST_STAGE_WAIT_CONN: {
+                //Wait for connection, nothing to do
+                break;
+            }
             case TEST_STAGE_DEV_OPEN: {
                 ESP_LOGD(MSC_CLIENT_TAG, "Open");
                 //Open the device
@@ -154,7 +162,16 @@ void msc_client_async_enum_task(void *arg)
             case TEST_STAGE_DEV_CLOSE: {
                 ESP_LOGD(MSC_CLIENT_TAG, "Close");
                 TEST_ASSERT_EQUAL(ESP_OK, usb_host_device_close(msc_obj.client_hdl, msc_obj.dev_hdl));
-                exit_loop = true;
+                enum_iter++;
+                if (enum_iter < TEST_ENUM_ITERATIONS) {
+                    //Start the next test iteration by disconnecting the device, then going back to TEST_STAGE_WAIT_CONN stage
+                    test_usb_set_phy_state(false, 0);
+                    test_usb_set_phy_state(true, 0);
+                    msc_obj.next_stage = TEST_STAGE_WAIT_CONN;
+                    skip_event_handling = true; //Need to execute TEST_STAGE_WAIT_CONN
+                } else {
+                    exit_loop = true;
+                }
                 break;
             }
             default:

components/usb/test/usb_host/test_usb_host_plugging.c

@@ -7,7 +7,6 @@
 #include <stdio.h>
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
-#include "freertos/timers.h"
 #include "esp_err.h"
 #include "esp_intr_alloc.h"
 #include "test_usb_common.h"
@@ -20,14 +19,20 @@
 
 // --------------------------------------------------- Test Cases ------------------------------------------------------
 
-//Safe approximation of time it takes to connect and enumerate the device
-#define TEST_FORCE_DCONN_DELAY_MS           400
+/*
+Test USB Host Library Sudden Disconnection Handling (no clients)
+Purpose:
+- Test that sudden disconnections are handled properly when there are no clients
+- Test that devices can reconnect after a sudden disconnection has been handled by the USB Host Library
 
-static void trigger_dconn_timer_cb(TimerHandle_t xTimer)
-{
-    printf("Forcing Sudden Disconnect\n");
-    test_usb_set_phy_state(false, 0);
-}
+Procedure:
+- Install USB Host Library
+- Wait for connection (and enumeration) to occur
+- Force a disconnection, then wait for disconnection to be handled (USB_HOST_LIB_EVENT_FLAGS_ALL_FREE)
+- Allow connections again, and repeat test for multiple iterations
+*/
+
+#define TEST_DCONN_NO_CLIENT_ITERATIONS     3
 
 TEST_CASE("Test USB Host sudden disconnection (no client)", "[usb_host][ignore]")
 {
@@ -40,32 +45,53 @@ TEST_CASE("Test USB Host sudden disconnection (no client)", "[usb_host][ignore]"
     ESP_ERROR_CHECK(usb_host_install(&host_config));
     printf("Installed\n");
 
-    //Allocate timer to force disconnection after a short delay
-    TimerHandle_t timer_hdl = xTimerCreate("dconn",
-                                           pdMS_TO_TICKS(TEST_FORCE_DCONN_DELAY_MS),
-                                           pdFALSE,
-                                           NULL,
-                                           trigger_dconn_timer_cb);
-    TEST_ASSERT_NOT_EQUAL(NULL, timer_hdl);
-    TEST_ASSERT_EQUAL(pdPASS, xTimerStart(timer_hdl, portMAX_DELAY));
-
+    bool connected = false;
+    int dconn_iter = 0;
     while (1) {
         //Start handling system events
         uint32_t event_flags;
         usb_host_lib_handle_events(portMAX_DELAY, &event_flags);
+        if (!connected) {
+            usb_host_lib_info_t lib_info;
+            TEST_ASSERT_EQUAL(ESP_OK, usb_host_lib_info(&lib_info));
+            if (lib_info.num_devices == 1) {
+                //We've just connected. Trigger a disconnect
+                connected = true;
+                printf("Forcing Sudden Disconnect\n");
+                test_usb_set_phy_state(false, 0);
+            }
+        }
         if (event_flags & USB_HOST_LIB_EVENT_FLAGS_ALL_FREE) {
-            printf("All devices cleaned up\n");
-            break;
+            //The device has disconnected and it's disconnection has been handled
+            printf("Dconn iter %d done\n", dconn_iter);
+            if (++dconn_iter < TEST_DCONN_NO_CLIENT_ITERATIONS) {
+                //Start next iteration
+                connected = false;
+                test_usb_set_phy_state(true, 0);
+            } else {
+                break;
+            }
         }
     }
 
-    //Cleanup timer
-    TEST_ASSERT_EQUAL(pdPASS, xTimerDelete(timer_hdl, portMAX_DELAY));
     //Clean up USB Host
     ESP_ERROR_CHECK(usb_host_uninstall());
     test_usb_deinit_phy();  //Deinitialize the internal USB PHY after testing
 }
 
+/*
+Test USB Host Library Sudden Disconnection Handling (with client)
+Purpose:
+- Test that sudden disconnections are handled properly when there are registered clients
+- Test that devices can reconnect after a sudden disconnection has been handled by the USB Host Library
+
+Procedure:
+    - Install USB Host Library
+    - Create a task to run an MSC client
+    - Start the MSC disconnect client task. It will open the device then force a disconnect for multiple iterations
+    - Wait for USB_HOST_LIB_EVENT_FLAGS_NO_CLIENTS and USB_HOST_LIB_EVENT_FLAGS_ALL_FREE before uninstalling
+*/
+
 #define TEST_FORCE_DCONN_NUM_TRANSFERS      3
 #define TEST_MSC_SCSI_TAG                   0xDEADBEEF
 
@@ -116,6 +142,24 @@ TEST_CASE("Test USB Host sudden disconnection (single client)", "[usb_host][igno
     test_usb_deinit_phy();  //Deinitialize the internal USB PHY after testing
 }
 
+/*
+Test USB Host Library Enumeration
+Purpose:
+- Test that the USB Host Library enumerates device correctly
+
+Procedure:
+- Install USB Host Library
+- Create a task to run an MSC client
+- Start the MSC enumeration client task. It will:
+    - Wait for device connection
+    - Open the device
+    - Check details of the device's enumeration
+    - Disconnect the device, and repeat the steps above for multiple iterations.
+- Wait for USB_HOST_LIB_EVENT_FLAGS_NO_CLIENTS and USB_HOST_LIB_EVENT_FLAGS_ALL_FREE before uninstalling
+*/
+
+#define TEST_ENUM_ITERATIONS    3
+
 TEST_CASE("Test USB Host enumeration", "[usb_host][ignore]")
 {
     test_usb_init_phy();    //Initialize the internal USB PHY and USB Controller for testing
@@ -129,20 +173,23 @@ TEST_CASE("Test USB Host enumeration", "[usb_host][ignore]")
 
     //Create task to run client that checks the enumeration of the device
     TaskHandle_t task_hdl;
-    xTaskCreatePinnedToCore(msc_client_async_enum_task, "async", 4096, NULL, 2, &task_hdl, 0);
+    xTaskCreatePinnedToCore(msc_client_async_enum_task, "async", 6144, NULL, 2, &task_hdl, 0);
     //Start the task
     xTaskNotifyGive(task_hdl);
 
-    while (1) {
+    bool all_clients_gone = false;
+    bool all_dev_free = false;
+    while (!all_clients_gone || !all_dev_free) {
         //Start handling system events
         uint32_t event_flags;
         usb_host_lib_handle_events(portMAX_DELAY, &event_flags);
         if (event_flags & USB_HOST_LIB_EVENT_FLAGS_NO_CLIENTS) {
             printf("No more clients\n");
             TEST_ASSERT_EQUAL(ESP_ERR_NOT_FINISHED, usb_host_device_free_all());
+            all_clients_gone = true;
         }
-        if (event_flags & USB_HOST_LIB_EVENT_FLAGS_ALL_FREE) {
-            break;
+        if (all_clients_gone && event_flags & USB_HOST_LIB_EVENT_FLAGS_ALL_FREE) {
+            all_dev_free = true;
         }
     }
 

components/usb/usbh.c

@@ -22,14 +22,15 @@
 #include "usb/usb_types_ch9.h"
 
 //Device action flags. LISTED IN THE ORDER THEY SHOULD BE HANDLED IN within usbh_process(). Some actions are mutually exclusive
-#define DEV_FLAG_ACTION_PIPE_HALT_AND_FLUSH         0x01    //Halt all non-default pipes then flush them (called after a device gone is gone)
-#define DEV_FLAG_ACTION_DEFAULT_PIPE_FLUSH          0x02    //Retire all URBS in the default pipe
-#define DEV_FLAG_ACTION_DEFAULT_PIPE_DEQUEUE        0x04    //Dequeue all URBs from default pipe
-#define DEV_FLAG_ACTION_DEFAULT_PIPE_CLEAR          0x08    //Move the default pipe to the active state
-#define DEV_FLAG_ACTION_SEND_GONE_EVENT             0x10    //Send a USB_HOST_CLIENT_EVENT_DEV_GONE event
-#define DEV_FLAG_ACTION_FREE                        0x20    //Free the device object
-#define DEV_FLAG_ACTION_PORT_DISABLE                0x40    //Request the hub driver to disable the port of the device
-#define DEV_FLAG_ACTION_SEND_NEW                    0x80    //Send a new device event
+#define DEV_FLAG_ACTION_PIPE_HALT_AND_FLUSH         0x0001  //Halt all non-default pipes then flush them (called after a device gone is gone)
+#define DEV_FLAG_ACTION_DEFAULT_PIPE_FLUSH          0x0002  //Retire all URBS in the default pipe
+#define DEV_FLAG_ACTION_DEFAULT_PIPE_DEQUEUE        0x0004  //Dequeue all URBs from default pipe
+#define DEV_FLAG_ACTION_DEFAULT_PIPE_CLEAR          0x0008  //Move the default pipe to the active state
+#define DEV_FLAG_ACTION_SEND_GONE_EVENT             0x0010  //Send a USB_HOST_CLIENT_EVENT_DEV_GONE event
+#define DEV_FLAG_ACTION_FREE                        0x0020  //Free the device object
+#define DEV_FLAG_ACTION_FREE_AND_RECOVER            0x0040  //Free the device object, but send a USBH_HUB_REQ_PORT_RECOVER request afterwards.
+#define DEV_FLAG_ACTION_PORT_DISABLE                0x0080  //Request the hub driver to disable the port of the device
+#define DEV_FLAG_ACTION_SEND_NEW                    0x0100  //Send a new device event
 
 #define EP_NUM_MIN                                  1
 #define EP_NUM_MAX                                  16
@@ -41,16 +42,16 @@ struct device_s {
         TAILQ_ENTRY(device_s) tailq_entry;
         union {
             struct {
-                uint32_t actions: 8;
                 uint32_t in_pending_list: 1;
                 uint32_t is_gone: 1;
                 uint32_t waiting_close: 1;
                 uint32_t waiting_port_disable: 1;
                 uint32_t waiting_free: 1;
-                uint32_t reserved19: 19;
+                uint32_t reserved27: 27;
             };
             uint32_t val;
         } flags;
+        uint32_t action_flags;
         int num_ctrl_xfers_inflight;
         usb_device_state_t state;
         uint32_t ref_count;
@@ -88,8 +89,8 @@ typedef struct {
     struct {
         usb_notif_cb_t notif_cb;
         void *notif_cb_arg;
-        usbh_hub_cb_t hub_cb;
-        void *hub_cb_arg;
+        usbh_hub_req_cb_t hub_req_cb;
+        void *hub_req_cb_arg;
         usbh_event_cb_t event_cb;
         void *event_cb_arg;
         usbh_ctrl_xfer_cb_t ctrl_xfer_cb;
@@ -202,7 +203,7 @@ static bool _dev_set_actions(device_t *dev_obj, uint32_t action_flags)
         //Move device form idle device list to callback device list
         TAILQ_REMOVE(&p_usbh_obj->dynamic.devs_idle_tailq, dev_obj, dynamic.tailq_entry);
         TAILQ_INSERT_TAIL(&p_usbh_obj->dynamic.devs_pending_tailq, dev_obj, dynamic.tailq_entry);
-        dev_obj->dynamic.flags.actions |= action_flags;
+        dev_obj->dynamic.action_flags |= action_flags;
         dev_obj->dynamic.flags.in_pending_list = 1;
         call_notif_cb = true;
     } else {
@@ -398,8 +399,8 @@ esp_err_t usbh_process(void)
         TAILQ_REMOVE(&p_usbh_obj->dynamic.devs_pending_tailq, dev_obj, dynamic.tailq_entry);
         TAILQ_INSERT_TAIL(&p_usbh_obj->dynamic.devs_idle_tailq, dev_obj, dynamic.tailq_entry);
         //Clear the device's flags
-        uint32_t action_flags = dev_obj->dynamic.flags.actions;
-        dev_obj->dynamic.flags.actions = 0;
+        uint32_t action_flags = dev_obj->dynamic.action_flags;
+        dev_obj->dynamic.action_flags = 0;
         dev_obj->dynamic.flags.in_pending_list = 0;
 
         /* ---------------------------------------------------------------------
@@ -447,16 +448,22 @@ esp_err_t usbh_process(void)
         - New device event is requested followed immediately by a disconnection
         - Port disable requested followed immediately by a disconnection
         */
-        if (action_flags & DEV_FLAG_ACTION_FREE) {
+        if (action_flags & (DEV_FLAG_ACTION_FREE | DEV_FLAG_ACTION_FREE_AND_RECOVER)) {
+            //Cache a copy of the port handle as we are about to free the device object
+            hcd_port_handle_t port_hdl = dev_obj->constant.port_hdl;
             ESP_LOGD(USBH_TAG, "Freeing device %d", dev_obj->constant.address);
             if (handle_dev_free(dev_obj)) {
                 ESP_LOGD(USBH_TAG, "Device all free");
                 p_usbh_obj->constant.event_cb((usb_device_handle_t)NULL, USBH_EVENT_DEV_ALL_FREE, p_usbh_obj->constant.event_cb_arg);
             }
+            //Check if we need to recover the device's port
+            if (action_flags & DEV_FLAG_ACTION_FREE_AND_RECOVER) {
+                p_usbh_obj->constant.hub_req_cb(port_hdl, USBH_HUB_REQ_PORT_RECOVER, p_usbh_obj->constant.hub_req_cb_arg);
+            }
         } else if (action_flags & DEV_FLAG_ACTION_PORT_DISABLE) {
             //Request that the HUB disables this device's port
             ESP_LOGD(USBH_TAG, "Disable device port %d", dev_obj->constant.address);
-            p_usbh_obj->constant.hub_cb(dev_obj->constant.port_hdl, USBH_HUB_EVENT_DISABLE_PORT, p_usbh_obj->constant.hub_cb_arg);
+            p_usbh_obj->constant.hub_req_cb(dev_obj->constant.port_hdl, USBH_HUB_REQ_PORT_DISABLE, p_usbh_obj->constant.hub_req_cb_arg);
         } else if (action_flags & DEV_FLAG_ACTION_SEND_NEW) {
             ESP_LOGD(USBH_TAG, "New device %d", dev_obj->constant.address);
             p_usbh_obj->constant.event_cb((usb_device_handle_t)dev_obj, USBH_EVENT_DEV_NEW, p_usbh_obj->constant.event_cb_arg);
@@ -559,16 +566,16 @@ esp_err_t usbh_dev_close(usb_device_handle_t dev_hdl)
     device_t *dev_obj = (device_t *)dev_hdl;
 
     USBH_ENTER_CRITICAL();
-    USBH_CHECK_FROM_CRIT(dev_obj->dynamic.num_ctrl_xfers_inflight == 0, ESP_ERR_INVALID_STATE);
     dev_obj->dynamic.ref_count--;
     bool call_notif_cb = false;
     if (dev_obj->dynamic.ref_count == 0) {
-        //Sanity check. This can only be set when ref count reaches 0
-        assert(!dev_obj->dynamic.flags.waiting_free);
+        //Sanity check.
+        assert(dev_obj->dynamic.num_ctrl_xfers_inflight == 0);  //There cannot be any control transfer inflight
+        assert(!dev_obj->dynamic.flags.waiting_free);   //This can only be set when ref count reaches 0
         if (dev_obj->dynamic.flags.is_gone) {
             //Device is already gone so it's port is already disabled. Trigger the USBH process to free the device
             dev_obj->dynamic.flags.waiting_free = 1;
-            call_notif_cb = _dev_set_actions(dev_obj, DEV_FLAG_ACTION_FREE);
+            call_notif_cb = _dev_set_actions(dev_obj, DEV_FLAG_ACTION_FREE_AND_RECOVER); //Port error occurred so we need to recover it
         } else if (dev_obj->dynamic.flags.waiting_close) {
             //Device is still connected but is no longer needed. Trigger the USBH process to request device's port be disabled
             dev_obj->dynamic.flags.waiting_port_disable = 1;
@@ -876,17 +883,17 @@ exit:
 
 // ------------------- Device Related ----------------------
 
-esp_err_t usbh_hub_is_installed(usbh_hub_cb_t hub_callback, void *callback_arg)
+esp_err_t usbh_hub_is_installed(usbh_hub_req_cb_t hub_req_callback, void *callback_arg)
 {
-    USBH_CHECK(hub_callback != NULL, ESP_ERR_INVALID_ARG);
+    USBH_CHECK(hub_req_callback != NULL, ESP_ERR_INVALID_ARG);
 
     USBH_ENTER_CRITICAL();
     //Check that USBH is already installed
     USBH_CHECK_FROM_CRIT(p_usbh_obj != NULL, ESP_ERR_INVALID_STATE);
     //Check that Hub has not be installed yet
-    USBH_CHECK_FROM_CRIT(p_usbh_obj->constant.hub_cb == NULL, ESP_ERR_INVALID_STATE);
-    p_usbh_obj->constant.hub_cb = hub_callback;
-    p_usbh_obj->constant.hub_cb_arg = callback_arg;
+    USBH_CHECK_FROM_CRIT(p_usbh_obj->constant.hub_req_cb == NULL, ESP_ERR_INVALID_STATE);
+    p_usbh_obj->constant.hub_req_cb = hub_req_callback;
+    p_usbh_obj->constant.hub_req_cb_arg = callback_arg;
     USBH_EXIT_CRITICAL();
 
     return ESP_OK;
@@ -909,46 +916,43 @@ esp_err_t usbh_hub_add_dev(hcd_port_handle_t port_hdl, usb_speed_t dev_speed, us
     return ret;
 }
 
-esp_err_t usbh_hub_mark_dev_gone(usb_device_handle_t dev_hdl)
+esp_err_t usbh_hub_pass_event(usb_device_handle_t dev_hdl, usbh_hub_event_t hub_event)
 {
     USBH_CHECK(dev_hdl != NULL, ESP_ERR_INVALID_ARG);
     device_t *dev_obj = (device_t *)dev_hdl;
 
-    USBH_ENTER_CRITICAL();
-    dev_obj->dynamic.flags.is_gone = 1;
     bool call_notif_cb;
-    //Check if the device can be freed now
-    if (dev_obj->dynamic.ref_count == 0) {
-        dev_obj->dynamic.flags.waiting_free = 1;
-        //Device is already waiting free so none of it's pipes will be in use. Can free immediately.
-        call_notif_cb = _dev_set_actions(dev_obj, DEV_FLAG_ACTION_FREE);
-    } else {
-        call_notif_cb = _dev_set_actions(dev_obj, DEV_FLAG_ACTION_PIPE_HALT_AND_FLUSH |
-                                                  DEV_FLAG_ACTION_DEFAULT_PIPE_FLUSH |
-                                                  DEV_FLAG_ACTION_DEFAULT_PIPE_DEQUEUE |
-                                                  DEV_FLAG_ACTION_SEND_GONE_EVENT);
-    }
-    USBH_EXIT_CRITICAL();
-
-    if (call_notif_cb) {
-        p_usbh_obj->constant.notif_cb(USB_NOTIF_SOURCE_USBH, false, p_usbh_obj->constant.notif_cb_arg);
+    switch (hub_event) {
+        case USBH_HUB_EVENT_PORT_ERROR: {
+            USBH_ENTER_CRITICAL();
+            dev_obj->dynamic.flags.is_gone = 1;
+            //Check if the device can be freed now
+            if (dev_obj->dynamic.ref_count == 0) {
+                dev_obj->dynamic.flags.waiting_free = 1;
+                //Device is already waiting free so none of it's pipes will be in use. Can free immediately.
+                call_notif_cb = _dev_set_actions(dev_obj, DEV_FLAG_ACTION_FREE_AND_RECOVER); //Port error occurred so we need to recover it
+            } else {
+                call_notif_cb = _dev_set_actions(dev_obj, DEV_FLAG_ACTION_PIPE_HALT_AND_FLUSH |
+                                                          DEV_FLAG_ACTION_DEFAULT_PIPE_FLUSH |
+                                                          DEV_FLAG_ACTION_DEFAULT_PIPE_DEQUEUE |
+                                                          DEV_FLAG_ACTION_SEND_GONE_EVENT);
+            }
+            USBH_EXIT_CRITICAL();
+            break;
+        }
+        case USBH_HUB_EVENT_PORT_DISABLED: {
+            USBH_ENTER_CRITICAL();
+            assert(dev_obj->dynamic.ref_count == 0);    //At this stage, the device should have been closed by all users
+            dev_obj->dynamic.flags.waiting_free = 1;
+            call_notif_cb = _dev_set_actions(dev_obj, DEV_FLAG_ACTION_FREE);
+            USBH_EXIT_CRITICAL();
+            break;
+        }
+        default:
+            return ESP_ERR_INVALID_ARG;
     }
-    return ESP_OK;
-}
-
-esp_err_t usbh_hub_dev_port_disabled(usb_device_handle_t dev_hdl)
-{
-    USBH_CHECK(dev_hdl != NULL, ESP_ERR_INVALID_ARG);
-    device_t *dev_obj = (device_t *)dev_hdl;
-
-    USBH_ENTER_CRITICAL();
-    assert(dev_obj->dynamic.ref_count == 0);    //At this stage, the device should have been closed by all users
-    dev_obj->dynamic.flags.waiting_free = 1;
-    bool call_notif_cb = _dev_set_actions(dev_obj, DEV_FLAG_ACTION_FREE);
-    USBH_EXIT_CRITICAL();
 
     if (call_notif_cb) {
-        ESP_LOGD(USBH_TAG, "Notif free");
         p_usbh_obj->constant.notif_cb(USB_NOTIF_SOURCE_USBH, false, p_usbh_obj->constant.notif_cb_arg);
     }
     return ESP_OK;