From 96fce0c9c41df7f1b3eddd8086412c2031ff1a38 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Wed, 19 Oct 2022 22:18:32 +0800 Subject: [PATCH] freertos: Fix/remove flakey tests after migration This commit fixes/ignores flakey freertos unit tests after migrating them to the test app: - Added vTaskDelay() before teardown to prevent memory leaks - Adjusted the "main" task's priority so that scheudling tasks would work - pytest now only runs tests that are not ignored - Reset tests are temporarily ignored. Will be enabled to dedicate reset tests. - Some flakey tests are fixed by adjusting delays and stack sizes. --- .../kernel/queue/test_freertos_mutex.c | 6 +++- .../kernel/tasks/test_eTaskGetState.c | 8 ++--- .../freertos/kernel/tasks/test_task_delay.c | 32 +++++++++++-------- .../kernel/tasks/test_task_suspend_resume.c | 12 ++++++- .../freertos/main/test_freertos_main.c | 18 ++++++++++- .../test_apps/freertos/pytest_freertos.py | 5 +-- .../test_apps/freertos/sdkconfig.defaults | 1 + 7 files changed, 60 insertions(+), 22 deletions(-) diff --git a/components/freertos/test_apps/freertos/kernel/queue/test_freertos_mutex.c b/components/freertos/test_apps/freertos/kernel/queue/test_freertos_mutex.c index 4d6d4a0b75..faddacab9b 100644 --- a/components/freertos/test_apps/freertos/kernel/queue/test_freertos_mutex.c +++ b/components/freertos/test_apps/freertos/kernel/queue/test_freertos_mutex.c @@ -20,7 +20,11 @@ static void mutex_release_task(void* arg) TEST_FAIL_MESSAGE("should not be reached"); } -TEST_CASE("mutex released not by owner causes an assert", "[freertos][reset=assert,SW_CPU_RESET]") +/* +Reset tests are temporarily ignored until the test app supports running them separately. +IDF-6096 +*/ +TEST_CASE("mutex released not by owner causes an assert", "[freertos][ignore][reset=assert,SW_CPU_RESET]") { SemaphoreHandle_t mutex = xSemaphoreCreateMutex(); xSemaphoreTake(mutex, portMAX_DELAY); diff --git a/components/freertos/test_apps/freertos/kernel/tasks/test_eTaskGetState.c b/components/freertos/test_apps/freertos/kernel/tasks/test_eTaskGetState.c index 7b5a48f7dd..cd742e0a8a 100644 --- a/components/freertos/test_apps/freertos/kernel/tasks/test_eTaskGetState.c +++ b/components/freertos/test_apps/freertos/kernel/tasks/test_eTaskGetState.c @@ -56,13 +56,13 @@ TEST_CASE("Test eTaskGetState", "[freertos]") // Create tasks of each state on each core for (int i = 0; i < portNUM_PROCESSORS; i++) { - TEST_ASSERT_EQUAL(pdPASS, xTaskCreatePinnedToCore(blocked_task, "blkd", configMINIMAL_STACK_SIZE, NULL, UNITY_FREERTOS_PRIORITY - 1, &blocked_tasks[i], i)); - TEST_ASSERT_EQUAL(pdPASS, xTaskCreatePinnedToCore(suspended_task, "susp", configMINIMAL_STACK_SIZE, NULL, UNITY_FREERTOS_PRIORITY - 1, &suspended_tasks[i], i)); - TEST_ASSERT_EQUAL(pdPASS, xTaskCreatePinnedToCore(loop_task, "rdy", configMINIMAL_STACK_SIZE, NULL, UNITY_FREERTOS_PRIORITY - 1, &ready_tasks[i], i)); + TEST_ASSERT_EQUAL(pdPASS, xTaskCreatePinnedToCore(blocked_task, "blkd", configMINIMAL_STACK_SIZE * 2, NULL, UNITY_FREERTOS_PRIORITY - 1, &blocked_tasks[i], i)); + TEST_ASSERT_EQUAL(pdPASS, xTaskCreatePinnedToCore(suspended_task, "susp", configMINIMAL_STACK_SIZE * 2, NULL, UNITY_FREERTOS_PRIORITY - 1, &suspended_tasks[i], i)); + TEST_ASSERT_EQUAL(pdPASS, xTaskCreatePinnedToCore(loop_task, "rdy", configMINIMAL_STACK_SIZE * 2, NULL, UNITY_FREERTOS_PRIORITY - 1, &ready_tasks[i], i)); if (i == UNITY_FREERTOS_CPU) { running_tasks[i] = xTaskGetCurrentTaskHandle(); } else { - xTaskCreatePinnedToCore(loop_task, "run", configMINIMAL_STACK_SIZE, NULL, UNITY_FREERTOS_PRIORITY, &running_tasks[i], i); + xTaskCreatePinnedToCore(loop_task, "run", configMINIMAL_STACK_SIZE * 2, NULL, UNITY_FREERTOS_PRIORITY, &running_tasks[i], i); } } diff --git a/components/freertos/test_apps/freertos/kernel/tasks/test_task_delay.c b/components/freertos/test_apps/freertos/kernel/tasks/test_task_delay.c index 9a54db40b5..a4eeae8756 100644 --- a/components/freertos/test_apps/freertos/kernel/tasks/test_task_delay.c +++ b/components/freertos/test_apps/freertos/kernel/tasks/test_task_delay.c @@ -27,8 +27,8 @@ Procedure: - For single core, run the test directly from the UnityTask - For SMP, run the test once on each core (using vTestOnAllCores()) Expected: - - The elapsed ticks should be TEST_VTASKDELAY_TICKS, with 1 tick of error allowed (in case ref clock functions last - long enough to cross a tick boundary). + - The elapsed ticks should be TEST_VTASKDELAY_TICKS, with 1 tick of error allowed (in case the delay and ref clock + functions last long enough to cross a tick boundary). - The elapsed time should be equivalent to TEST_VTASKDELAY_TICKS tick periods, with 1 tick period of error allowed (in case ref clock functions last longer that a tick period). */ @@ -57,8 +57,8 @@ static void test_vTaskDelay(void *arg) tick_end = xTaskGetTickCount(); ref_clock_end = portTEST_REF_CLOCK_GET_TIME(); - /* Check that elapsed ticks and ref clock is accurate. We allow 1 tick of error in case vTaskDelay() was called - * right before/after the tick boundary. */ + /* Check that elapsed ticks and ref clock is accurate. We allow 1 tick of error in case vTaskDelay() or + * portTEST_REF_CLOCK_GET_TIME() last long enough to cross a tick boundary */ #if ( configUSE_16_BIT_TICKS == 1 ) TEST_ASSERT_UINT16_WITHIN(1, TEST_VTASKDELAY_TICKS, tick_end - tick_start); #else @@ -102,8 +102,8 @@ Procedure: - For single core, run the test directly from the UnityTask - For SMP, run the test once on each core (using vTestOnAllCores()) Expected: - - The elapsed ticks should be exactly TEST_VTASKDELAYUNTIL_TICKS since vTaskDelayUntil() is relative to the previous - wake time + - The elapsed ticks should be TEST_VTASKDELAYUNTIL_TICKS, with 1 tick of error allowed (in case the delay and ref + clock functions last long enough to cross a tick boundary). - The elapsed time should be equivalent to TEST_VTASKDELAYUNTIL_TICKS tick periods, with 1 tick period of error allowed (in case ref clock functions last longer that a tick period). */ @@ -115,13 +115,13 @@ Expected: static void test_vTaskDelayUntil(void *arg) { - /* Delay until the next tick boundary */ - vTaskDelay(1); - for (int i = 0; i < TEST_VTASKDELAYUNTIL_ITERATIONS; i++) { TickType_t tick_start, tick_end, last_wake_tick; portTEST_REF_CLOCK_TYPE ref_clock_start, ref_clock_end; + /* Delay until the next tick boundary */ + vTaskDelay(1); + /* Get the current tick count and ref clock time */ tick_start = xTaskGetTickCount(); last_wake_tick = tick_start; @@ -133,10 +133,16 @@ static void test_vTaskDelayUntil(void *arg) tick_end = xTaskGetTickCount(); ref_clock_end = portTEST_REF_CLOCK_GET_TIME(); - /* Check that the elapsed ticks is accurate. Elapsed ticks should be exact as vTaskDelayUntil() executes a - * delay relative to last_wake_tick. */ - TEST_ASSERT_EQUAL(TEST_VTASKDELAYUNTIL_TICKS, tick_end - tick_start); - TEST_ASSERT_EQUAL(tick_end, last_wake_tick); + + /* Check that elapsed ticks and ref clock is accurate. We allow 1 tick of error in case vTaskDelayUntil() or + * portTEST_REF_CLOCK_GET_TIME() last long enough to cross a tick boundary */ + #if ( configUSE_16_BIT_TICKS == 1 ) + TEST_ASSERT_UINT16_WITHIN(1, TEST_VTASKDELAYUNTIL_TICKS, tick_end - tick_start); + TEST_ASSERT_UINT16_WITHIN(1, tick_end, last_wake_tick); + #else + TEST_ASSERT_UINT32_WITHIN(1, TEST_VTASKDELAYUNTIL_TICKS, tick_end - tick_start); + TEST_ASSERT_UINT32_WITHIN(1, tick_end, last_wake_tick); + #endif /* Check that the elapsed ref clock time is accurate. We allow 1 tick time worth of error to account for the * the execution time of the ref clock functions. */ diff --git a/components/freertos/test_apps/freertos/kernel/tasks/test_task_suspend_resume.c b/components/freertos/test_apps/freertos/kernel/tasks/test_task_suspend_resume.c index dfee45eedb..307c91cb77 100644 --- a/components/freertos/test_apps/freertos/kernel/tasks/test_task_suspend_resume.c +++ b/components/freertos/test_apps/freertos/kernel/tasks/test_task_suspend_resume.c @@ -80,7 +80,17 @@ TEST_CASE("Suspend/resume task on same core", "[freertos]") } #ifndef CONFIG_FREERTOS_UNICORE -TEST_CASE("Suspend/resume task on other core", "[freertos]") +/* +Note: This test is ignore for now due to a known issue (xKernelLock contention + task state change will lead to a race +condition). More specifically: + +- test_suspend_resume() suspends task_count() thus moving it to the suspended list +- But task_count() is already contesting the xKernelLock. +- Changes its own task list once it takes the xKernelLock +- xKernelLock never receives the crosscore interrupt as it was contesting for xKernelLock +Addressed in IDF-5844 +*/ +TEST_CASE("Suspend/resume task on other core", "[freertos][ignore]") { test_suspend_resume(!UNITY_FREERTOS_CPU); } diff --git a/components/freertos/test_apps/freertos/main/test_freertos_main.c b/components/freertos/test_apps/freertos/main/test_freertos_main.c index e8247d9657..2293213827 100644 --- a/components/freertos/test_apps/freertos/main/test_freertos_main.c +++ b/components/freertos/test_apps/freertos/main/test_freertos_main.c @@ -4,11 +4,14 @@ * SPDX-License-Identifier: Apache-2.0 */ +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" #include "unity.h" #include "unity_test_runner.h" #include "esp_heap_caps.h" +#include "test_utils.h" -#define TEST_MEMORY_LEAK_THRESHOLD (-256) +#define TEST_MEMORY_LEAK_THRESHOLD (-512) static size_t before_free_8bit; static size_t before_free_32bit; @@ -28,6 +31,8 @@ void setUp(void) void tearDown(void) { + // Add a short delay of 10ms to allow the idle task to free an remaining memory + vTaskDelay(pdMS_TO_TICKS(10)); size_t after_free_8bit = heap_caps_get_free_size(MALLOC_CAP_8BIT); size_t after_free_32bit = heap_caps_get_free_size(MALLOC_CAP_32BIT); check_leak(before_free_8bit, after_free_8bit, "8BIT"); @@ -36,5 +41,16 @@ void tearDown(void) void app_main(void) { + /* + Some FreeRTOS tests are reliant on the main task being at priority UNITY_FREERTOS_PRIORITY to test scheduling + behavior. Thus, we raise the main task's priority before any tasks are run. See IDF-6088 + */ + vTaskPrioritySet(NULL, UNITY_FREERTOS_PRIORITY); + printf(" ______ _____ _______ ____ _____\n"); + printf("| ____| | __ \\__ __/ __ \\ / ____|\n"); + printf("| |__ _ __ ___ ___| |__) | | | | | | | (___\n"); + printf("| __| '__/ _ \\/ _ \\ _ / | | | | | |\\___ \\\n"); + printf("| | | | | __/ __/ | \\ \\ | | | |__| |____) |\n"); + printf("|_| |_| \\___|\\___|_| \\_\\ |_| \\____/|_____/\n"); unity_run_menu(); } diff --git a/components/freertos/test_apps/freertos/pytest_freertos.py b/components/freertos/test_apps/freertos/pytest_freertos.py index dee8f04e11..02b927997a 100644 --- a/components/freertos/test_apps/freertos/pytest_freertos.py +++ b/components/freertos/test_apps/freertos/pytest_freertos.py @@ -18,5 +18,6 @@ CONFIGS = [ @pytest.mark.parametrize('config', CONFIGS, indirect=True) def test_freertos(dut: Dut) -> None: dut.expect_exact('Press ENTER to see the list of tests') - dut.write('*') - dut.expect_unity_test_output() + dut.write('![ignore]') + # All of the FreeRTOS tests combined take > 60s to run. So we use a 120s timeout + dut.expect_unity_test_output(timeout=120) diff --git a/components/freertos/test_apps/freertos/sdkconfig.defaults b/components/freertos/test_apps/freertos/sdkconfig.defaults index 44149a08d4..8905783d27 100644 --- a/components/freertos/test_apps/freertos/sdkconfig.defaults +++ b/components/freertos/test_apps/freertos/sdkconfig.defaults @@ -1,3 +1,4 @@ +CONFIG_ESP_MAIN_TASK_STACK_SIZE=8192 CONFIG_ESP_TASK_WDT=n CONFIG_FREERTOS_HZ=1000 CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS=3