From 05ded5bd8d01ebf1fd8ab9e7db40d582e2bfb628 Mon Sep 17 00:00:00 2001 From: Jeff Tenney Date: Tue, 13 Apr 2021 11:05:46 -0700 Subject: [PATCH] Remove support for tmrCOMMAND_START_DONT_TRACE (#305) * Remove support for tmrCOMMAND_START_DONT_TRACE Prior to this commit, the timer task used tmrCOMMAND_START_DONT_TRACE to reload a "backlogged" auto-reload timer -- one for which a reload operation would have started a period that had already elapsed. If the command queue contained a stop or delete command when tmrCOMMAND_START_DONT_TRACE was put into the queue for a reload, the timer unexpectedly restarted when the timer task processed tmrCOMMAND_START_DONT_TRACE. This commit implements a new method of reloading auto-reload timers and eliminates support for tmrCOMMAND_START_DONT_TRACE. No other code sends this private command. However, the symbol tmrCOMMAND_START_DONT_TRACE remains defined, with its original command value, so as not to impact trace applications. Also fix one-shot timers that were not reliably being marked as not active: - when they expired before the start command could be processed - when the expiration was processed during the timer list switch Also improve consistency: - Always reload auto-reload timers *before* calling the callback. - Always call traceTIMER_EXPIRED() just prior to the callback. * fix indent * Revert unnecessary change to prvTimerTask() Change was intended to faithfully work through a backlog that spanned a list switch, and before processing a stop or delete command. But, (1) it isn't important to do that, and (2) the code didn't accomplish the intention when *two* auto-reload timers were backlogged across a list switch. Best to simply leave this part of the code as it was before. * fix style Co-authored-by: Joseph Julicher --- timers.c | 113 ++++++++++++++++++++++--------------------------------- 1 file changed, 44 insertions(+), 69 deletions(-) diff --git a/timers.c b/timers.c index 203a07778..8032a1fe8 100644 --- a/timers.c +++ b/timers.c @@ -55,7 +55,8 @@ #if ( configUSE_TIMERS == 1 ) /* Misc definitions. */ - #define tmrNO_DELAY ( TickType_t ) 0U + #define tmrNO_DELAY ( ( TickType_t ) 0U ) + #define tmrMAX_TIME_BEFORE_OVERFLOW ( ( TickType_t ) -1 ) /* The name assigned to the timer service task. This can be overridden by * defining trmTIMER_SERVICE_TASK_NAME in FreeRTOSConfig.h. */ @@ -172,6 +173,15 @@ const TickType_t xTimeNow, const TickType_t xCommandTime ) PRIVILEGED_FUNCTION; +/* + * Reload the specified auto-reload timer. If the reloading is backlogged, + * clear the backlog, calling the callback for each additional reload. When + * this function returns, the next expiry time is after xTimeNow. + */ + static void prvReloadTimer( Timer_t * const pxTimer, + TickType_t xExpiredTime, + const TickType_t xTimeNow ) PRIVILEGED_FUNCTION; + /* * An active timer has reached its expire time. Reload the timer if it is an * auto-reload timer, then call its callback. @@ -502,45 +512,47 @@ } /*-----------------------------------------------------------*/ + static void prvReloadTimer( Timer_t * const pxTimer, + TickType_t xExpiredTime, + const TickType_t xTimeNow ) + { + /* Insert the timer into the appropriate list for the next expiry time. + * If the next expiry time has already passed, advance the expiry time, + * call the callback function, and try again. */ + while ( prvInsertTimerInActiveList( pxTimer, ( xExpiredTime + pxTimer->xTimerPeriodInTicks ), xTimeNow, xExpiredTime ) != pdFALSE ) + { + /* Advance the expiry time. */ + xExpiredTime += pxTimer->xTimerPeriodInTicks; + + /* Call the timer callback. */ + traceTIMER_EXPIRED( pxTimer ); + pxTimer->pxCallbackFunction( ( TimerHandle_t ) pxTimer ); + } + } +/*-----------------------------------------------------------*/ + static void prvProcessExpiredTimer( const TickType_t xNextExpireTime, const TickType_t xTimeNow ) { - BaseType_t xResult; Timer_t * const pxTimer = ( Timer_t * ) listGET_OWNER_OF_HEAD_ENTRY( pxCurrentTimerList ); /*lint !e9087 !e9079 void * is used as this macro is used with tasks and co-routines too. Alignment is known to be fine as the type of the pointer stored and retrieved is the same. */ /* Remove the timer from the list of active timers. A check has already * been performed to ensure the list is not empty. */ - ( void ) uxListRemove( &( pxTimer->xTimerListItem ) ); - traceTIMER_EXPIRED( pxTimer ); /* If the timer is an auto-reload timer then calculate the next * expiry time and re-insert the timer in the list of active timers. */ if( ( pxTimer->ucStatus & tmrSTATUS_IS_AUTORELOAD ) != 0 ) { - /* The timer is inserted into a list using a time relative to anything - * other than the current time. It will therefore be inserted into the - * correct list relative to the time this task thinks it is now. */ - if( prvInsertTimerInActiveList( pxTimer, ( xNextExpireTime + pxTimer->xTimerPeriodInTicks ), xTimeNow, xNextExpireTime ) != pdFALSE ) - { - /* The timer expired before it was added to the active timer - * list. Reload it now. */ - xResult = xTimerGenericCommand( pxTimer, tmrCOMMAND_START_DONT_TRACE, xNextExpireTime, NULL, tmrNO_DELAY ); - configASSERT( xResult ); - ( void ) xResult; - } - else - { - mtCOVERAGE_TEST_MARKER(); - } + prvReloadTimer( pxTimer, xNextExpireTime, xTimeNow ); } else { pxTimer->ucStatus &= ~tmrSTATUS_IS_ACTIVE; - mtCOVERAGE_TEST_MARKER(); } /* Call the timer callback. */ + traceTIMER_EXPIRED( pxTimer ); pxTimer->pxCallbackFunction( ( TimerHandle_t ) pxTimer ); } /*-----------------------------------------------------------*/ @@ -741,7 +753,7 @@ { DaemonTaskMessage_t xMessage; Timer_t * pxTimer; - BaseType_t xTimerListsWereSwitched, xResult; + BaseType_t xTimerListsWereSwitched; TickType_t xTimeNow; while( xQueueReceive( xTimerQueue, &xMessage, tmrNO_DELAY ) != pdFAIL ) /*lint !e603 xMessage does not have to be initialised as it is passed out, not in, and it is not used unless xQueueReceive() returns pdTRUE. */ @@ -802,7 +814,6 @@ case tmrCOMMAND_START_FROM_ISR: case tmrCOMMAND_RESET: case tmrCOMMAND_RESET_FROM_ISR: - case tmrCOMMAND_START_DONT_TRACE: /* Start or restart a timer. */ pxTimer->ucStatus |= tmrSTATUS_IS_ACTIVE; @@ -810,19 +821,18 @@ { /* The timer expired before it was added to the active * timer list. Process it now. */ - pxTimer->pxCallbackFunction( ( TimerHandle_t ) pxTimer ); - traceTIMER_EXPIRED( pxTimer ); - if( ( pxTimer->ucStatus & tmrSTATUS_IS_AUTORELOAD ) != 0 ) { - xResult = xTimerGenericCommand( pxTimer, tmrCOMMAND_START_DONT_TRACE, xMessage.u.xTimerParameters.xMessageValue + pxTimer->xTimerPeriodInTicks, NULL, tmrNO_DELAY ); - configASSERT( xResult ); - ( void ) xResult; + prvReloadTimer( pxTimer, xMessage.u.xTimerParameters.xMessageValue + pxTimer->xTimerPeriodInTicks, xTimeNow ); } else { - mtCOVERAGE_TEST_MARKER(); + pxTimer->ucStatus &= ~tmrSTATUS_IS_ACTIVE; } + + /* Call the timer callback. */ + traceTIMER_EXPIRED( pxTimer ); + pxTimer->pxCallbackFunction( ( TimerHandle_t ) pxTimer ); } else { @@ -889,10 +899,8 @@ static void prvSwitchTimerLists( void ) { - TickType_t xNextExpireTime, xReloadTime; + TickType_t xNextExpireTime; List_t * pxTemp; - Timer_t * pxTimer; - BaseType_t xResult; /* The tick count has overflowed. The timer lists must be switched. * If there are any timers still referenced from the current timer list @@ -902,43 +910,10 @@ { xNextExpireTime = listGET_ITEM_VALUE_OF_HEAD_ENTRY( pxCurrentTimerList ); - /* Remove the timer from the list. */ - pxTimer = ( Timer_t * ) listGET_OWNER_OF_HEAD_ENTRY( pxCurrentTimerList ); /*lint !e9087 !e9079 void * is used as this macro is used with tasks and co-routines too. Alignment is known to be fine as the type of the pointer stored and retrieved is the same. */ - ( void ) uxListRemove( &( pxTimer->xTimerListItem ) ); - traceTIMER_EXPIRED( pxTimer ); - - /* Execute its callback, then send a command to restart the timer if - * it is an auto-reload timer. It cannot be restarted here as the lists - * have not yet been switched. */ - pxTimer->pxCallbackFunction( ( TimerHandle_t ) pxTimer ); - - if( ( pxTimer->ucStatus & tmrSTATUS_IS_AUTORELOAD ) != 0 ) - { - /* Calculate the reload value, and if the reload value results in - * the timer going into the same timer list then it has already expired - * and the timer should be re-inserted into the current list so it is - * processed again within this loop. Otherwise a command should be sent - * to restart the timer to ensure it is only inserted into a list after - * the lists have been swapped. */ - xReloadTime = ( xNextExpireTime + pxTimer->xTimerPeriodInTicks ); - - if( xReloadTime > xNextExpireTime ) - { - listSET_LIST_ITEM_VALUE( &( pxTimer->xTimerListItem ), xReloadTime ); - listSET_LIST_ITEM_OWNER( &( pxTimer->xTimerListItem ), pxTimer ); - vListInsert( pxCurrentTimerList, &( pxTimer->xTimerListItem ) ); - } - else - { - xResult = xTimerGenericCommand( pxTimer, tmrCOMMAND_START_DONT_TRACE, xNextExpireTime, NULL, tmrNO_DELAY ); - configASSERT( xResult ); - ( void ) xResult; - } - } - else - { - mtCOVERAGE_TEST_MARKER(); - } + /* Process the expired timer. For auto-reload timers, be careful to + * process only expirations that occur on the current list. Further + * expirations must wait until after the lists are switched. */ + prvProcessExpiredTimer( xNextExpireTime, tmrMAX_TIME_BEFORE_OVERFLOW ); } pxTemp = pxCurrentTimerList;