diff --git a/components/usb/hub.c b/components/usb/hub.c index 9aed57fad8..6c95c384a6 100644 --- a/components/usb/hub.c +++ b/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. + 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_REQ_PORT_RECOVER: p_hub_driver_obj->dynamic.flags.actions |= HUB_DRIVER_FLAG_ACTION_PORT_RECOVER; break; - case USBH_HUB_EVENT_DISABLE_PORT: - p_hub_driver_obj->dynamic.driver_state = HUB_DRIVER_STATE_ROOT_POWERED; - call_port_disable = true; - 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; diff --git a/components/usb/private_include/usbh.h b/components/usb/private_include/usbh.h index 4dd523c7d7..35f0562130 100644 --- a/components/usb/private_include/usbh.h +++ b/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_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_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_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 + * @param dev_hdl Device handle + * @param hub_event Hub event * @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 - * @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 ------------------- diff --git a/components/usb/test/usb_host/msc_client_async_dconn.c b/components/usb/test/usb_host/msc_client_async_dconn.c index 92de464abd..061b8acce7 100644 --- a/components/usb/test/usb_host/msc_client_async_dconn.c +++ b/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: diff --git a/components/usb/test/usb_host/msc_client_async_enum.c b/components/usb/test/usb_host/msc_client_async_enum.c index ccedce51fc..9f0bcb1ec6 100644 --- a/components/usb/test/usb_host/msc_client_async_enum.c +++ b/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: diff --git a/components/usb/test/usb_host/test_usb_host_plugging.c b/components/usb/test/usb_host/test_usb_host_plugging.c index c6c8e5d04c..4198b3205f 100644 --- a/components/usb/test/usb_host/test_usb_host_plugging.c +++ b/components/usb/test/usb_host/test_usb_host_plugging.c @@ -7,7 +7,6 @@ #include #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; } } diff --git a/components/usb/usbh.c b/components/usb/usbh.c index b50a8fec18..b1bda071a8 100644 --- a/components/usb/usbh.c +++ b/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,26 +916,41 @@ 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); + 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; } - 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); @@ -936,24 +958,6 @@ esp_err_t usbh_hub_mark_dev_gone(usb_device_handle_t dev_hdl) 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; -} - // ----------------- Enumeration Related ------------------- esp_err_t usbh_hub_enum_fill_dev_addr(usb_device_handle_t dev_hdl, uint8_t dev_addr)