From 90aa6c041d2f0b9a9658c5b4cf60df3ba9d8d342 Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 14:00:43 +0100 Subject: [PATCH 1/3] 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 | 22 ++++++++++++++++ host/usb/src/usb_host.c | 10 ++++++++ host/usb/src/usbh.c | 35 +++++++++----------------- 8 files changed, 106 insertions(+), 41 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 5b578f7d..f7ce4d28 100644 --- a/host/usb/src/hub.c +++ b/host/usb/src/hub.c @@ -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 6fd601ea1d507fde0da409b93479730b0f5d2011 Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 14:11:43 +0100 Subject: [PATCH 2/3] 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 71769ce6040ef1545cd1250addbd7131f0e099da Mon Sep 17 00:00:00 2001 From: Roman Leonov Date: Fri, 7 Mar 2025 14:03:21 +0100 Subject: [PATCH 3/3] 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);