Skip to content

Commit

Permalink
Merge branch 'bugfix/optimize_hidh_disconnection' into 'master'
Browse files Browse the repository at this point in the history 
Bugfix/Fix HID Host improper handle of freeing resource in esp_hid component

Closes BT-2909

See merge request espressif/esp-idf!20919
  • Loading branch information
@wmy-espressif
wmy-espressif committed Nov 23, 2022
2 parents b30e28e + a025dfd commit 11fde1a
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 55 deletions.
4 changes: 3 additions & 1 deletion components/esp_hid/private/esp_hidh_private.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,9 @@ esp_hidh_dev_report_t *esp_hidh_dev_get_input_report_by_id_and_proto(esp_hidh_de
esp_hidh_dev_report_t *esp_hidh_dev_get_input_report_by_proto_and_data(esp_hidh_dev_t *dev, int protocol_mode,
size_t len, const uint8_t *data, bool *has_report_id);
esp_hidh_dev_report_t *esp_hidh_dev_get_report_by_handle(esp_hidh_dev_t *dev, uint16_t handle); //BLE Only
void esp_hidh_process_event_data_handler(void *event_handler_arg, esp_event_base_t event_base, int32_t event_id,
void esp_hidh_preprocess_event_handler(void *event_handler_arg, esp_event_base_t event_base, int32_t event_id,
void *event_data);
void esp_hidh_post_process_event_handler(void *event_handler_arg, esp_event_base_t event_base, int32_t event_id,
void *event_data);
void esp_hidh_dev_lock(esp_hidh_dev_t *dev);
void esp_hidh_dev_unlock(esp_hidh_dev_t *dev);
Expand Down
21 changes: 18 additions & 3 deletions components/esp_hid/src/ble_hidh.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ static esp_event_loop_handle_t event_loop_handle;
static uint8_t *s_read_data_val = NULL;
static uint16_t s_read_data_len = 0;
static esp_gatt_status_t s_read_status = ESP_GATT_OK;
static esp_event_handler_t s_event_callback;

static esp_gatt_status_t read_char(esp_gatt_if_t gattc_if, uint16_t conn_id, uint16_t handle, esp_gatt_auth_req_t auth_req, uint8_t **out, uint16_t *out_len)
{
Expand Down Expand Up @@ -616,6 +617,18 @@ static void esp_ble_hidh_dev_dump(esp_hidh_dev_t *dev, FILE *fp)

}

static void esp_ble_hidh_event_handler_wrapper(void *event_handler_arg, esp_event_base_t event_base, int32_t event_id,
void *event_data)
{
esp_hidh_preprocess_event_handler(event_handler_arg, event_base, event_id, event_data);

if (s_event_callback) {
s_event_callback(event_handler_arg, event_base, event_id, event_data);
}

esp_hidh_post_process_event_handler(event_handler_arg, event_base, event_id, event_data);
}

esp_err_t esp_ble_hidh_init(const esp_hidh_config_t *config)
{
esp_err_t ret;
Expand Down Expand Up @@ -653,10 +666,10 @@ esp_err_t esp_ble_hidh_init(const esp_hidh_config_t *config)
break;
}
WAIT_CB();

s_event_callback = config->callback;
ret = esp_event_handler_register_with(event_loop_handle, ESP_HIDH_EVENTS, ESP_EVENT_ANY_ID,
esp_hidh_process_event_data_handler, NULL);
ret |= esp_event_handler_register_with(event_loop_handle, ESP_HIDH_EVENTS, ESP_EVENT_ANY_ID, config->callback,
config->callback_arg);
esp_ble_hidh_event_handler_wrapper, config->callback_arg);
} while (0);

if (ret != ESP_OK) {
Expand Down Expand Up @@ -690,6 +703,8 @@ esp_err_t esp_ble_hidh_deinit(void)
}
vSemaphoreDelete(s_ble_hidh_cb_semaphore);
s_ble_hidh_cb_semaphore = NULL;
s_event_callback = NULL;

return err;
}

