From f4eaa7c7b63abdd4aa12447f26c796abf9e8b389 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Fri, 25 Jun 2021 10:51:59 -0400 Subject: [PATCH 1/2] Fix #1083, timer reconfig tests The underlying OS mechanism that rejects calls to timer APIs from timer callbacks does not work on anything except POSIX. This skips these unit tests on non-POSIX platforms, and also adds documentation clearly indicating that the API must not be called from a timer context. --- src/os/inc/osapi-timebase.h | 17 ++++++++++++++++- src/os/inc/osapi-timer.h | 18 ++++++++++++++++++ src/os/posix/build_options.cmake | 3 +-- src/os/rtems/build_options.cmake | 3 +-- src/os/vxworks/build_options.cmake | 3 +-- .../time-base-api-test/time-base-api-test.c | 6 ++++-- src/unit-tests/ostimer-test/ut_ostimer_test.c | 4 ++++ 7 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/os/inc/osapi-timebase.h b/src/os/inc/osapi-timebase.h index 39b23e977..2a51c3c80 100644 --- a/src/os/inc/osapi-timebase.h +++ b/src/os/inc/osapi-timebase.h @@ -73,6 +73,9 @@ typedef struct * be configured to support at least (OS_MAX_TASKS + OS_MAX_TIMEBASES) threads, * to account for the helper threads associated with time base objects. * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. + * * @param[out] timebase_id will be set to the non-zero ID of the newly-created resource @nonnull * @param[in] timebase_name The name of the time base @nonnull * @param[in] external_sync A synchronization function for BSP hardware-based timer ticks @@ -101,6 +104,9 @@ int32 OS_TimeBaseCreate(osal_id_t *timebase_id, const char *timebase_name, OS_Ti * This function has no effect for time bases that are using * a BSP-provided external_sync function. * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. + * * @param[in] timebase_id The timebase resource to configure * @param[in] start_time The amount of delay for the first tick, in microseconds. * @param[in] interval_time The amount of delay between ticks, in microseconds. @@ -120,6 +126,9 @@ int32 OS_TimeBaseSet(osal_id_t timebase_id, uint32 start_time, uint32 interval_t * The helper task and any other resources associated with the time base * abstraction will be freed. * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. + * * @param[in] timebase_id The timebase resource to delete * * @return Execution status, see @ref OSReturnCodes @@ -135,6 +144,9 @@ int32 OS_TimeBaseDelete(osal_id_t timebase_id); * * Given a time base name, find and output the ID associated with it. * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. + * * @param[out] timebase_id will be set to the non-zero ID of the matching resource @nonnull * @param[in] timebase_name The name of the timebase resource to find @nonnull * @@ -155,7 +167,10 @@ int32 OS_TimeBaseGetIdByName(osal_id_t *timebase_id, const char *timebase_name); * relevant information about the time base resource. * * This function will pass back a pointer to structure that contains - * all of the relevant info( name and creator) about the specified timebase. + * all of the relevant info( name and creator) about the specified timebase. + * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. * * @param[in] timebase_id The timebase resource ID * @param[out] timebase_prop Buffer to store timebase properties @nonnull diff --git a/src/os/inc/osapi-timer.h b/src/os/inc/osapi-timer.h index 4c10b3f4d..b66c20684 100644 --- a/src/os/inc/osapi-timer.h +++ b/src/os/inc/osapi-timer.h @@ -67,6 +67,9 @@ typedef struct * @note clock_accuracy comes from the underlying OS tick value. The nearest integer * microsecond value is returned, so may not be exact. * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. + * * @sa OS_TimerCallback_t * * @param[out] timer_id Will be set to the non-zero resource ID of the timer object @nonnull @@ -109,6 +112,9 @@ int32 OS_TimerCreate(osal_id_t *timer_id, const char *timer_name, uint32 *clock_ * function by the OSAL, and the arg parameter is passed through from the * callback_arg argument on this call. * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. + * * @sa OS_ArgCallback_t * * @param[out] timer_id Will be set to the non-zero resource ID of the timer object @nonnull @@ -149,6 +155,9 @@ int32 OS_TimerAdd(osal_id_t *timer_id, const char *timer_name, osal_id_t timebas * or interval_msec parameters are less than the accuracy, they will be rounded * up to the accuracy of the timer. * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. + * * @param[in] timer_id The timer ID to operate on * @param[in] start_time Time in microseconds to the first expiration * @param[in] interval_time Time in microseconds between subsequent intervals, value @@ -171,6 +180,9 @@ int32 OS_TimerSet(osal_id_t timer_id, uint32 start_time, uint32 interval_time); * The application callback associated with the timer will be stopped, * and the resources freed for future use. * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. + * * @param[in] timer_id The timer ID to operate on * * @return Execution status, see @ref OSReturnCodes @@ -187,6 +199,9 @@ int32 OS_TimerDelete(osal_id_t timer_id); * * Outputs the ID associated with the given timer, if it exists. * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. + * * @param[out] timer_id Will be set to the timer ID corresponding to the name @nonnull * @param[in] timer_name The timer name to find @nonnull * @@ -206,6 +221,9 @@ int32 OS_TimerGetIdByName(osal_id_t *timer_id, const char *timer_name); * This function takes timer_id, and looks it up in the OS table. It puts all of the * information known about that timer into a structure pointer to by timer_prop. * + * @note This configuration API must not be used from the context of a timer callback. + * Timers should only be configured from the context of normal OSAL tasks. + * * @param[in] timer_id The timer ID to operate on * @param[out] timer_prop Buffer containing timer properties @nonnull * - creator: the OS task ID of the task that created this timer diff --git a/src/os/posix/build_options.cmake b/src/os/posix/build_options.cmake index c1f609301..15f3ad03e 100644 --- a/src/os/posix/build_options.cmake +++ b/src/os/posix/build_options.cmake @@ -5,5 +5,4 @@ ########################################################################## # this file is a placeholder for POSIX-specific compile tuning -# currently no extra flags/definitions needed - +add_definitions(-D_POSIX_OS_) diff --git a/src/os/rtems/build_options.cmake b/src/os/rtems/build_options.cmake index 83768a639..0a457e1e9 100644 --- a/src/os/rtems/build_options.cmake +++ b/src/os/rtems/build_options.cmake @@ -5,5 +5,4 @@ ########################################################################## # this file is a placeholder for RTEMS-specific compile tuning -# currently no extra flags/definitions needed - +add_definitions(-D_RTEMS_OS_) diff --git a/src/os/vxworks/build_options.cmake b/src/os/vxworks/build_options.cmake index 4009d0cee..c3e9553a0 100644 --- a/src/os/vxworks/build_options.cmake +++ b/src/os/vxworks/build_options.cmake @@ -5,5 +5,4 @@ ########################################################################## # this file is a placeholder for VxWorks-specific compile tuning -# currently no extra flags/definitions needed - +add_definitions(-D_VXWORKS_OS_) diff --git a/src/tests/time-base-api-test/time-base-api-test.c b/src/tests/time-base-api-test/time-base-api-test.c index 928b3c0d0..7cf35913d 100644 --- a/src/tests/time-base-api-test/time-base-api-test.c +++ b/src/tests/time-base-api-test/time-base-api-test.c @@ -35,9 +35,9 @@ #include "uttest.h" #include "utbsp.h" -static OS_timebase_prop_t SyncTimeBaseProp; +OS_timebase_prop_t SyncTimeBaseProp; -static uint32 NumSyncs = 0; +uint32 NumSyncs = 0; static uint32 UT_TimerSync(osal_id_t timer_id) { @@ -48,6 +48,7 @@ static uint32 UT_TimerSync(osal_id_t timer_id) * calls the various functions on the first time through and stores the * result, which is checked/asserted in the main task. */ +#ifdef _POSIX_OS_ if (NumSyncs == 0) { UtAssert_INT32_EQ(OS_TimeBaseCreate(&timer_id, "sync", 0), OS_ERR_INCORRECT_OBJ_STATE); @@ -56,6 +57,7 @@ static uint32 UT_TimerSync(osal_id_t timer_id) UtAssert_INT32_EQ(OS_TimeBaseGetIdByName(&timer_id, "TimeBaseC"), OS_ERR_INCORRECT_OBJ_STATE); UtAssert_INT32_EQ(OS_TimeBaseGetInfo(timer_id, &SyncTimeBaseProp), OS_ERR_INCORRECT_OBJ_STATE); } +#endif ++NumSyncs; OS_TaskDelay(1); diff --git a/src/unit-tests/ostimer-test/ut_ostimer_test.c b/src/unit-tests/ostimer-test/ut_ostimer_test.c index 5fb8083ac..31a76ad25 100644 --- a/src/unit-tests/ostimer-test/ut_ostimer_test.c +++ b/src/unit-tests/ostimer-test/ut_ostimer_test.c @@ -203,7 +203,11 @@ void UtTest_Setup(void) UtTest_Add(UT_os_timergetidbyname_test, UT_os_setup_timergetidbyname_test, NULL, "OS_TimerGetIdByName"); UtTest_Add(UT_os_timergetinfo_test, UT_os_setup_timergetinfo_test, NULL, "OS_TimerGetInfo"); UtTest_Add(UT_os_timerset_test, UT_os_setup_timerset_test, NULL, "OS_TimerSet"); + + /* the reconfig test only works on POSIX */ +#ifdef _POSIX_OS_ UtTest_Add(UT_os_timerreconf_test, NULL, NULL, "TimerReconfig"); +#endif } /*================================================================================* From a8db3cda0b7779e09f4f031604d2e9fb366c9e90 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Wed, 7 Jul 2021 12:34:22 -0400 Subject: [PATCH 2/2] Update #1083, correct comment in timebase callback Per review feedback, removes comment that was stale and no longer valid, and describe why only POSIX_OS is enabled here on this check. --- src/tests/time-base-api-test/time-base-api-test.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/tests/time-base-api-test/time-base-api-test.c b/src/tests/time-base-api-test/time-base-api-test.c index 7cf35913d..a46f7facd 100644 --- a/src/tests/time-base-api-test/time-base-api-test.c +++ b/src/tests/time-base-api-test/time-base-api-test.c @@ -43,10 +43,10 @@ static uint32 UT_TimerSync(osal_id_t timer_id) { /* * Calls to time base configuration from the context of a sync function - * should be rejected with OS_ERR_INCORRECT_OBJ_STATE. However because - * UtAssert is not fully thread-safe, this does not assert here, it just - * calls the various functions on the first time through and stores the - * result, which is checked/asserted in the main task. + * should be rejected with OS_ERR_INCORRECT_OBJ_STATE. Note that only the + * POSIX provides the mechanism for this error check to actually work - On + * other platforms the error checking may not be possible, depending on how + * OS_TaskGetId_Impl() responds when called from a non-OSAL task. */ #ifdef _POSIX_OS_ if (NumSyncs == 0)