From 3d32efc91ed26cff43597e716a520960f3944f03 Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 14 Feb 2025 11:47:13 +0100 Subject: [PATCH 01/11] refactor(enum): Removed parent_dev_hdl and parent_port_num, replaced with node uid --- host/usb/private_include/enum.h | 30 ++------- host/usb/private_include/hub.h | 43 ++++++------- host/usb/src/enum.c | 105 ++++++++------------------------ host/usb/src/hub.c | 62 +++++++++++++------ host/usb/src/usb_host.c | 16 +++-- 5 files changed, 101 insertions(+), 155 deletions(-) diff --git a/host/usb/private_include/enum.h b/host/usb/private_include/enum.h index 59b708d5..e4adc382 100644 --- a/host/usb/private_include/enum.h +++ b/host/usb/private_include/enum.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 */ @@ -44,31 +44,9 @@ typedef enum { } enum_event_t; typedef struct { - enum_event_t event; /**< Enumerator driver event */ - union { - struct { - unsigned int uid; /**< Device unique ID */ - usb_device_handle_t parent_dev_hdl; /**< Parent of the enumerating device */ - uint8_t parent_port_num; /**< Parent port number of the enumerating device */ - } started; /**< ENUM_EVENT_STARTED specific data */ - - struct { - usb_device_handle_t parent_dev_hdl; /**< Parent of the enumerating device */ - uint8_t parent_port_num; /**< Parent port number of the enumerating device */ - } reset_req; /**< ENUM_EVENT_RESET_REQUIRED specific data */ - - struct { - usb_device_handle_t parent_dev_hdl; /**< Parent of the enumerating device */ - uint8_t parent_port_num; /**< Parent port number of the enumerating device */ - usb_device_handle_t dev_hdl; /**< Handle of the enumerating device */ - uint8_t dev_addr; /**< Address of the enumerating device */ - } complete; /**< ENUM_EVENT_COMPLETED specific data */ - - struct { - usb_device_handle_t parent_dev_hdl; /**< Parent of the enumerating device */ - uint8_t parent_port_num; /**< Parent port number of the enumerating device */ - } canceled; /**< ENUM_EVENT_CANCELED specific data */ - }; + enum_event_t event; /**< Enumerator driver event */ + unsigned int node_uid; /**< Unique node ID */ + usb_device_handle_t dev_hdl; /**< Handle of the enumerating device */ } enum_event_data_t; // ---------------------------- Callbacks -------------------------------------- diff --git a/host/usb/private_include/hub.h b/host/usb/private_include/hub.h index e47e11c1..e3fe727f 100644 --- a/host/usb/private_include/hub.h +++ b/host/usb/private_include/hub.h @@ -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 */ @@ -131,66 +131,67 @@ esp_err_t hub_root_stop(void); /** * @brief Indicate to the Hub driver that a device's port can be recycled * - * The device connected to the port has been freed. The Hub driver can now recycled the port + * The device connected to the port has been freed. + * The Hub driver can now recycled the node and re-enable the port while it it still present. * * @note This function should only be called from the Host Library task * - * @param[in] parent_dev_hdl Parent device handle (is used to get the External Hub handle) - * @param[in] parent_port_num Parent number (is used to specify the External Port) - * @param[in] dev_uid Device's unique ID + * @param[in] node_uid Device's node unique ID * * @return * - ESP_OK: device's port can be recycled * - ESP_ERR_INVALID_STATE: Hub driver is not installed * - ESP_ERR_NOT_SUPPORTED: Recycling External Port is not available (External Hub support disabled), * or ext hub port error + * - ESP_ERR_NOT_FOUND: Device's node is not found */ -esp_err_t hub_port_recycle(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num, unsigned int dev_uid); +esp_err_t hub_node_recycle(unsigned int node_uid); /** - * @brief Reset the port + * @brief Reset the device in the port, related to the specific Device Tree node * * @note This function should only be called from the Host Library task * - * @param[in] parent_dev_hdl Parent device handle (is used to get the External Hub handle) - * @param[in] parent_port_num Parent number (is used to specify the External Port) + * @param[in] node_uid Device's node unique ID + * * @return - * - ESP_OK: Port reset successful + * - ESP_OK if device in port reset successful * - ESP_ERR_INVALID_STATE: Hub driver is not installed * - ESP_ERR_NOT_SUPPORTED: Resetting External Port is not available (External Hub support disabled), * or ext hub port error + * - ESP_ERR_NOT_FOUND: Device's node is not found */ -esp_err_t hub_port_reset(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num); +esp_err_t hub_node_reset(unsigned int node_uid); /** - * @brief Activate the port + * @brief Port, related to the specific Device Tree node, has an active device * * @note This function should only be called from the Host Library task * - * @param[in] parent_dev_hdl Parent device handle (is used to get the External Hub handle) - * @param[in] parent_port_num Parent number (is used to specify the External Port) + * @param[in] node_uid Device's node unique ID * * @return - * - ESP_OK: Port activated successfully - * - ESP_ERR_NOT_SUPPORTED: Activating External Port is not available (External Hub support disabled), + * - ESP_OK if Port, related to the Device Tree node was activated successfully + * - ESP_ERR_NOT_SUPPORTED if activating Port is not available (External Hub support disabled), * or ext hub port error + * - ESP_ERR_NOT_FOUND if Device's node is not found */ -esp_err_t hub_port_active(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num); +esp_err_t hub_node_active(unsigned int node_uid); /** - * @brief Disable the port + * @brief Disable the port, related to the specific Device Tree node * * @note This function should only be called from the Host Library task * - * @param[in] parent_dev_hdl Parent device handle (is used to get the External Hub handle) - * @param[in] parent_port_num Parent number (is used to specify the External Port) + * @param[in] node_uid Device's node unique ID * * @return * - ESP_OK: Port has been disabled without error * - ESP_ERR_INVALID_STATE: Port can't be disabled in current state * - ESP_ERR_NOT_SUPPORTED: This function is not support by the selected port + * - ESP_ERR_NOT_FOUND: Device's node is not found */ -esp_err_t hub_port_disable(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num); +esp_err_t hub_node_disable(unsigned int node_uid); /** * @brief Notify Hub driver that new device has been attached diff --git a/host/usb/src/enum.c b/host/usb/src/enum.c index 7d76e3ae..9d4f7cde 100644 --- a/host/usb/src/enum.c +++ b/host/usb/src/enum.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -132,12 +132,8 @@ typedef struct { typedef struct { struct { // Device related objects, initialized at start of a particular enumeration - unsigned int dev_uid; /**< Unique device ID being enumerated */ + unsigned int node_uid; /**< Unique node ID of device being enumerated */ usb_device_handle_t dev_hdl; /**< Handle of device being enumerated */ - // Parent info for optimization and more clean debug output - usb_device_handle_t parent_dev_hdl; /**< Device's parent handle */ - uint8_t parent_dev_addr; /**< Device's parent address */ - uint8_t parent_port_num; /**< Device's parent port number */ // Parameters, updated during enumeration enum_stage_t stage; /**< Current enumeration stage */ enum_device_params_t dev_params; /**< Parameters of device under enumeration */ @@ -253,12 +249,11 @@ static esp_err_t select_active_configuration(void) // User's request NOT to enumerate the USB device if (!enum_proceed) { - ESP_LOGW(ENUM_TAG, "[%d:%d] Abort request of enumeration process (%#x:%#x)", - p_enum_driver->single_thread.parent_dev_addr, - p_enum_driver->single_thread.parent_port_num, + ESP_LOGW(ENUM_TAG, "Canceled enumeration of the device %#x:%#x, configuration value=%d", dev_desc->idProduct, - dev_desc->idVendor); - enum_cancel(p_enum_driver->single_thread.dev_uid); + dev_desc->idVendor, + bConfigurationValue); + enum_cancel(p_enum_driver->single_thread.node_uid); return ESP_OK; } @@ -279,13 +274,9 @@ static esp_err_t second_reset_request(void) // Notify USB Host enum_event_data_t event_data = { .event = ENUM_EVENT_RESET_REQUIRED, - .reset_req = { - .parent_dev_hdl = p_enum_driver->single_thread.parent_dev_hdl, - .parent_port_num = p_enum_driver->single_thread.parent_port_num, - }, + .node_uid = p_enum_driver->single_thread.node_uid, }; p_enum_driver->constant.enum_event_cb(&event_data, p_enum_driver->constant.enum_event_cb_arg); - return ESP_OK; } @@ -739,10 +730,8 @@ static esp_err_t control_request(enum_stage_t stage) ret = usbh_dev_submit_ctrl_urb(p_enum_driver->single_thread.dev_hdl, p_enum_driver->constant.urb); if (ret != ESP_OK) { - ESP_LOGE(ENUM_TAG, "[%d:%d] Control transfer submit error (%#x), stage '%s'", - p_enum_driver->single_thread.parent_dev_addr, - p_enum_driver->single_thread.parent_port_num, - ret, + ESP_LOGE(ENUM_TAG, "Control transfer submit error %s, stage '%s'", + esp_err_to_name(ret), enum_stage_strings[stage]); } @@ -781,9 +770,7 @@ static esp_err_t control_response_handling(enum_stage_t stage) // Check Control IN transfer returned the expected correct number of bytes if (expected_num_bytes != 0 && expected_num_bytes != ctrl_xfer->actual_num_bytes) { - ESP_LOGW(ENUM_TAG, "[%d:%d] Unexpected (%d) device response length (expected %d)", - p_enum_driver->single_thread.parent_dev_addr, - p_enum_driver->single_thread.parent_port_num, + ESP_LOGW(ENUM_TAG, "Unexpected (%d) device response length (expected %d)", ctrl_xfer->actual_num_bytes, expected_num_bytes); if (ctrl_xfer->actual_num_bytes < expected_num_bytes) { @@ -847,9 +834,8 @@ static esp_err_t control_response_handling(enum_stage_t stage) static esp_err_t stage_cancel(void) { // There should be device under enumeration + const unsigned int node_uid = p_enum_driver->single_thread.node_uid; usb_device_handle_t dev_hdl = p_enum_driver->single_thread.dev_hdl; - usb_device_handle_t parent_dev_hdl = p_enum_driver->single_thread.parent_dev_hdl; - uint8_t parent_port_num = p_enum_driver->single_thread.parent_port_num; if (dev_hdl) { ESP_ERROR_CHECK(usbh_dev_enum_unlock(dev_hdl)); @@ -857,21 +843,15 @@ static esp_err_t stage_cancel(void) } // Clean up variables device from enumerator - p_enum_driver->single_thread.dev_uid = 0; + p_enum_driver->single_thread.node_uid = 0; p_enum_driver->single_thread.dev_hdl = NULL; - p_enum_driver->single_thread.parent_dev_hdl = NULL; - p_enum_driver->single_thread.parent_dev_addr = 0; - p_enum_driver->single_thread.parent_port_num = 0; p_enum_driver->constant.urb->transfer.context = NULL; // Propagate the event enum_event_data_t event_data = { .event = ENUM_EVENT_CANCELED, - .canceled = { - .parent_dev_hdl = parent_dev_hdl, - .parent_port_num = parent_port_num, - }, + .node_uid = node_uid, }; p_enum_driver->constant.enum_event_cb(&event_data, p_enum_driver->constant.enum_event_cb_arg); return ESP_OK; @@ -884,10 +864,8 @@ static esp_err_t stage_cancel(void) */ static esp_err_t stage_complete(void) { + unsigned int node_uid = p_enum_driver->single_thread.node_uid; usb_device_handle_t dev_hdl = p_enum_driver->single_thread.dev_hdl; - usb_device_handle_t parent_dev_hdl = p_enum_driver->single_thread.parent_dev_hdl; - uint8_t parent_dev_addr = p_enum_driver->single_thread.parent_dev_addr; - uint8_t parent_port_num = p_enum_driver->single_thread.parent_port_num; uint8_t dev_addr = 0; ESP_ERROR_CHECK(usbh_dev_get_addr(dev_hdl, &dev_addr)); @@ -896,11 +874,8 @@ static esp_err_t stage_complete(void) ESP_ERROR_CHECK(usbh_dev_close(dev_hdl)); // Release device from enumerator - p_enum_driver->single_thread.dev_uid = 0; + p_enum_driver->single_thread.node_uid = 0; p_enum_driver->single_thread.dev_hdl = NULL; - p_enum_driver->single_thread.parent_dev_hdl = NULL; - p_enum_driver->single_thread.parent_dev_addr = 0; - p_enum_driver->single_thread.parent_port_num = 0; // Release device from enumerator p_enum_driver->constant.urb->transfer.context = NULL; @@ -912,19 +887,12 @@ static esp_err_t stage_complete(void) // Increase device address to use new value during the next enumeration process get_next_dev_addr(); - ESP_LOGD(ENUM_TAG, "[%d:%d] Processing complete, new device address %d", - parent_dev_addr, - parent_port_num, - dev_addr); + ESP_LOGD(ENUM_TAG, "Processing complete, new device address %d", dev_addr); enum_event_data_t event_data = { .event = ENUM_EVENT_COMPLETED, - .complete = { - .dev_hdl = dev_hdl, - .dev_addr = dev_addr, - .parent_dev_hdl = parent_dev_hdl, - .parent_port_num = parent_port_num, - }, + .node_uid = node_uid, + .dev_hdl = dev_hdl, }; p_enum_driver->constant.enum_event_cb(&event_data, p_enum_driver->constant.enum_event_cb_arg); return ESP_OK; @@ -1005,10 +973,7 @@ static bool set_next_stage(bool last_stage_pass) // Find the next stage if (last_stage_pass) { // Last stage was successful - ESP_LOGD(ENUM_TAG, "[%d:%d] %s OK", - p_enum_driver->single_thread.parent_dev_addr, - p_enum_driver->single_thread.parent_port_num, - enum_stage_strings[last_stage]); + ESP_LOGD(ENUM_TAG, "%s OK", enum_stage_strings[last_stage]); // Get next stage if (last_stage == ENUM_STAGE_COMPLETE || last_stage == ENUM_STAGE_CANCEL) { @@ -1055,10 +1020,7 @@ static bool set_next_stage(bool last_stage_pass) break; default: // Stage is not allowed to failed. Cancel enumeration. - ESP_LOGE(ENUM_TAG, "[%d:%d] %s FAILED", - p_enum_driver->single_thread.parent_dev_addr, - p_enum_driver->single_thread.parent_port_num, - enum_stage_strings[last_stage]); + ESP_LOGE(ENUM_TAG, "%s FAILED", enum_stage_strings[last_stage]); next_stage = ENUM_STAGE_CANCEL; break; } @@ -1211,25 +1173,14 @@ esp_err_t enum_start(unsigned int uid) // Get device info usb_device_info_t dev_info; - uint8_t parent_dev_addr = 0; ESP_ERROR_CHECK(usbh_dev_get_info(dev_hdl, &dev_info)); - if (dev_info.parent.dev_hdl) { - ESP_ERROR_CHECK(usbh_dev_get_addr(dev_info.parent.dev_hdl, &parent_dev_addr)); - } - // Stage ENUM_STAGE_GET_SHORT_DEV_DESC - ESP_LOGD(ENUM_TAG, "[%d:%d] Start processing, device address %d", - parent_dev_addr, - dev_info.parent.port_num, - 0); + ESP_LOGD(ENUM_TAG, "Start processing device with address %d", 0); p_enum_driver->single_thread.stage = ENUM_STAGE_GET_SHORT_DEV_DESC; - p_enum_driver->single_thread.dev_uid = uid; + p_enum_driver->single_thread.node_uid = uid; p_enum_driver->single_thread.dev_hdl = dev_hdl; - p_enum_driver->single_thread.parent_dev_hdl = dev_info.parent.dev_hdl; - p_enum_driver->single_thread.parent_dev_addr = parent_dev_addr; - p_enum_driver->single_thread.parent_port_num = dev_info.parent.port_num; // Save device handle to the URB transfer context p_enum_driver->constant.urb->transfer.context = (void *) dev_hdl; // Device params @@ -1241,14 +1192,9 @@ esp_err_t enum_start(unsigned int uid) // Notify USB Host about starting enumeration process enum_event_data_t event_data = { .event = ENUM_EVENT_STARTED, - .started = { - .uid = uid, - .parent_dev_hdl = dev_info.parent.dev_hdl, - .parent_port_num = dev_info.parent.port_num, - }, + .node_uid = uid, }; p_enum_driver->constant.enum_event_cb(&event_data, p_enum_driver->constant.enum_event_cb_arg); - // Request processing p_enum_driver->constant.proc_req_cb(USB_PROC_REQ_SOURCE_ENUM, false, p_enum_driver->constant.proc_req_cb_arg); return ret; @@ -1276,10 +1222,7 @@ esp_err_t enum_cancel(unsigned int uid) p_enum_driver->single_thread.stage = ENUM_STAGE_CANCEL; - ESP_LOGV(ENUM_TAG, "[%d:%d] Cancel at %s", - p_enum_driver->single_thread.parent_dev_addr, - p_enum_driver->single_thread.parent_port_num, - enum_stage_strings[old_stage]); + ESP_LOGV(ENUM_TAG, "Cancel at %s", enum_stage_strings[old_stage]); if (stage_need_process(old_stage)) { // These stages are required to trigger processing in the enum_process() diff --git a/host/usb/src/hub.c b/host/usb/src/hub.c index 8af9d09b..70f4dff1 100644 --- a/host/usb/src/hub.c +++ b/host/usb/src/hub.c @@ -145,6 +145,20 @@ static bool root_port_callback(hcd_port_handle_t port_hdl, hcd_port_event_t port // ---------------------- Internal Logic ------------------------ +dev_tree_node_t *dev_tree_node_get_by_uid(unsigned int node_uid) +{ + dev_tree_node_t *dev_tree_node = NULL; + dev_tree_node_t *dev_tree_iter; + // Search the device tree nodes list for a device node with the specified parent + TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->single_thread.dev_nodes_tailq, tailq_entry) { + if (dev_tree_iter->uid == node_uid) { + dev_tree_node = dev_tree_iter; + break; + } + } + return dev_tree_node; +} + /** * @brief Creates new device tree node and propagate HUB_EVENT_CONNECTED event * @@ -667,20 +681,23 @@ esp_err_t hub_root_stop(void) return ret; } -esp_err_t hub_port_recycle(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num, unsigned int dev_uid) +esp_err_t hub_node_recycle(unsigned int node_uid) { HUB_DRIVER_ENTER_CRITICAL(); HUB_DRIVER_CHECK_FROM_CRIT(p_hub_driver_obj != NULL, ESP_ERR_INVALID_STATE); HUB_DRIVER_EXIT_CRITICAL(); esp_err_t ret; - if (parent_port_num == 0) { + dev_tree_node_t *dev_tree_node = dev_tree_node_get_by_uid(node_uid); + HUB_DRIVER_CHECK(dev_tree_node != NULL, ESP_ERR_NOT_FOUND); + + if (dev_tree_node->parent_dev_hdl == NULL) { ret = root_port_recycle(); } else { #if ENABLE_USB_HUBS ext_hub_handle_t ext_hub_hdl = NULL; - ext_hub_get_handle(parent_dev_hdl, &ext_hub_hdl); - ret = ext_hub_port_recycle(ext_hub_hdl, parent_port_num); + ext_hub_get_handle(dev_tree_node->parent_dev_hdl, &ext_hub_hdl); + ret = ext_hub_port_recycle(ext_hub_hdl, dev_tree_node->parent_port_num); if (ret != ESP_OK) { ESP_LOGE(HUB_DRIVER_TAG, "Ext hub port recycle error: %s", esp_err_to_name(ret)); } @@ -691,20 +708,23 @@ esp_err_t hub_port_recycle(usb_device_handle_t parent_dev_hdl, uint8_t parent_po } if (ret == ESP_OK) { - ESP_ERROR_CHECK(dev_tree_node_remove_by_parent(parent_dev_hdl, parent_port_num)); + ESP_ERROR_CHECK(dev_tree_node_remove_by_parent(dev_tree_node->parent_dev_hdl, dev_tree_node->parent_port_num)); } return ret; } -esp_err_t hub_port_reset(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num) +esp_err_t hub_node_reset(unsigned int node_uid) { HUB_DRIVER_ENTER_CRITICAL(); HUB_DRIVER_CHECK_FROM_CRIT(p_hub_driver_obj != NULL, ESP_ERR_INVALID_STATE); HUB_DRIVER_EXIT_CRITICAL(); esp_err_t ret; - if (parent_port_num == 0) { + dev_tree_node_t *dev_tree_node = dev_tree_node_get_by_uid(node_uid); + HUB_DRIVER_CHECK(dev_tree_node != NULL, ESP_ERR_NOT_FOUND); + + if (dev_tree_node->parent_dev_hdl == NULL) { ret = hcd_port_command(p_hub_driver_obj->constant.root_port_hdl, HCD_PORT_CMD_RESET); if (ret != ESP_OK) { ESP_LOGE(HUB_DRIVER_TAG, "Failed to issue root port reset"); @@ -714,8 +734,8 @@ esp_err_t hub_port_reset(usb_device_handle_t parent_dev_hdl, uint8_t parent_port } else { #if ENABLE_USB_HUBS ext_hub_handle_t ext_hub_hdl = NULL; - ext_hub_get_handle(parent_dev_hdl, &ext_hub_hdl); - ret = ext_hub_port_reset(ext_hub_hdl, parent_port_num); + ext_hub_get_handle(dev_tree_node->parent_dev_hdl, &ext_hub_hdl); + ret = ext_hub_port_reset(ext_hub_hdl, dev_tree_node->parent_port_num); if (ret != ESP_OK) { ESP_LOGE(HUB_DRIVER_TAG, "Ext hub port reset error: %s", esp_err_to_name(ret)); } @@ -727,19 +747,22 @@ esp_err_t hub_port_reset(usb_device_handle_t parent_dev_hdl, uint8_t parent_port return ret; } -esp_err_t hub_port_active(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num) +esp_err_t hub_node_active(unsigned int node_uid) { esp_err_t ret; - if (parent_port_num == 0) { + dev_tree_node_t *dev_tree_node = dev_tree_node_get_by_uid(node_uid); + HUB_DRIVER_CHECK(dev_tree_node != NULL, ESP_ERR_NOT_FOUND); + + if (dev_tree_node->parent_dev_hdl == NULL) { // Root port no need to be activated ret = ESP_OK; } else { #if ENABLE_USB_HUBS // External Hub port ext_hub_handle_t ext_hub_hdl = NULL; - ext_hub_get_handle(parent_dev_hdl, &ext_hub_hdl); - ret = ext_hub_port_active(ext_hub_hdl, parent_port_num); + ext_hub_get_handle(dev_tree_node->parent_dev_hdl, &ext_hub_hdl); + ret = ext_hub_port_active(ext_hub_hdl, dev_tree_node->parent_port_num); if (ret != ESP_OK) { ESP_LOGE(HUB_DRIVER_TAG, "Ext hub port activation error: %s", esp_err_to_name(ret)); } @@ -751,20 +774,23 @@ esp_err_t hub_port_active(usb_device_handle_t parent_dev_hdl, uint8_t parent_por return ret; } -esp_err_t hub_port_disable(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num) +esp_err_t hub_node_disable(unsigned int node_uid) { esp_err_t ret; - if (parent_port_num == 0) { + dev_tree_node_t *dev_tree_node = dev_tree_node_get_by_uid(node_uid); + HUB_DRIVER_CHECK(dev_tree_node != NULL, ESP_ERR_NOT_FOUND); + + if (dev_tree_node->parent_dev_hdl == NULL) { ret = root_port_disable(); } else { #if ENABLE_USB_HUBS // External Hub port ext_hub_handle_t ext_hub_hdl = NULL; - ext_hub_get_handle(parent_dev_hdl, &ext_hub_hdl); - ret = ext_hub_port_disable(ext_hub_hdl, parent_port_num); + ext_hub_get_handle(dev_tree_node->parent_dev_hdl, &ext_hub_hdl); + ret = ext_hub_port_disable(ext_hub_hdl, dev_tree_node->parent_port_num); #else - ESP_LOGW(HUB_DRIVER_TAG, "Activating External Port is not available (External Hub support disabled)"); + ESP_LOGW(HUB_DRIVER_TAG, "Disabling External Port is not available (External Hub support disabled)"); ret = ESP_ERR_NOT_SUPPORTED; #endif // ENABLE_USB_HUBS } diff --git a/host/usb/src/usb_host.c b/host/usb/src/usb_host.c index a7e774c1..64559b94 100644 --- a/host/usb/src/usb_host.c +++ b/host/usb/src/usb_host.c @@ -344,11 +344,9 @@ static void usbh_event_callback(usbh_event_data_t *event_data, void *arg) break; } case USBH_EVENT_DEV_FREE: { - // Let the Hub driver know that the device is free and its port can be recycled - // Port could be absent, no need to verify - hub_port_recycle(event_data->dev_free_data.parent_dev_hdl, - event_data->dev_free_data.port_num, - event_data->dev_free_data.dev_uid); + // Let the Hub driver know that the device is free and its node can be free and port re-enabled + // Node could be already freed on device disconnect (when clients still holding the device opened), no need to verify result + hub_node_recycle(event_data->dev_free_data.dev_uid); break; } case USBH_EVENT_ALL_FREE: { @@ -397,16 +395,16 @@ static void enum_event_callback(enum_event_data_t *event_data, void *arg) break; case ENUM_EVENT_RESET_REQUIRED: // Device may be gone, don't need to verify result - hub_port_reset(event_data->reset_req.parent_dev_hdl, event_data->reset_req.parent_port_num); + hub_node_reset(event_data->node_uid); break; case ENUM_EVENT_COMPLETED: // Notify port that device completed enumeration - hub_port_active(event_data->complete.parent_dev_hdl, event_data->complete.parent_port_num); + hub_node_active(event_data->node_uid); // Propagate a new device event - ESP_ERROR_CHECK(usbh_devs_new_dev_event(event_data->complete.dev_hdl)); + ESP_ERROR_CHECK(usbh_devs_new_dev_event(event_data->dev_hdl)); break; case ENUM_EVENT_CANCELED: - hub_port_disable(event_data->canceled.parent_dev_hdl, event_data->canceled.parent_port_num); + hub_node_disable(event_data->node_uid); break; default: abort(); // Should never occur From e7f6323f38158caf5f500cc80bb57db79be68747 Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 10:16:13 +0100 Subject: [PATCH 02/11] refactor(hub): Removed parent_dev_hdl from dev tree node structure --- host/usb/private_include/hub.h | 8 ++-- host/usb/src/hub.c | 87 +++++++++++++++++----------------- 2 files changed, 47 insertions(+), 48 deletions(-) diff --git a/host/usb/private_include/hub.h b/host/usb/private_include/hub.h index e3fe727f..7827174b 100644 --- a/host/usb/private_include/hub.h +++ b/host/usb/private_include/hub.h @@ -186,10 +186,10 @@ esp_err_t hub_node_active(unsigned int node_uid); * @param[in] node_uid Device's node unique ID * * @return - * - ESP_OK: Port has been disabled without error - * - ESP_ERR_INVALID_STATE: Port can't be disabled in current state - * - ESP_ERR_NOT_SUPPORTED: This function is not support by the selected port - * - ESP_ERR_NOT_FOUND: Device's node is not found + * - ESP_ERR_INVALID_STATE if Hub driver is not installed + * - ESP_ERR_NOT_SUPPORTED if the function is not support by the selected port + * - ESP_ERR_NOT_FOUND if device's tree node is not found by uid + * - ESP_OK if Port has been disabled without error */ esp_err_t hub_node_disable(unsigned int node_uid); diff --git a/host/usb/src/hub.c b/host/usb/src/hub.c index 70f4dff1..e42d71f1 100644 --- a/host/usb/src/hub.c +++ b/host/usb/src/hub.c @@ -69,9 +69,9 @@ typedef enum { */ struct dev_tree_node_s { TAILQ_ENTRY(dev_tree_node_s) tailq_entry; /**< Entry for the device tree node object tailq */ - unsigned int uid; /**< Device's unique ID */ - usb_device_handle_t parent_dev_hdl; /**< Device's parent handle */ - uint8_t parent_port_num; /**< Device's parent port number */ + unsigned int uid; /**< Device's unique ID */ + void* parent; /**< Device's parent context */ + uint8_t port_num; /**< Device's parent port number */ }; typedef struct dev_tree_node_s dev_tree_node_t; @@ -145,7 +145,7 @@ static bool root_port_callback(hcd_port_handle_t port_hdl, hcd_port_event_t port // ---------------------- Internal Logic ------------------------ -dev_tree_node_t *dev_tree_node_get_by_uid(unsigned int node_uid) +static dev_tree_node_t *dev_tree_node_get_by_uid(unsigned int node_uid) { dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; @@ -164,7 +164,7 @@ dev_tree_node_t *dev_tree_node_get_by_uid(unsigned int node_uid) * * @return esp_err_t */ -static esp_err_t dev_tree_node_new(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num, usb_speed_t speed) +static esp_err_t dev_tree_node_new(void* parent, uint8_t port_num, usb_speed_t speed) { esp_err_t ret; // Allocate memory for a new device tree node @@ -182,8 +182,8 @@ static esp_err_t dev_tree_node_new(usb_device_handle_t parent_dev_hdl, uint8_t p assert(dev_tree_node->uid != 0); // No overflow possible } - dev_tree_node->parent_dev_hdl = parent_dev_hdl; - dev_tree_node->parent_port_num = parent_port_num; + dev_tree_node->parent = parent; + dev_tree_node->port_num = port_num; // Initialize and register a new USBH Device with the assigned UID usbh_dev_params_t params = { @@ -191,8 +191,8 @@ static esp_err_t dev_tree_node_new(usb_device_handle_t parent_dev_hdl, uint8_t p .speed = speed, .root_port_hdl = p_hub_driver_obj->constant.root_port_hdl, // Always the same for all devices // TODO: IDF-10023 Move parent-child tree management responsibility to Hub Driver - .parent_dev_hdl = parent_dev_hdl, - .parent_port_num = parent_port_num, + .parent_dev_hdl = NULL, + .parent_port_num = port_num, }; ret = usbh_devs_add(¶ms); @@ -203,7 +203,7 @@ static esp_err_t dev_tree_node_new(usb_device_handle_t parent_dev_hdl, uint8_t p TAILQ_INSERT_TAIL(&p_hub_driver_obj->single_thread.dev_nodes_tailq, dev_tree_node, tailq_entry); - ESP_LOGD(HUB_DRIVER_TAG, "Device tree node (uid=%d): new", dev_tree_node->uid); + ESP_LOGD(HUB_DRIVER_TAG, "Device tree node (port %d, uid %d): new", dev_tree_node->port_num, dev_tree_node->uid); hub_event_data_t event_data = { .event = HUB_EVENT_CONNECTED, @@ -220,21 +220,21 @@ static esp_err_t dev_tree_node_new(usb_device_handle_t parent_dev_hdl, uint8_t p return ret; } -static esp_err_t dev_tree_node_reset_completed(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num) +static esp_err_t dev_tree_node_reset_completed(void* parent, uint8_t port_num) { dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; // Search the device tree nodes list for a device node with the specified parent TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->single_thread.dev_nodes_tailq, tailq_entry) { - if (dev_tree_iter->parent_dev_hdl == parent_dev_hdl && - dev_tree_iter->parent_port_num == parent_port_num) { + if (dev_tree_iter->parent == parent && + dev_tree_iter->port_num == port_num) { dev_tree_node = dev_tree_iter; break; } } if (dev_tree_node == NULL) { - ESP_LOGE(HUB_DRIVER_TAG, "Reset completed, but device tree node with port=%d not found", parent_port_num); + ESP_LOGE(HUB_DRIVER_TAG, "Reset completed, but device tree node (port %d) not found", port_num); return ESP_ERR_NOT_FOUND; } @@ -248,25 +248,25 @@ static esp_err_t dev_tree_node_reset_completed(usb_device_handle_t parent_dev_hd return ESP_OK; } -static esp_err_t dev_tree_node_dev_gone(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num) +static esp_err_t dev_tree_node_dev_gone(void* parent, uint8_t port_num) { dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; // Search the device tree nodes list for a device node with the specified parent TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->single_thread.dev_nodes_tailq, tailq_entry) { - if (dev_tree_iter->parent_dev_hdl == parent_dev_hdl && - dev_tree_iter->parent_port_num == parent_port_num) { + if (dev_tree_iter->parent == parent && + dev_tree_iter->port_num == port_num) { dev_tree_node = dev_tree_iter; break; } } if (dev_tree_node == NULL) { - ESP_LOGW(HUB_DRIVER_TAG, "Device tree node (parent_port=%d): not found", parent_port_num); + ESP_LOGW(HUB_DRIVER_TAG, "Device tree node (port %d): not found", port_num); return ESP_ERR_NOT_FOUND; } - ESP_LOGD(HUB_DRIVER_TAG, "Device tree node (uid=%d): device gone", dev_tree_node->uid); + ESP_LOGD(HUB_DRIVER_TAG, "Device tree node (port %d, uid=%d): device gone", port_num, dev_tree_node->uid); hub_event_data_t event_data = { .event = HUB_EVENT_DISCONNECTED, @@ -284,25 +284,25 @@ static esp_err_t dev_tree_node_dev_gone(usb_device_handle_t parent_dev_hdl, uint * * @return esp_err_t */ -static esp_err_t dev_tree_node_remove_by_parent(usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num) +static esp_err_t dev_tree_node_remove_by_parent(void* parent, uint8_t port_num) { dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; // Search the device tree nodes list for a device node with the specified parent TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->single_thread.dev_nodes_tailq, tailq_entry) { - if (dev_tree_iter->parent_dev_hdl == parent_dev_hdl && - dev_tree_iter->parent_port_num == parent_port_num) { + if (dev_tree_iter->parent == parent && + dev_tree_iter->port_num == port_num) { dev_tree_node = dev_tree_iter; break; } } if (dev_tree_node == NULL) { - ESP_LOGW(HUB_DRIVER_TAG, "Device tree node (parent_port=%d): not found", parent_port_num); + ESP_LOGW(HUB_DRIVER_TAG, "Device tree node (port %d): not found", port_num); return ESP_ERR_NOT_FOUND; } - ESP_LOGD(HUB_DRIVER_TAG, "Device tree node (uid=%d): freeing", dev_tree_node->uid); + ESP_LOGD(HUB_DRIVER_TAG, "Device tree node (port %d, uid %d): freeing", port_num, dev_tree_node->uid); TAILQ_REMOVE(&p_hub_driver_obj->single_thread.dev_nodes_tailq, dev_tree_node, tailq_entry); heap_caps_free(dev_tree_node); @@ -365,7 +365,7 @@ static void ext_port_event_callback(ext_port_hdl_t port_hdl, ext_port_event_data } } - if (dev_tree_node_new(event_data->connected.parent_dev_hdl, event_data->connected.parent_port_num, port_speed) != ESP_OK) { + if (dev_tree_node_new(ext_hub_hdl, event_data->connected.parent_port_num, port_speed) != ESP_OK) { ESP_LOGE(HUB_DRIVER_TAG, "Failed to add new downstream device"); goto new_ds_dev_err; } @@ -374,11 +374,11 @@ static void ext_port_event_callback(ext_port_hdl_t port_hdl, ext_port_event_data ext_hub_port_disable(ext_hub_hdl, event_data->connected.parent_port_num); break; case EXT_PORT_RESET_COMPLETED: - ESP_ERROR_CHECK(dev_tree_node_reset_completed(event_data->reset_completed.parent_dev_hdl, event_data->reset_completed.parent_port_num)); + ESP_ERROR_CHECK(dev_tree_node_reset_completed(ext_hub_hdl, event_data->reset_completed.parent_port_num)); break; case EXT_PORT_DISCONNECTED: // The node could be freed by now, no need to verify the result here - dev_tree_node_dev_gone(event_data->disconnected.parent_dev_hdl, event_data->disconnected.parent_port_num); + dev_tree_node_dev_gone(ext_hub_hdl, event_data->disconnected.parent_port_num); break; default: // Should never occur @@ -691,13 +691,12 @@ esp_err_t hub_node_recycle(unsigned int node_uid) dev_tree_node_t *dev_tree_node = dev_tree_node_get_by_uid(node_uid); HUB_DRIVER_CHECK(dev_tree_node != NULL, ESP_ERR_NOT_FOUND); - if (dev_tree_node->parent_dev_hdl == NULL) { + if (dev_tree_node->parent == NULL) { + // Root Hub Ports ret = root_port_recycle(); } else { #if ENABLE_USB_HUBS - ext_hub_handle_t ext_hub_hdl = NULL; - ext_hub_get_handle(dev_tree_node->parent_dev_hdl, &ext_hub_hdl); - ret = ext_hub_port_recycle(ext_hub_hdl, dev_tree_node->parent_port_num); + ret = ext_hub_port_recycle((ext_hub_handle_t) dev_tree_node->parent, dev_tree_node->port_num); if (ret != ESP_OK) { ESP_LOGE(HUB_DRIVER_TAG, "Ext hub port recycle error: %s", esp_err_to_name(ret)); } @@ -708,7 +707,7 @@ esp_err_t hub_node_recycle(unsigned int node_uid) } if (ret == ESP_OK) { - ESP_ERROR_CHECK(dev_tree_node_remove_by_parent(dev_tree_node->parent_dev_hdl, dev_tree_node->parent_port_num)); + ESP_ERROR_CHECK(dev_tree_node_remove_by_parent(dev_tree_node->parent, dev_tree_node->port_num)); } return ret; @@ -724,7 +723,7 @@ esp_err_t hub_node_reset(unsigned int node_uid) dev_tree_node_t *dev_tree_node = dev_tree_node_get_by_uid(node_uid); HUB_DRIVER_CHECK(dev_tree_node != NULL, ESP_ERR_NOT_FOUND); - if (dev_tree_node->parent_dev_hdl == NULL) { + if (dev_tree_node->parent == NULL) { ret = hcd_port_command(p_hub_driver_obj->constant.root_port_hdl, HCD_PORT_CMD_RESET); if (ret != ESP_OK) { ESP_LOGE(HUB_DRIVER_TAG, "Failed to issue root port reset"); @@ -733,9 +732,7 @@ esp_err_t hub_node_reset(unsigned int node_uid) } } else { #if ENABLE_USB_HUBS - ext_hub_handle_t ext_hub_hdl = NULL; - ext_hub_get_handle(dev_tree_node->parent_dev_hdl, &ext_hub_hdl); - ret = ext_hub_port_reset(ext_hub_hdl, dev_tree_node->parent_port_num); + ret = ext_hub_port_reset((ext_hub_handle_t) dev_tree_node->parent, dev_tree_node->port_num); if (ret != ESP_OK) { ESP_LOGE(HUB_DRIVER_TAG, "Ext hub port reset error: %s", esp_err_to_name(ret)); } @@ -749,20 +746,21 @@ esp_err_t hub_node_reset(unsigned int node_uid) esp_err_t hub_node_active(unsigned int node_uid) { + HUB_DRIVER_ENTER_CRITICAL(); + HUB_DRIVER_CHECK_FROM_CRIT(p_hub_driver_obj != NULL, ESP_ERR_INVALID_STATE); + HUB_DRIVER_EXIT_CRITICAL(); esp_err_t ret; dev_tree_node_t *dev_tree_node = dev_tree_node_get_by_uid(node_uid); HUB_DRIVER_CHECK(dev_tree_node != NULL, ESP_ERR_NOT_FOUND); - if (dev_tree_node->parent_dev_hdl == NULL) { + if (dev_tree_node->parent == NULL) { // Root port no need to be activated ret = ESP_OK; } else { #if ENABLE_USB_HUBS // External Hub port - ext_hub_handle_t ext_hub_hdl = NULL; - ext_hub_get_handle(dev_tree_node->parent_dev_hdl, &ext_hub_hdl); - ret = ext_hub_port_active(ext_hub_hdl, dev_tree_node->parent_port_num); + ret = ext_hub_port_active((ext_hub_handle_t) dev_tree_node->parent, dev_tree_node->port_num); if (ret != ESP_OK) { ESP_LOGE(HUB_DRIVER_TAG, "Ext hub port activation error: %s", esp_err_to_name(ret)); } @@ -776,19 +774,20 @@ esp_err_t hub_node_active(unsigned int node_uid) esp_err_t hub_node_disable(unsigned int node_uid) { + HUB_DRIVER_ENTER_CRITICAL(); + HUB_DRIVER_CHECK_FROM_CRIT(p_hub_driver_obj != NULL, ESP_ERR_INVALID_STATE); + HUB_DRIVER_EXIT_CRITICAL(); esp_err_t ret; dev_tree_node_t *dev_tree_node = dev_tree_node_get_by_uid(node_uid); HUB_DRIVER_CHECK(dev_tree_node != NULL, ESP_ERR_NOT_FOUND); - if (dev_tree_node->parent_dev_hdl == NULL) { + if (dev_tree_node->parent == NULL) { ret = root_port_disable(); } else { #if ENABLE_USB_HUBS // External Hub port - ext_hub_handle_t ext_hub_hdl = NULL; - ext_hub_get_handle(dev_tree_node->parent_dev_hdl, &ext_hub_hdl); - ret = ext_hub_port_disable(ext_hub_hdl, dev_tree_node->parent_port_num); + ret = ext_hub_port_disable((ext_hub_handle_t) dev_tree_node->parent, dev_tree_node->port_num); #else ESP_LOGW(HUB_DRIVER_TAG, "Disabling External Port is not available (External Hub support disabled)"); ret = ESP_ERR_NOT_SUPPORTED; From f2cf5e9a4f9e38f22dd47113c34976c24bf590f8 Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 10:19:10 +0100 Subject: [PATCH 03/11] refactor(ext_hub): Removed unused calls as hub driver doesn't have parent_dev_hdl --- host/usb/private_include/ext_hub.h | 13 ------------- host/usb/src/ext_hub.c | 8 -------- 2 files changed, 21 deletions(-) diff --git a/host/usb/private_include/ext_hub.h b/host/usb/private_include/ext_hub.h index a5f463e7..e78b219f 100644 --- a/host/usb/private_include/ext_hub.h +++ b/host/usb/private_include/ext_hub.h @@ -82,19 +82,6 @@ void *ext_hub_get_client(void); // -------------------------- External Hub API --------------------------------- -/** - * @brief Get External Hub device handle by USBH device handle - * - * @param[in] dev_hdl USBH device handle - * @param[out] ext_hub_hdl External Hub device handle - * - * @return - * - ESP_OK: External Hub device handle successfully obtained - * - ESP_ERR_INVALID_STATE: External Hub driver is not installed - * - ESP_ERR_NOT_FOUND: Device not found - */ -esp_err_t ext_hub_get_handle(usb_device_handle_t dev_hdl, ext_hub_handle_t *ext_hub_hdl); - /** * @brief Add new device * diff --git a/host/usb/src/ext_hub.c b/host/usb/src/ext_hub.c index 9ba5f7b9..4f804817 100644 --- a/host/usb/src/ext_hub.c +++ b/host/usb/src/ext_hub.c @@ -1215,14 +1215,6 @@ void *ext_hub_get_client(void) // -------------------------- External Hub API --------------------------------- // ----------------------------------------------------------------------------- -esp_err_t ext_hub_get_handle(usb_device_handle_t dev_hdl, ext_hub_handle_t *ext_hub_hdl) -{ - EXT_HUB_ENTER_CRITICAL(); - EXT_HUB_CHECK_FROM_CRIT(p_ext_hub_driver != NULL, ESP_ERR_INVALID_STATE); - EXT_HUB_EXIT_CRITICAL(); - return get_dev_by_hdl(dev_hdl, ext_hub_hdl); -} - static esp_err_t find_first_intf_desc(const usb_config_desc_t *config_desc, device_config_t *hub_config) { bool iface_found = false; From 564b02da5e4e6bf9fdb3e465944b865d8bf18acd Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 10:45:48 +0100 Subject: [PATCH 04/11] refactor(ext_hub): Added device speed getter to stop using parent_dev_hdl --- host/usb/private_include/ext_hub.h | 15 ++++++++++++ host/usb/src/ext_hub.c | 37 +++++++++++++++++------------- host/usb/src/hub.c | 9 ++++---- 3 files changed, 40 insertions(+), 21 deletions(-) diff --git a/host/usb/private_include/ext_hub.h b/host/usb/private_include/ext_hub.h index e78b219f..cd96d5e6 100644 --- a/host/usb/private_include/ext_hub.h +++ b/host/usb/private_include/ext_hub.h @@ -82,6 +82,21 @@ void *ext_hub_get_client(void); // -------------------------- External Hub API --------------------------------- +/** + * @brief Get the External Hub device USB speed + * + * @note Device should be in list of devices + * + * @paramp[in] ext_hub_hdl External Hub device handle + * @param[out] speed USB speed + * @return + * - ESP_ERR_NOT_ALLOWED if the External Hub driver is not installed + * - ESP_ERR_INVALID_ARG if the handle or speed is NULL + * - ESP_ERR_NOT_FOUND if the device is not found + * - ESP_OK if the speed was obtained + */ +esp_err_t ext_hub_get_speed(ext_hub_handle_t ext_hub_hdl, usb_speed_t *speed); + /** * @brief Add new device * diff --git a/host/usb/src/ext_hub.c b/host/usb/src/ext_hub.c index 4f804817..087184d4 100644 --- a/host/usb/src/ext_hub.c +++ b/host/usb/src/ext_hub.c @@ -779,36 +779,27 @@ static void device_free(ext_hub_dev_t *ext_hub_dev) heap_caps_free(ext_hub_dev); } -static esp_err_t get_dev_by_hdl(usb_device_handle_t dev_hdl, ext_hub_dev_t **ext_hub_hdl) +static bool dev_is_in_list(ext_hub_dev_t *ext_hub_dev) { - esp_err_t ret = ESP_OK; - // Go through the Hubs lists to find the hub with the specified device address - ext_hub_dev_t *found_hub = NULL; + bool is_in_list = false; ext_hub_dev_t *hub = NULL; EXT_HUB_ENTER_CRITICAL(); TAILQ_FOREACH(hub, &p_ext_hub_driver->dynamic.ext_hubs_pending_tailq, dynamic.tailq_entry) { - if (hub->constant.dev_hdl == dev_hdl) { - found_hub = hub; + if (hub == ext_hub_dev) { + is_in_list = true; goto exit; } } - TAILQ_FOREACH(hub, &p_ext_hub_driver->dynamic.ext_hubs_tailq, dynamic.tailq_entry) { - if (hub->constant.dev_hdl == dev_hdl) { - found_hub = hub; + if (hub == ext_hub_dev) { + is_in_list = true; goto exit; } } - exit: - if (found_hub == NULL) { - ret = ESP_ERR_NOT_FOUND; - } EXT_HUB_EXIT_CRITICAL(); - - *ext_hub_hdl = found_hub; - return ret; + return is_in_list; } static esp_err_t get_dev_by_addr(uint8_t dev_addr, ext_hub_dev_t **ext_hub_hdl) @@ -1294,6 +1285,20 @@ static esp_err_t find_first_intf_desc(const usb_config_desc_t *config_desc, devi return (iface_found) ? ESP_OK : ESP_ERR_NOT_FOUND; } +esp_err_t ext_hub_get_speed(ext_hub_handle_t ext_hub_hdl, usb_speed_t *speed) +{ + EXT_HUB_ENTER_CRITICAL(); + EXT_HUB_CHECK_FROM_CRIT(p_ext_hub_driver != NULL, ESP_ERR_NOT_ALLOWED); + EXT_HUB_EXIT_CRITICAL(); + EXT_HUB_CHECK(ext_hub_hdl != NULL && speed != NULL, ESP_ERR_INVALID_ARG); + EXT_HUB_CHECK(dev_is_in_list(ext_hub_hdl), ESP_ERR_NOT_FOUND); + ext_hub_dev_t *ext_hub_dev = (ext_hub_dev_t *)ext_hub_hdl; + usb_device_info_t dev_info; + ESP_ERROR_CHECK(usbh_dev_get_info(ext_hub_dev->constant.dev_hdl, &dev_info)); + *speed = dev_info.speed; + return ESP_OK; +} + esp_err_t ext_hub_new_dev(uint8_t dev_addr) { EXT_HUB_ENTER_CRITICAL(); diff --git a/host/usb/src/hub.c b/host/usb/src/hub.c index e42d71f1..c4455e04 100644 --- a/host/usb/src/hub.c +++ b/host/usb/src/hub.c @@ -352,11 +352,10 @@ static void ext_port_event_callback(ext_port_hdl_t port_hdl, ext_port_event_data goto new_ds_dev_err; } - // TODO: IDF-10023 Move responsibility of parent-child tree building to Hub Driver instead of USBH - usb_device_info_t parent_dev_info; - ESP_ERROR_CHECK(usbh_dev_get_info(event_data->connected.parent_dev_hdl, &parent_dev_info)); - if (parent_dev_info.speed == USB_SPEED_HIGH) { - if (port_speed != parent_dev_info.speed) { + usb_speed_t parent_speed; + ESP_ERROR_CHECK(ext_hub_get_speed(ext_hub_hdl, &parent_speed)); + if (parent_speed == USB_SPEED_HIGH) { + if (port_speed != parent_speed) { ESP_LOGE(HUB_DRIVER_TAG, "Connected device is %s, transaction translator (TT) is not supported", (char *[]) { "LS", "FS", "HS" From 1c6a8b862a7e0da5733390e7a7c511128eb7b7b8 Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 12:24:16 +0100 Subject: [PATCH 05/11] refactor(ext_port): Removed parent_dev_hdl and port_num from event --- host/usb/private_include/ext_port.h | 25 +------------------------ host/usb/src/ext_port.c | 24 +----------------------- host/usb/src/hub.c | 18 +++++++++--------- 3 files changed, 11 insertions(+), 56 deletions(-) diff --git a/host/usb/private_include/ext_port.h b/host/usb/private_include/ext_port.h index 791b58fd..64a17307 100644 --- a/host/usb/private_include/ext_port.h +++ b/host/usb/private_include/ext_port.h @@ -47,29 +47,6 @@ typedef enum { EXT_PORT_DISCONNECTED, /**< Port has a device disconnection event */ } ext_port_event_t; -/** - * @brief Event data object for External Port driver events - */ -typedef struct { - ext_port_event_t event; - union { - struct { - usb_device_handle_t parent_dev_hdl; /**< Ports' parent device handle */ - uint8_t parent_port_num; /**< Ports' parent port number */ - } connected; /**< EXT_PORT_CONNECTED event specific data */ - - struct { - usb_device_handle_t parent_dev_hdl; /**< Ports' parent device handle */ - uint8_t parent_port_num; /**< Ports' parent port number */ - } reset_completed; /**< EXT_PORT_RESET_COMPLETED event specific data */ - - struct { - usb_device_handle_t parent_dev_hdl; /**< Ports' parent device handle */ - uint8_t parent_port_num; /**< Ports' parent port number */ - } disconnected; /**< EXT_PORT_DISCONNECTED event specific data */ - }; -} ext_port_event_data_t; - typedef enum { EXT_PORT_PARENT_REQ_CONTROL = 0x01, /** Port requires action from the parent Hub */ EXT_PORT_PARENT_REQ_PROC_COMPLETED /** All ports were handled */ @@ -101,7 +78,7 @@ typedef void (*ext_port_cb_t)(void *user_arg); * * @note For the Hub Driver only */ -typedef void (*ext_port_event_cb_t)(ext_port_hdl_t port_hdl, ext_port_event_data_t *event_data, void *arg); +typedef void (*ext_port_event_cb_t)(ext_port_hdl_t port_hdl, ext_port_event_t event, void *arg); /** * @brief Callback used to indicate that the External Port driver requires a Hub class specific request diff --git a/host/usb/src/ext_port.c b/host/usb/src/ext_port.c index 603595a9..26157ae2 100644 --- a/host/usb/src/ext_port.c +++ b/host/usb/src/ext_port.c @@ -411,29 +411,7 @@ static void port_stop_handling(ext_port_t *ext_port) */ static void port_event(ext_port_t *ext_port, ext_port_event_t event) { - ext_port_event_data_t event_data = { - .event = event, - }; - switch (event) { - case EXT_PORT_CONNECTED: - event_data.connected.parent_dev_hdl = ext_port->constant.parent_dev_hdl; - event_data.connected.parent_port_num = ext_port->constant.port_num; - break; - case EXT_PORT_RESET_COMPLETED: - event_data.reset_completed.parent_dev_hdl = ext_port->constant.parent_dev_hdl; - event_data.reset_completed.parent_port_num = ext_port->constant.port_num; - break; - case EXT_PORT_DISCONNECTED: - event_data.disconnected.parent_dev_hdl = ext_port->constant.parent_dev_hdl; - event_data.disconnected.parent_port_num = ext_port->constant.port_num; - break; - default: - // Should never occur - abort(); - break; - } - - p_ext_port_driver->constant.event_cb((ext_port_hdl_t)ext_port, &event_data, p_ext_port_driver->constant.event_cb_arg); + p_ext_port_driver->constant.event_cb((ext_port_hdl_t)ext_port, event, p_ext_port_driver->constant.event_cb_arg); } /** diff --git a/host/usb/src/hub.c b/host/usb/src/hub.c index c4455e04..ec5aeb8a 100644 --- a/host/usb/src/hub.c +++ b/host/usb/src/hub.c @@ -337,18 +337,18 @@ static void ext_port_callback(void *user_arg) p_hub_driver_obj->constant.proc_req_cb(USB_PROC_REQ_SOURCE_HUB, false, p_hub_driver_obj->constant.proc_req_cb_arg); } -static void ext_port_event_callback(ext_port_hdl_t port_hdl, ext_port_event_data_t *event_data, void *arg) +static void ext_port_event_callback(ext_port_hdl_t port_hdl, ext_port_event_t event, void *arg) { + uint8_t port_num; ext_hub_handle_t ext_hub_hdl = (ext_hub_handle_t) ext_port_get_context(port_hdl); + ESP_ERROR_CHECK(ext_port_get_port_num(port_hdl, &port_num)); - switch (event_data->event) { + switch (event) { case EXT_PORT_CONNECTED: // First reset is done by ext_port logic usb_speed_t port_speed; - if (ext_hub_port_get_speed(ext_hub_hdl, - event_data->connected.parent_port_num, - &port_speed) != ESP_OK) { + if (ext_hub_port_get_speed(ext_hub_hdl, port_num, &port_speed) != ESP_OK) { goto new_ds_dev_err; } @@ -364,20 +364,20 @@ static void ext_port_event_callback(ext_port_hdl_t port_hdl, ext_port_event_data } } - if (dev_tree_node_new(ext_hub_hdl, event_data->connected.parent_port_num, port_speed) != ESP_OK) { + if (dev_tree_node_new(ext_hub_hdl, port_num, port_speed) != ESP_OK) { ESP_LOGE(HUB_DRIVER_TAG, "Failed to add new downstream device"); goto new_ds_dev_err; } break; new_ds_dev_err: - ext_hub_port_disable(ext_hub_hdl, event_data->connected.parent_port_num); + ext_hub_port_disable(ext_hub_hdl, port_num); break; case EXT_PORT_RESET_COMPLETED: - ESP_ERROR_CHECK(dev_tree_node_reset_completed(ext_hub_hdl, event_data->reset_completed.parent_port_num)); + ESP_ERROR_CHECK(dev_tree_node_reset_completed(ext_hub_hdl, port_num)); break; case EXT_PORT_DISCONNECTED: // The node could be freed by now, no need to verify the result here - dev_tree_node_dev_gone(ext_hub_hdl, event_data->disconnected.parent_port_num); + dev_tree_node_dev_gone(ext_hub_hdl, port_num); break; default: // Should never occur From 91bbf510cea3de6de96d31282ea93db749857edf Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 12:36:26 +0100 Subject: [PATCH 06/11] change(ext_port_test): Applied new ext_port_event callback --- .../ext_port/main/ext_port_common.c | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/host/usb/test/target_test/ext_port/main/ext_port_common.c b/host/usb/test/target_test/ext_port/main/ext_port_common.c index d1e0df57..7ca8e72e 100644 --- a/host/usb/test/target_test/ext_port/main/ext_port_common.c +++ b/host/usb/test/target_test/ext_port/main/ext_port_common.c @@ -72,6 +72,11 @@ const char *const ext_port_event_string[] = { "-> Disconnected", }; +typedef struct { + ext_port_hdl_t port_hdl; + ext_port_event_t event; +} ext_port_event_msg_t; + typedef struct { ext_port_hdl_t port_hdl; ext_port_parent_request_data_t data; @@ -89,10 +94,13 @@ static void test_ext_port_callback(void *user_arg) xSemaphoreGive(process_req_cb); } -static void test_ext_port_event_callback(ext_port_hdl_t port_hdl, ext_port_event_data_t *event_data, void *arg) +static void test_ext_port_event_callback(ext_port_hdl_t port_hdl, ext_port_event_t event, void *arg) { QueueHandle_t ext_port_event_queue = (QueueHandle_t)arg; - ext_port_event_data_t msg = *event_data; + ext_port_event_msg_t msg = { + .port_hdl = port_hdl, + .event = event, + }; xQueueSend(ext_port_event_queue, &msg, portMAX_DELAY); } @@ -118,17 +126,20 @@ static void test_wait_ext_port_process_request(void) TEST_ASSERT_EQUAL(pdTRUE, xSemaphoreTake(_process_cd_req, pdMS_TO_TICKS(EXT_PORT_PROC_CB_TIMEOUT_MS))); } -static void test_wait_ext_port_event(ext_port_event_t event) +static void test_wait_ext_port_event(ext_port_hdl_t port_hdl, ext_port_event_t event) { + uint8_t port_num; // Get the port event queue from the port's context variable QueueHandle_t ext_port_evt_queue = _ext_port_event_queue; TEST_ASSERT_NOT_NULL(ext_port_evt_queue); // Wait for port callback to send an event message - ext_port_event_data_t msg; + ext_port_event_msg_t msg; BaseType_t ret = xQueueReceive(ext_port_evt_queue, &msg, pdMS_TO_TICKS(EXT_PORT_EVENT_TIMEOUT_MS)); TEST_ASSERT_EQUAL_MESSAGE(pdPASS, ret, "External Hub port event not generated on time"); + TEST_ASSERT_EQUAL_MESSAGE(ESP_OK, ext_port_get_port_num(port_hdl, &port_num), "Port number not equal"); // Check the contents of that event message - printf("\tHub port event: %s\n", ext_port_event_string[msg.event]); + printf("\tHub port %d event: %s\n", port_num, ext_port_event_string[msg.event]); + TEST_ASSERT_EQUAL_MESSAGE(port_hdl, msg.port_hdl, "Unexpected External Hub port handle"); TEST_ASSERT_EQUAL_MESSAGE(event, msg.event, "Unexpected External Hub port event"); } @@ -175,7 +186,7 @@ void test_ext_port_setup(void) _ext_port_hub_req_queue = xQueueCreate(TEST_EXT_PORT_QUEUE_LEN, sizeof(ext_port_hub_request_msg_t)); TEST_ASSERT_NOT_NULL(_ext_port_hub_req_queue); // Create a queue for ext port event - _ext_port_event_queue = xQueueCreate(TEST_EXT_PORT_QUEUE_LEN, sizeof(ext_port_event_data_t)); + _ext_port_event_queue = xQueueCreate(TEST_EXT_PORT_QUEUE_LEN, sizeof(ext_port_event_msg_t)); TEST_ASSERT_NOT_NULL(_ext_port_event_queue); // Install External Port driver ext_port_driver_config_t ext_port_config = { @@ -379,7 +390,7 @@ usb_speed_t test_ext_port_connected(uint8_t port1, ext_port_hdl_t port_hdl) // Process the port TEST_ASSERT_EQUAL(ESP_OK, ext_port_process()); // Processing the port after clear reset should generate the connection event - test_wait_ext_port_event(EXT_PORT_CONNECTED); + test_wait_ext_port_event(port_hdl, EXT_PORT_CONNECTED); // Get device speed usb_speed_t dev_speed; TEST_ASSERT_EQUAL(ESP_OK, port_api->get_speed(port_hdl, &dev_speed)); @@ -440,7 +451,7 @@ void test_ext_port_imitate_disconnection(uint8_t port1, ext_port_hdl_t port_hdl) // Process the port TEST_ASSERT_EQUAL(ESP_OK, ext_port_process()); // The External Port driver should indicate that device has been disconnected and port disconnected - test_wait_ext_port_event(EXT_PORT_DISCONNECTED); + test_wait_ext_port_event(port_hdl, EXT_PORT_DISCONNECTED); } void test_ext_port_disable(uint8_t port1, ext_port_hdl_t port_hdl) @@ -463,7 +474,7 @@ void test_ext_port_disable(uint8_t port1, ext_port_hdl_t port_hdl) EXT_PORT_PARENT_REQ_CONTROL, USB_B_REQUEST_HUB_CLEAR_FEATURE)); // When port is not gone, it should create a DISCONNECTION event - test_wait_ext_port_event(EXT_PORT_DISCONNECTED); + test_wait_ext_port_event(port_hdl, EXT_PORT_DISCONNECTED); // Disable the port hub_port_disable(port1); // After completion, trigger the port processing @@ -500,7 +511,7 @@ void test_ext_port_gone(ext_port_hdl_t port_hdl, test_gone_flag_t flag) // Port is enabled, port gone returns ESP_ERR_NOT_FINISHED TEST_ASSERT_EQUAL(ESP_ERR_NOT_FINISHED, port_api->gone(port_hdl)); // Mark the connected port as gone should trigger disconnect event if the port has a device - test_wait_ext_port_event(EXT_PORT_DISCONNECTED); + test_wait_ext_port_event(port_hdl, EXT_PORT_DISCONNECTED); return; } // Port doesn't have a device, port gone returns ESP_OK From ba9e2a766b8d9b44ba99ed9c59ce52da298f5bbd Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 12:53:20 +0100 Subject: [PATCH 07/11] refactor(ext_port): Removed parent_dev_hdl from the port object --- host/usb/private_include/ext_port.h | 3 +- host/usb/src/ext_hub.c | 3 +- host/usb/src/ext_port.c | 182 ++++++++-------------------- 3 files changed, 53 insertions(+), 135 deletions(-) diff --git a/host/usb/private_include/ext_port.h b/host/usb/private_include/ext_port.h index 64a17307..9a66e903 100644 --- a/host/usb/private_include/ext_port.h +++ b/host/usb/private_include/ext_port.h @@ -106,8 +106,7 @@ typedef struct { */ typedef struct { void *context; /**< Ports' parent external Hub handle */ - usb_device_handle_t parent_dev_hdl; /**< Ports' parent device handle */ - uint8_t parent_port_num; /**< Ports' parent port number */ + uint8_t port_num; /**< Ports' parent port number */ uint16_t port_power_delay_ms; /**< Ports' Power on time to Power Good, ms */ } ext_port_config_t; diff --git a/host/usb/src/ext_hub.c b/host/usb/src/ext_hub.c index 087184d4..31c39408 100644 --- a/host/usb/src/ext_hub.c +++ b/host/usb/src/ext_hub.c @@ -416,8 +416,7 @@ static esp_err_t device_port_new(ext_hub_dev_t *ext_hub_dev, uint8_t port_idx) { ext_port_config_t port_config = { .context = (void *) ext_hub_dev, - .parent_dev_hdl = ext_hub_dev->constant.dev_hdl, - .parent_port_num = port_idx + 1, + .port_num = port_idx + 1, .port_power_delay_ms = ext_hub_dev->constant.hub_desc->bPwrOn2PwrGood * 2, }; diff --git a/host/usb/src/ext_port.c b/host/usb/src/ext_port.c index 26157ae2..a1ce7989 100644 --- a/host/usb/src/ext_port.c +++ b/host/usb/src/ext_port.c @@ -74,9 +74,6 @@ struct ext_port_s { uint8_t dev_reset_attempts; /**< Ports' device reset failure */ struct { - // Ports' parent info for optimisation and better debug output - usb_device_handle_t parent_dev_hdl; /**< Ports' parent device handle */ - uint8_t parent_dev_addr; /**< Ports' parent device bus address */ // Port related constant members void *context; /**< Ports' parent External Hub handle */ uint8_t port_num; /**< Ports' parent External Hub Port number */ @@ -388,8 +385,7 @@ static void port_stop_handling(ext_port_t *ext_port) } if (ext_port->action_flags) { // Port should be freed but has actions - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Port stop handling. Dropped actions 0x%"PRIx32"", - ext_port->constant.parent_dev_addr, + ESP_LOGD(EXT_PORT_TAG, "Port%d stop handling. Dropped actions 0x%"PRIx32"", ext_port->constant.port_num, ext_port->action_flags); } @@ -429,61 +425,6 @@ static ext_port_t *get_port_from_pending_list(void) return ext_port; } -/** - * @brief Allocates port object - * - * Allocates new por object with following parameters: - * - Port state: USB_PORT_STATE_NOT_CONFIGURED - * - Port device status: PORT_DEV_NOT_PRESENT - * - * @param[in] context Ports' parent hub handle - * @param[in] parent_dev_hdl Ports' parent device handle - * @param[in] parent_port_num Ports' parent port number - * @param[in] port_delay_ms Ports' Power on time to Power Good, ms - * @param[out] port_obj Port objects' pointer - * @return - * - ESP_ERR_INVALID_ARG: Unable to allocate the port object: parent hub handle and parent device handle must be not NULL - * - ESP_ERR_NO_MEM: Unable to allocate the port object: no memory - * - ESP_ERR_NOT_FINISHED: Unable to allocate the port object: parent device is not available - * - ESP_OK: Port object created successfully - */ -static esp_err_t port_alloc(void *context, usb_device_handle_t parent_dev_hdl, uint8_t parent_port_num, uint16_t port_delay_ms, ext_port_t **port_obj) -{ - uint8_t parent_dev_addr = 0; - EXT_PORT_CHECK(context != NULL && parent_dev_hdl != NULL, ESP_ERR_INVALID_ARG); - // This is the one exception from the requirement to use only the Ext Hub Driver API. - // TODO: IDF-10023 Move responsibility of parent-child tree building to Hub Driver instead of USBH - EXT_PORT_CHECK(usbh_dev_get_addr(parent_dev_hdl, &parent_dev_addr) == ESP_OK, ESP_ERR_NOT_FINISHED); - - ext_port_t *ext_port = heap_caps_calloc(1, sizeof(ext_port_t), MALLOC_CAP_DEFAULT); - if (ext_port == NULL) { - return ESP_ERR_NO_MEM; - } - - ext_port->constant.parent_dev_hdl = parent_dev_hdl; - ext_port->constant.parent_dev_addr = parent_dev_addr; - ext_port->constant.context = context; - ext_port->constant.port_num = parent_port_num; -#if (EXT_PORT_POWER_ON_CUSTOM_DELAY) - ext_port->constant.power_on_delay_ms = EXT_PORT_POWER_ON_CUSTOM_DELAY_MS; -#else - // We don't need any additional delay in case port_delay_ms == 0, because this usually means - // that parent Hub device has no power switches - ext_port->constant.power_on_delay_ms = port_delay_ms; -#endif // EXT_PORT_POWER_ON_CUSTOM_DELAY - - ext_port->state = USB_PORT_STATE_NOT_CONFIGURED; - ext_port->dev_state = PORT_DEV_NOT_PRESENT; - - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Port has been added (PwrOn2PwrGood=%d ms)", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num, - ext_port->constant.power_on_delay_ms); - - *port_obj = ext_port; - return ESP_OK; -} - /** * @brief Port object handling complete * @@ -503,8 +444,7 @@ static esp_err_t handle_complete(ext_port_t *ext_port) bool all_ports_were_handled = true; bool has_pending_ports = false; - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Port is %s", - ext_port->constant.parent_dev_addr, + ESP_LOGD(EXT_PORT_TAG, "Port%d is %s", ext_port->constant.port_num, ext_port->flags.is_gone ? "gone" : (ext_port->dev_state == PORT_DEV_PRESENT) ? "active" : "idle"); @@ -571,9 +511,7 @@ static bool handle_port_status(ext_port_t *ext_port) { bool need_processing = false; if (port_is_in_reset(ext_port)) { - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Port still in reset, wait and repeat get status...", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGD(EXT_PORT_TAG, "Port%d still in reset, wait and repeat get status...", ext_port->constant.port_num); port_request_status(ext_port); need_processing = true; } @@ -601,9 +539,7 @@ static void handle_port_connection(ext_port_t *ext_port) case USB_PORT_STATE_DISABLED: if (port_has_connection(ext_port)) { if (ext_port->dev_state == PORT_DEV_PRESENT) { - ESP_LOGE(EXT_PORT_TAG, "[%d:%d] Mismatch port state and device status", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGE(EXT_PORT_TAG, "Port%d mismatch state and device status", ext_port->constant.port_num); has_device = true; } else { // New device connected, flush reset attempts @@ -617,9 +553,7 @@ static void handle_port_connection(ext_port_t *ext_port) case USB_PORT_STATE_RESETTING: if (!port_has_connection(ext_port)) { if (ext_port->dev_state == PORT_DEV_PRESENT) { - ESP_LOGE(EXT_PORT_TAG, "[%d:%d] Failed to issue downstream port reset", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGE(EXT_PORT_TAG, "Port%d failed to issue reset", ext_port->constant.port_num); has_device = true; } else { ext_port->state = USB_PORT_STATE_DISCONNECTED; @@ -662,8 +596,7 @@ static bool handle_port_changes(ext_port_t *ext_port) need_processing = true; } else if (port_has_changed_from_enable(ext_port)) { // For more information, refer to section 11.8.1 Port Error of usb_2.0 specification - ESP_LOGE(EXT_PORT_TAG, "[%d:%d] Error on port (state=%d, dev=%d)", - ext_port->constant.parent_dev_addr, + ESP_LOGE(EXT_PORT_TAG, "Port%d error: state=%d, dev=%d", ext_port->constant.port_num, ext_port->state, ext_port->dev_state == PORT_DEV_PRESENT); @@ -711,9 +644,7 @@ static void handle_port_state(ext_port_t *ext_port) port_set_feature(ext_port, USB_FEATURE_PORT_RESET); need_handling = true; } else { - ESP_LOGE(EXT_PORT_TAG, "[%d:%d] Unable to reset the device", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGE(EXT_PORT_TAG, "Port%d unable to reset the device", ext_port->constant.port_num); } } break; @@ -744,9 +675,7 @@ static void handle_port_state(ext_port_t *ext_port) } else { // Port in resetting state and doesn't have connection // Error case, could be, when device was removed during port reset - ESP_LOGE(EXT_PORT_TAG, "[%d:%d] Device gone during port reset, recover port", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGE(EXT_PORT_TAG, "Port%d device gone during reset, recover port", ext_port->constant.port_num); new_state = USB_PORT_STATE_DISCONNECTED; } break; @@ -772,9 +701,7 @@ static void handle_port_state(ext_port_t *ext_port) } need_handling = true; } else { - ESP_LOGE(EXT_PORT_TAG, "[%d:%d] Enabled, but doesn't have connection", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGE(EXT_PORT_TAG, "Port%d enabled, but doesn't have connection", ext_port->constant.port_num); } } else { // If port was enabled, there should be an active device @@ -784,9 +711,7 @@ static void handle_port_state(ext_port_t *ext_port) ext_port->flags.waiting_recycle = 1; } else { // Error state - ESP_LOGE(EXT_PORT_TAG, "[%d:%d] Enabled, but doesn't have a device", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGE(EXT_PORT_TAG, "Port%d enabled, but doesn't have a device", ext_port->constant.port_num); new_state = USB_PORT_STATE_DISCONNECTED; } } @@ -798,7 +723,7 @@ static void handle_port_state(ext_port_t *ext_port) } if (curr_state != new_state) { - ESP_LOGD(EXT_PORT_TAG, "New state: %d", new_state); + ESP_LOGD(EXT_PORT_TAG, "Port%d state change: %d->%d", ext_port->constant.port_num, curr_state, new_state); ext_port->state = new_state; } @@ -821,11 +746,10 @@ static void handle_port_state(ext_port_t *ext_port) */ static void handle_port(ext_port_t *ext_port) { - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] change=0x%04x, status=0x%04x, state=%d", - ext_port->constant.parent_dev_addr, + ESP_LOGD(EXT_PORT_TAG, "Port%d [%04x.%04x], state=%d", ext_port->constant.port_num, - ext_port->status.wPortChange.val, ext_port->status.wPortStatus.val, + ext_port->status.wPortChange.val, ext_port->state); if (handle_port_changes(ext_port)) { @@ -850,8 +774,7 @@ static void handle_recycle(ext_port_t *ext_port) { assert(ext_port->flags.waiting_recycle == 1); - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Port recycle (state=%d, dev=%d)", - ext_port->constant.parent_dev_addr, + ESP_LOGD(EXT_PORT_TAG, "Port%d recycle (state=%d, dev=%d)", ext_port->constant.port_num, ext_port->state, ext_port->dev_state); @@ -890,8 +813,7 @@ static void handle_recycle(ext_port_t *ext_port) */ static void handle_disable(ext_port_t *ext_port) { - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Disable (state=%d, dev=%d)", - ext_port->constant.parent_dev_addr, + ESP_LOGD(EXT_PORT_TAG, "Port%d disable (state=%d, dev=%d)", ext_port->constant.port_num, ext_port->state, ext_port->dev_state); @@ -902,8 +824,7 @@ static void handle_disable(ext_port_t *ext_port) if (ext_port->state == USB_PORT_STATE_ENABLED) { if (port_has_connection(ext_port)) { - ESP_LOGE(EXT_PORT_TAG, "[%d:%d] Port disabled, reset attempts=%d", - ext_port->constant.parent_dev_addr, + ESP_LOGE(EXT_PORT_TAG, "Port%d disabled, reset attempts=%d", ext_port->constant.port_num, ext_port->dev_reset_attempts); @@ -939,34 +860,45 @@ static void handle_disable(ext_port_t *ext_port) * @param[in] port_cfg Port configuration * @param[out] port_hdl Port object handle * @return - * - ESP_ERR_INVALID_STATE: The External Port Driver has not been installed - * - ESP_ERR_INVALID_ARG: Unable to create the port, arguments couldn't be NULL - * - ESP_ERR_NO_MEM: Unable to allocate the port object: no memory - * - ESP_ERR_NOT_FINISHED: Unable to allocate the port object: parent device is not available - * - ESP_OK: Port has been created and added to the pending list + * - ESP_ERR_INVALID_STATE if the External Port Driver has not been installed + * - ESP_ERR_INVALID_ARG if unable to create the port, arguments couldn't be NULL + * - ESP_ERR_NO_MEM if unable to allocate the port object: no memory + * - ESP_OK if Port has been created and added to the pending list */ static esp_err_t port_new(void *port_cfg, void **port_hdl) { EXT_PORT_CHECK(p_ext_port_driver != NULL, ESP_ERR_INVALID_STATE); EXT_PORT_CHECK(port_cfg != NULL && port_hdl != NULL, ESP_ERR_INVALID_ARG); - ext_port_t *port = NULL; ext_port_config_t *config = (ext_port_config_t *)port_cfg; - esp_err_t ret = port_alloc(config->context, - config->parent_dev_hdl, - config->parent_port_num, - config->port_power_delay_ms, - &port); + // For the external port, context is a parent hub handle, cannot be NULL + EXT_PORT_CHECK(config->context != NULL, ESP_ERR_INVALID_ARG); - if (ret != ESP_OK) { - *port_hdl = NULL; - goto exit; + ext_port_t *ext_port = heap_caps_calloc(1, sizeof(ext_port_t), MALLOC_CAP_DEFAULT); + if (ext_port == NULL) { + return ESP_ERR_NO_MEM; } - port_set_actions(port, PORT_ACTION_HANDLE); - *port_hdl = (ext_port_hdl_t) port; -exit: - return ret; + ext_port->constant.context = config->context; + ext_port->constant.port_num = config->port_num; +#if (EXT_PORT_POWER_ON_CUSTOM_DELAY) + ext_port->constant.power_on_delay_ms = EXT_PORT_POWER_ON_CUSTOM_DELAY_MS; +#else + // We don't need any additional delay in case port_power_delay_ms == 0, because this usually means + // that parent Hub device has no power switches + ext_port->constant.power_on_delay_ms = config->port_power_delay_ms; +#endif // EXT_PORT_POWER_ON_CUSTOM_DELAY + + ext_port->state = USB_PORT_STATE_NOT_CONFIGURED; + ext_port->dev_state = PORT_DEV_NOT_PRESENT; + + ESP_LOGD(EXT_PORT_TAG, "Port%d has been added (PwrOn2PwrGood=%d ms)", + ext_port->constant.port_num, + ext_port->constant.power_on_delay_ms); + + port_set_actions(ext_port, PORT_ACTION_HANDLE); + *port_hdl = (ext_port_hdl_t) ext_port; + return ESP_OK; } /** @@ -1016,9 +948,7 @@ static esp_err_t port_reset(void *port_hdl) EXT_PORT_CHECK(port_has_connection(ext_port), ESP_ERR_INVALID_STATE); - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Port reset request", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGD(EXT_PORT_TAG, "Port%d reset request", ext_port->constant.port_num); // Reset can be triggered only when port is enabled EXT_PORT_CHECK(ext_port->state == USB_PORT_STATE_ENABLED, ESP_ERR_INVALID_STATE); @@ -1084,9 +1014,7 @@ static esp_err_t port_active(void *port_hdl) EXT_PORT_CHECK(port_hdl != NULL, ESP_ERR_INVALID_ARG); ext_port_t *ext_port = (ext_port_t *) port_hdl; - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Port has an enumerated device", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGD(EXT_PORT_TAG, "Port%d has an enumerated device", ext_port->constant.port_num); ext_port->flags.has_enum_device = 1; @@ -1111,9 +1039,7 @@ static esp_err_t port_disable(void *port_hdl) EXT_PORT_CHECK(port_hdl != NULL, ESP_ERR_INVALID_ARG); ext_port_t *ext_port = (ext_port_t *) port_hdl; - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Disable", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGD(EXT_PORT_TAG, "Port%d disable", ext_port->constant.port_num); EXT_PORT_CHECK(ext_port->state == USB_PORT_STATE_ENABLED, ESP_ERR_INVALID_STATE); @@ -1141,9 +1067,7 @@ static esp_err_t port_delete(void *port_hdl) assert(ext_port->dev_state == PORT_DEV_NOT_PRESENT); // Port should not have a device assert(ext_port->flags.in_pending_list == 0); // Port should not be in pending list - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Freeing", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num); + ESP_LOGD(EXT_PORT_TAG, "Port%d freeing", ext_port->constant.port_num); heap_caps_free(ext_port); @@ -1168,8 +1092,7 @@ static esp_err_t port_gone(void *port_hdl) EXT_PORT_CHECK(port_hdl != NULL, ESP_ERR_INVALID_ARG); ext_port_t *ext_port = (ext_port_t *)port_hdl; - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Port is gone (state=%d, dev=%d)", - ext_port->constant.parent_dev_addr, + ESP_LOGD(EXT_PORT_TAG, "Port%d is gone (state=%d, dev=%d)", ext_port->constant.port_num, ext_port->state, ext_port->dev_state); @@ -1368,10 +1291,7 @@ esp_err_t ext_port_process(void) while (action_flags) { // Keep processing until all port's action have been handled - ESP_LOGD(EXT_PORT_TAG, "[%d:%d] Processing actions 0x%"PRIx32"", - ext_port->constant.parent_dev_addr, - ext_port->constant.port_num, - action_flags); + ESP_LOGD(EXT_PORT_TAG, "Port%d actions 0x%"PRIx32"", ext_port->constant.port_num, action_flags); if (action_flags & PORT_ACTION_HANDLE) { handle_port(ext_port); From 911af877214e7c3c37911e536fa79c33b8aeaa29 Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 12:56:41 +0100 Subject: [PATCH 08/11] change(ext_port_test): Applied new port alloc configuration (without parent_dev_hdl) --- .../target_test/ext_port/main/test_ext_port.c | 45 ++++++++----------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/host/usb/test/target_test/ext_port/main/test_ext_port.c b/host/usb/test/target_test/ext_port/main/test_ext_port.c index a81fe5bb..4cbd95a3 100644 --- a/host/usb/test/target_test/ext_port/main/test_ext_port.c +++ b/host/usb/test/target_test/ext_port/main/test_ext_port.c @@ -92,9 +92,8 @@ TEST_CASE("Port: disconnected", "[low_speed][full_speed][high_speed]") TEST_ASSERT_TRUE(TEST_PORT_NUM_EMPTY <= port_num); // Create External Port ext_port_config_t port_config = { - .context = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_dev_hdl = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_port_num = TEST_PORT_NUM_EMPTY, + .context = (void *) hub_get_context(), + .port_num = TEST_PORT_NUM_EMPTY, .port_power_delay_ms = hub_get_port_poweron_delay_ms(), }; ext_port_hdl_t port_hdl = test_ext_port_new(&port_config); @@ -136,9 +135,8 @@ TEST_CASE("Port: enumerate child device Low-speed", "[ext_port][low_speed]") TEST_ASSERT_TRUE(TEST_PORT_NUM_DEVICE_LS <= port_num); // Create External Port ext_port_config_t port_config = { - .context = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_dev_hdl = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_port_num = TEST_PORT_NUM_DEVICE_LS, + .context = (void *) hub_get_context(), + .port_num = TEST_PORT_NUM_DEVICE_LS, .port_power_delay_ms = hub_get_port_poweron_delay_ms(), }; ext_port_hdl_t port_hdl = test_ext_port_new(&port_config); @@ -193,9 +191,8 @@ TEST_CASE("Port: enumerate child device Full-speed", "[ext_port][full_speed]") TEST_ASSERT_TRUE(TEST_PORT_NUM_DEVICE_FSHS <= port_num); // Create External Port ext_port_config_t port_config = { - .context = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_dev_hdl = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_port_num = TEST_PORT_NUM_DEVICE_FSHS, + .context = (void *) hub_get_context(), + .port_num = TEST_PORT_NUM_DEVICE_FSHS, .port_power_delay_ms = hub_get_port_poweron_delay_ms(), }; ext_port_hdl_t port_hdl = test_ext_port_new(&port_config); @@ -250,9 +247,8 @@ TEST_CASE("Port: enumerate child device High-speed", "[ext_port][high_speed]") TEST_ASSERT_TRUE(TEST_PORT_NUM_DEVICE_FSHS <= port_num); // Create External Port ext_port_config_t port_config = { - .context = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_dev_hdl = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_port_num = TEST_PORT_NUM_DEVICE_FSHS, + .context = (void *) hub_get_context(), + .port_num = TEST_PORT_NUM_DEVICE_FSHS, .port_power_delay_ms = hub_get_port_poweron_delay_ms(), }; ext_port_hdl_t port_hdl = test_ext_port_new(&port_config); @@ -307,9 +303,8 @@ TEST_CASE("Port: recycle", "[ext_port][full_speed][high_speed]") TEST_ASSERT_TRUE(TEST_PORT_NUM_DEVICE_FSHS <= port_num); // Create External Port ext_port_config_t port_config = { - .context = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_dev_hdl = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_port_num = TEST_PORT_NUM_DEVICE_FSHS, + .context = (void *) hub_get_context(), + .port_num = TEST_PORT_NUM_DEVICE_FSHS, .port_power_delay_ms = hub_get_port_poweron_delay_ms(), }; ext_port_hdl_t port_hdl = test_ext_port_new(&port_config); @@ -352,9 +347,8 @@ TEST_CASE("Port: recycle when port is gone", "[ext_port][low_speed][full_speed][ TEST_ASSERT_TRUE(TEST_PORT_NUM_DEVICE_FSHS <= port_num); // Create External Port ext_port_config_t port_config = { - .context = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_dev_hdl = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_port_num = TEST_PORT_NUM_DEVICE_FSHS, + .context = (void *) hub_get_context(), + .port_num = TEST_PORT_NUM_DEVICE_FSHS, .port_power_delay_ms = hub_get_port_poweron_delay_ms(), }; ext_port_hdl_t port_hdl = test_ext_port_new(&port_config); @@ -397,9 +391,8 @@ TEST_CASE("Port: disable", "[ext_port][full_speed][high_speed]") TEST_ASSERT_TRUE(TEST_PORT_NUM_DEVICE_FSHS <= port_num); // Create External Port ext_port_config_t port_config = { - .context = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_dev_hdl = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_port_num = TEST_PORT_NUM_DEVICE_FSHS, + .context = (void *) hub_get_context(), + .port_num = TEST_PORT_NUM_DEVICE_FSHS, .port_power_delay_ms = hub_get_port_poweron_delay_ms(), }; ext_port_hdl_t port_hdl = test_ext_port_new(&port_config); @@ -440,9 +433,8 @@ TEST_CASE("Port: gone in state - powered on", "[ext_port][full_speed][high_speed TEST_ASSERT_TRUE(TEST_PORT_NUM_DEVICE_FSHS <= port_num); // Create External Port ext_port_config_t port_config = { - .context = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_dev_hdl = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_port_num = TEST_PORT_NUM_DEVICE_FSHS, + .context = (void *) hub_get_context(), + .port_num = TEST_PORT_NUM_DEVICE_FSHS, .port_power_delay_ms = hub_get_port_poweron_delay_ms(), }; ext_port_hdl_t port_hdl = test_ext_port_new(&port_config); @@ -478,9 +470,8 @@ TEST_CASE("Port: gone in state - enabled", "[ext_port][full_speed][high_speed]") TEST_ASSERT_TRUE(TEST_PORT_NUM_DEVICE_FSHS <= port_num); // Create External Port ext_port_config_t port_config = { - .context = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_dev_hdl = (void *) hub_get_context() /* use any before IDF-10023 */, - .parent_port_num = TEST_PORT_NUM_DEVICE_FSHS, + .context = (void *) hub_get_context(), + .port_num = TEST_PORT_NUM_DEVICE_FSHS, .port_power_delay_ms = hub_get_port_poweron_delay_ms(), }; ext_port_hdl_t port_hdl = test_ext_port_new(&port_config); From b3f4109cbecb03f63d1b3f84587c98b25ecffd37 Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 14:00:43 +0100 Subject: [PATCH 09/11] refactor(usbh): Moved parent information from usbh to hub driver --- host/usb/include/usb/usb_types_stack.h | 8 +++--- host/usb/private_include/ext_hub.h | 15 +++++++++++ host/usb/private_include/hub.h | 16 ++++++++++++ host/usb/private_include/usbh.h | 29 +++++++++++---------- host/usb/src/ext_hub.c | 12 +++++++++ host/usb/src/hub.c | 32 +++++++++++++++++++---- host/usb/src/usb_host.c | 10 ++++++++ host/usb/src/usbh.c | 35 +++++++++----------------- 8 files changed, 111 insertions(+), 46 deletions(-) diff --git a/host/usb/include/usb/usb_types_stack.h b/host/usb/include/usb/usb_types_stack.h index bb2c2906..002dc0c6 100644 --- a/host/usb/include/usb/usb_types_stack.h +++ b/host/usb/include/usb/usb_types_stack.h @@ -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 */ @@ -74,8 +74,10 @@ typedef bool (*usb_host_enum_filter_cb_t)(const usb_device_desc_t *dev_desc, uin * @brief Parent device information */ typedef struct { - usb_device_handle_t dev_hdl; /**< Device's parent handle */ - uint8_t port_num; /**< Device's parent port number */ + // IDF-12502: Remove the parent dev_hdl from parent_info structure (breaking change) + usb_device_handle_t dev_hdl __attribute__((deprecated)); /**< Device's parent handle: Deprecated, as dev_hdl only valid when device was opened via usbh_devs_open() */ + uint8_t dev_addr; /**< Device's parent bus address */ + uint8_t port_num; /**< Device's parent port number */ } usb_parent_dev_info_t; /** diff --git a/host/usb/private_include/ext_hub.h b/host/usb/private_include/ext_hub.h index cd96d5e6..42e1e1d5 100644 --- a/host/usb/private_include/ext_hub.h +++ b/host/usb/private_include/ext_hub.h @@ -97,6 +97,21 @@ void *ext_hub_get_client(void); */ esp_err_t ext_hub_get_speed(ext_hub_handle_t ext_hub_hdl, usb_speed_t *speed); +/** + * @brief Get the External Hub device USB address + * + * @note Device should be in list of devices + * + * @paramp[in] ext_hub_hdl External Hub device handle + * @param[out] addr USB address + * @return + * - ESP_ERR_NOT_ALLOWED if the External Hub driver is not installed + * - ESP_ERR_INVALID_ARG if the handle or addr is NULL + * - ESP_ERR_NOT_FOUND if the device is not found + * - ESP_OK if the address was obtained + */ +esp_err_t ext_hub_get_dev_addr(ext_hub_handle_t ext_hub_hdl, uint8_t *addr); + /** * @brief Add new device * diff --git a/host/usb/private_include/hub.h b/host/usb/private_include/hub.h index 7827174b..9952895e 100644 --- a/host/usb/private_include/hub.h +++ b/host/usb/private_include/hub.h @@ -193,6 +193,22 @@ esp_err_t hub_node_active(unsigned int node_uid); */ esp_err_t hub_node_disable(unsigned int node_uid); +/** + * @brief Get the node information of the device + * + * @note This function should only be called from the Host Library task + * + * @param[in] node_uid Device's node unique ID + * @param[out] info Device information + * + * @return + * - ESP_ERR_INVALID_STATE if the Hub driver is not installed + * - ESP_ERR_INVALID_ARG if the info pointer is NULL + * - ESP_ERR_NOT_FOUND if the device node is not found + * - ESP_OK if device's information obtained successfully + */ +esp_err_t hub_node_get_info(unsigned int node_uid, usb_parent_dev_info_t *info); + /** * @brief Notify Hub driver that new device has been attached * diff --git a/host/usb/private_include/usbh.h b/host/usb/private_include/usbh.h index a07ff447..daa942f0 100644 --- a/host/usb/private_include/usbh.h +++ b/host/usb/private_include/usbh.h @@ -269,21 +269,6 @@ esp_err_t usbh_devs_add(usbh_dev_params_t *params); */ esp_err_t usbh_devs_remove(unsigned int uid); -/** - * @brief Get a device's connection information - * - * @note Can be called without opening the device - * - * @param[in] uid Unique ID assigned to the device - * @param[out] parent_info Parent device handle - * - * @return - * - ESP_OK: Device parent info obtained successfully - * - ESP_ERR_INVALID_ARG: Invalid argument - * - ESP_ERR_NOT_FOUND: Device with provided uid not found - */ -esp_err_t usbh_devs_get_parent_info(unsigned int uid, usb_parent_dev_info_t *parent_info); - /** * @brief Mark that all devices should be freed at the next possible opportunity * @@ -342,6 +327,20 @@ esp_err_t usbh_devs_new_dev_event(usb_device_handle_t dev_hdl); esp_err_t usbh_dev_close(usb_device_handle_t dev_hdl); // ------------------------------ Getters -------------------------------------- +/** + * @brief Get a device's UID + * + * @note Callers of this function must have opened the device via usbh_devs_open() + * + * @param[in] dev_hdl Device handle + * @param[out] uid Device's UID + * + * @return + * - ESP_ERR_INVALID_ARG if invalid argument + * - ESP_OK if Device's UID obtained successfully + */ +esp_err_t usbh_dev_get_uid(usb_device_handle_t dev_hdl, unsigned int *uid); + /** * @brief Get a device's address * diff --git a/host/usb/src/ext_hub.c b/host/usb/src/ext_hub.c index 31c39408..08647a2e 100644 --- a/host/usb/src/ext_hub.c +++ b/host/usb/src/ext_hub.c @@ -1298,6 +1298,18 @@ esp_err_t ext_hub_get_speed(ext_hub_handle_t ext_hub_hdl, usb_speed_t *speed) return ESP_OK; } +esp_err_t ext_hub_get_dev_addr(ext_hub_handle_t ext_hub_hdl, uint8_t *addr) +{ + EXT_HUB_ENTER_CRITICAL(); + EXT_HUB_CHECK_FROM_CRIT(p_ext_hub_driver != NULL, ESP_ERR_NOT_ALLOWED); + EXT_HUB_EXIT_CRITICAL(); + EXT_HUB_CHECK(ext_hub_hdl != NULL && addr != NULL, ESP_ERR_INVALID_ARG); + EXT_HUB_CHECK(dev_is_in_list(ext_hub_hdl), ESP_ERR_NOT_FOUND); + ext_hub_dev_t *ext_hub_dev = (ext_hub_dev_t *)ext_hub_hdl; + *addr = ext_hub_dev->constant.dev_addr; + return ESP_OK; +} + esp_err_t ext_hub_new_dev(uint8_t dev_addr) { EXT_HUB_ENTER_CRITICAL(); diff --git a/host/usb/src/hub.c b/host/usb/src/hub.c index ec5aeb8a..f7ce4d28 100644 --- a/host/usb/src/hub.c +++ b/host/usb/src/hub.c @@ -70,7 +70,7 @@ typedef enum { struct dev_tree_node_s { TAILQ_ENTRY(dev_tree_node_s) tailq_entry; /**< Entry for the device tree node object tailq */ unsigned int uid; /**< Device's unique ID */ - void* parent; /**< Device's parent context */ + void *parent; /**< Device's parent context */ uint8_t port_num; /**< Device's parent port number */ }; @@ -164,7 +164,7 @@ static dev_tree_node_t *dev_tree_node_get_by_uid(unsigned int node_uid) * * @return esp_err_t */ -static esp_err_t dev_tree_node_new(void* parent, uint8_t port_num, usb_speed_t speed) +static esp_err_t dev_tree_node_new(void *parent, uint8_t port_num, usb_speed_t speed) { esp_err_t ret; // Allocate memory for a new device tree node @@ -220,7 +220,7 @@ static esp_err_t dev_tree_node_new(void* parent, uint8_t port_num, usb_speed_t s return ret; } -static esp_err_t dev_tree_node_reset_completed(void* parent, uint8_t port_num) +static esp_err_t dev_tree_node_reset_completed(void *parent, uint8_t port_num) { dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; @@ -248,7 +248,7 @@ static esp_err_t dev_tree_node_reset_completed(void* parent, uint8_t port_num) return ESP_OK; } -static esp_err_t dev_tree_node_dev_gone(void* parent, uint8_t port_num) +static esp_err_t dev_tree_node_dev_gone(void *parent, uint8_t port_num) { dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; @@ -284,7 +284,7 @@ static esp_err_t dev_tree_node_dev_gone(void* parent, uint8_t port_num) * * @return esp_err_t */ -static esp_err_t dev_tree_node_remove_by_parent(void* parent, uint8_t port_num) +static esp_err_t dev_tree_node_remove_by_parent(void *parent, uint8_t port_num) { dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; @@ -795,6 +795,28 @@ esp_err_t hub_node_disable(unsigned int node_uid) return ret; } +esp_err_t hub_node_get_info(unsigned int node_uid, usb_parent_dev_info_t *parent_info) +{ + HUB_DRIVER_ENTER_CRITICAL(); + HUB_DRIVER_CHECK_FROM_CRIT(p_hub_driver_obj != NULL, ESP_ERR_INVALID_STATE); + HUB_DRIVER_EXIT_CRITICAL(); + HUB_DRIVER_CHECK(parent_info != NULL, ESP_ERR_INVALID_ARG); + + dev_tree_node_t *dev_tree_node = dev_tree_node_get_by_uid(node_uid); + HUB_DRIVER_CHECK(dev_tree_node != NULL, ESP_ERR_NOT_FOUND); + uint8_t parent_dev_addr = 0; +#if (ENABLE_USB_HUBS) + if (dev_tree_node->parent) { + ESP_ERROR_CHECK(ext_hub_get_dev_addr((ext_hub_handle_t) dev_tree_node->parent, &parent_dev_addr)); + } +#endif // ENABLE_USB_HUBS + + memset(parent_info, 0, sizeof(usb_parent_dev_info_t)); + parent_info->dev_addr = parent_dev_addr; + parent_info->port_num = dev_tree_node->port_num; + return ESP_OK; +} + esp_err_t hub_dev_new(uint8_t dev_addr) { HUB_DRIVER_ENTER_CRITICAL(); diff --git a/host/usb/src/usb_host.c b/host/usb/src/usb_host.c index 64559b94..9b6599ac 100644 --- a/host/usb/src/usb_host.c +++ b/host/usb/src/usb_host.c @@ -1094,7 +1094,17 @@ esp_err_t usb_host_device_addr_list_fill(int list_len, uint8_t *dev_addr_list, i esp_err_t usb_host_device_info(usb_device_handle_t dev_hdl, usb_device_info_t *dev_info) { + unsigned int uid; HOST_CHECK(dev_hdl != NULL && dev_info != NULL, ESP_ERR_INVALID_ARG); + esp_err_t ret; + + ESP_ERROR_CHECK(usbh_dev_get_uid(dev_hdl, &uid)); + + ret = hub_node_get_info(uid, &dev_info->parent); + if (ret != ESP_OK) { + ESP_LOGE(USB_HOST_TAG, "Unable to get dev tree node info, parent information not available"); + return ret; + } return usbh_dev_get_info(dev_hdl, dev_info); } diff --git a/host/usb/src/usbh.c b/host/usb/src/usbh.c index cb047b42..f45a0e09 100644 --- a/host/usb/src/usbh.c +++ b/host/usb/src/usbh.c @@ -929,27 +929,6 @@ esp_err_t usbh_devs_remove(unsigned int uid) return ret; } -esp_err_t usbh_devs_get_parent_info(unsigned int uid, usb_parent_dev_info_t *parent_info) -{ - USBH_CHECK(parent_info, ESP_ERR_INVALID_ARG); - esp_err_t ret = ESP_FAIL; - device_t *dev_obj = NULL; - - USBH_ENTER_CRITICAL(); - dev_obj = _find_dev_from_uid(uid); - if (dev_obj == NULL) { - ret = ESP_ERR_NOT_FOUND; - goto exit; - } else { - parent_info->dev_hdl = dev_obj->constant.parent_dev_hdl; - parent_info->port_num = dev_obj->constant.parent_port_num; - ret = ESP_OK; - } -exit: - USBH_EXIT_CRITICAL(); - return ret; -} - esp_err_t usbh_devs_mark_all_free(void) { USBH_ENTER_CRITICAL(); @@ -1075,6 +1054,18 @@ esp_err_t usbh_dev_close(usb_device_handle_t dev_hdl) // ---------------------------- Getters ---------------------------------------- // ----------------------------------------------------------------------------- +esp_err_t usbh_dev_get_uid(usb_device_handle_t dev_hdl, unsigned int *uid) +{ + USBH_CHECK(dev_hdl != NULL && uid != NULL, ESP_ERR_INVALID_ARG); + device_t *dev_obj = (device_t *)dev_hdl; + + USBH_ENTER_CRITICAL(); + *uid = dev_obj->constant.uid; + USBH_EXIT_CRITICAL(); + + return ESP_OK; +} + esp_err_t usbh_dev_get_addr(usb_device_handle_t dev_hdl, uint8_t *dev_addr) { USBH_CHECK(dev_hdl != NULL && dev_addr != NULL, ESP_ERR_INVALID_ARG); @@ -1092,8 +1083,6 @@ esp_err_t usbh_dev_get_info(usb_device_handle_t dev_hdl, usb_device_info_t *dev_ USBH_CHECK(dev_hdl != NULL && dev_info != NULL, ESP_ERR_INVALID_ARG); device_t *dev_obj = (device_t *)dev_hdl; - dev_info->parent.dev_hdl = dev_obj->constant.parent_dev_hdl; - dev_info->parent.port_num = dev_obj->constant.parent_port_num; dev_info->speed = dev_obj->constant.speed; dev_info->dev_addr = dev_obj->constant.address; // Device descriptor might not have been set yet From ac146635a26eb012f91783536059b3f9d7b47e7f Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 14:11:43 +0100 Subject: [PATCH 10/11] refactor(hub): Moved list to dymanic member as the get parent info could come any moment --- host/usb/src/hub.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/host/usb/src/hub.c b/host/usb/src/hub.c index f7ce4d28..13ee64b8 100644 --- a/host/usb/src/hub.c +++ b/host/usb/src/hub.c @@ -87,11 +87,8 @@ typedef struct { } flags; /**< Hub flags */ root_port_state_t root_port_state; /**< Root port state */ unsigned int port_reqs; /**< Root port request flag */ - } dynamic; /**< Dynamic members. Require a critical section */ - - struct { TAILQ_HEAD(tailhead_devs, dev_tree_node_s) dev_nodes_tailq; /**< Tailq of attached devices */ - } single_thread; /**< Single thread members don't require a critical section so long as they are never accessed from multiple threads */ + } dynamic; /**< Dynamic members. Require a critical section */ struct { hcd_port_handle_t root_port_hdl; /**< Root port HCD handle */ @@ -150,12 +147,14 @@ static dev_tree_node_t *dev_tree_node_get_by_uid(unsigned int node_uid) dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; // Search the device tree nodes list for a device node with the specified parent - TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->single_thread.dev_nodes_tailq, tailq_entry) { + HUB_DRIVER_ENTER_CRITICAL(); + TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->dynamic.dev_nodes_tailq, tailq_entry) { if (dev_tree_iter->uid == node_uid) { dev_tree_node = dev_tree_iter; break; } } + HUB_DRIVER_EXIT_CRITICAL(); return dev_tree_node; } @@ -200,9 +199,9 @@ static esp_err_t dev_tree_node_new(void *parent, uint8_t port_num, usb_speed_t s // Device registration may fail if there are no available HCD channels. goto fail; } - - TAILQ_INSERT_TAIL(&p_hub_driver_obj->single_thread.dev_nodes_tailq, dev_tree_node, tailq_entry); - + HUB_DRIVER_ENTER_CRITICAL(); + TAILQ_INSERT_TAIL(&p_hub_driver_obj->dynamic.dev_nodes_tailq, dev_tree_node, tailq_entry); + HUB_DRIVER_EXIT_CRITICAL(); ESP_LOGD(HUB_DRIVER_TAG, "Device tree node (port %d, uid %d): new", dev_tree_node->port_num, dev_tree_node->uid); hub_event_data_t event_data = { @@ -225,13 +224,15 @@ static esp_err_t dev_tree_node_reset_completed(void *parent, uint8_t port_num) dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; // Search the device tree nodes list for a device node with the specified parent - TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->single_thread.dev_nodes_tailq, tailq_entry) { + HUB_DRIVER_ENTER_CRITICAL(); + TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->dynamic.dev_nodes_tailq, tailq_entry) { if (dev_tree_iter->parent == parent && dev_tree_iter->port_num == port_num) { dev_tree_node = dev_tree_iter; break; } } + HUB_DRIVER_EXIT_CRITICAL(); if (dev_tree_node == NULL) { ESP_LOGE(HUB_DRIVER_TAG, "Reset completed, but device tree node (port %d) not found", port_num); @@ -253,13 +254,15 @@ static esp_err_t dev_tree_node_dev_gone(void *parent, uint8_t port_num) dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; // Search the device tree nodes list for a device node with the specified parent - TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->single_thread.dev_nodes_tailq, tailq_entry) { + HUB_DRIVER_ENTER_CRITICAL(); + TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->dynamic.dev_nodes_tailq, tailq_entry) { if (dev_tree_iter->parent == parent && dev_tree_iter->port_num == port_num) { dev_tree_node = dev_tree_iter; break; } } + HUB_DRIVER_EXIT_CRITICAL(); if (dev_tree_node == NULL) { ESP_LOGW(HUB_DRIVER_TAG, "Device tree node (port %d): not found", port_num); @@ -289,13 +292,18 @@ static esp_err_t dev_tree_node_remove_by_parent(void *parent, uint8_t port_num) dev_tree_node_t *dev_tree_node = NULL; dev_tree_node_t *dev_tree_iter; // Search the device tree nodes list for a device node with the specified parent - TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->single_thread.dev_nodes_tailq, tailq_entry) { + HUB_DRIVER_ENTER_CRITICAL(); + TAILQ_FOREACH(dev_tree_iter, &p_hub_driver_obj->dynamic.dev_nodes_tailq, tailq_entry) { if (dev_tree_iter->parent == parent && dev_tree_iter->port_num == port_num) { dev_tree_node = dev_tree_iter; break; } } + if (dev_tree_node) { + TAILQ_REMOVE(&p_hub_driver_obj->dynamic.dev_nodes_tailq, dev_tree_node, tailq_entry); + } + HUB_DRIVER_EXIT_CRITICAL(); if (dev_tree_node == NULL) { ESP_LOGW(HUB_DRIVER_TAG, "Device tree node (port %d): not found", port_num); @@ -303,8 +311,6 @@ static esp_err_t dev_tree_node_remove_by_parent(void *parent, uint8_t port_num) } ESP_LOGD(HUB_DRIVER_TAG, "Device tree node (port %d, uid %d): freeing", port_num, dev_tree_node->uid); - - TAILQ_REMOVE(&p_hub_driver_obj->single_thread.dev_nodes_tailq, dev_tree_node, tailq_entry); heap_caps_free(dev_tree_node); return ESP_OK; } @@ -596,8 +602,8 @@ esp_err_t hub_install(hub_config_t *hub_config, void **client_ret) hub_driver_obj->constant.proc_req_cb_arg = hub_config->proc_req_cb_arg; hub_driver_obj->constant.event_cb = hub_config->event_cb; hub_driver_obj->constant.event_cb_arg = hub_config->event_cb_arg; - TAILQ_INIT(&hub_driver_obj->single_thread.dev_nodes_tailq); // Driver is not installed, we can modify dynamic section outside of the critical section + TAILQ_INIT(&hub_driver_obj->dynamic.dev_nodes_tailq); hub_driver_obj->dynamic.root_port_state = ROOT_PORT_STATE_NOT_POWERED; HUB_DRIVER_ENTER_CRITICAL(); From d9045f676471d66e213e2bb76392f13d25512aa7 Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 14:03:21 +0100 Subject: [PATCH 11/11] refactor(usbh): Cleaned up parent_dev_hdl from the usbh object --- host/usb/private_include/usbh.h | 4 ---- host/usb/src/hub.c | 3 --- host/usb/src/usbh.c | 8 -------- 3 files changed, 15 deletions(-) diff --git a/host/usb/private_include/usbh.h b/host/usb/private_include/usbh.h index daa942f0..6603fdbf 100644 --- a/host/usb/private_include/usbh.h +++ b/host/usb/private_include/usbh.h @@ -59,8 +59,6 @@ typedef struct { } dev_gone_data; struct { unsigned int dev_uid; - usb_device_handle_t parent_dev_hdl; - uint8_t port_num; } dev_free_data; }; } usbh_event_data_t; @@ -139,8 +137,6 @@ typedef struct { unsigned int uid; /**< Unique ID assigned to the device */ usb_speed_t speed; /**< Device's speed */ hcd_port_handle_t root_port_hdl; /**< Handle of the port that the device is connected to */ - usb_device_handle_t parent_dev_hdl; /**< Parent's device handle */ - uint8_t parent_port_num; /**< Parent's port number */ } usbh_dev_params_t; // ---------------------- USBH Processing Functions ---------------------------- diff --git a/host/usb/src/hub.c b/host/usb/src/hub.c index 13ee64b8..79efe9bc 100644 --- a/host/usb/src/hub.c +++ b/host/usb/src/hub.c @@ -189,9 +189,6 @@ static esp_err_t dev_tree_node_new(void *parent, uint8_t port_num, usb_speed_t s .uid = dev_tree_node->uid, .speed = speed, .root_port_hdl = p_hub_driver_obj->constant.root_port_hdl, // Always the same for all devices - // TODO: IDF-10023 Move parent-child tree management responsibility to Hub Driver - .parent_dev_hdl = NULL, - .parent_port_num = port_num, }; ret = usbh_devs_add(¶ms); diff --git a/host/usb/src/usbh.c b/host/usb/src/usbh.c index f45a0e09..671734d2 100644 --- a/host/usb/src/usbh.c +++ b/host/usb/src/usbh.c @@ -79,8 +79,6 @@ struct device_s { // Assigned on device allocation and remain constant for the device's lifetime hcd_pipe_handle_t default_pipe; /**< Pipe handle for Control EP0 */ hcd_port_handle_t port_hdl; /**< HCD port handle */ - usb_device_handle_t parent_dev_hdl; /**< Device's parent device handle. NULL if device is connected to the root port */ - uint8_t parent_port_num; /**< Device's parent port number. 0 if device connected to the root port */ usb_speed_t speed; /**< Device's speed */ unsigned int uid; /**< Device's Unique ID */ /* @@ -388,8 +386,6 @@ static esp_err_t device_alloc(usbh_dev_params_t *params, device_t **dev_obj_ret) dev_obj->dynamic.state = USB_DEVICE_STATE_DEFAULT; dev_obj->constant.default_pipe = default_pipe_hdl; dev_obj->constant.port_hdl = params->root_port_hdl; - dev_obj->constant.parent_dev_hdl = params->parent_dev_hdl; - dev_obj->constant.parent_port_num = params->parent_port_num; dev_obj->constant.speed = params->speed; dev_obj->constant.uid = params->uid; // Note: Enumeration related dev_obj->constant fields are initialized later using usbh_dev_set_...() functions @@ -587,8 +583,6 @@ static inline void handle_free(device_t *dev_obj) { // Cache a copy of the device's address as we are about to free the device object const unsigned int dev_uid = dev_obj->constant.uid; - usb_device_handle_t parent_dev_hdl = dev_obj->constant.parent_dev_hdl; - const uint8_t parent_port_num = dev_obj->constant.parent_port_num; bool all_free; ESP_LOGD(USBH_TAG, "Freeing device %d", dev_obj->constant.address); @@ -613,8 +607,6 @@ static inline void handle_free(device_t *dev_obj) .event = USBH_EVENT_DEV_FREE, .dev_free_data = { .dev_uid = dev_uid, - .parent_dev_hdl = parent_dev_hdl, - .port_num = parent_port_num, } }; p_usbh_obj->constant.event_cb(&event_data, p_usbh_obj->constant.event_cb_arg);