Expand Down
21 changes: 18 additions & 3 deletions components/esp_hid/src/bt_hidh.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ typedef struct {
typedef struct {
fixed_queue_t *connection_queue; /* Queue of connection */
esp_event_loop_handle_t event_loop_handle;
esp_event_handler_t event_callback;
} hidh_local_param_t;

static hidh_local_param_t hidh_local_param;
Expand Down Expand Up @@ -172,6 +173,8 @@ static void free_local_param(void)
if (hidh_local_param.connection_queue) {
fixed_queue_free(hidh_local_param.connection_queue, free);
}

hidh_local_param.event_callback = NULL;
}

static void open_failed_cb(esp_hidh_dev_t *dev, esp_hidh_status_t status, esp_hidh_event_data_t *p,
Expand Down Expand Up @@ -992,6 +995,18 @@ static void esp_bt_hidh_dev_dump(esp_hidh_dev_t *dev, FILE *fp)
}
}

static void esp_bt_hidh_event_handler_wrapper(void *event_handler_arg, esp_event_base_t event_base, int32_t event_id,
void *event_data)
{
esp_hidh_preprocess_event_handler(event_handler_arg, event_base, event_id, event_data);

if (hidh_local_param.event_callback) {
hidh_local_param.event_callback(event_handler_arg, event_base, event_id, event_data);
}

esp_hidh_post_process_event_handler(event_handler_arg, event_base, event_id, event_data);
}

esp_err_t esp_bt_hidh_init(const esp_hidh_config_t *config)
{
esp_err_t ret = ESP_OK;
Expand Down Expand Up @@ -1019,10 +1034,10 @@ esp_err_t esp_bt_hidh_init(const esp_hidh_config_t *config)
ret = ESP_FAIL;
break;
}

