CAN clock task not reliably woken up in the presence of CAN errors

[dc42]

See Duet3D/Duet3Expansion#25 (comment). Adding a timeout may not be the best solution, we really need to ensure that tasks are woken up reliably when messages arrive.

[timschneider]

I may found the root cause of it and it solved the problem on my side.

Task creation race condition in TaskBase

A task can start running before AddToList() is called, causing GiveFromISR to do nothing and return because it uses TaskBase's taskID which is 0 and not set yet.

RRFLibraries\src\RTOSIface\RTOSIface.h

	void Create(TaskFunction_t pxTaskCode, c_string pcName, void *_ecv_null pvParameters, unsigned int uxPriority) noexcept
	{
		{
			xTaskCreateStatic(pxTaskCode, pcName, StackWords, pvParameters, uxPriority, stack, this);
			AddToList();
		}
	}

fixed version without suspending the scheduler but preventing a yield to the newly created task.

	void Create(TaskFunction_t pxTaskCode, c_string pcName, void *_ecv_null pvParameters, unsigned int uxPriority) noexcept
	{
		{
			// create with lowest priority to prevent a yield to that thread
			const TaskHandle_t handle = xTaskCreateStatic(pxTaskCode, pcName, StackWords, pvParameters, 0, stack, this);
			AddToList();
			vTaskPrioritySet(handle, uxPriority);
		}
	}

or even stronger with disabled scheduler

void Create(TaskFunction_t pxTaskCode, c_string pcName, void *_ecv_null pvParameters, unsigned int uxPriority) noexcept
	{
		vTaskSuspendAll();
		{
			xTaskCreateStatic(pxTaskCode, pcName, StackWords, pvParameters, uxPriority, stack, this);
			AddToList();
		}
		xTaskResumeAll();
	}

The race condition will unfold its impact in the CoreN2G\src\CanDevice.cpp

		// Test whether messages have been received into receive buffers
		if (ir & CAN_(IR_DRX))
		{
			// Check which receive buffers have new messages
			uint32_t newData;

			while (((newData = hw->REG(NDAT1)) & rxBuffersWaiting) != 0)
			{
				const unsigned int rxBufferNumber = LowestSetBit(newData);
				rxBuffersWaiting &= ~((uint32_t)1 << rxBufferNumber);
				const unsigned int waitingIndex = rxBufferNumber + (unsigned int)RxBufferNumber::buffer0;
				if (waitingIndex < ARRAY_SIZE(rxTaskWaiting))
				{
					TaskBase::GiveFromISR(rxTaskWaiting[waitingIndex], NotifyIndices::CanDevice);
				}
			}
		}

will remove the waiting task from the list of waiting tasks and call RRFLibraries\src\RTOSIface\RTOSIface.cpp GiveFromISR

// Wake up this task from an ISR
void TaskBase::GiveFromISR(uint32_t index) noexcept
{
	if (taskId != 0)			// check that the task has been created and not terminated
	{
		BaseType_t higherPriorityTaskWoken = pdFALSE;
		vTaskNotifyGiveIndexedFromISR(GetFreeRTOSHandle(), index, &higherPriorityTaskWoken);
		portYIELD_FROM_ISR(higherPriorityTaskWoken);
	}
}

but taskId is 0 as AddToList() is not called yet. This will end up in the task waiting for the notification in a deadlock as noone will ever register the waiting task in the list of waiting tasks for the notification again.

I may found a few other bugs:

Is this line in CanDevice.cpp intended to not assign the masked version?

			while (((newData = hw->REG(NDAT1)) & rxBuffersWaiting) != 0)
			{
				const unsigned int rxBufferNumber = LowestSetBit(newData);
				rxBuffersWaiting &= ~((uint32_t)1 << rxBufferNumber);
				const unsigned int waitingIndex = rxBufferNumber + (unsigned int)RxBufferNumber::buffer0;
				if (waitingIndex < ARRAY_SIZE(rxTaskWaiting))
				{
					TaskBase::GiveFromISR(rxTaskWaiting[waitingIndex], NotifyIndices::CanDevice);
				}
			}

vs.

while (((newData = hw->REG(NDAT1) & rxBuffersWaiting)) != 0)

as for my understanding the first version could endup in a deadlock if NDAT1 holds data buffer 0 and buffer 1 and rxBuffersWaiting is waiting for buffer 1; rxBufferNumber = LowestSetBit(newData); will endup in buffer 0 - don't know if this is right and how relevant this is.

