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

@@ -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);
     }
 }
 

test_apps/main/test_relay_chn.c

@@ -6,6 +6,7 @@
 #include <freertos/FreeRTOS.h>
 #include <freertos/task.h>
 #include "sdkconfig.h" // For accessing CONFIG_* values
+#include <string.h>
 
 // Test GPIOs and channel IDs
 // Please ensure these GPIOs are correct and suitable for your board.
@@ -42,8 +43,9 @@ TEST_CASE("Relay channels initialize correctly to FREE state", "[relay_chn]") {
 
 // TEST_CASE 1: Test that relays do nothing when an invlid channel id given
 TEST_CASE("Run forward does nothing if channel id is invalid", "[relay_chn]") {
-    for (uint8_t i = relay_chn_count*2; i < relay_chn_count; i++) {
-        relay_chn_run_forward(i); // relay_chn_run_forward returns void
+    for (uint8_t i = 0; i < relay_chn_count; i++) {
+        int invalid_id = relay_chn_count * 2 + i;
+        relay_chn_run_forward(invalid_id); // relay_chn_run_forward returns void
         // Short delay for state to update
         vTaskDelay(pdMS_TO_TICKS(test_delay_margin_ms));
         TEST_ASSERT_EQUAL(RELAY_CHN_STATE_FREE, relay_chn_get_state(i));
@@ -62,8 +64,9 @@ TEST_CASE("Relay channels run forward and update state", "[relay_chn]") {
 
 // TEST_CASE 3: Test that relays do nothing when an invlid channel id given
 TEST_CASE("Run reverse does nothing if channel id is invalid", "[relay_chn]") {
-    for (uint8_t i = relay_chn_count*2; i < relay_chn_count; i++) {
-        relay_chn_run_reverse(i); // relay_chn_run_forward returns void
+    for (uint8_t i = 0; i < relay_chn_count; i++) {
+        int invalid_id = relay_chn_count * 2 + i;
+        relay_chn_run_reverse(invalid_id); // relay_chn_run_forward returns void
         // Short delay for state to update
         vTaskDelay(pdMS_TO_TICKS(test_delay_margin_ms));
         TEST_ASSERT_EQUAL(RELAY_CHN_STATE_FREE, relay_chn_get_state(i));
@@ -102,27 +105,31 @@ TEST_CASE("Relay channels stop and update to FREE state", "[relay_chn]") {
 
 // TEST_CASE 6: Get state should return UNDEFINED when id is not valid
 TEST_CASE("Get state returns UNDEFINED when id is invalid", "[relay_chn]") {
-    for (uint8_t i = relay_chn_count*2; i < relay_chn_count; i++) {
-        TEST_ASSERT_EQUAL(RELAY_CHN_STATE_UNDEFINED, relay_chn_get_state(i));
+    for (uint8_t i = 0; i < relay_chn_count; i++) {
+        int invalid_id = relay_chn_count * 2 + i;
+        TEST_ASSERT_EQUAL(RELAY_CHN_STATE_UNDEFINED, relay_chn_get_state(invalid_id));
     }
     // Test for running states also
     relay_chn_run_forward(RELAY_CHN_ID_ALL);
     vTaskDelay(pdMS_TO_TICKS(test_delay_margin_ms));
-    for (uint8_t i = relay_chn_count*2; i < relay_chn_count; i++) {
-        TEST_ASSERT_EQUAL(RELAY_CHN_STATE_UNDEFINED, relay_chn_get_state(i));
+    for (uint8_t i = 0; i < relay_chn_count; i++) {
+        int invalid_id = relay_chn_count * 2 + i;
+        TEST_ASSERT_EQUAL(RELAY_CHN_STATE_UNDEFINED, relay_chn_get_state(invalid_id));
     }
 }
 
 // TEST_CASE 7: Get state string should return "UNKNOWN" when id is not valid
 TEST_CASE("Get state string returns UNKNOWN when id is invalid", "[relay_chn]") {
-    for (uint8_t i = relay_chn_count*2; i < relay_chn_count; i++) {
-        TEST_ASSERT_EQUAL_STRING("UNKNOWN", relay_chn_get_state_str(i));
+    for (uint8_t i = 0; i < relay_chn_count; i++) {
+        int invalid_id = relay_chn_count * 2 + i;
+        TEST_ASSERT_EQUAL_STRING("UNKNOWN", relay_chn_get_state_str(invalid_id));
     }
     // Test for running states also
     relay_chn_run_forward(RELAY_CHN_ID_ALL);
     vTaskDelay(pdMS_TO_TICKS(test_delay_margin_ms));
-    for (uint8_t i = relay_chn_count*2; i < relay_chn_count; i++) {
-        TEST_ASSERT_EQUAL_STRING("UNKNOWN", relay_chn_get_state_str(i));
+    for (uint8_t i = 0; i < relay_chn_count; i++) {
+        int invalid_id = relay_chn_count * 2 + i;
+        TEST_ASSERT_EQUAL_STRING("UNKNOWN", relay_chn_get_state_str(invalid_id));
     }
 }
 
@@ -236,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)