hidh_local_param.event_callback = config->callback;
ret = esp_event_handler_register_with(hidh_local_param.event_loop_handle, ESP_HIDH_EVENTS, ESP_EVENT_ANY_ID,
esp_hidh_process_event_data_handler, NULL);
ret |= esp_event_handler_register_with(hidh_local_param.event_loop_handle, ESP_HIDH_EVENTS, ESP_EVENT_ANY_ID,
config->callback, config->callback_arg);
esp_bt_hidh_event_handler_wrapper, config->callback_arg);
if (ret != ESP_OK) {
ESP_LOGE(TAG, "event_loop register failed!");
ret = ESP_FAIL;
Expand Down
65 changes: 17 additions & 48 deletions components/esp_hid/src/esp_hidh.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,8 @@ ESP_EVENT_DEFINE_BASE(ESP_HIDH_EVENTS);
static const char *TAG = "ESP_HIDH";

static esp_hidh_dev_head_t s_esp_hidh_devices;
static esp_timer_handle_t s_esp_hidh_timer;
static SemaphoreHandle_t s_esp_hidh_devices_semaphore = NULL;

static void esp_hidh_dev_delay_free(void *arg);

static inline void lock_devices(void)
{
if (s_esp_hidh_devices_semaphore != NULL) {
Expand Down Expand Up @@ -74,17 +71,6 @@ esp_err_t esp_hidh_init(const esp_hidh_config_t *config)

TAILQ_INIT(&s_esp_hidh_devices);

esp_timer_create_args_t timer_config = {
.callback = &esp_hidh_dev_delay_free,
.arg = NULL,
.name = "hidh_timer"
};

if ((err = esp_timer_create(&timer_config, &s_esp_hidh_timer)) != ESP_OK) {
ESP_LOGE(TAG, "%s create timer failed!", __func__);
return err;
}

s_esp_hidh_devices_semaphore = xSemaphoreCreateMutex();
if (s_esp_hidh_devices_semaphore == NULL) {
ESP_LOGE(TAG, "xSemaphoreCreateMutex failed!");
Expand All @@ -108,8 +94,6 @@ esp_err_t esp_hidh_init(const esp_hidh_config_t *config)
if (err != ESP_OK) {
vSemaphoreDelete(s_esp_hidh_devices_semaphore);
s_esp_hidh_devices_semaphore = NULL;
esp_timer_delete(s_esp_hidh_timer);
s_esp_hidh_timer = NULL;
}

return err;
Expand All @@ -123,11 +107,6 @@ esp_err_t esp_hidh_deinit(void)
return err;
}

if (esp_timer_is_active(s_esp_hidh_timer)) {
ESP_LOGE(TAG, "Busy, try again later!");
return ESP_ERR_NOT_FINISHED;
}

if (!TAILQ_EMPTY(&s_esp_hidh_devices)) {
ESP_LOGE(TAG, "Please disconnect all devices first!");
return err;
Expand All @@ -151,8 +130,6 @@ esp_err_t esp_hidh_deinit(void)
TAILQ_INIT(&s_esp_hidh_devices);
vSemaphoreDelete(s_esp_hidh_devices_semaphore);
s_esp_hidh_devices_semaphore = NULL;
esp_timer_delete(s_esp_hidh_timer);
s_esp_hidh_timer = NULL;
}
return err;
}
Expand Down Expand Up @@ -742,20 +719,6 @@ esp_err_t esp_hidh_dev_free_inner(esp_hidh_dev_t *dev)
return ret;
}

static void esp_hidh_dev_delay_free(void *arg)
{
esp_hidh_dev_t *d = NULL;
esp_hidh_dev_t *next = NULL;
lock_devices();
TAILQ_FOREACH_SAFE(d, &s_esp_hidh_devices, devices, next) {
if (!d->in_use) {
TAILQ_REMOVE(&s_esp_hidh_devices, d, devices);
esp_hidh_dev_resources_free(d);
}
}
unlock_devices();
}

#if CONFIG_BLUEDROID_ENABLED
esp_hidh_dev_t *esp_hidh_dev_get_by_bda(esp_bd_addr_t bda)
{
Expand Down Expand Up @@ -805,10 +768,10 @@ esp_hidh_dev_t *esp_hidh_dev_get_by_conn_id(uint16_t conn_id)

/**
* The deep copy data append the end of the esp_hidh_event_data_t, move the data pointer to the correct address. This is
* a workaround way, it's better to use flexiable array in the interface.
* a workaround way, it's better to use flexible array in the interface.
*/
void esp_hidh_process_event_data_handler(void *event_handler_arg, esp_event_base_t event_base, int32_t event_id,
void *event_data)
void esp_hidh_preprocess_event_handler(void *event_handler_arg, esp_event_base_t event_base, int32_t event_id,
void *event_data)
{
esp_hidh_event_t event = (esp_hidh_event_t)event_id;
esp_hidh_event_data_t *param = (esp_hidh_event_data_t *)event_data;
Expand All @@ -824,19 +787,25 @@ void esp_hidh_process_event_data_handler(void *event_handler_arg, esp_event_base
param->feature.data = (uint8_t *)param + sizeof(esp_hidh_event_data_t);
}
break;
default:
break;
}
}

void esp_hidh_post_process_event_handler(void *event_handler_arg, esp_event_base_t event_base, int32_t event_id,
void *event_data)
{
esp_hidh_event_t event = (esp_hidh_event_t)event_id;
esp_hidh_event_data_t *param = (esp_hidh_event_data_t *)event_data;

switch (event) {
case ESP_HIDH_OPEN_EVENT:
if (param->open.status != ESP_OK) {
if (s_esp_hidh_timer && !esp_timer_is_active(s_esp_hidh_timer) &&
esp_timer_start_once(s_esp_hidh_timer, ESP_HIDH_DELAY_FREE_TO) != ESP_OK) {
ESP_LOGE(TAG, "%s set hidh timer failed!", __func__);
}
esp_hidh_dev_free_inner(param->open.dev);
}
break;
case ESP_HIDH_CLOSE_EVENT:
if (s_esp_hidh_timer && !esp_timer_is_active(s_esp_hidh_timer) &&
esp_timer_start_once(s_esp_hidh_timer, ESP_HIDH_DELAY_FREE_TO) != ESP_OK) {
ESP_LOGE(TAG, "%s set hidh timer failed!", __func__);
}
esp_hidh_dev_free_inner(param->close.dev);
break;
default:
break;
Expand Down

0 comments on commit 11fde1a

Please sign in to comment.