diff --git a/components/freertos/event_groups.c b/components/freertos/event_groups.c index 9467d7c9c9..2def2c1199 100644 --- a/components/freertos/event_groups.c +++ b/components/freertos/event_groups.c @@ -535,6 +535,10 @@ BaseType_t xMatchFound = pdFALSE; pxListEnd = listGET_END_MARKER( pxList ); /*lint !e826 !e740 !e9087 The mini list structure is used as the list end to save RAM. This is checked and valid. */ taskENTER_CRITICAL( &pxEventBits->eventGroupMux ); + /* The critical section above only takes the event groups spinlock. However, we are about to traverse a task list. + Thus we need call the function below to take the task list spinlock located in tasks.c. Not doing so will risk + the task list's being changed while be are traversing it. */ + vTaskTakeEventListLock(); { traceEVENT_GROUP_SET_BITS( xEventGroup, uxBitsToSet ); @@ -606,6 +610,8 @@ BaseType_t xMatchFound = pdFALSE; bit was set in the control word. */ pxEventBits->uxEventBits &= ~uxBitsToClear; } + /* Release the previously held task list spinlock, then release the event group spinlock. */ + vTaskReleaseEventListLock(); taskEXIT_CRITICAL( &pxEventBits->eventGroupMux ); return pxEventBits->uxEventBits; @@ -620,6 +626,10 @@ const List_t *pxTasksWaitingForBits = &( pxEventBits->xTasksWaitingForBits ); traceEVENT_GROUP_DELETE( xEventGroup ); taskENTER_CRITICAL( &pxEventBits->eventGroupMux ); + /* The critical section above only takes the event groups spinlock. However, we are about to traverse a task list. + Thus we need call the function below to take the task list spinlock located in tasks.c. Not doing so will risk + the task list's being changed while be are traversing it. */ + vTaskTakeEventListLock(); { while( listCURRENT_LIST_LENGTH( pxTasksWaitingForBits ) > ( UBaseType_t ) 0 ) { @@ -629,6 +639,8 @@ const List_t *pxTasksWaitingForBits = &( pxEventBits->xTasksWaitingForBits ); xTaskRemoveFromUnorderedEventList( pxTasksWaitingForBits->xListEnd.pxNext, eventUNBLOCKED_DUE_TO_BIT_SET ); } } + /* Release the previously held task list spinlock. */ + vTaskReleaseEventListLock(); taskEXIT_CRITICAL( &pxEventBits->eventGroupMux ); #if( ( configSUPPORT_DYNAMIC_ALLOCATION == 1 ) && ( configSUPPORT_STATIC_ALLOCATION == 0 ) ) diff --git a/components/freertos/include/freertos/task.h b/components/freertos/include/freertos/task.h index 559945805e..6be331368b 100644 --- a/components/freertos/include/freertos/task.h +++ b/components/freertos/include/freertos/task.h @@ -2369,6 +2369,23 @@ void vTaskPlaceOnUnorderedEventList( List_t * pxEventList, const TickType_t xIte */ void vTaskPlaceOnEventListRestricted( List_t * const pxEventList, TickType_t xTicksToWait, const BaseType_t xWaitIndefinitely ) PRIVILEGED_FUNCTION; +/* + * THIS FUNCTION MUST NOT BE USED FROM APPLICATION CODE. IT IS AN + * INTERFACE WHICH IS FOR THE EXCLUSIVE USE OF THE SCHEDULER. + * + * This function is a wrapper to take the "xTaskQueueMutex" spinlock of tasks.c. + * This lock is taken whenver any of the task lists or event lists are + * accessed/modified, such as when adding/removing tasks to/from the delayed + * task list or various event lists. + * + * This functions is meant to be called by xEventGroupSetBits() and + * vEventGroupDelete() as both those functions will access event lists (instead + * of delegating the entire responsibility to one of vTask...EventList() + * functions). + */ +void vTaskTakeEventListLock( void ); +void vTaskReleaseEventListLock( void ); + /* * THIS FUNCTION MUST NOT BE USED FROM APPLICATION CODE. IT IS AN * INTERFACE WHICH IS FOR THE EXCLUSIVE USE OF THE SCHEDULER. diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index 17710c1850..7755f91ef5 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -3563,6 +3563,18 @@ UBaseType_t i, uxTargetCPU; } /*-----------------------------------------------------------*/ +void vTaskTakeEventListLock( void ) +{ + /* We call the tasks.c critical section macro to take xTaskQueueMutex */ + taskENTER_CRITICAL(&xTaskQueueMutex); +} + +void vTaskReleaseEventListLock( void ) +{ + /* We call the tasks.c critical section macro to release xTaskQueueMutex */ + taskEXIT_CRITICAL(&xTaskQueueMutex); +} + BaseType_t xTaskRemoveFromUnorderedEventList( ListItem_t * pxEventListItem, const TickType_t xItemValue ) { TCB_t *pxUnblockedTCB; diff --git a/components/freertos/test/test_freertos_eventgroups.c b/components/freertos/test/test_freertos_eventgroups.c index db298504c0..226a893dc0 100644 --- a/components/freertos/test/test_freertos_eventgroups.c +++ b/components/freertos/test/test_freertos_eventgroups.c @@ -7,6 +7,7 @@ #include "freertos/event_groups.h" #include "driver/timer.h" #include "unity.h" +#include "test_utils.h" #ifdef CONFIG_IDF_TARGET_ESP32S2 #define int_clr_timers int_clr @@ -40,7 +41,8 @@ static void task_event_group_call_response(void *param) printf("Task %d done\n", task_num); xSemaphoreGive(done_sem); - vTaskDelete(NULL); + // Wait to be deleted + vTaskSuspend(NULL); } TEST_CASE("FreeRTOS Event Groups", "[freertos]") @@ -48,6 +50,8 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]") eg = xEventGroupCreate(); done_sem = xSemaphoreCreateCounting(NUM_TASKS, 0); + TaskHandle_t task_handles[NUM_TASKS]; + /* Note: task_event_group_call_response all have higher priority than this task, so on this core they will always preempt this task. @@ -55,7 +59,7 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]") or they get out of sync. */ for (int c = 0; c < NUM_TASKS; c++) { - xTaskCreatePinnedToCore(task_event_group_call_response, "tsk_call_resp", 4096, (void *)c, configMAX_PRIORITIES - 1, NULL, c % portNUM_PROCESSORS); + xTaskCreatePinnedToCore(task_event_group_call_response, "tsk_call_resp", 4096, (void *)c, configMAX_PRIORITIES - 1, &task_handles[c], c % portNUM_PROCESSORS); } /* Tasks all start instantly, but this task will resume running at the same time as the higher priority tasks on the @@ -66,15 +70,19 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]") /* signal all tasks with "CALL" bit... */ xEventGroupSetBits(eg, BIT_CALL); - /* Only wait for 1 tick, the wakeup should be immediate... */ - TEST_ASSERT_EQUAL_HEX16(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, 1)); + /* Wait until all tasks have set their respective response bits */ + TEST_ASSERT_EQUAL_HEX16(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, portMAX_DELAY)); } - /* Ensure all tasks cleaned up correctly */ + /* Ensure all tasks have suspend themselves */ for (int c = 0; c < NUM_TASKS; c++) { TEST_ASSERT( xSemaphoreTake(done_sem, 100/portTICK_PERIOD_MS) ); } + for (int c = 0; c < NUM_TASKS; c++) { + test_utils_task_delete(task_handles[c]); + } + vSemaphoreDelete(done_sem); vEventGroupDelete(eg); }