And in task.c of freeRTOS (merged with the upstream freeRTOS project)

    uint32_t ulTaskGenericNotifyTake( UBaseType_t uxIndexToWaitOn,
                                      BaseType_t xClearCountOnExit,
                                      TickType_t xTicksToWait )
    {
        uint32_t ulReturn;
        BaseType_t xAlreadyYielded, xShouldBlock = pdFALSE;

        traceENTER_ulTaskGenericNotifyTake( uxIndexToWaitOn, xClearCountOnExit, xTicksToWait );

        configASSERT( uxIndexToWaitOn < configTASK_NOTIFICATION_ARRAY_ENTRIES );

        /* Only block if the notification count is not already non-zero. */
        if( ( pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] == 0UL ) && ( xTicksToWait > ( TickType_t ) 0 ) )
        {
        	/* We MUST suspend the scheduler before exiting the critical
			 * section (i.e. before enabling interrupts).
			 *
			 * If we do not do so, a notification sent from an ISR, which
			 * happens after exiting the critical section and before
			 * suspending the scheduler, will get lost. The sequence of
			 * events will be:
			 * 1. Exit critical section.
			 * 2. Interrupt - ISR calls xTaskNotifyFromISR which adds the
			 *    task to the Ready list.
			 * 3. Suspend scheduler.
			 * 4. prvAddCurrentTaskToDelayedList moves the task to the
			 *    delayed or suspended list.
			 * 5. Resume scheduler does not touch the task (because it is
			 *    not on the pendingReady list), effectively losing the
			 *    notification from the ISR.
			 *
			 * The same does not happen when we suspend the scheduler before
			 * exiting the critical section. The sequence of events in this
			 * case will be:
			 * 1. Suspend scheduler.
			 * 2. Exit critical section.
			 * 3. Interrupt - ISR calls xTaskNotifyFromISR which adds the
			 *    task to the pendingReady list as the scheduler is
			 *    suspended.
			 * 4. prvAddCurrentTaskToDelayedList adds the task to delayed or
			 *    suspended list. Note that this operation does not nullify
			 *    the add to pendingReady list done in the above step because
			 *    a different list item, namely xEventListItem, is used for
			 *    adding the task to the pendingReady list. In other words,
			 *    the task still remains on the pendingReady list.
			 * 5. Resume scheduler moves the task from pendingReady list to
			 *    the Ready list.
			 */
        	taskENTER_CRITICAL();
        	{
        		vTaskSuspendAll();
        		{
        			/* Only block if the notification count is not already non-zero. */
        			if( pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] == 0UL )
        			{
        				/* Mark this task as waiting for a notification. */
						pxCurrentTCB->ucNotifyState[ uxIndexToWaitOn ] = taskWAITING_NOTIFICATION;

						/* Arrange to wait for a notification. */
						xShouldBlock = pdTRUE;
        			}
        			else
					{
						mtCOVERAGE_TEST_MARKER();
					}
				}
				taskEXIT_CRITICAL();

				/* We are now out of the critical section but the scheduler is still
				 * suspended, so we are safe to do non-deterministic operations such
				 * as prvAddCurrentTaskToDelayedList. */
				if( xShouldBlock == pdTRUE )
				{
					traceTASK_NOTIFY_TAKE_BLOCK( uxIndexToWaitOn );
					prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE );
				}
				else
				{
					mtCOVERAGE_TEST_MARKER();
				}
        	}
        	xAlreadyYielded = xTaskResumeAll();

			/* Force a reschedule if xTaskResumeAll has not already done so. */
			if( ( xShouldBlock == pdTRUE ) && ( xAlreadyYielded == pdFALSE ) )
			{
				taskYIELD_WITHIN_API();
			}
			else
			{
				mtCOVERAGE_TEST_MARKER();
			}
        }

/* Force a reschedule if xTaskResumeAll has not already done so. */
if( ( xShouldBlock == pdTRUE ) && ( xAlreadyYielded == pdFALSE ) )

[dc42]

Thanks Tim, I think you've nailed it. I'll implement the first of your suggested changes because I think that should be sufficient.

[dc42]

Tim, I also agree with your change to CanDevice.cpp so I've implemented that too. Regarding the change to FreeRTOS, which upstream version of FreeRTOS did you get that code from?

[timschneider]

The above ulTaskGenericNotifyTake is a mix/merge of the Duet3D FreeRTOS version with the upstream FreeRTOS version. I initially posted only a portion of the merged function.

Upstream reference: FreeRTOS-Kernel tasks.c#L7701-L7796

While investigating a race condition in the wakeup path, I noticed that ulTaskGenericNotifyTake differs from the upstream version in a few ways:

1. NotifyState is set regardless of xTicksToWait — even for non-blocking calls where we don't intend to wait:

   /* Mark this task as waiting for a notification. */
   pxCurrentTCB->ucNotifyState[ uxIndexToWaitOn ] = taskWAITING_NOTIFICATION;

2. NotifyState is set even when the scheduler is still running and not suspended.

These differences weren't the cause of my issue, but I thought they were worth mentioning.

