From d1a02e1854a198e513acdf3dcc6b766fca2f1727 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Tue, 17 Dec 2024 11:46:58 +0100 Subject: [PATCH 1/4] fix(esp_event): Fix event loop profiling in handler_execute function handler_execute function is looking to match the handler only in the list of loop events but does not look in the base event handler list nor the id event handler list. So unless the event handler is registered to be triggered for all event bases and all event ids of an event loop, its profiling fields (invoked and time) are not updated when it is called. This commit updates the search for the matching handler to also look in base event list and ID event list. Closes https://github.com/espressif/esp-idf/issues/15041 --- components/esp_event/esp_event.c | 46 ++++++++++++++---- .../private_include/esp_event_internal.h | 20 +++----- .../test_apps/main/test_event_common.cpp | 48 ++++++++++++++++++- .../esp_event/test_apps/pytest_esp_event.py | 15 +++++- tools/ci/check_copyright_ignore.txt | 1 - 5 files changed, 103 insertions(+), 27 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index d1b0e7bb4e..516483288a 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2018-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2018-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -22,7 +22,7 @@ /* ---------------------------- Definitions --------------------------------- */ #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING -// LOOP @ rx: dr: +// LOOP @ rx: dr: #define LOOP_DUMP_FORMAT "LOOP @%p,%s rx:%" PRIu32 " dr:%" PRIu32 "\n" // handler @
ev: inv: time: #define HANDLER_DUMP_FORMAT " HANDLER @%p ev:%s,%s inv:%" PRIu32 " time:%lld us\n" @@ -148,14 +148,42 @@ static void handler_execute(esp_event_loop_instance_t* loop, esp_event_handler_n // This happens in "handler instance can unregister itself" test case. // To prevent memory corruption error it's necessary to check if pointer is still valid. esp_event_loop_node_t* loop_node; - esp_event_handler_node_t* handler_node; SLIST_FOREACH(loop_node, &(loop->loop_nodes), next) { + // try to find the handler amongst the list of handlers that are registered on the + // loop level (i.e., registered to be triggered for all events) + esp_event_handler_node_t* handler_node; SLIST_FOREACH(handler_node, &(loop_node->handlers), next) { if(handler_node == handler) { handler->invoked++; handler->time += diff; } } + + // try to find the handler amongst the list of handlers that are registered on the + // base level (i.e., registered to be triggered for all events of a specific event base) + esp_event_base_node_t* base_node; + SLIST_FOREACH(base_node, &(loop_node->base_nodes), next) { + esp_event_handler_node_t* base_handler_node; + SLIST_FOREACH(base_handler_node, &(base_node->handlers), next) { + if (base_handler_node == handler) { + handler->invoked++; + handler->time += diff; + } + } + + // try to find the handler amongst the list of handlers that are registered on the + // ID level (i.e., registered to be triggered for a specific event ID of a specific event base) + esp_event_id_node_t* id_node; + SLIST_FOREACH(id_node, &(base_node->id_nodes), next) { + esp_event_handler_node_t* id_handler_node; + SLIST_FOREACH(id_handler_node, &(id_node->handlers), next) { + if (id_handler_node == handler) { + handler->invoked++; + handler->time += diff; + } + } + } + } } xSemaphoreGive(loop->profiling_mutex); @@ -894,7 +922,7 @@ esp_err_t esp_event_post_to(esp_event_loop_handle_t event_loop, esp_event_base_t } #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING - atomic_fetch_add(&loop->events_recieved, 1); + atomic_fetch_add(&loop->events_received, 1); #endif return ESP_OK; @@ -942,7 +970,7 @@ esp_err_t esp_event_isr_post_to(esp_event_loop_handle_t event_loop, esp_event_ba } #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING - atomic_fetch_add(&loop->events_recieved, 1); + atomic_fetch_add(&loop->events_received, 1); #endif return ESP_OK; @@ -971,13 +999,13 @@ esp_err_t esp_event_dump(FILE* file) portENTER_CRITICAL(&s_event_loops_spinlock); SLIST_FOREACH(loop_it, &s_event_loops, next) { - uint32_t events_recieved, events_dropped; + uint32_t events_received, events_dropped; - events_recieved = atomic_load(&loop_it->events_recieved); + events_received = atomic_load(&loop_it->events_received); events_dropped = atomic_load(&loop_it->events_dropped); - PRINT_DUMP_INFO(dst, sz, LOOP_DUMP_FORMAT, loop_it, loop_it->task != NULL ? loop_it->name : "none" , - events_recieved, events_dropped); + PRINT_DUMP_INFO(dst, sz, LOOP_DUMP_FORMAT, loop_it, loop_it->task != NULL ? loop_it->name : "none", + events_received, events_dropped); int sz_bak = sz; diff --git a/components/esp_event/private_include/esp_event_internal.h b/components/esp_event/private_include/esp_event_internal.h index dd403f6d7c..9c05cd665a 100644 --- a/components/esp_event/private_include/esp_event_internal.h +++ b/components/esp_event/private_include/esp_event_internal.h @@ -1,16 +1,8 @@ -// Copyright 2018 Espressif Systems (Shanghai) PTE LTD -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at - -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +/* + * SPDX-FileCopyrightText: 2018-2025 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #ifndef ESP_EVENT_INTERNAL_H_ #define ESP_EVENT_INTERNAL_H_ @@ -84,7 +76,7 @@ typedef struct esp_event_loop_instance { esp_event_loop_nodes_t loop_nodes; /**< set of linked lists containing the registered handlers for the loop */ #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING - atomic_uint_least32_t events_recieved; /**< number of events successfully posted to the loop */ + atomic_uint_least32_t events_received; /**< number of events successfully posted to the loop */ atomic_uint_least32_t events_dropped; /**< number of events dropped due to queue being full */ SemaphoreHandle_t profiling_mutex; /**< mutex used for profiliing */ SLIST_ENTRY(esp_event_loop_instance) next; /**< next event loop in the list */ diff --git a/components/esp_event/test_apps/main/test_event_common.cpp b/components/esp_event/test_apps/main/test_event_common.cpp index 9b327cd599..09e2bc5c13 100644 --- a/components/esp_event/test_apps/main/test_event_common.cpp +++ b/components/esp_event/test_apps/main/test_event_common.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Unlicense OR CC0-1.0 */ @@ -1571,3 +1571,49 @@ TEST_CASE("default event loop: registering event handler instance without instan TEST_ESP_OK(esp_event_loop_delete_default()); } + +#if CONFIG_ESP_EVENT_LOOP_PROFILING +static void handler_all(void* arg, esp_event_base_t event_base, int32_t event_id, void* data) +{ + printf("Event received: base=%s, id=%" PRId32 ", data=%p\n", event_base, event_id, data); +} + +static void handler_base(void* arg, esp_event_base_t event_base, int32_t event_id, void* data) +{ + printf("Event received: base=%s, id=%" PRId32 ", data=%p\n", event_base, event_id, data); +} + +static void handler_id(void* arg, esp_event_base_t event_base, int32_t event_id, void* data) +{ + printf("Event received: base=%s, id=%" PRId32 ", data=%p\n", event_base, event_id, data); +} + +TEST_CASE("profiling reports valid values", "[event][default]") +{ + TEST_ESP_OK(esp_event_loop_create_default()); + + /* register handler for event base 1 and event id 1 */ + TEST_ESP_OK(esp_event_handler_register(s_test_base1, TEST_EVENT_BASE1_EV1, handler_id, NULL)); + + /* register handler for event base 1 and all event ids */ + TEST_ESP_OK(esp_event_handler_register(s_test_base1, ESP_EVENT_ANY_ID, handler_base, NULL)); + + /* register handler for all event bases and all event ids */ + TEST_ESP_OK(esp_event_handler_register(ESP_EVENT_ANY_BASE, ESP_EVENT_ANY_ID, handler_all, NULL)); + + /* post an event on event base 1, event id 1 */ + TEST_ESP_OK(esp_event_post(s_test_base1, TEST_EVENT_BASE1_EV1, NULL, 0, pdMS_TO_TICKS(1000))); + + /* post an event 1 from base 1 and check the dump. + * - 3 handlers invoked, exec time is not 0 */ + esp_event_dump(stdout); + + /* unregister handlers */ + TEST_ESP_OK(esp_event_handler_unregister(ESP_EVENT_ANY_BASE, ESP_EVENT_ANY_ID, handler_all)); + TEST_ESP_OK(esp_event_handler_unregister(s_test_base1, ESP_EVENT_ANY_ID, handler_base)); + TEST_ESP_OK(esp_event_handler_unregister(s_test_base1, TEST_EVENT_BASE1_EV1, handler_id)); + + /* delete loop */ + TEST_ESP_OK(esp_event_loop_delete_default()); +} +#endif // CONFIG_ESP_EVENT_LOOP_PROFILING diff --git a/components/esp_event/test_apps/pytest_esp_event.py b/components/esp_event/test_apps/pytest_esp_event.py index 8e9cbd5538..c5d87ecad3 100644 --- a/components/esp_event/test_apps/pytest_esp_event.py +++ b/components/esp_event/test_apps/pytest_esp_event.py @@ -1,6 +1,5 @@ -# SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD +# SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD # SPDX-License-Identifier: CC0-1.0 - import pytest from pytest_embedded import Dut @@ -43,3 +42,15 @@ def test_esp_event_posix_simulator(dut: Dut) -> None: dut.expect_exact('Press ENTER to see the list of tests.') dut.write('*') dut.expect(r'\d{2} Tests 0 Failures 0 Ignored', timeout=120) + + +@pytest.mark.esp32 +@pytest.mark.generic +def test_esp_event_profiling(dut: Dut) -> None: + dut.expect_exact('Press ENTER to see the list of tests.') + dut.write('"profiling reports valid values"') + # look for all references of handlers invoked at least 1 time + # with an execution time superior to 0 us + matches = dut.expect(r'HANDLER .+ inv:[1-9][0-9]{0,} time:[1-9][0-9]{0,} us', timeout=2) + matches_arr = matches.group().split(b'\r\n') + assert (len(matches_arr) == 3) diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index 328f0ffcff..8c5acd8555 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -403,7 +403,6 @@ components/esp_event/esp_event_private.c components/esp_event/host_test/esp_event_unit_test/main/esp_event_test.cpp components/esp_event/host_test/fixtures.hpp components/esp_event/include/esp_event_loop.h -components/esp_event/private_include/esp_event_internal.h components/esp_event/private_include/esp_event_private.h components/esp_hid/include/esp_hid_common.h components/esp_hid/include/esp_hidd.h From 0938688e9b218b89f00d3d93e8b917b44a645fe7 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Tue, 31 Dec 2024 12:11:37 +0100 Subject: [PATCH 2/4] fix(esp_event): Handler unregistration by itself issue when esp_event_handler_unregister_with_internal cannot take the loop mutex (e.g., when the handler unregisters itself), create an event with a special base identifier and add it to the queue of the corresponding loop to postpone the removal of the handler from the list at a time when the loop mutex can be successfully taken. --- components/esp_event/esp_event.c | 251 +++++++++++------- .../private_include/esp_event_internal.h | 10 +- .../test_apps/main/test_event_common.cpp | 124 ++++++++- .../esp_event/test_apps/pytest_esp_event.py | 13 + 4 files changed, 303 insertions(+), 95 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index 516483288a..4edb3ff2da 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -38,6 +38,7 @@ static const char* TAG = "event"; static const char* esp_event_any_base = "any"; +static const char* esp_event_handler_cleanup = "cleanup"; #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING static SLIST_HEAD(esp_event_loop_instance_list_t, esp_event_loop_instance) s_event_loops = @@ -142,51 +143,8 @@ static void handler_execute(esp_event_loop_instance_t* loop, esp_event_handler_n #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING diff = esp_timer_get_time() - start; - xSemaphoreTake(loop->profiling_mutex, portMAX_DELAY); - - // At this point handler may be already unregistered. - // This happens in "handler instance can unregister itself" test case. - // To prevent memory corruption error it's necessary to check if pointer is still valid. - esp_event_loop_node_t* loop_node; - SLIST_FOREACH(loop_node, &(loop->loop_nodes), next) { - // try to find the handler amongst the list of handlers that are registered on the - // loop level (i.e., registered to be triggered for all events) - esp_event_handler_node_t* handler_node; - SLIST_FOREACH(handler_node, &(loop_node->handlers), next) { - if(handler_node == handler) { - handler->invoked++; - handler->time += diff; - } - } - - // try to find the handler amongst the list of handlers that are registered on the - // base level (i.e., registered to be triggered for all events of a specific event base) - esp_event_base_node_t* base_node; - SLIST_FOREACH(base_node, &(loop_node->base_nodes), next) { - esp_event_handler_node_t* base_handler_node; - SLIST_FOREACH(base_handler_node, &(base_node->handlers), next) { - if (base_handler_node == handler) { - handler->invoked++; - handler->time += diff; - } - } - - // try to find the handler amongst the list of handlers that are registered on the - // ID level (i.e., registered to be triggered for a specific event ID of a specific event base) - esp_event_id_node_t* id_node; - SLIST_FOREACH(id_node, &(base_node->id_nodes), next) { - esp_event_handler_node_t* id_handler_node; - SLIST_FOREACH(id_handler_node, &(id_node->handlers), next) { - if (id_handler_node == handler) { - handler->invoked++; - handler->time += diff; - } - } - } - } - } - - xSemaphoreGive(loop->profiling_mutex); + handler->invoked++; + handler->time += diff; #endif } @@ -391,8 +349,8 @@ static esp_err_t base_node_remove_handler(esp_event_base_node_t* base_node, int3 if (SLIST_EMPTY(&(it->handlers))) { SLIST_REMOVE(&(base_node->id_nodes), it, esp_event_id_node, next); free(it); - return ESP_OK; } + return ESP_OK; } } } @@ -416,8 +374,8 @@ static esp_err_t loop_node_remove_handler(esp_event_loop_node_t* loop_node, esp_ if (SLIST_EMPTY(&(it->handlers)) && SLIST_EMPTY(&(it->id_nodes))) { SLIST_REMOVE(&(loop_node->base_nodes), it, esp_event_base_node, next); free(it); - return ESP_OK; } + return ESP_OK; } } } @@ -426,6 +384,23 @@ static esp_err_t loop_node_remove_handler(esp_event_loop_node_t* loop_node, esp_ return ESP_ERR_NOT_FOUND; } +static esp_err_t loop_remove_handler(esp_event_remove_handler_context_t* ctx) +{ + esp_event_loop_node_t *it, *temp; + SLIST_FOREACH_SAFE(it, &(ctx->loop->loop_nodes), next, temp) { + esp_err_t res = loop_node_remove_handler(it, ctx->event_base, ctx->event_id, ctx->handler_ctx, ctx->legacy); + + if (res == ESP_OK) { + if (SLIST_EMPTY(&(it->base_nodes)) && SLIST_EMPTY(&(it->handlers))) { + SLIST_REMOVE(&(ctx->loop->loop_nodes), it, esp_event_loop_node, next); + free(it); + } + return ESP_OK; + } + } + return ESP_ERR_NOT_FOUND; +} + static void handler_instances_remove_all(esp_event_handler_nodes_t* handlers) { esp_event_handler_node_t *it, *temp; @@ -474,6 +449,104 @@ static void inline __attribute__((always_inline)) post_instance_delete(esp_event memset(post, 0, sizeof(*post)); } +static esp_err_t find_and_unregister_handler(esp_event_remove_handler_context_t* ctx) +{ + esp_event_handler_node_t *handler_to_unregister = NULL; + esp_event_handler_node_t *handler; + esp_event_loop_node_t *loop_node; + esp_event_base_node_t *base_node; + esp_event_id_node_t *id_node; + + SLIST_FOREACH(loop_node, &(ctx->loop->loop_nodes), next) { + // Execute loop level handlers + SLIST_FOREACH(handler, &(loop_node->handlers), next) { + if (ctx->legacy) { + if (handler->handler_ctx->handler == ctx->handler_ctx->handler) { + handler_to_unregister = handler; + break; + } + } else { + if (handler->handler_ctx == ctx->handler_ctx) { + handler_to_unregister = handler; + break; + } + } + } + + if (handler_to_unregister != NULL) { + break; + } + + SLIST_FOREACH(base_node, &(loop_node->base_nodes), next) { + if (base_node->base == ctx->event_base) { + // Execute base level handlers + SLIST_FOREACH(handler, &(base_node->handlers), next) { + if (ctx->legacy) { + if (handler->handler_ctx->handler == ctx->handler_ctx->handler) { + handler_to_unregister = handler; + break; + } + } else { + if (handler->handler_ctx == ctx->handler_ctx) { + handler_to_unregister = handler; + break; + } + } + } + + if (handler_to_unregister != NULL) { + break; + } + + SLIST_FOREACH(id_node, &(base_node->id_nodes), next) { + if (id_node->id == ctx->event_id) { + // Execute id level handlers + SLIST_FOREACH(handler, &(id_node->handlers), next) { + if (ctx->legacy) { + if (handler->handler_ctx->handler == ctx->handler_ctx->handler) { + handler_to_unregister = handler; + break; + } + } else { + if (handler->handler_ctx == ctx->handler_ctx) { + handler_to_unregister = handler; + break; + } + } + } + } + } + } + } + } + + if (handler_to_unregister == NULL) { + /* handler not found in the lists, return */ + return ESP_ERR_NOT_FOUND; + } + + if (handler_to_unregister->unregistered) { + /* the handler was found in a list but has already be marked + * as unregistered. It means an event was already created to + * remove from the list. return OK but do nothing */ + return ESP_OK; + } + /* handler found in the lists and not already marked as unregistered. Mark it as unregistered + * and post an event to remove it from the lists */ + handler_to_unregister->unregistered = true; + if (ctx->legacy) { + /* in case of legacy code, we have to copy the handler_ctx content since it was created in the calling function */ + esp_event_handler_instance_context_t *handler_ctx_copy = calloc(1, sizeof(esp_event_handler_instance_context_t)); + if (!handler_ctx_copy) { + return ESP_ERR_NO_MEM; + } + handler_ctx_copy->arg = ctx->handler_ctx->arg; + handler_ctx_copy->handler = ctx->handler_ctx->handler; + ctx->handler_ctx = handler_ctx_copy; + } + return esp_event_post_to(ctx->loop, esp_event_handler_cleanup, 0, ctx, sizeof(esp_event_remove_handler_context_t), portMAX_DELAY); +} + /* ---------------------------- Public API --------------------------------- */ esp_err_t esp_event_loop_create(const esp_event_loop_args_t* event_loop_args, esp_event_loop_handle_t* event_loop) @@ -509,14 +582,6 @@ esp_err_t esp_event_loop_create(const esp_event_loop_args_t* event_loop_args, es goto on_err; } -#ifdef CONFIG_ESP_EVENT_LOOP_PROFILING - loop->profiling_mutex = xSemaphoreCreateMutex(); - if (loop->profiling_mutex == NULL) { - ESP_LOGE(TAG, "create event loop profiling mutex failed"); - goto on_err; - } -#endif - SLIST_INIT(&(loop->loop_nodes)); // Create the loop task if requested @@ -562,12 +627,6 @@ on_err: vSemaphoreDelete(loop->mutex); } -#ifdef CONFIG_ESP_EVENT_LOOP_PROFILING - if (loop->profiling_mutex != NULL) { - vSemaphoreDelete(loop->profiling_mutex); - } -#endif - free(loop); return err; @@ -594,10 +653,25 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick int64_t remaining_ticks = ticks_to_run; #endif - while(xQueueReceive(loop->queue, &post, ticks_to_run) == pdTRUE) { + while (xQueueReceive(loop->queue, &post, remaining_ticks) == pdTRUE) { // The event has already been unqueued, so ensure it gets executed. xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); + // check if the event retrieve from the queue is the internal event that is + // triggered when a handler needs to be removed.. + if (post.base == esp_event_handler_cleanup) { + assert(post.data.ptr != NULL); + esp_event_remove_handler_context_t* ctx = (esp_event_remove_handler_context_t*)post.data.ptr; + loop_remove_handler(ctx); + + // if the handler unregistration request came from legacy code, + // we have to free handler_ctx pointer since it points to memory + // allocated by esp_event_handler_unregister_with_internal + if (ctx->legacy) { + free(ctx->handler_ctx); + } + } + loop->running_task = xTaskGetCurrentTaskHandle(); bool exec = false; @@ -610,24 +684,30 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick SLIST_FOREACH_SAFE(loop_node, &(loop->loop_nodes), next, temp_node) { // Execute loop level handlers SLIST_FOREACH_SAFE(handler, &(loop_node->handlers), next, temp_handler) { - handler_execute(loop, handler, post); - exec |= true; + if (!handler->unregistered) { + handler_execute(loop, handler, post); + exec |= true; + } } SLIST_FOREACH_SAFE(base_node, &(loop_node->base_nodes), next, temp_base) { if (base_node->base == post.base) { // Execute base level handlers SLIST_FOREACH_SAFE(handler, &(base_node->handlers), next, temp_handler) { - handler_execute(loop, handler, post); - exec |= true; + if (!handler->unregistered) { + handler_execute(loop, handler, post); + exec |= true; + } } SLIST_FOREACH_SAFE(id_node, &(base_node->id_nodes), next, temp_id_node) { if (id_node->id == post.id) { // Execute id level handlers SLIST_FOREACH_SAFE(handler, &(id_node->handlers), next, temp_handler) { - handler_execute(loop, handler, post); - exec |= true; + if (!handler->unregistered) { + handler_execute(loop, handler, post); + exec |= true; + } } // Skip to next base node break; @@ -674,14 +754,10 @@ esp_err_t esp_event_loop_delete(esp_event_loop_handle_t event_loop) esp_event_loop_instance_t* loop = (esp_event_loop_instance_t*) event_loop; SemaphoreHandle_t loop_mutex = loop->mutex; -#ifdef CONFIG_ESP_EVENT_LOOP_PROFILING - SemaphoreHandle_t loop_profiling_mutex = loop->profiling_mutex; -#endif xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING - xSemaphoreTake(loop->profiling_mutex, portMAX_DELAY); portENTER_CRITICAL(&s_event_loops_spinlock); SLIST_REMOVE(&s_event_loops, loop, esp_event_loop_instance, next); portEXIT_CRITICAL(&s_event_loops_spinlock); @@ -711,10 +787,6 @@ esp_err_t esp_event_loop_delete(esp_event_loop_handle_t event_loop) free(loop); // Free loop mutex before deleting xSemaphoreGiveRecursive(loop_mutex); -#ifdef CONFIG_ESP_EVENT_LOOP_PROFILING - xSemaphoreGive(loop_profiling_mutex); - vSemaphoreDelete(loop_profiling_mutex); -#endif vSemaphoreDelete(loop_mutex); return ESP_OK; @@ -814,24 +886,21 @@ esp_err_t esp_event_handler_unregister_with_internal(esp_event_loop_handle_t eve } esp_event_loop_instance_t* loop = (esp_event_loop_instance_t*) event_loop; + esp_event_remove_handler_context_t remove_handler_ctx = {loop, event_base, event_id, handler_ctx, legacy}; - xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); - - esp_event_loop_node_t *it, *temp; - - SLIST_FOREACH_SAFE(it, &(loop->loop_nodes), next, temp) { - esp_err_t res = loop_node_remove_handler(it, event_base, event_id, handler_ctx, legacy); - - if (res == ESP_OK && SLIST_EMPTY(&(it->base_nodes)) && SLIST_EMPTY(&(it->handlers))) { - SLIST_REMOVE(&(loop->loop_nodes), it, esp_event_loop_node, next); - free(it); - break; - } + /* remove the handler if the mutex is taken successfully. + * otherwise it will be removed from the list later */ + esp_err_t res = ESP_FAIL; + if (xSemaphoreTake(loop->mutex, 0) == pdTRUE) { + res = loop_remove_handler(&remove_handler_ctx); + xSemaphoreGive(loop->mutex); + } else { + xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); + res = find_and_unregister_handler(&remove_handler_ctx); + xSemaphoreGiveRecursive(loop->mutex); } - xSemaphoreGiveRecursive(loop->mutex); - - return ESP_OK; + return res; } esp_err_t esp_event_handler_unregister_with(esp_event_loop_handle_t event_loop, esp_event_base_t event_base, diff --git a/components/esp_event/private_include/esp_event_internal.h b/components/esp_event/private_include/esp_event_internal.h index 9c05cd665a..f7b1612996 100644 --- a/components/esp_event/private_include/esp_event_internal.h +++ b/components/esp_event/private_include/esp_event_internal.h @@ -31,6 +31,7 @@ typedef struct esp_event_handler_node { int64_t time; /**< total runtime of this handler across all calls */ #endif SLIST_ENTRY(esp_event_handler_node) next; /**< next event handler in the list */ + bool unregistered; } esp_event_handler_node_t; typedef SLIST_HEAD(esp_event_handler_instances, esp_event_handler_node) esp_event_handler_nodes_t; @@ -78,11 +79,18 @@ typedef struct esp_event_loop_instance { #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING atomic_uint_least32_t events_received; /**< number of events successfully posted to the loop */ atomic_uint_least32_t events_dropped; /**< number of events dropped due to queue being full */ - SemaphoreHandle_t profiling_mutex; /**< mutex used for profiliing */ SLIST_ENTRY(esp_event_loop_instance) next; /**< next event loop in the list */ #endif } esp_event_loop_instance_t; +typedef struct esp_event_remove_handler_context_t { + esp_event_loop_instance_t* loop; /**< Instance of the event loop from which the handler has to be removed */ + esp_event_base_t event_base; /**< The event base identification of the handler that has to be removed */ + int32_t event_id; /**< The event identification value of the handler that has to be removed */ + esp_event_handler_instance_context_t* handler_ctx; /**< The handler context of the handler that has to be removed */ + bool legacy; /**< Set to true when the handler unregistration request was made from legacy code */ +} esp_event_remove_handler_context_t; + #if CONFIG_ESP_EVENT_POST_FROM_ISR typedef union esp_event_post_data { uint32_t val; diff --git a/components/esp_event/test_apps/main/test_event_common.cpp b/components/esp_event/test_apps/main/test_event_common.cpp index 09e2bc5c13..569b97902e 100644 --- a/components/esp_event/test_apps/main/test_event_common.cpp +++ b/components/esp_event/test_apps/main/test_event_common.cpp @@ -1575,19 +1575,27 @@ TEST_CASE("default event loop: registering event handler instance without instan #if CONFIG_ESP_EVENT_LOOP_PROFILING static void handler_all(void* arg, esp_event_base_t event_base, int32_t event_id, void* data) { - printf("Event received: base=%s, id=%" PRId32 ", data=%p\n", event_base, event_id, data); + printf("base=%s, id=%" PRId32 ", data=%p\n", event_base, event_id, data); } static void handler_base(void* arg, esp_event_base_t event_base, int32_t event_id, void* data) { - printf("Event received: base=%s, id=%" PRId32 ", data=%p\n", event_base, event_id, data); + printf("base=%s, id=%" PRId32 ", data=%p\n", event_base, event_id, data); } static void handler_id(void* arg, esp_event_base_t event_base, int32_t event_id, void* data) { - printf("Event received: base=%s, id=%" PRId32 ", data=%p\n", event_base, event_id, data); + printf("base=%s, id=%" PRId32 ", data=%p\n", event_base, event_id, data); } +/* This test executes the following steps: + * 1) register handler_id for a event id A and event base B, + * 2) register handler_base for a given event base, + * 3) register handler_all for on the loop level, + * 4) post an event (A, B), + * 5) call esp_event_dump and make sure by parsing the string IN PYTEST ENVIRONMENT that + * all handlers profiling data is printed + * 6) unregister the handlers successfully */ TEST_CASE("profiling reports valid values", "[event][default]") { TEST_ESP_OK(esp_event_loop_create_default()); @@ -1616,4 +1624,114 @@ TEST_CASE("profiling reports valid values", "[event][default]") /* delete loop */ TEST_ESP_OK(esp_event_loop_delete_default()); } + +static void handler_id2(void* arg, esp_event_base_t event_base, int32_t event_id, void* data) +{ + printf("event received: base=%s, id=%" PRId32 ", data=%p\n", event_base, event_id, data); + + /* self unregistering handler */ + TEST_ESP_OK(esp_event_handler_unregister(s_test_base1, TEST_EVENT_BASE1_EV1, handler_id2)); + + /* register a new handler on id level */ + TEST_ESP_OK(esp_event_handler_register(s_test_base1, TEST_EVENT_BASE1_EV1, handler_id, NULL)); +} + +/* This test executes the following steps: + * 1) register handler_id2 for a given event id and event base, + * 2) post an event to trigger the handler_id2, + * 3) unregister the handler_id2 during the execution of the handler_id2 and register + * handler_id instead. + * 4) call esp_event_dump and make sure by parsing the string IN PYTEST ENVIRONMENT that + * 1 handler profiling data is printed + * 5) unregister the handler_id successfully */ +TEST_CASE("esp_event_dump does not show self unregistered handler", "[event][default]") +{ + TEST_ESP_OK(esp_event_loop_create_default()); + + /* register handler for event base 1 and event id 1 */ + TEST_ESP_OK(esp_event_handler_register(s_test_base1, TEST_EVENT_BASE1_EV1, handler_id2, NULL)); + + /* post an event on event base 1, event id 1 */ + TEST_ESP_OK(esp_event_post(s_test_base1, TEST_EVENT_BASE1_EV1, NULL, 0, pdMS_TO_TICKS(1000))); + + /* post an event 1 from base 1 and check the dump. + * - 1 handler invoked 0 times, exec time is 0 us */ + esp_event_dump(stdout); + + /* unregister handler id */ + TEST_ESP_OK(esp_event_handler_unregister(s_test_base1, TEST_EVENT_BASE1_EV1, handler_id)); + + /* delete loop */ + TEST_ESP_OK(esp_event_loop_delete_default()); +} + +static SemaphoreHandle_t s_event_mutex; +static StaticSemaphore_t s_event_mutex_buf; +static size_t s_handler_triggered = 0; + +static void self_unregistering_handler(void* arg, esp_event_base_t event_base, int32_t event_id, void* data) +{ + xSemaphoreTake(s_event_mutex, portMAX_DELAY); + + /* self unregistering handler */ + TEST_ESP_OK(esp_event_handler_unregister(s_test_base1, TEST_EVENT_BASE1_EV1, self_unregistering_handler)); + + s_handler_triggered++; + + xSemaphoreGive(s_event_mutex); +} + +/* This test makes sure that once a handler is unregistered, it is never called again. + * This test follows the update in esp event unregistration process where a handler can + * be marked as unregistered but not removed directly from the list it belongs to. + * The test creates a handler triggered by a specific combination of event base / event id + * and generates 2 consecutive events that should trigger the handler. The handler unregisters + * itself. Make sure that the second event does not trigger the handler again. + * + * Both event posts need to be queued before the unregistration happens, to make sure that event if + * the handler is still present in the handlers lists when the second event triggers, it will not + * be called. To make sure of that, the execution of the handler is blocked by a mutex released from + * test after the 2 events are posted. */ +TEST_CASE("self unregistered handlers are never called again after they return", "[event][default]") +{ + s_event_mutex = xSemaphoreCreateMutexStatic(&s_event_mutex_buf); + TEST_ASSERT_NOT_NULL(s_event_mutex); + + esp_err_t ret = esp_event_loop_create_default(); + printf("esp_event_loop_create_default %d\n", ret); + TEST_ESP_OK(ret); + + /* register handler for event base 1 and event id 1 */ + ret = esp_event_handler_register(s_test_base1, TEST_EVENT_BASE1_EV1, self_unregistering_handler, NULL); + printf("esp_event_handler_register %d\n", ret); + TEST_ESP_OK(ret); + + /* take the mutex to block the execution of the self_unregistering_handler */ + xSemaphoreTake(s_event_mutex, portMAX_DELAY); + + /* post 2 times the event on event base 1, event id 1 */ + ret = esp_event_post(s_test_base1, TEST_EVENT_BASE1_EV1, NULL, 0, pdMS_TO_TICKS(1000)); + printf("esp_event_post %d\n", ret); + TEST_ESP_OK(ret); + ret = esp_event_post(s_test_base1, TEST_EVENT_BASE1_EV1, NULL, 0, pdMS_TO_TICKS(1000)); + printf("esp_event_post %d\n", ret); + TEST_ESP_OK(ret); + + /* release the mutex to execute the self_unregistering_handler */ + xSemaphoreGive(s_event_mutex); + + /* make sure the handler was called only once */ + TEST_ASSERT(s_handler_triggered == 1); + + /* delete mutex */ + vSemaphoreDelete(s_event_mutex); + + /* reset the static variable in case the test gets called once more */ + s_handler_triggered = 0; + + /* delete loop */ + ret = esp_event_loop_delete_default(); + printf("esp_event_loop_delete_default %d\n", ret); + TEST_ESP_OK(ret); +} #endif // CONFIG_ESP_EVENT_LOOP_PROFILING diff --git a/components/esp_event/test_apps/pytest_esp_event.py b/components/esp_event/test_apps/pytest_esp_event.py index c5d87ecad3..71ee1bfe20 100644 --- a/components/esp_event/test_apps/pytest_esp_event.py +++ b/components/esp_event/test_apps/pytest_esp_event.py @@ -54,3 +54,16 @@ def test_esp_event_profiling(dut: Dut) -> None: matches = dut.expect(r'HANDLER .+ inv:[1-9][0-9]{0,} time:[1-9][0-9]{0,} us', timeout=2) matches_arr = matches.group().split(b'\r\n') assert (len(matches_arr) == 3) + dut.expect('1 Tests 0 Failures 0 Ignored', timeout=120) + dut.expect_exact("Enter next test, or 'enter' to see menu") + + dut.write('"esp_event_dump does not show self unregistered handler"') + # look for 1 handlers never invoked + matches = dut.expect(r'HANDLER .+ inv:0 time:0 us', timeout=2) + matches_arr = matches.group().split(b'\r\n') + assert (len(matches_arr) == 1) + dut.expect('1 Tests 0 Failures 0 Ignored', timeout=120) + dut.expect_exact("Enter next test, or 'enter' to see menu") + + dut.write('"self unregistered handlers are never called again after they return"') + dut.expect('1 Tests 0 Failures 0 Ignored', timeout=120) From bcf635dcd5f8f65bfef40a6dd7e9143f5d0eb15e Mon Sep 17 00:00:00 2001 From: David Cermak Date: Fri, 31 Jan 2025 12:31:31 +0100 Subject: [PATCH 3/4] fix(esp_event): Fix minor no-ISR post regression from 15f6775f5d5bd9ee73072e914118024d15b685eb --- components/esp_event/esp_event.c | 15 +++++---------- .../private_include/esp_event_internal.h | 4 ---- .../test_apps/main/test_event_common.cpp | 1 + .../esp_event/test_apps/sdkconfig.ci.no_isr_post | 3 +++ 4 files changed, 9 insertions(+), 14 deletions(-) create mode 100644 components/esp_event/test_apps/sdkconfig.ci.no_isr_post diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index 4edb3ff2da..3925787b2b 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -137,7 +137,7 @@ static void handler_execute(esp_event_loop_instance_t* loop, esp_event_handler_n (*(handler->handler_ctx->handler))(handler->handler_ctx->arg, post.base, post.id, data_ptr); #else - (*(handler->handler_ctx->handler))(handler->handler_ctx->arg, post.base, post.id, post.data); + (*(handler->handler_ctx->handler))(handler->handler_ctx->arg, post.base, post.id, post.data.ptr); #endif #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING @@ -438,14 +438,11 @@ static void loop_node_remove_all_handler(esp_event_loop_node_t* loop_node) static void inline __attribute__((always_inline)) post_instance_delete(esp_event_post_instance_t* post) { #if CONFIG_ESP_EVENT_POST_FROM_ISR - if (post->data_allocated && post->data.ptr) { + if (post->data_allocated) +#endif + { free(post->data.ptr); } -#else - if (post->data) { - free(post->data); - } -#endif memset(post, 0, sizeof(*post)); } @@ -944,12 +941,10 @@ esp_err_t esp_event_post_to(esp_event_loop_handle_t event_loop, esp_event_base_t } memcpy(event_data_copy, event_data, event_data_size); -#if CONFIG_ESP_EVENT_POST_FROM_ISR post.data.ptr = event_data_copy; +#if CONFIG_ESP_EVENT_POST_FROM_ISR post.data_allocated = true; post.data_set = true; -#else - post.data = event_data_copy; #endif } post.base = event_base; diff --git a/components/esp_event/private_include/esp_event_internal.h b/components/esp_event/private_include/esp_event_internal.h index f7b1612996..76304c689c 100644 --- a/components/esp_event/private_include/esp_event_internal.h +++ b/components/esp_event/private_include/esp_event_internal.h @@ -91,14 +91,10 @@ typedef struct esp_event_remove_handler_context_t { bool legacy; /**< Set to true when the handler unregistration request was made from legacy code */ } esp_event_remove_handler_context_t; -#if CONFIG_ESP_EVENT_POST_FROM_ISR typedef union esp_event_post_data { uint32_t val; void *ptr; } esp_event_post_data_t; -#else -typedef void* esp_event_post_data_t; -#endif /// Event posted to the event queue typedef struct esp_event_post_instance { diff --git a/components/esp_event/test_apps/main/test_event_common.cpp b/components/esp_event/test_apps/main/test_event_common.cpp index 569b97902e..37b656275b 100644 --- a/components/esp_event/test_apps/main/test_event_common.cpp +++ b/components/esp_event/test_apps/main/test_event_common.cpp @@ -8,6 +8,7 @@ #include #include +#include "inttypes.h" #include "sdkconfig.h" #include "freertos/FreeRTOS.h" #include "freertos/task.h" diff --git a/components/esp_event/test_apps/sdkconfig.ci.no_isr_post b/components/esp_event/test_apps/sdkconfig.ci.no_isr_post new file mode 100644 index 0000000000..1b0464b258 --- /dev/null +++ b/components/esp_event/test_apps/sdkconfig.ci.no_isr_post @@ -0,0 +1,3 @@ +# This configuration checks the event loop if posting from ISR is disabled +CONFIG_ESP_TASK_WDT_INIT=n +CONFIG_POST_EVENTS_FROM_ISR=n From fddc0bacedf9d6319f1fbf3a860e4ce7a6eff1aa Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 4 Feb 2025 10:56:35 +0100 Subject: [PATCH 4/4] fix(esp_eth): Fix test code to unregister event correctly --- components/esp_eth/test_apps/main/esp_eth_test_apps.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/components/esp_eth/test_apps/main/esp_eth_test_apps.c b/components/esp_eth/test_apps/main/esp_eth_test_apps.c index 69eec14e3a..fa707e4d2e 100644 --- a/components/esp_eth/test_apps/main/esp_eth_test_apps.c +++ b/components/esp_eth/test_apps/main/esp_eth_test_apps.c @@ -434,7 +434,10 @@ cleanup: TEST_ESP_OK(esp_eth_driver_uninstall(eth_handle)); TEST_ESP_OK(phy->del(phy)); TEST_ESP_OK(mac->del(mac)); +#ifndef CONFIG_TARGET_ETH_PHY_DEVICE_W5500 +// only unregister events if the device != W5500, since w5500 doesn't support loopback and we don't register the event TEST_ESP_OK(esp_event_handler_unregister(ETH_EVENT, ESP_EVENT_ANY_ID, eth_event_handler)); +#endif TEST_ESP_OK(esp_event_loop_delete_default()); extra_cleanup(); vEventGroupDelete(eth_event_group);