feat(usb_host): Apply review comments, add dconn test

peter-marcisovsky · peter-marcisovsky · commit 3cd4cc8ccb58 · 2025-10-15T17:17:32.000+02:00
diff --git a/host/usb/include/usb/usb_host.h b/host/usb/include/usb/usb_host.h
@@ -53,7 +53,7 @@ typedef enum {
 } usb_host_client_event_t;
 
 /**
- * @brief USB Host lib power management
+ * @brief USB Host lib power management timer type
  */
 typedef enum {
     USB_HOST_LIB_PM_SUSPEND_ONE_SHOT,               /**< USB Host lib power management -> Auto suspend one-shot timer */
@@ -292,25 +292,26 @@ esp_err_t usb_host_lib_root_port_suspend(void);
 esp_err_t usb_host_lib_root_port_resume(void);
 
 /**
- * @brief Set auto power management timer (auto suspend/resume)
+ * @brief Set auto power management timer
  *
- * - The function sets the auto suspend (resume) timer, used for global suspend (resume) of the root port
- * - The timer is a one-shot timer which expires after the set period, only if there is no activity on the USB Bus
+ * - The function sets the auto suspend timer, used for global suspend of the root port
+ * - The timer is either one-shot or periodic
+ * - The timerexpires after the set period, only if there is no activity on the USB Bus
  * - The timer resets (if enabled) every time, the usb_host_client_handle_events() handles any client events,
- *   or the usb_host_lib_handle_events() handles any host lib events, thus checking any activity
- *   on all the registered clients or inside the host lib
+ *   or the usb_host_lib_handle_events() handles any host lib events, thus checking any activity on all the
+ *   registered clients or inside the host lib
  * - Once the timer expires, an auto_pm_timer_cb() is called, which delivers USB Host lib event flags
  *
- * @note set the timer interval to 0, to disable the timer (in case NO auto suspend (resume) functionality is required)
+ * @note set the timer interval to 0, to disable the timer (in case NO auto suspend functionality is required anymore)
  * @note this function is not ISR safe
- * @param[in] pm_action Power management action (Suspend/Resume)
- * @param[in] auto_pm_interval_ms Interval after which the automatic suspend (resume) occurs (0 to disable)
+ * @param[in] timer_type Power management timer type (periodic/one-shot)
+ * @param[in] timer_interval_ms Interval after which a usb_host lib event flag is delivered (0 to disable running timer)
  * @return
  *    - ESP_OK: Timer set or stopped
  *    - ESP_ERR_INVALID_STATE: USB Host lib is not installed
  *    - ESP_FAIL: Timer was not configured correctly
  */
-esp_err_t usb_host_lib_set_auto_pm(usb_host_lib_pm_t pm_action, size_t auto_pm_interval_ms);
+esp_err_t usb_host_lib_set_auto_pm(usb_host_lib_pm_t timer_type, size_t timer_interval_ms);
 
 // ------------------------------------------------ Client Functions ---------------------------------------------------
 
diff --git a/host/usb/private_include/hub.h b/host/usb/private_include/hub.h
@@ -112,7 +112,7 @@ esp_err_t hub_uninstall(void);
  * @note This function should only be called from the Host Library task
  *
  * @return
- *    - ESP_OK: Hub driver started successfully
+ *    - ESP_OK: Root port has been powered on
  *    - ESP_ERR_INVALID_STATE: Hub driver is not installed, or root port is in other state than not powered
  */
 esp_err_t hub_root_start(void);
@@ -123,7 +123,7 @@ esp_err_t hub_root_start(void);
  * This will power OFF the root port
  *
  * @return
- *    - ESP_OK: Hub driver stopped successfully
+ *    - ESP_OK: Root port has been powered off
  *    - ESP_ERR_INVALID_STATE: Hub driver is not installed, or root port is in not powered state
  */
 esp_err_t hub_root_stop(void);
diff --git a/host/usb/src/hcd_dwc.c b/host/usb/src/hcd_dwc.c
@@ -1337,13 +1337,15 @@ static esp_err_t _port_cmd_bus_suspend(port_t *port)
     HCD_ENTER_CRITICAL();
 
     if (port->state != HCD_PORT_STATE_SUSPENDING) {
+        // Port state unexpectedly changed
         ret = ESP_ERR_INVALID_RESPONSE;
         goto exit;
     }
 
     // Here we are calling ll directly instead of hal, to allow the suspend/resume feature to be used as a managed usb
     // component in all active IDF releases (Currently IDF 5.4.x, IDF 5.5.x and IDF 6.0) and all it's minor releases
     // because the hal function usb_dwc_hal_port_check_if_suspended(port->hal) starts to be supported in IDF 5.5.2, (and IDF 5.4.2)
+    // TODO: use usb_dwc_hal_hprt_get_port_suspend() instead of usb_dwc_ll_hprt_get_port_suspend() when IDF 5.5 is EOL
 
     // Sanity check, the root port should have entered the suspended state after the SUSPEND_ENTRY_MS delay
     if (!usb_dwc_ll_hprt_get_port_suspend(port->hal->dev)) {
diff --git a/host/usb/src/usb_host.c b/host/usb/src/usb_host.c
@@ -159,7 +159,7 @@ typedef struct {
     struct {
         SemaphoreHandle_t event_sem;
         SemaphoreHandle_t mux_lock;
-        TimerHandle_t auto_pm_timer;    // Freertos timer used for automatic suspend/resume
+        TimerHandle_t auto_pm_timer;    // Freertos timer used for automatic power management
         usb_phy_handle_t phy_handle;    // Will be NULL if host library is installed with skip_phy_setup
         void *enum_client;              // Pointer to Enum driver (acting as a client). Used to reroute completed USBH control transfers
         void *hub_client;               // Pointer to External Hub driver (acting as a client). Used to reroute completed USBH control transfers. NULL, when External Hub Driver not available.
@@ -928,7 +928,7 @@ esp_err_t usb_host_lib_root_port_resume(void)
     return ret;
 }
 
-esp_err_t usb_host_lib_set_auto_pm(usb_host_lib_pm_t pm_action, size_t auto_pm_interval_ms)
+esp_err_t usb_host_lib_set_auto_pm(usb_host_lib_pm_t timer_type, size_t timer_interval_ms)
 {
     HOST_ENTER_CRITICAL();
     HOST_CHECK_FROM_CRIT(p_host_lib_obj != NULL, ESP_ERR_INVALID_STATE);
@@ -937,7 +937,7 @@ esp_err_t usb_host_lib_set_auto_pm(usb_host_lib_pm_t pm_action, size_t auto_pm_i
     TimerHandle_t suspend_tmr = p_host_lib_obj->constant.auto_pm_timer;
 
     // Interval is 0, stop the timer
-    if (auto_pm_interval_ms == 0) {
+    if (timer_interval_ms == 0) {
         if (xTimerIsTimerActive(suspend_tmr) == pdTRUE) {
             ESP_RETURN_ON_FALSE(xTimerStop(suspend_tmr, portMAX_DELAY), ESP_FAIL, USB_HOST_TAG, "Timer could not be stopped");
         }
@@ -946,7 +946,7 @@ esp_err_t usb_host_lib_set_auto_pm(usb_host_lib_pm_t pm_action, size_t auto_pm_i
     }
 
     // Set timer reload mode: One-Shot or periodic
-    switch (pm_action) {
+    switch (timer_type) {
     case USB_HOST_LIB_PM_SUSPEND_ONE_SHOT:
         vTimerSetReloadMode(suspend_tmr, pdFALSE);
         break;
@@ -958,11 +958,11 @@ esp_err_t usb_host_lib_set_auto_pm(usb_host_lib_pm_t pm_action, size_t auto_pm_i
     }
 
     // Change the timer period, this also starts the timer if stopped
-    ESP_RETURN_ON_FALSE(xTimerChangePeriod(suspend_tmr, pdMS_TO_TICKS(auto_pm_interval_ms), portMAX_DELAY),
+    ESP_RETURN_ON_FALSE(xTimerChangePeriod(suspend_tmr, pdMS_TO_TICKS(timer_interval_ms), portMAX_DELAY),
                         ESP_FAIL, USB_HOST_TAG, "Timer period could not be changed");
 
     ESP_LOGD(USB_HOST_TAG, "Auto suspend timer set to %d ms, %s mode and started",
-             auto_pm_interval_ms, ((pm_action == USB_HOST_LIB_PM_SUSPEND_ONE_SHOT) ? ("One-Shot") : ("Periodic")));
+             timer_interval_ms, ((timer_type == USB_HOST_LIB_PM_SUSPEND_ONE_SHOT) ? ("One-Shot") : ("Periodic")));
 
     return ESP_OK;
 }
diff --git a/host/usb/src/usbh.c b/host/usb/src/usbh.c
@@ -633,7 +633,7 @@ static inline void handle_prop_resume_event(device_t *dev_obj)
 
 static inline void handle_prop_all_idle_event(void)
 {
-    ESP_LOGD(USBH_TAG, "ALl devices idle");
+    ESP_LOGD(USBH_TAG, "All devices idle");
     usbh_event_data_t event_data = {
         .event = USBH_EVENT_ALL_IDLE,
     };
diff --git a/host/usb/test/target_test/common/dev_msc.c b/host/usb/test/target_test/common/dev_msc.c
@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2024-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -99,7 +99,7 @@ static const dev_msc_info_t dev_info = {
     .bInterfaceNumber = 0x00,
     .bAlternateSetting = 0x00,
     .in_ep_addr = 0x81,
-    .out_up_addr = 0x02,
+    .out_ep_addr = 0x02,
     .scsi_sector_size = 512,
 };
 
diff --git a/host/usb/test/target_test/common/dev_msc.h b/host/usb/test/target_test/common/dev_msc.h
@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2024-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -34,7 +34,7 @@ typedef struct {
     uint8_t bInterfaceNumber;
     uint8_t bAlternateSetting;
     uint8_t in_ep_addr;
-    uint8_t out_up_addr;
+    uint8_t out_ep_addr;
     unsigned int scsi_sector_size;
 } dev_msc_info_t;
 
diff --git a/host/usb/test/target_test/usb_host/main/msc_client_async_dconn.c b/host/usb/test/target_test/usb_host/main/msc_client_async_dconn.c
@@ -234,7 +234,7 @@ void msc_client_async_dconn_task(void *arg)
                                    msc_obj.dev_info->scsi_sector_size,
                                    msc_obj.test_param.msc_scsi_xfer_tag);
             xfer_out->num_bytes = sizeof(mock_msc_bulk_cbw_t);
-            xfer_out->bEndpointAddress = msc_obj.dev_info->out_up_addr;
+            xfer_out->bEndpointAddress = msc_obj.dev_info->out_ep_addr;
             TEST_ASSERT_EQUAL(ESP_OK, usb_host_transfer_submit(xfer_out));
             // Next stage set from transfer callback
             break;
diff --git a/host/usb/test/target_test/usb_host/main/msc_client_async_seq.c b/host/usb/test/target_test/usb_host/main/msc_client_async_seq.c
@@ -236,7 +236,7 @@ void msc_client_async_seq_task(void *arg)
                                    msc_obj.dev_info->scsi_sector_size,
                                    msc_obj.test_param.msc_scsi_xfer_tag);
             xfer_out->num_bytes = sizeof(mock_msc_bulk_cbw_t);
-            xfer_out->bEndpointAddress = msc_obj.dev_info->out_up_addr;
+            xfer_out->bEndpointAddress = msc_obj.dev_info->out_ep_addr;
             TEST_ASSERT_EQUAL(ESP_OK, usb_host_transfer_submit(xfer_out));
             // Test that an inflight transfer cannot be resubmitted
             TEST_ASSERT_EQUAL(ESP_ERR_NOT_FINISHED, usb_host_transfer_submit(xfer_out));
diff --git a/host/usb/test/target_test/usb_host/main/msc_client_suspend.c b/host/usb/test/target_test/usb_host/main/msc_client_suspend.c
@@ -318,7 +318,7 @@ void msc_client_async_suspend_resume_task(void *arg)
                                    msc_obj.dev_info->scsi_sector_size,
                                    msc_obj.test_param.msc_scsi_xfer_tag);
             xfer_out->num_bytes = sizeof(mock_msc_bulk_cbw_t);
-            xfer_out->bEndpointAddress = msc_obj.dev_info->out_up_addr;
+            xfer_out->bEndpointAddress = msc_obj.dev_info->out_ep_addr;
             TEST_ASSERT_EQUAL(ESP_OK, usb_host_transfer_submit(xfer_out));
             // Next stage set from transfer callback
             break;
diff --git a/host/usb/test/target_test/usb_host/main/msc_client_transfer_resume.c b/host/usb/test/target_test/usb_host/main/msc_client_transfer_resume.c
@@ -278,7 +278,7 @@ void msc_client_async_resume_by_transfer_task(void *arg)
                                    msc_obj.dev_info->scsi_sector_size,
                                    msc_obj.test_param.msc_scsi_xfer_tag);
             xfer_out->num_bytes = sizeof(mock_msc_bulk_cbw_t);
-            xfer_out->bEndpointAddress = msc_obj.dev_info->out_up_addr;
+            xfer_out->bEndpointAddress = msc_obj.dev_info->out_ep_addr;
             TEST_ASSERT_EQUAL(ESP_OK, usb_host_transfer_submit(xfer_out));
             // Test that pending transfers can't be resubmitted
             TEST_ASSERT_EQUAL(ESP_ERR_NOT_FINISHED, usb_host_transfer_submit(xfer_out));
diff --git a/host/usb/test/target_test/usb_host/main/test_usb_host_plugging.c b/host/usb/test/target_test/usb_host/main/test_usb_host_plugging.c
@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -49,7 +49,7 @@ TEST_CASE("Test USB Host sudden disconnection (no client)", "[usb_host][low_spee
                 connected = true;
                 printf("Forcing Sudden Disconnect\n");
                 // Trigger a disconnect by powering OFF the root port
-                usb_host_lib_set_root_port_power(false);
+                TEST_ASSERT_EQUAL(ESP_OK, usb_host_lib_set_root_port_power(false));
             }
         }
         if (event_flags & USB_HOST_LIB_EVENT_FLAGS_ALL_FREE) {
@@ -59,7 +59,92 @@ TEST_CASE("Test USB Host sudden disconnection (no client)", "[usb_host][low_spee
                 // Start next iteration
                 connected = false;
                 // Allow connections again by powering ON the root port
-                usb_host_lib_set_root_port_power(true);
+                TEST_ASSERT_EQUAL(ESP_OK, usb_host_lib_set_root_port_power(true));
+            } else {
+                break;
+            }
+        }
+    }
+}
+
+/*
+Test USB Host Library root port suspend with Sudden Disconnection Handling (no clients)
+Purpose:
+- Test that sudden disconnections after suspending are handled properly when there are no clients
+- Test that devices can reconnect after suspending with a sudden disconnection has been handled by the USB Host Library
+
+Procedure:
+- Install USB Host Library:
+- Wait for connection to occur and suspend the root port
+    - Let the usb_host lib to handle events caused by root port suspend call
+    - 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
+- Wait for connection to occur and suspend the root port
+    - Don't handle events (just mark the root port for suspending)
+    - 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_SUSPEND_ITERATIONS   6
+
+TEST_CASE("Test USB Host suspend + disconnection (no client)", "[usb_host][low_speed][full_speed][high_speed]")
+{
+    bool connected = false;
+    bool suspended = 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_connected;
+            TEST_ASSERT_EQUAL(ESP_OK, usb_host_lib_info(&lib_info_connected));
+            if (lib_info_connected.num_devices == 1) {
+                // We've just connected. Trigger root port suspend
+                connected = true;
+
+                printf("Suspending the root port\n");
+                // Suspend the root port
+                TEST_ASSERT_EQUAL(ESP_OK, usb_host_lib_root_port_suspend());
+
+                // In first test variation, continue to the end of the loop, to handle events connected to the port suspend
+                // In second variation don't handle port suspend, disconnect immediately
+                if (dconn_iter < TEST_DCONN_SUSPEND_ITERATIONS / 2) {
+                    continue;
+                }
+            }
+        }
+
+        if (!suspended) {
+            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 started suspend sequence. Trigger a disconnect
+                suspended = true;
+
+                if (dconn_iter < TEST_DCONN_SUSPEND_ITERATIONS / 2) {
+                    // Port suspend call has been handled, expect the port to be suspended
+                    TEST_ASSERT_MESSAGE(lib_info.root_port_suspended, "Root port should be suspended");
+                } else {
+                    // Port suspend call has not been handled, expect the port not to be suspended
+                    TEST_ASSERT_MESSAGE(!lib_info.root_port_suspended, "Root port should not be suspended");
+                }
+                printf("Forcing Sudden Disconnect\n");
+                // Trigger a disconnect by powering OFF the root port
+                TEST_ASSERT_EQUAL(ESP_OK, usb_host_lib_set_root_port_power(false));
+            }
+        }
+
+        if (event_flags & USB_HOST_LIB_EVENT_FLAGS_ALL_FREE) {
+            // The device has disconnected and it's disconnection has been handled
+            printf("Dconn iter %d done\n", dconn_iter);
+            if (++dconn_iter < TEST_DCONN_SUSPEND_ITERATIONS) {
+                // Start next iteration
+                connected = false;
+                suspended = false;
+                // Allow connections again by powering ON the root port
+                TEST_ASSERT_EQUAL(ESP_OK, usb_host_lib_set_root_port_power(true));
             } else {
                 break;
             }
diff --git a/host/usb/test/target_test/usb_host/main/test_usb_host_pm_timer.c b/host/usb/test/target_test/usb_host/main/test_usb_host_pm_timer.c
@@ -338,7 +338,7 @@ static void pm_timer_measure_task(void *arg)
 }
 
 /*
-Test USB Host auto Suspend/Resume timer event flags delivery
+Test USB Host auto Suspend timer event flags delivery
 
 Purpose:
 - Test the PM Timer event flags delivery upon timer settings
@@ -526,7 +526,7 @@ static void pm_timer_cancel_task(void *arg)
 }
 
 /*
-Test USB Host auto Suspend/Resume timer cancellation
+Test USB Host auto Suspend timer cancellation
 
 Purpose:
 - Test the PM Timer cancellation upon device disconnection or reconnection

Original file line number	Diff line number	Diff line change
`@@ -633,7 +633,7 @@ static inline void handle_prop_resume_event(device_t *dev_obj)`
`633`	`633`
`634`	`634`	`static inline void handle_prop_all_idle_event(void)`
`635`	`635`	`{`
`636`		`- ESP_LOGD(USBH_TAG, "ALl devices idle");`
	`636`	`+ ESP_LOGD(USBH_TAG, "All devices idle");`
`637`	`637`	`usbh_event_data_t event_data = {`
`638`	`638`	`.event = USBH_EVENT_ALL_IDLE,`
`639`	`639`	`};`