Timer START_DONT_TRACE Race Condition?

Kernel
21

I took a look at the latest timer.c on GitHub (11.1.0). It looks like it has been changed to sit in a while loop until it can add the next timer to the list. That is better then adding a command to the command queue but not sure the while loop is a good idea. Could that potentially delay other timers from being processed?

Also, in the reload case, it looks like it will call the callback twice for the first tick. Once in the prvReloadTImer() and again when it returns from that function.

timer.c line 699

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

In prvProcessReceivedCommands(). timer.c line 1010

                        if( ( pxTimer->ucStatus & tmrSTATUS_IS_AUTORELOAD ) != 0U )
                        {
                            prvReloadTimer( pxTimer, xMessage.u.xTimerParameters.xMessageValue + pxTimer->xTimerPeriodInTicks, xTimeNow );
                        }
                        else
                        {
                            pxTimer->ucStatus &= ( ( uint8_t ) ~tmrSTATUS_IS_ACTIVE );
                        }

                        /* Call the timer callback. */
                        traceTIMER_EXPIRED( pxTimer );
                        pxTimer->pxCallbackFunction( ( TimerHandle_t ) pxTimer );
22

In principle, prvReloadTimer() just reloads an auto-reload timer. Just one timer. Once it is reloaded, the caller calls the callback.

The while loop you mentioned doesn’t normally iterate at all. It iterates only if the timer is backlogged. For example, if the current tick count is 100, and there is a timer with an auto-reload period of 10, and the timer is due to expire at time 65. That timer is backlogged because the expiries scheduled for time 65, 75, 85, and 95, all need to be processed as soon as possible. In that case, prvReloadTimer() handles the callbacks for times 65, 75, and 85. The caller handles the callback for time 95.

23

This reads as if the stop command is placed in the queue before the start command. Isn’t this out of order?

24

Ok, I see it now. Thanks for the detailed explanation.

Now, to convince SiLabs to update their library. I checked their latest. They are still using FreeRTOS 10.4.3.

Or I update their portable code. Is there a migration guide that can help me if I go down this path? Or, are the changes small enough it probably won’t be a big deal?

25

Yes. That is the race condition that started this thread back in 2021. It has been fixed but SiLabs is still using an old version of FreeRTOS in their library.

26

Thanks! Is there no way to sequence the ISRs so that the start message is placed into the timer queue before the stop message? Maybe a critical section in the ISR to disable other interrupts while the start or stop function is being processed would work.

27

The fix created by @jefftenney doesn’t appear to required any portable (aka port) layer functions. Updating the library seems like the simplest way. Could you try pointing the SDK module script to a newer version of FreeRTOS? Or you could try replacing the files it’s looking at here.

Another option could be to fork their repository and update the FreeRTOS files before installing.

28

The ISRs are sequenced so the start command is put in the command queue before the stop command.

But the old way the reload (when the reload is after the timer expiry) was done in the FreeRTOS timer, was to add another “reload start” command to the command queue, which in some cases, queued it after a the stop command.

Flow looked like this:
-Button pressed edge ISR sends start command
-Button released edge ISR sends stop command
-Time expires for start command
-Timer task processed start command but since it’s time expired, it added a “reload start” command and called the application callback
-Timer task processed stop command
-Timer task processed “reload start” command

From there the timer would run normally until another stop command is processed.

29

Did you try to update the FreeRTOS to the latest version and see if the problem still persists?

30

Not yet. My customer does not want to pay me to update SiLabs’ library. We opened a ticket with SiLabs. As soon as they update their library we will try it.

31

I copied the timer.c from this pull request and tested it.

The race condition is fixed, the timer stops.

But, when start is processed, it appears that it calls the callback once for every tick that has elapsed since the timer was stopped. The longer between the stop and start commands, the more times the callback is called.

I browsed through the file history after PR #305 (comments only) and did not see any that appeared to address this. Is there a version that addresses this?

32

Looking at the code in Timer.c, you might try restarting by using a command to change the period (to the original period you wanted) as that looks to reset the time base. The code for Start and Restart seems to presume that you do want to continue from where you left off. Not sure it that is really the right choice, but that is what the code does.

33

Thanks, using xTimerChangePeriodFromISR to start the timers instead of xTimerStartFromISR fixed it.

1 Like
34

Are you using tickless idle? Unfortunately, when you call xTimerStartFromISR() from the ISR that woke the CPU from a tickless period, the timer starts from the uncorrected system tick count because the ISR runs before the tickless logic corrects the system tick count to account for the time spent in tickless idle. Your workaround – calling xTimerChangePeriodFromISR() – is the recommended one.