From db62a7b5b244efab7484013d5f45d04dbbc32fc9 Mon Sep 17 00:00:00 2001 From: ismail Date: Mon, 14 Jul 2025 14:24:26 +0300 Subject: [PATCH] Fix listener memory allocation bug. - Replaced the buggy, oldschool, plain pointer based list approach with more robust FreeRTOS linked list implementation for the listener API. Fixes #1049. - Added relevant test cases. Refs #1030. --- src/relay_chn.c | 153 ++++++++++++++++++-------------- test_apps/main/test_relay_chn.c | 122 +++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 69 deletions(-) diff --git a/src/relay_chn.c b/src/relay_chn.c index cbeab97..4e719ba 100644 --- a/src/relay_chn.c +++ b/src/relay_chn.c @@ -23,6 +23,7 @@ #include "esp_event_base.h" #include "esp_event.h" #include "relay_chn.h" +#include "freertos/idf_additions.h" #include "sdkconfig.h" @@ -180,13 +181,19 @@ static esp_err_t relay_chn_dispatch_tilt_cmd(relay_chn_t *relay_chn, relay_chn_t /** - * @brief Structure to manage the state change listeners. + * @brief Structure to hold a listener entry in the linked list. */ -struct relay_chn_state_listener_manager_type { - uint8_t listener_count; ///< The number of registered listeners. - relay_chn_state_listener_t *listeners; ///< The list that holds references to the registered listeners. -} relay_chn_state_listener_manager; +typedef struct relay_chn_listener_entry_type { + relay_chn_state_listener_t listener; ///< The listener function pointer. + ListItem_t list_item; ///< FreeRTOS list item. +} relay_chn_listener_entry_t; +/** + * @brief The list that holds references to the registered listeners. + * + * Uses a FreeRTOS list for safe and dynamic management of listeners. + */ +static List_t relay_chn_listener_list; static relay_chn_t relay_channels[RELAY_CHN_COUNT]; static esp_event_loop_handle_t relay_chn_event_loop; @@ -360,12 +367,8 @@ esp_err_t relay_chn_create(const gpio_num_t* gpio_map, uint8_t gpio_count) ESP_RETURN_ON_ERROR(ret, TAG, "Failed to initialize tilt feature"); #endif - // Init the state listener manager - relay_chn_state_listener_manager.listeners = malloc(sizeof(relay_chn_state_listener_t*)); - if (relay_chn_state_listener_manager.listeners == NULL) { - ESP_LOGE(TAG, "Failed to initialize memory for the listeners!"); - ret = ESP_ERR_NO_MEM; - } + // Init the state listener list + vListInitialise(&relay_chn_listener_list); return ret; } @@ -377,9 +380,11 @@ void relay_chn_destroy(void) relay_chn_event_loop = NULL; // Free the listeners - if (relay_chn_state_listener_manager.listeners != NULL) { - free(relay_chn_state_listener_manager.listeners); - relay_chn_state_listener_manager.listeners = NULL; + while (listCURRENT_LIST_LENGTH(&relay_chn_listener_list) > 0) { + ListItem_t *pxItem = listGET_HEAD_ENTRY(&relay_chn_listener_list); + relay_chn_listener_entry_t *entry = listGET_LIST_ITEM_OWNER(pxItem); + uxListRemove(pxItem); + free(entry); } // Destroy the timers and reset GPIOs @@ -402,74 +407,77 @@ void relay_chn_destroy(void) } } -static int relay_chn_listener_index(relay_chn_state_listener_t listener) +/** + * @brief Find a listener entry in the list by its function pointer. + * + * This function replaces the old index-based search and is used to check + * for the existence of a listener before registration or for finding it + * during unregistration. + * + * @param listener The listener function pointer to find. + * @return Pointer to the listener entry if found, otherwise NULL. + */ +static relay_chn_listener_entry_t* find_listener_entry(relay_chn_state_listener_t listener) { - for (int i = 0; i < relay_chn_state_listener_manager.listener_count; i++) { - if (relay_chn_state_listener_manager.listeners[i] == listener) { - // This is the listener to unregister. Check if it is in the middle - ESP_LOGD(TAG, "relay_chn_listener_index: Listener %p; found at index %d.", listener, i); - return i; + // Iterate through the linked list of listeners + for (ListItem_t *pxListItem = listGET_HEAD_ENTRY(&relay_chn_listener_list); + pxListItem != listGET_END_MARKER(&relay_chn_listener_list); + pxListItem = listGET_NEXT(pxListItem)) { + + relay_chn_listener_entry_t *entry = (relay_chn_listener_entry_t *) listGET_LIST_ITEM_OWNER(pxListItem); + if (entry->listener == listener) { + // Found the listener, return the entry + return entry; } } - return -1; + + // Listener was not found in the list + return NULL; } esp_err_t relay_chn_register_listener(relay_chn_state_listener_t listener) { - if (listener == NULL) { - ESP_LOGE(TAG, "relay_chn_register_listener: A NULL listener given."); - return ESP_ERR_INVALID_ARG; - } + ESP_RETURN_ON_FALSE(listener, ESP_ERR_INVALID_ARG, TAG, "Listener cannot be NULL"); - if (relay_chn_listener_index(listener) > -1) { - ESP_LOGD(TAG, "relay_chn_register_listener: The listener %p is already registered.", listener); + // Check for duplicates + if (find_listener_entry(listener) != NULL) { + ESP_LOGD(TAG, "Listener %p already registered", listener); return ESP_OK; } - ESP_LOGD(TAG, "relay_chn_register_listener: Register listener: %p", listener); - relay_chn_state_listener_manager.listeners[relay_chn_state_listener_manager.listener_count] = listener; - // Update listener count - relay_chn_state_listener_manager.listener_count++; + // Allocate memory for the new listener entry + relay_chn_listener_entry_t *entry = malloc(sizeof(relay_chn_listener_entry_t)); + ESP_RETURN_ON_FALSE(entry, ESP_ERR_NO_MEM, TAG, "Failed to allocate memory for listener"); + // Initialize and insert the new listener + entry->listener = listener; + vListInitialiseItem(&(entry->list_item)); + listSET_LIST_ITEM_OWNER(&(entry->list_item), (void *)entry); + vListInsertEnd(&relay_chn_listener_list, &(entry->list_item)); + + ESP_LOGD(TAG, "Registered listener %p", listener); return ESP_OK; } void relay_chn_unregister_listener(relay_chn_state_listener_t listener) { - if (listener == NULL) { - ESP_LOGD(TAG, "relay_chn_unregister_listener: A NULL listener given, nothing to do."); - return; - } - // Search the listener in the listeners list and get its index if exists - int i = relay_chn_listener_index(listener); - if (i == -1) { - ESP_LOGD(TAG, "relay_chn_unregister_listener: %p is not registered already.", listener); + if (listener == NULL) + { + ESP_LOGD(TAG, "Cannot unregister a NULL listener."); return; } - uint8_t max_index = relay_chn_state_listener_manager.listener_count - 1; - // Check whether the listener's index is in the middle - if (i == max_index) { - // free(&relay_chn_state_listener_manager.listeners[i]); - relay_chn_state_listener_manager.listeners[i] = NULL; + // Find the listener entry in the list + relay_chn_listener_entry_t *entry = find_listener_entry(listener); + + if (entry != NULL) { + // Remove the item from the list and free the allocated memory + uxListRemove(&(entry->list_item)); + free(entry); + ESP_LOGD(TAG, "Unregistered listener %p", listener); + } else { + ESP_LOGD(TAG, "Listener %p not found for unregistration.", listener); } - else { - // It is in the middle, so align the next elements in the list and then free the last empty pointer - // Align the next elements - uint8_t num_of_elements = max_index - i; - relay_chn_state_listener_t *pnext = NULL; - // (i + j): current index; (i + j + 1): next index - for (uint8_t j = 0; j < num_of_elements; j++) { - uint8_t current_index = i + j; - uint8_t next_index = current_index + 1; - pnext = &relay_chn_state_listener_manager.listeners[next_index]; - relay_chn_state_listener_manager.listeners[current_index] = *pnext; - } - // free(&relay_chn_state_listener_manager.listeners[max_index]); // Free the last element - relay_chn_state_listener_manager.listeners[max_index] = NULL; // Free the last element - } - // Decrease listener count - relay_chn_state_listener_manager.listener_count--; } /** @@ -524,17 +532,24 @@ static esp_err_t relay_chn_start_esp_timer_once(esp_timer_handle_t esp_timer, ui static void relay_chn_update_state(relay_chn_t *relay_chn, relay_chn_state_t new_state) { - relay_chn_state_t old = relay_chn->state; + relay_chn_state_t old_state = relay_chn->state; + + // Only update and notify if the state has actually changed. + if (old_state == new_state) { + return; + } + relay_chn->state = new_state; - for (uint8_t i = 0; i < relay_chn_state_listener_manager.listener_count; i++) { - relay_chn_state_listener_t listener = relay_chn_state_listener_manager.listeners[i]; - if (listener == NULL) { - relay_chn_state_listener_manager.listener_count -= 1; - ESP_LOGD(TAG, "relay_chn_update_state: A listener is NULL at index: %u", i); + // Iterate through the linked list of listeners and notify them. + for (ListItem_t *pxListItem = listGET_HEAD_ENTRY(&relay_chn_listener_list); + pxListItem != listGET_END_MARKER(&relay_chn_listener_list); + pxListItem = listGET_NEXT(pxListItem)) { + relay_chn_listener_entry_t *entry = (relay_chn_listener_entry_t *) listGET_LIST_ITEM_OWNER(pxListItem); + if (entry && entry->listener) { + // Emit the state change to the listeners + entry->listener(relay_chn->id, old_state, new_state); } - // Emit the state change to the listeners - listener(relay_chn->id, old, new_state); } } diff --git a/test_apps/main/test_relay_chn.c b/test_apps/main/test_relay_chn.c index c92651a..9d9eea2 100644 --- a/test_apps/main/test_relay_chn.c +++ b/test_apps/main/test_relay_chn.c @@ -6,6 +6,7 @@ #include #include #include "sdkconfig.h" // For accessing CONFIG_* values +#include // Test GPIOs and channel IDs // Please ensure these GPIOs are correct and suitable for your board. @@ -242,6 +243,127 @@ TEST_CASE("FREE to Running transition without inertia", "[relay_chn][inertia]") TEST_ASSERT_EQUAL(RELAY_CHN_STATE_FORWARD, relay_chn_get_state(ch)); } +// --- Listener Test Globals --- +typedef struct { + uint8_t chn_id; + relay_chn_state_t old_state; + relay_chn_state_t new_state; + int call_count; +} listener_callback_info_t; + +static listener_callback_info_t listener1_info; +static listener_callback_info_t listener2_info; + +// --- Listener Test Helper Functions --- + +// Clear the memory from possible garbage values +static void reset_listener_info(listener_callback_info_t* info) { + memset(info, 0, sizeof(listener_callback_info_t)); +} + +static void test_listener_1(uint8_t chn_id, relay_chn_state_t old_state, relay_chn_state_t new_state) { + listener1_info.chn_id = chn_id; + listener1_info.old_state = old_state; + listener1_info.new_state = new_state; + listener1_info.call_count++; +} + +static void test_listener_2(uint8_t chn_id, relay_chn_state_t old_state, relay_chn_state_t new_state) { + listener2_info.chn_id = chn_id; + listener2_info.old_state = old_state; + listener2_info.new_state = new_state; + listener2_info.call_count++; +} + +// ### Listener Functionality Tests + +TEST_CASE("Listener is called on state change", "[relay_chn][listener]") { + uint8_t ch = 0; + reset_listener_info(&listener1_info); + + // 1. Register the listener + TEST_ESP_OK(relay_chn_register_listener(test_listener_1)); + + // 2. Trigger a state change + relay_chn_run_forward(ch); + vTaskDelay(pdMS_TO_TICKS(test_delay_margin_ms)); // Allow event to be processed + + // 3. Verify the listener was called with correct parameters + TEST_ASSERT_EQUAL(1, listener1_info.call_count); + TEST_ASSERT_EQUAL(ch, listener1_info.chn_id); + TEST_ASSERT_EQUAL(RELAY_CHN_STATE_FREE, listener1_info.old_state); + TEST_ASSERT_EQUAL(RELAY_CHN_STATE_FORWARD, listener1_info.new_state); + + // 4. Unregister to clean up + relay_chn_unregister_listener(test_listener_1); +} + +TEST_CASE("Unregistered listener is not called", "[relay_chn][listener]") { + uint8_t ch = 0; + reset_listener_info(&listener1_info); + + // 1. Register and then immediately unregister the listener + TEST_ESP_OK(relay_chn_register_listener(test_listener_1)); + relay_chn_unregister_listener(test_listener_1); + + // 2. Trigger a state change + relay_chn_run_forward(ch); + vTaskDelay(pdMS_TO_TICKS(test_delay_margin_ms)); + + // 3. Verify the listener was NOT called + TEST_ASSERT_EQUAL(0, listener1_info.call_count); +} + +TEST_CASE("Multiple listeners are called on state change", "[relay_chn][listener]") { + uint8_t ch = 0; + reset_listener_info(&listener1_info); + reset_listener_info(&listener2_info); + + // 1. Register two different listeners + TEST_ESP_OK(relay_chn_register_listener(test_listener_1)); + TEST_ESP_OK(relay_chn_register_listener(test_listener_2)); + + // 2. Trigger a state change + relay_chn_run_forward(ch); + vTaskDelay(pdMS_TO_TICKS(test_delay_margin_ms)); + + // 3. Verify listener 1 was called correctly + TEST_ASSERT_EQUAL(1, listener1_info.call_count); + TEST_ASSERT_EQUAL(RELAY_CHN_STATE_FREE, listener1_info.old_state); + TEST_ASSERT_EQUAL(RELAY_CHN_STATE_FORWARD, listener1_info.new_state); + + // 4. Verify listener 2 was also called correctly + TEST_ASSERT_EQUAL(1, listener2_info.call_count); + TEST_ASSERT_EQUAL(RELAY_CHN_STATE_FREE, listener2_info.old_state); + TEST_ASSERT_EQUAL(RELAY_CHN_STATE_FORWARD, listener2_info.new_state); + + // 5. Clean up + relay_chn_unregister_listener(test_listener_1); + relay_chn_unregister_listener(test_listener_2); +} + +TEST_CASE("Listener registration handles invalid arguments and duplicates", "[relay_chn][listener]") { + reset_listener_info(&listener1_info); + + // 1. Registering a NULL listener should fail + TEST_ASSERT_EQUAL(ESP_ERR_INVALID_ARG, relay_chn_register_listener(NULL)); + + // 2. Unregistering a NULL listener should not crash + relay_chn_unregister_listener(NULL); + + // 3. Registering the same listener twice should be handled gracefully + TEST_ESP_OK(relay_chn_register_listener(test_listener_1)); + TEST_ESP_OK(relay_chn_register_listener(test_listener_1)); // Second call should be a no-op + + // 4. Trigger a state change and verify the listener is only called ONCE + relay_chn_run_forward(0); + vTaskDelay(pdMS_TO_TICKS(test_delay_margin_ms)); + TEST_ASSERT_EQUAL(1, listener1_info.call_count); + + // 5. Clean up + relay_chn_unregister_listener(test_listener_1); +} + // ### Tilt Functionality Tests (Conditional)