Looks like there is an issue with vTaskDelayUntil

[ashokabbi]

Hi,
I am using FreeRTOS to modify the existing port of AUDO 1782 to towards the AURIX TC3xx.
I found strange behavior when I use the vTaskDelayUntil function.
Problem was that, only highest prio task was being activated continuously, without actual delay, and other tasks are in ready state forever.

When I debugged the vTaskDelayUntil (obviously I checked the port.c several times), I found position of the below code line in bold as suspicious.

	void vTaskDelayUntil( TickType_t * const pxPreviousWakeTime, const TickType_t xTimeIncrement )
	{
                :
                :

			/* Update the wake time ready for the next call. */
**			*pxPreviousWakeTime = xTimeToWake;**

			if( xShouldDelay != pdFALSE )
			{
				traceTASK_DELAY_UNTIL( xTimeToWake );

				/* prvAddCurrentTaskToDelayedList() needs the block time, not
				the time to wake, so subtract the current tick count. */
				prvAddCurrentTaskToDelayedList( xTimeToWake - xConstTickCount, pdFALSE );
			}
			else
			{
				mtCOVERAGE_TEST_MARKER();
			}
                :
                :
    }

As this line always gets executed whenever this function gets called, this would modify the value at pxPreviousWakeTime even if it was not the time to awake (which is xShouldDelay= pdTrue).

This works when I moved this line inside true path of ** if( xShouldDelay != pdFALSE ) ** as below and then the behaviour was as expected.

void vTaskDelayUntil( TickType_t * const pxPreviousWakeTime, const TickType_t xTimeIncrement )
	{
                :
                :

			if( xShouldDelay != pdFALSE )
			{
				
				/* Update the wake time ready for the next call. */
**				*pxPreviousWakeTime = xTimeToWake;**

				traceTASK_DELAY_UNTIL( xTimeToWake );

				/* prvAddCurrentTaskToDelayedList() needs the block time, not
				the time to wake, so subtract the current tick count. */
				prvAddCurrentTaskToDelayedList( xTimeToWake - xConstTickCount, pdFALSE );
			}
			else
			{
				mtCOVERAGE_TEST_MARKER();
			}
                :
                :
    }

As I said, I am not having complete overview of the implementation, need to seek other’s opinion here…
Below is the complete modified function:

#if ( INCLUDE_vTaskDelayUntil == 1 )

	void vTaskDelayUntil( TickType_t * const pxPreviousWakeTime, const TickType_t xTimeIncrement )
	{
	TickType_t xTimeToWake;
	BaseType_t xAlreadyYielded, xShouldDelay = pdFALSE;

		configASSERT( pxPreviousWakeTime );
		configASSERT( ( xTimeIncrement > 0U ) );
		configASSERT( uxSchedulerSuspended == 0 );

		vTaskSuspendAll();
		{
			/* Minor optimisation.  The tick count cannot change in this
			block. */
			const TickType_t xConstTickCount = xTickCount;

			/* Generate the tick time at which the task wants to wake. */
			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; 						/* TODO: Ashok: Modified code */

			if( xShouldDelay != pdFALSE )
			{
				/* Update the wake time ready for the next call. */
				*pxPreviousWakeTime = xTimeToWake;						/* TODO: Ashok: Modified code */

				traceTASK_DELAY_UNTIL( xTimeToWake );

				/* prvAddCurrentTaskToDelayedList() needs the block time, not
				the time to wake, so subtract the current tick count. */
				prvAddCurrentTaskToDelayedList( xTimeToWake - xConstTickCount, pdFALSE );
			}
			else
			{
				mtCOVERAGE_TEST_MARKER();
			}
		}
		xAlreadyYielded = xTaskResumeAll();

		/* Force a reschedule if xTaskResumeAll has not already done so, we may
		have put ourselves to sleep. */
		if( xAlreadyYielded == pdFALSE )
		{
			portYIELD_WITHIN_API();
		}
		else
		{
			mtCOVERAGE_TEST_MARKER();
		}
	}

#endif /* INCLUDE_vTaskDelayUntil */

[richard-damon]

vTaskDelayUntil is defied to not delay if the time implied by pxPreviousWakeTime + xTimeIncrement has already passed, but in that case will just increment the current value of vTaskDelayUntil, and continue.

Thinking a bit, one thing that might be able to cause that situation would be if the task has its delay aborted by some other task, and goes into another delay with pxPreviousWakeTime now in the future, that would initiate a series of no delays until pxPreviousWakeTIme wraps around.

