From cb871f0472a59d418ccf9a217397f3eeccf20f95 Mon Sep 17 00:00:00 2001 From: Rusty Eddy Date: Tue, 4 Feb 2020 07:38:44 -0800 Subject: [PATCH 1/5] Added semi-colon to esp_event_loop_create(...) Title sums it up. --- docs/en/api-reference/system/esp_event.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/api-reference/system/esp_event.rst b/docs/en/api-reference/system/esp_event.rst index b0a96e4ce..cc6c41aea 100644 --- a/docs/en/api-reference/system/esp_event.rst +++ b/docs/en/api-reference/system/esp_event.rst @@ -54,7 +54,7 @@ In code, the flow above may look like as follows: esp_event_loop_handle_t loop_handle; - esp_event_loop_create(&loop_args, &loop_handle) + esp_event_loop_create(&loop_args, &loop_handle); // 3. Register event handler defined in (1). MY_EVENT_BASE and MY_EVENT_ID specifies a hypothetical // event that handler run_on_event should execute on when it gets posted to the loop. From 66949e3b5466d88df1d1cc170ec535983ec9b37a Mon Sep 17 00:00:00 2001 From: Xentec Date: Tue, 1 Oct 2019 01:31:09 +0200 Subject: [PATCH 2/5] esp_event: fix crash when unregistering a handler instance in itself When a handler instance is the last one in the list und unregisters itself, the handler iterator will be invalidated by entering free'd memory. Same applies for event base and id, if they become empty. Merges https://github.com/espressif/esp-idf/pull/4139 --- components/esp_event/esp_event.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index 99a3cf70b..5659b6378 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -526,30 +526,30 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick bool exec = false; - esp_event_handler_instance_t *handler; + esp_event_handler_instance_t *handler, *temp_handler; esp_event_loop_node_t *loop_node; - esp_event_base_node_t *base_node; - esp_event_id_node_t *id_node; + esp_event_base_node_t *base_node, *temp_base; + esp_event_id_node_t *id_node, *temp_id_node; SLIST_FOREACH(loop_node, &(loop->loop_nodes), next) { // Execute loop level handlers - SLIST_FOREACH(handler, &(loop_node->handlers), next) { + SLIST_FOREACH_SAFE(handler, &(loop_node->handlers), next, temp_handler) { handler_execute(loop, handler, post); exec |= true; } - SLIST_FOREACH(base_node, &(loop_node->base_nodes), next) { + SLIST_FOREACH_SAFE(base_node, &(loop_node->base_nodes), next, temp_base) { if (base_node->base == post.base) { // Execute base level handlers - SLIST_FOREACH(handler, &(base_node->handlers), next) { + SLIST_FOREACH_SAFE(handler, &(base_node->handlers), next, temp_handler) { handler_execute(loop, handler, post); exec |= true; } - SLIST_FOREACH(id_node, &(base_node->id_nodes), next) { + 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(handler, &(id_node->handlers), next) { + SLIST_FOREACH_SAFE(handler, &(id_node->handlers), next, temp_handler) { handler_execute(loop, handler, post); exec |= true; } From b79062aeec14501919e7a0028b523462a7c150ce Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Fri, 18 Oct 2019 12:54:42 +0800 Subject: [PATCH 3/5] esp_event: iterate loop nodes safely as well --- components/esp_event/esp_event.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index 5659b6378..4882d1a60 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -527,11 +527,11 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick bool exec = false; esp_event_handler_instance_t *handler, *temp_handler; - esp_event_loop_node_t *loop_node; + esp_event_loop_node_t *loop_node, *temp_node; esp_event_base_node_t *base_node, *temp_base; esp_event_id_node_t *id_node, *temp_id_node; - SLIST_FOREACH(loop_node, &(loop->loop_nodes), next) { + 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); From 39e8e2003a0c95e76548337e62c9bad0b5c26ac9 Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Tue, 8 Oct 2019 15:32:51 +0800 Subject: [PATCH 4/5] esp_event: test that handlers can unregister themselves --- components/esp_event/test/test_event.c | 67 ++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/components/esp_event/test/test_event.c b/components/esp_event/test/test_event.c index 0efc2d492..45e276767 100644 --- a/components/esp_event/test/test_event.c +++ b/components/esp_event/test/test_event.c @@ -203,6 +203,7 @@ static void test_event_simple_handler_registration_task(void* args) vTaskDelete(NULL); } + static void test_handler_post_w_task(void* event_handler_arg, esp_event_base_t event_base, int32_t event_id, void* event_data) { simple_arg_t* arg = (simple_arg_t*) event_handler_arg; @@ -254,6 +255,17 @@ static void test_handler_post_wo_task(void* event_handler_arg, esp_event_base_t } } +static void test_handler_unregister_itself(void* event_handler_arg, esp_event_base_t event_base, int32_t event_id, void* event_data) +{ + esp_event_loop_handle_t* loop = (esp_event_loop_handle_t*) event_data; + int* unregistered = (int*) event_handler_arg; + + (*unregistered) += (event_base == s_test_base1 ? 0 : 10) + event_id + 1; + + // Unregister this handler for this event + TEST_ASSERT_EQUAL(ESP_OK, esp_event_handler_unregister_with(*loop, event_base, event_id, test_handler_unregister_itself)); +} + static void test_post_from_handler_loop_task(void* args) { esp_event_loop_handle_t event_loop = (esp_event_loop_handle_t) args; @@ -472,6 +484,61 @@ TEST_CASE("can unregister handler", "[event]") TEST_TEARDOWN(); } +TEST_CASE("handler can unregister itself", "[event]") +{ + /* this test aims to verify that handlers can unregister themselves */ + + TEST_SETUP(); + + esp_event_loop_handle_t loop; + esp_event_loop_args_t loop_args = test_event_get_default_loop_args(); + + loop_args.task_name = NULL; + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_create(&loop_args, &loop)); + + int unregistered = 0; + + /* + * s_test_base1, ev1 = 1 + * s_test_base1, ev2 = 2 + * s_test_base2, ev1 = 11 + * s_test_base2, ev2 = 12 + */ + int expected_unregistered = 0; + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_handler_register_with(loop, s_test_base1, TEST_EVENT_BASE1_EV1, test_handler_unregister_itself, &unregistered)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_handler_register_with(loop, s_test_base1, TEST_EVENT_BASE1_EV2, test_handler_unregister_itself, &unregistered)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_handler_register_with(loop, s_test_base2, TEST_EVENT_BASE2_EV1, test_handler_unregister_itself, &unregistered)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_handler_register_with(loop, s_test_base2, TEST_EVENT_BASE2_EV2, test_handler_unregister_itself, &unregistered)); + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base1, TEST_EVENT_BASE1_EV2, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_run(loop, pdMS_TO_TICKS(10))); + expected_unregistered = 2; // base1, ev2 + TEST_ASSERT_EQUAL(expected_unregistered, unregistered); + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base1, TEST_EVENT_BASE1_EV1, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base2, TEST_EVENT_BASE2_EV1, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_run(loop, pdMS_TO_TICKS(10))); + expected_unregistered += 1 + 11; // base1, ev1 + base2, ev1 + TEST_ASSERT_EQUAL(expected_unregistered, unregistered); + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base2, TEST_EVENT_BASE2_EV2, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_run(loop, pdMS_TO_TICKS(10))); + expected_unregistered += 12; // base2, ev2 + TEST_ASSERT_EQUAL(expected_unregistered, unregistered); + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base1, TEST_EVENT_BASE1_EV1, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base1, TEST_EVENT_BASE1_EV2, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base2, TEST_EVENT_BASE2_EV1, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base2, TEST_EVENT_BASE2_EV2, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_run(loop, pdMS_TO_TICKS(10))); + TEST_ASSERT_EQUAL(expected_unregistered, unregistered); // all handlers unregistered + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_delete(loop)); + + TEST_TEARDOWN(); +} + TEST_CASE("can exit running loop at approximately the set amount of time", "[event]") { /* this test aims to verify that running loop does not block indefinitely in cases where From aed56d4da9680b7bdceac17700d7b7ee9549d7fa Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Fri, 25 Oct 2019 13:14:05 +0800 Subject: [PATCH 5/5] esp_event: remove extra line from source file --- components/esp_event/test/test_event.c | 1 - 1 file changed, 1 deletion(-) diff --git a/components/esp_event/test/test_event.c b/components/esp_event/test/test_event.c index 45e276767..5ca910f2d 100644 --- a/components/esp_event/test/test_event.c +++ b/components/esp_event/test/test_event.c @@ -203,7 +203,6 @@ static void test_event_simple_handler_registration_task(void* args) vTaskDelete(NULL); } - static void test_handler_post_w_task(void* event_handler_arg, esp_event_base_t event_base, int32_t event_id, void* event_data) { simple_arg_t* arg = (simple_arg_t*) event_handler_arg;