Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

list.c: improve code comments to point to official documentation about problems which may cause code to get stuck inside of list.c #1051

Merged
merged 3 commits into from
May 13, 2024
Merged

list.c: improve code comments to point to official documentation about problems which may cause code to get stuck inside of list.c #1051

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee02d99
list.c: improve documentation about initializing binary semaphores
ElectricRCAircraftGuy May 8, 2024
b67980c
list.c: add comment
ElectricRCAircraftGuy May 8, 2024
36f4ffe
Code review suggestions
aggarg May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions list.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,15 +184,24 @@ void vListInsert( List_t * const pxList,
* 4) Using a queue or semaphore before it has been initialised or
* before the scheduler has been started (are interrupts firing
* before vTaskStartScheduler() has been called?).
* 5) If the FreeRTOS port supports interrupt nesting then ensure that
* 5) Attempting to 'take' binary semaphores created using
* `xSemaphoreCreateBinary()` or `xSemaphoreCreateBinaryStatic()`
* APIs, before 'giving' them. Binary semaphores created using
* `xSemaphoreCreateBinary()` or `xSemaphoreCreateBinaryStatic()`,
* are created in a state such that the semaphore must first be
* 'given' using xSemaphoreGive() API before it can be 'taken' using
* xSemaphoreTake() API.
Comment on lines +187 to +193
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new statement seems misleading. Taking a binary semaphore that has not yet been given results in a yield, not getting stuck in this loop. However, taking a binary semaphore before it has been created does end up this way. Item 4 above covers that case already.

There's nothing wrong with attempting to take a binary semaphore that has not yet been given -- in fact that is the most common use of a binary semaphore -- so maybe we're better off without this new statement in the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are exactly right @jefftenney. I verified that with the following code on a Cortex-M7:

void Task( void * param )
{
    SemaphoreHandle_t xBinarySem;

    ( void ) param;

    xBinarySem = xSemaphoreCreateBinary();

    xSemaphoreTake( xBinarySem, portMAX_DELAY );

    for( ;; )
    {
        HAL_GPIO_TogglePin( LD1_GPIO_Port, LD1_Pin );
        vTaskDelay( pdMS_TO_TICKS( 1000 ) );
    }
}
void app_main( void )
{
    xTaskCreate( Task,
                 "Task",
                 configMINIMAL_STACK_SIZE,
                 NULL,
                 tskIDLE_PRIORITY | portPRIVILEGE_BIT,
                 NULL );

    vTaskStartScheduler();

    for( ;; );
}

I'll revert it in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in this PR - #1056. Thanks again @jefftenney !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jefftenney and @aggarg , thank you both for taking a look at this. Is there an explanation for the behavior I was seeing, then? I have a TaskSetup() function inside main() which gets called before we call vTaskStartScheduler(); at the end of main(). TaskSetup() contains, among other things, this:

xI2C1mutex = xSemaphoreCreateBinaryStatic(&xI2C1mutexBuffer);	// I2C1 bus
xI2C2mutex = xSemaphoreCreateBinaryStatic(&xI2C2mutexBuffer);	// I2C2 bus
xI2C4mutex = xSemaphoreCreateBinaryStatic(&xI2C4mutexBuffer);	// I2C4 bus

Like that, my debugger gets stuck forever inside of vListInsert() in list.c.

If I change it to this, however, it works fine:

xI2C1mutex = xSemaphoreCreateBinaryStatic(&xI2C1mutexBuffer);	// I2C1 bus
xI2C2mutex = xSemaphoreCreateBinaryStatic(&xI2C2mutexBuffer);	// I2C2 bus
xI2C4mutex = xSemaphoreCreateBinaryStatic(&xI2C4mutexBuffer);	// I2C4 bus

xSemaphoreGive(xI2C1mutex);
xSemaphoreGive(xI2C2mutex);
xSemaphoreGive(xI2C4mutex);

Furthermore, I think bullet 4 still needs something extra. It says:

*   4) Using a queue or semaphore before it has been initialised or
*      before the scheduler has been started (are interrupts firing
*      before vTaskStartScheduler() has been called?).

So, does this include regular mutexes, recursive mutexes, and binary semaphores? Or, just binary semaphores? I mean, a mutex is a type of semaphore.

Also, what does it mean to "be initialised"?

@jefftenney , your wording above, "taking a binary semaphore before it has been created", makes more sense than using the word "initialised", unless initialized means something different, such as "created and taken", as seems to be implied by the official documentation:

The semaphore is created in the 'empty' [I read this as "uninitialized"] state, meaning the semaphore must first be given using the xSemaphoreGive() API function before it can subsequently be taken (obtained) using the xSemaphoreTake() function.

So, something in this documentation needs to be improved and or clarified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ElectricRCAircraftGuy The term "create" and "initialize" are equivalent here. One possible improvement to the official docs would be to use "create" exclusively, because that term matches the API function names.

The term "empty" does not refer to the created/uncreated state. Instead it describes one of the two states of a semaphore after it has been created. It is either empty or full. Some people say taken or given. Some people say unsignaled or signaled. Application usage of the semaphore changes it back and forth between these two states.

Another improvement could be to add clarity as you suggested. Something like this (probably needs work):

The semaphore is created in the 'empty' state, meaning the semaphore must first be given using the xSemaphoreGive() API function before xSemaphoreTake() can successfully take the semaphore.

All of this still leaves you with your mystery. Do any of the 5 items listed in the code comment apply? Something might be corrupting kernel data structures and your startup calls to xSemaphoreGive() might be hiding the issue.

* 6) If the FreeRTOS port supports interrupt nesting then ensure that
* the priority of the tick interrupt is at or below
* configMAX_SYSCALL_INTERRUPT_PRIORITY.
**********************************************************************/

for( pxIterator = ( ListItem_t * ) &( pxList->xListEnd ); pxIterator->pxNext->xItemValue <= xValueOfInsertion; pxIterator = pxIterator->pxNext )
{
/* There is nothing to do here, just iterating to the wanted
* insertion position. */
* insertion position.
* IF YOU FIND YOUR CODE STUCK HERE, SEE THE NOTE JUST ABOVE.
*/
}
}

Expand Down