[rtel (Richard Barry)]

@richard-damon

vTaskDelayUntil is defied to not delay if the time implied by pxPreviousWakeTime + xTimeIncrement has already passed, but in that case will just increment the current value of vTaskDelayUntil, and continue.

Yes, that is the intended behaviour.

@richard-damon

Thinking a bit, one thing that might be able to cause that situation would be if the task has its delay aborted by some other task, and goes into another delay with pxPreviousWakeTime now in the future, that would initiate a series of no delays until pxPreviousWakeTIme wraps around.

Would need to check the code, but I don’t think that could happen. If the last unblock time was in the future - say the tick count was 100, the last unblock time 105, and the block period was 10 (a situation that should not happen but would need to see what happens if a delay is aborted, as you say) then the next unblock time would be calculated as 105+10=15 - and the task would block for until the tick count was 115 - effectively missing an unblock occurrence - but if the task was forcibly unblocked using xTaskAbortDelay() then presumably missing unblock occurrence was the intended behaviour.

Like I say - I would need to look at the code in more detail which I can’t do right now.

[richard-damon]

I think what I see, which you can verify by looking better with more understanding is this case could cause problems:

xCurrentTick=100
pxPreviousWakeTime = 100
do a vTaskDelayUntil for 10
now pxPreviousWakeTime = 110 and task blocks
xCurrentTick = 101
AbortDelay

task does a vTaskDelayUnit for 10
xTimeToWake = 111
function sees xTickCount < pxPreviousWakeTIme and assume that xTickCount has overflowed, so is looking for the Wake time to also overflow (which is hasn’t) or it considers that we are still behind, so doesn’t delay. With a 32 bit tick, this could need to loop LOTS of times to catch up.
The logic probably predates xTaskAbortDelay(), and may need a bit more logic (and saved values) to handle that case. Biggest issue is likely when delays and such get to be well above 1/2 of the maximum value of the Tick counter, then it may be hard to distingish between the case of wrap around or not. (Maybe that says the additional information needed is the overflow counter)

[rtel (Richard Barry)]

@richard-damon

function sees xTickCount < pxPreviousWakeTIme and assume that xTickCount has overflowed,

Got it - that was the bit I was missing without looking at the code Will double check when I can.

[jefftenney (Jeff)]

Richard D making sure I understand your scenario, if the caller of vTaskDelayUntil() makes sure never to pass a pxPreviousWakeTime that is in the future, does that resolve the concern?

[richard-damon]

The issue is the question of what is the ‘future’ gets murky. The problem is that the xTickCount wraps around after a period of time and goes back to zero. Thus pxPreviousWakeTime > xTickCount doesn’t always mean a time in the future, but might mean that xTickCount has wrapped about since the last call. There is another counter, that counts the number of time xTickCount has wrapped, and comparing that could resolve the issue. If that counter was saved with pxPreviousWakeTIme, the if it is the same as the current value, then pxPreviousWakeTIme is a future point in time, so we will need to block, or if it is different, then we do like the existing code and assume a wrapped counter.

Of course, the issue is where to save this extra data, that would be an API change.

