Skip to content

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

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

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 2 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion list.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,15 @@ 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?).
* - This includes initializing binary semaphores before taking them. If
* you create one with `xSemaphoreCreateBinary()` or
* `xSemaphoreCreateBinaryStatic()`, you must call `xSemaphoreGive()`
* before calling `xSemaphoreTake(). See:
* https://freertos.org/xSemaphoreCreateBinaryStatic.html:
* > The semaphore is created in the 'empty' state, meaning the
* > semaphore must first be given using the xSemaphoreGive() API
* > function before it can subsequently be taken (obtained) using the
* > xSemaphoreTake() function.
Copy link
Member

Choose a reason for hiding this comment

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

I don't want to expand this comment so much. Maybe point 4 could be changed to...

        *   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?). See
        *      respective object documentation for more.

Copy link
Contributor Author

@ElectricRCAircraftGuy ElectricRCAircraftGuy May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can see the only part you recommend adding then is this:

See respective object documentation for more.

That doesn't add anything at all in my opinion. I've already been reading the respective object documentation for years, and actively reading it recently for months.

The scenario is this:

I'm in a several million line codebase.
I did a 1.2 million line (400+ files changed) merge, where there were dozens of files with significant conflicts and in which it took me days to resolve them.

I have no idea why the code no longer runs, but I keep getting stuck in this for loop now and don't know why. What "respective object documentation" should I read in such a situation!? We are using half of the FreeRTOS objects in the kernel and I don't know why my code won't run after a 1.2 million line merge, and which of these objects is now being misused.

By adding my two comments here, when debugging, I'd see this in my debugger, where the green line indicates where the debugger is stuck:

image

Except in the yellow area I'd then see this:

IF YOU FIND YOUR CODE STUCK HERE, SEE THE NOTE JUST ABOVE.

...and then I'd read above. And when I read above, I'd then see this:

    *     - This includes initializing binary semaphores before taking them. If
    *       you create one with `xSemaphoreCreateBinary()` or
    *       `xSemaphoreCreateBinaryStatic()`, you must call `xSemaphoreGive()`
    *       before calling `xSemaphoreTake(). See:
    *       https://freertos.org/xSemaphoreCreateBinaryStatic.html:
    *       > The semaphore is created in the 'empty' state, meaning the
    *       > semaphore must first be given using the xSemaphoreGive() API
    *       > function before it can subsequently be taken (obtained) using the
    *       > xSemaphoreTake() function.

and then I'd go search my merge commit for changes to semaphores, see we converted a mutex to a binary semaphore, and add the xSemaphoreGive() call after creating it (which calls got botched and removed in the merge). Boom done. Would have saved me 2 weeks of non-stop debugging.

* 5) If the FreeRTOS port supports interrupt nesting then ensure that
* the priority of the tick interrupt is at or below
* configMAX_SYSCALL_INTERRUPT_PRIORITY.
Expand All @@ -192,7 +201,9 @@ void vListInsert( List_t * const pxList,
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.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this. It's 10 lines from the comment block above explaining common causes.

Copy link
Contributor Author

@ElectricRCAircraftGuy ElectricRCAircraftGuy May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here's my logic:

Here's the view from the MPLAB X IDE while debugging and halting on this for loop, on which the code is stuck. My eyes naturally focus on the contents of the for loop, in yellow:

image

The purpose of this line:

IF YOU FIND YOUR CODE STUCK HERE, SEE THE NOTE JUST ABOVE.

Is to tell me to look at the notes above, or else I won't, because I'm looking inside the for loop in which the debugger is stuck, instead.

}
}

Expand Down
Loading