Bug in vTaskStartScheduler()

2013-02-03
2013-09-16
  • 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

     
  • Richard
    Richard
    2013-02-03

    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.

     
  • Richard Damon
    Richard Damon
    2013-02-03

    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.

     
  • 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.

     
  • Richard
    Richard
    2013-02-04

    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.

     
  • 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

     
  • Richard
    Richard
    2013-02-10

    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.