[BUG] xTaskDelayUntil updates pxPreviousWakeTime incorrectly in case of deadline miss

[INOMO-RKr]

Describe the bug
If the targeted wake-up time is already in the past, i.e. xTaskDelayUntil( &previousWakeUpTime, xTimeIncrement ) == pdFALSE then previousWakeUpTime is incremented by the specied delay ticks, and goes out of sync with the actual ticks.

Target

• All

To Reproduce
Call the function in a way that the specified deadline is missed.

Expected behavior
After a call to xTaskDelayUnitl(&previousWakeUpTime, xTimeIncrement ) the variable previousWakeUpTime holds the current ticks.
according to the functions documentation:

/**
 * @param pxPreviousWakeTime Pointer to a variable that holds the time at which the
 * task was last unblocked.  The variable must be initialised with the current time
 * prior to its first use (see the example below).  Following this the variable is
 * automatically updated within xTaskDelayUntil ().
*/

OR
The documentation clearly describes the current behavior.

Additional context
Relevant section in source code

FreeRTOS-Kernel/tasks.c

Lines 1231 to 1265 in 151fb04

	xTimeToWake = *pxPreviousWakeTime + xTimeIncrement;
	if( xConstTickCount < *pxPreviousWakeTime )
	{
	/* The tick count has overflowed since this function was
	* lasted called. In this case the only time we should ever
	* actually delay is if the wake time has also overflowed,
	* and the wake time is greater than the tick time. When this
	* is the case it is as if neither time had overflowed. */
	if( ( xTimeToWake < *pxPreviousWakeTime ) && ( xTimeToWake > xConstTickCount ) )
	{
	xShouldDelay = pdTRUE;
	}
	else
	{
	mtCOVERAGE_TEST_MARKER();
	}
	}
	else
	{
	/* The tick time has not overflowed. In this case we will
	* delay if either the wake time has overflowed, and/or the
	* tick time is less than the wake time. */
	if( ( xTimeToWake < *pxPreviousWakeTime ) || ( xTimeToWake > xConstTickCount ) )
	{
	xShouldDelay = pdTRUE;
	}
	else
	{
	mtCOVERAGE_TEST_MARKER();
	}
	}
	/* Update the wake time ready for the next call. */
	*pxPreviousWakeTime = xTimeToWake;

            /* Generate the tick time at which the task wants to wake. */
            xTimeToWake = *pxPreviousWakeTime + xTimeIncrement;
/*...
Lines omitted for clarity 
...*/
            *pxPreviousWakeTime = xTimeToWake; 

it is clear that the update of *pxPreviousWakeTime is the same irregardless of if the deadline is missed or not.

Example
This causes in the following example the previousWakeTime and xTaskGetTickCount() to diverge.

