Bug in vTaskStartScheduler()

Kernel
#1

friedl wrote on Sunday, February 03, 2013:

Hello Richard!

I’ve just implemented vPortEndScheduler on a port and had to find out that there is a bug in vTaskStartScheduler().

The following will always end in the assert stopping the whole thing:

        if( xPortStartScheduler() != pdFALSE )
        {
            /* Should not reach here as if the scheduler is running the
            function will not return. */
        }
        else
        {
            /* Should only reach here if a task calls xTaskEndScheduler(). */
        }
    }
    /* This line will only be reached if the kernel could not be started. */
    configASSERT( xReturn );

The corrected version, and btw. the comments where wrong as well:

        xReturn = xPortStartScheduler();
        if( xReturn != pdFALSE )
        {
            /* Should only reach here if a task calls xTaskEndScheduler(). */
        }
        else
        {
            /* Should not reach here as if the scheduler is running the
            function will not return. */
        }
    }
    /* This line will only be reached if the kernel could not be started. */
    configASSERT( xReturn );

This has been tested and verified.

If you want a patch file drop me a note.

Regards
Friedl

#2

rtel wrote on Sunday, February 03, 2013:

The end scheduler function only exists for the PC port - which runs on top of DOS and can return to DOS when the scheduler is ended.  None of the other ports have any that can be returned to.

In the PC port the end scheduler function is implemented using setjmp() and longjmp().  Looking at the code (I have not looked for a long time) it seems that pdFALSE is returned if the function exits because the end scheduler function was called.  So the code:

if( xPortStartScheduler() != pdFALSE )
{
    /* Should not reach here as if the scheduler is running the
    function will not return. */
}

would be correct because pdFALSE was not returned so the scheduler never started in the first place, and the else part:

if( xPortStartScheduler() != pdFALSE )
{
}
else
{
    /* Should only reach here if a task calls xTaskEndScheduler(). */
}

would also be correct because if false was returned then the function exited because xTaskEndScheduler() was called.

This is actually a really obscure part of the code that seemed like a good idea when the RTOS was originally written, but was in fact never used.

Regards.

#3

richard_damon wrote on Sunday, February 03, 2013:

I think Friedl was referring to the lines

/* This line will only be reached if the kernel could not be started. */
    configASSERT( xReturn );

which are testing the xReturn flag which wasn’t set by the previous code.

Although looking at the code for vTaskStartScheduler, it looks like this flag should have been set earlier from creating the Idle task or the Timer task, so the assert shouldn’t have been firing unless the function had failed to start things up, or if in the process of starting and stoping the task scheduler the stack used by the main() code which called this got corrupted, like being used for the interrupt stack, as done in some ports.

#4

friedl wrote on Sunday, February 03, 2013:

To get the logic right the function vTaskStartScheduler( ) has to evaluate the return value of xPortStartScheduler(), if it ever returns.

Then xPortStartScheduler() has to return pdPASS/pdTRUE because if succeeded in starting the scheduler and now returns or the assert at the end of vTaskStartScheduler( ) will stop the show.

#5

rtel wrote on Monday, February 04, 2013:

I’m still not sure I understand the problem.

I think Friedl was referring to the lines

/* This line will only be reached if the kernel could not be started. */
    configASSERT( xReturn );

which are testing the xReturn flag which wasn’t set by the previous code.

Looking at this again, the assert() call is checking for the scheduler not starting because either the idle task or the timer task could not be created - that is the only time xReturn is set.  The timer and idle task creation is outside of the code snippet in the original post.  In which case, the comment assumes that vTaskEndScheduler() is not a valid way of returning from xPortStartScheduler() - which in all but one case it is not.  To cover the case when it is I have changed the comment to read:

/* This line will only be reached if the kernel could not be started, or
vTaskEndScheduler() was called (vTaskEndScheduler() is not implemented for
most ports). */
configASSERT( xReturn );

To get the logic right the function vTaskStartScheduler( ) has to evaluate the return value of xPortStartScheduler(), if it ever returns.

In the single case where vTaskEndScheduler() is implemented when the scheduler returns it returns to a point within the xPortStartScheduler() function, then xPortStartScheduler() returns.  xPortStartScheduler() should only return if the scheduler could not be started, so it can be assumed that if it returns then vTaskEndScheduler() was called - which makes the comments in the original post make sense.

Regards.

#6

friedl wrote on Sunday, February 10, 2013:

Hi Richard!

Regarding your comment at the tracker
Tracker - Bugs - patch: update comments and handling of xPortStartScheduler() - ID: 3603273:

I currently don’t see that change in the repos though - see
tasks.c.

If you’d really move the assert from the end of vTaskStartScheduler( )
into the else of “if( xReturn == pdPASS )” then I agree with your
implementation.

I myself would have kept the assert at the very end of
vTaskStartScheduler() and that’s why I suggested to correct
the logic of assigning and evaluating xReturn.

But if you commit what you’ve posted at tracker then that’s
fine with me.

Regards
Friedl

#7

rtel wrote on Sunday, February 10, 2013:

None of the recent changes are committed yet. I’m going through the tracker items as I’m hoping to create a new release soon.

Regards.