For reference, here is the complete function I ended up with:

#if ( configUSE_TASK_NOTIFICATIONS == 1 )

    uint32_t ulTaskGenericNotifyTake( UBaseType_t uxIndexToWaitOn,
                                      BaseType_t xClearCountOnExit,
                                      TickType_t xTicksToWait )
    {
        uint32_t ulReturn;
        BaseType_t xAlreadyYielded, xShouldBlock = pdFALSE;

        traceENTER_ulTaskGenericNotifyTake( uxIndexToWaitOn, xClearCountOnExit, xTicksToWait );

        configASSERT( uxIndexToWaitOn < configTASK_NOTIFICATION_ARRAY_ENTRIES );

        /* Only block if the notification count is not already non-zero. */
        if( ( pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] == 0UL ) && ( xTicksToWait > ( TickType_t ) 0 ) )
        {
        	/* We MUST suspend the scheduler before exiting the critical
			 * section (i.e. before enabling interrupts).
			 *
			 * If we do not do so, a notification sent from an ISR, which
			 * happens after exiting the critical section and before
			 * suspending the scheduler, will get lost. The sequence of
			 * events will be:
			 * 1. Exit critical section.
			 * 2. Interrupt - ISR calls xTaskNotifyFromISR which adds the
			 *    task to the Ready list.
			 * 3. Suspend scheduler.
			 * 4. prvAddCurrentTaskToDelayedList moves the task to the
			 *    delayed or suspended list.
			 * 5. Resume scheduler does not touch the task (because it is
			 *    not on the pendingReady list), effectively losing the
			 *    notification from the ISR.
			 *
			 * The same does not happen when we suspend the scheduler before
			 * exiting the critical section. The sequence of events in this
			 * case will be:
			 * 1. Suspend scheduler.
			 * 2. Exit critical section.
			 * 3. Interrupt - ISR calls xTaskNotifyFromISR which adds the
			 *    task to the pendingReady list as the scheduler is
			 *    suspended.
			 * 4. prvAddCurrentTaskToDelayedList adds the task to delayed or
			 *    suspended list. Note that this operation does not nullify
			 *    the add to pendingReady list done in the above step because
			 *    a different list item, namely xEventListItem, is used for
			 *    adding the task to the pendingReady list. In other words,
			 *    the task still remains on the pendingReady list.
			 * 5. Resume scheduler moves the task from pendingReady list to
			 *    the Ready list.
			 */
        	taskENTER_CRITICAL();
        	{
        		vTaskSuspendAll();
        		{
        			/* Only block if the notification count is not already non-zero. */
        			if( pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] == 0UL )
        			{
        				/* Mark this task as waiting for a notification. */
						pxCurrentTCB->ucNotifyState[ uxIndexToWaitOn ] = taskWAITING_NOTIFICATION;

						/* Arrange to wait for a notification. */
						xShouldBlock = pdTRUE;
        			}
        			else
					{
						mtCOVERAGE_TEST_MARKER();
					}
				}
				taskEXIT_CRITICAL();

				/* We are now out of the critical section but the scheduler is still
				 * suspended, so we are safe to do non-deterministic operations such
				 * as prvAddCurrentTaskToDelayedList. */
				if( xShouldBlock == pdTRUE )
				{
					traceTASK_NOTIFY_TAKE_BLOCK( uxIndexToWaitOn );
					prvAddCurrentTaskToDelayedList( xTicksToWait, pdTRUE );
				}
				else
				{
					mtCOVERAGE_TEST_MARKER();
				}
        	}
        	xAlreadyYielded = xTaskResumeAll();

			/* Force a reschedule if xTaskResumeAll has not already done so. */
			if( ( xShouldBlock == pdTRUE ) && ( xAlreadyYielded == pdFALSE ) )
			{
				taskYIELD_WITHIN_API();
			}
			else
			{
				mtCOVERAGE_TEST_MARKER();
			}
        }

        taskENTER_CRITICAL();
        {
            traceTASK_NOTIFY_TAKE( uxIndexToWaitOn );
            ulReturn = pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ];

            if( ulReturn != 0UL )
            {
                if( xClearCountOnExit != pdFALSE )
                {
                    pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] = ( uint32_t ) 0UL;
                }
                else
                {
                    pxCurrentTCB->ulNotifiedValue[ uxIndexToWaitOn ] = ulReturn - ( uint32_t ) 1;
                }
            }
            else
            {
                mtCOVERAGE_TEST_MARKER();
            }

            pxCurrentTCB->ucNotifyState[ uxIndexToWaitOn ] = taskNOT_WAITING_NOTIFICATION;
        }
        taskEXIT_CRITICAL();

        traceRETURN_ulTaskGenericNotifyTake( ulReturn );

        return ulReturn;
    }