void exampleTaskFunction(void)
{
    TickType_t previousWakeUpTime = xTaskGetTicks();
    const TickType_t cDelayTicks = 5;
    while(true)
    {
        vTaskDelay(10*cDelayTicks) /*Simple way to force a deadline miss*/
       if(!xTaskDelayUntil(&previousWakeUpTime, cDelayTicks) )
      { /*Always true*/
        assert(xTaskGetTicks() != previousWakeUpTime);
      }
}

[kstribrnAmzn]

I can see how this behavior is not well documented but the behavior does not diverge from the documentation so I'm hesitant to call this a bug. Please let me known any documentation suggestions you have. You can also raise a PR to the kernel updating the documentation yourself.

[kstribrnAmzn]

Annotated your example a bit...

void exampleTaskFunction(void)
{
    TickType_t previousWakeUpTime = xTaskGetTicks(); // Let's say this gets executed on tick 7
    const TickType_t cDelayTicks = 5; // This is also executed on tick 7
    // Let's say the task gets interrupted here for 200 ticks.
    // And let's imagine the entire while loop is executed within tick 207
    while(true)
    {
       if(!xTaskDelayUntil(&previousWakeUpTime, cDelayTicks) ) 
      { /*Always true*/
        assert(xTaskGetTicks() != previousWakeUpTime);
      }
}

What value of previousWakeUpTime were you expecting upon completion of the first iteration of the while loop? 207?

[INOMO-RKr]

Thank you for your reply

Yes, I expect 207.

In general, I would expect that upon exit of xTaskDelayUntil(&previousWakeUpTime, cDelayTicks) previousWakeUpTime == xTaskGetTicks() holds true. (Unless maybe another task preempts after calling the function and delays the return).

Currently

If the function returns immediately, I interpret the documentation as either
a. the task is not blocked and thus previousWakeUpTime should not be updated at all; or
b. the task is unblocked immediately and previousWakeUpTime updated to the current tick count

I cannot see the current behavior: previousWakeUpTime holds the time at which the task should have been last unblocked if everything went as expected.

A case for my interpretation
Of course you can (rightfully so!) argue that a missed deadline must be treated in an error-handler which resolves any issues caused by the miss.

Potential issues arise in case of multiple deadline misses in succession (i.e. the time between the executions of xTaskDelayUntil()is greater than cDelayTicks). This accumulates by how much previousWakeUpTime is lagging behind the tick-counter

• Once the inter-call delay is shorter than cDelayTicks this will cause all missed executions to be done as fast as possible in a sort of "catch-up", instead of continuing with a regular period.
  • Both behaviors have valid use-cases, and which one is default belongs in the documentation
    (currently the later appears to be intended:

    FreeRTOS-Kernel/include/task.h

    Lines 805 to 806 in 151fb04

    	* Delay a task until a specified time. This function can be used by periodic
    	* tasks to ensure a constant execution frequency.

    )
• If the accumulated lasted long enough for the tick-count to overflow multiple times, this behavior will change to only "catch up" a limited amount (until the overflowed tick-counter and previousWakeUpTime appear coherent again)
  • Yes, if the time between calls to xTaskDelayUntil is long enough for the tick-counter to overflow multiple times, behavior gets weird as well! However, the likelihood of this occurring is much lower since there is no accumulation.

[INOMO-RKr]

Unfortunately I know about the reality that there are most likely applications that rely on the current implementation without even knowing it.
With this in mind, updating the documentation is probably the safer way.
Curious to hear your perspective on this!

[kstribrnAmzn]

Unfortunately I know about the reality that there are most likely applications that rely on the current implementation without even knowing it.

This is a wise assumption. I was thinking the same thing. I don't have a way to know how many applications are relying upon this behavior and would be affected by a change. Changing this behavior could cause problems for existing applications. I do agree that documentation could be improved here. It took me a few reads through to discover there is an implicit requirement given in the documentation.

The requirement is for a "periodic task". To ensure a task is executing periodically, it needs to be a high priority. If a task is a high priority then it shouldn't be preempted by tasks for long periods of time which would make this case a bit of a non-issue. If a task is meant to be periodic and isn't a high priority, then all higher priority tasks need to be blocked frequently to allow the low priority task to execute. Long running, high priority tasks are an anti-pattern when working with FreeRTOS. The use of xTaskDelayUntil should be most often restricted to higher priority, short running tasks - as the task is blocking at a set frequency .

Note: I know I'm ignoring ISRs, but in typical cases these execute fast (often less than a tick) and actually stop the tick count from incrementing while being handled.

[kstribrnAmzn]

I'll improve the code comment and our website documentation. I won't be changing the code. If we receive more feedback on this behavior in the future we will reevaluate this.

[kstribrnAmzn]

If you need this behavior now you are welcome to fork this kernel to update the xTaskDelayUntil function.

[kstribrnAmzn]

Website page has been updated - https://freertos.org/Documentation/02-Kernel/04-API-references/02-Task-control/03-xTaskDelayUntil

Kernel has also been updated - 536914b

Closing this issue.