Another answer would be to see if it was more or less than 1/2 portMAX_DELAY ahead, and decide based on that (essentially, treat the difference as a signed value, so you are up to 1/2 portMAX_DELAY ahead or behind the tick. That might cause issues with ports that have a 16-bit tick counter and fast ticks, deciding that once you pass being 32 seconds behind you are now ahead.

And we can’t delay the update until we wake (currently) because the data isn’t saved anywhere to do so, that that would be one possiblity.

The caller could check this sort of thing themselves, and if pxPreviousWakeTIme is just slightly in the future (like less than the delay period) reset it to now. Something like:

now = xGetTickCount();
if(previousWakeTIme - now <= delayTime) previousWakeTime = now

This will prevent the new future from looking like the distant past.

[ashokabbi]

I don’t know why *pxPreviousWakeTime = xTimeToWake; is required to be outside of the check which checks if the time is passed.

@richard-damon,
For the update of previousWakeTime, I expected, handling as you stated should work.
now = xGetTickCount();
if(previousWakeTIme - now <= delayTime) previousWakeTime = now

But it doesn’t work I don’t know why.

The code below is optimized towards the overflow. I have AURIX controller which is 32 bit proc. and this code works (though I have question as above)

/*** working code****/
void vTaskDelayUntil( TickType_t * const pxPreviousWakeTime, const TickType_t xTimeIncrement )
{
TickType_t xTimeToWake;
BaseType_t xAlreadyYielded;

	configASSERT( pxPreviousWakeTime );
	configASSERT( ( xTimeIncrement > 0U ) );
	configASSERT( uxSchedulerSuspended == 0 );

	vTaskSuspendAll();
	{
		const TickType_t xConstTickCount = xTickCount;
		xTimeToWake = *pxPreviousWakeTime + xTimeIncrement;

		/** Ashok: Below code will work because of unsigned 32 bit arithmetic even at timer wrapping condition
		 * As an example (with 32bit registers),
		 * if pxPreviousWakeTime = 0xFFFFFFFE and current xConstTickCount = 2 (after overflow),
		 * the unsigned arithmetic 2 - 0xFFFFFFFE will be 4 (which is the actual distance)
		 * If there is no overflow, this would any way work
		 */
		if( (xConstTickCount - *pxPreviousWakeTime) <  xTimeIncrement)
		{
			traceTASK_DELAY_UNTIL( xTimeToWake );

			/* prvAddCurrentTaskToDelayedList() needs the block time, not
			the time to wake, so subtract the current tick count. */
			prvAddCurrentTaskToDelayedList( xTimeToWake - xConstTickCount, pdFALSE );
		}
		else
		{
			mtCOVERAGE_TEST_MARKER();
		}
	}

	/* Update the wake time ready for the next call. */
	*pxPreviousWakeTime = xTimeToWake;

	xAlreadyYielded = xTaskResumeAll();

	/* Force a reschedule if xTaskResumeAll has not already done so, we may
	have put ourselves to sleep. */
	if( xAlreadyYielded == pdFALSE )
	{
		portYIELD_WITHIN_API();
	}
	else
	{
		mtCOVERAGE_TEST_MARKER();
	}
}

[jefftenney (Jeff)]

@richard-damon

The issue is the question of what is the ‘future’ gets murky.

That’s for sure. The logic at the beginning of vTaskDelayUntil() is evidence of that. But it relies on pxPreviousWakeTime being in the past.

@ashokabbi Does any of your code ever abort the delay? Maybe we’re off target here.

[ashokabbi]

@jefftenney
“Does any of your code ever abort the delay? Maybe we’re off target here”
Actually I did not see that…
My problem is when I define empty tasks as below (with only the counts being updated):

static __attribute__((__noreturn__)) void periodicTask_1ms(void *arg)
{
	TickType_t xLastWakeTime;
	(void)arg;
	xLastWakeTime = xTaskGetTickCount();

	while (1)
	{
		vTaskDelayUntil(&xLastWakeTime, pdMS_TO_TICKS(1));
		count_1ms++;
	}
}

static __attribute__((__noreturn__)) void periodicTask_5ms(void *arg)
{
	TickType_t xLastWakeTime;
	(void)arg;
	xLastWakeTime = xTaskGetTickCount();

	while (1)
	{
		vTaskDelayUntil(&xLastWakeTime, pdMS_TO_TICKS(5));
		count_5ms++;
	}
}

I see that only the highest prio task being activated as in the picture…

If I do something within the task body, the other counts are updated correctly.

[ashokabbi]

Hi All,
I found the problem in my portmacro.h.
Problem was, in the portYIELD() where I trigger the interrupt.
I missed the DSYNC after updating the interrupt register. (DSYNC instruction is the data barrier to clear all the data access pipeline of the TriCore)

Sorry for triggering the discussion… there is no problem in the vTaskDelayUnitl().
This debugging helped me to understand vTaskDelayUntil()…

However… you may consider the optimized implementation as above it is working fine…

[rtel (Richard Barry)]

Thanks for taking the time to report back. It would also be interested for others if you could post your port layer as a zip file here.

[ashokabbi]

Hi @rtel,
Yes, I am interested to put this port back…
Is there any way I test my port with some test suite?
I checked in the GitHub for the test code, didn’t find one…
Is there any other link?

[rtel (Richard Barry)]

See https://www.freertos.org/FreeRTOS-Coding-Standard-and-Style-Guide.html#Testing - most of the testing is done in the Windows port for convenience and the Zynq port to ensure running in a real time environment. There are currently many other tests being written but these are more to do with memory and thread safety proofs (as per https://www.freertos.org/2020/05/ensuring-the-memory-safety-of-freertos-part-2.html).