CoreMutex aquiring for FreeRTOS

[hreintke]

Hi,

Working with the FreeRTOS library at the moment.

Running with Picoprobe(CMSIS-DAP) as upload method and Pico SDK as USB stack, so using 2 USB ports.

Noticed that I do not get all Serial.printf(...) data on the USB port.

This is my current smallest testprogram :

#include "Arduino.h"
#include "freertos.h"
#include "task.h"
#include <map>

static inline TickType_t MsToTicks(TickType_t milliseconds)
{
    return milliseconds / portTICK_PERIOD_MS;
}

void ledTask(void*);
void setup()
{
    Serial.begin(115200);

    pinMode(16, OUTPUT);
    pinMode(32, OUTPUT);
    xTaskCreate(ledTask,"Led16", 1024, reinterpret_cast<void*>(16), configMAX_PRIORITIES - 1, NULL);
    xTaskCreate(ledTask,"Led32", 1024, reinterpret_cast<void*>(32), configMAX_PRIORITIES - 1, NULL);
}

void ledTask(void* param)
{
    int ledPin = reinterpret_cast<int>(param);
    int taskCount = 0;
    bool ledValue = false;
    char* taskName = pcTaskGetName(NULL);
    for(;;)
    {
        vTaskDelay(MsToTicks(1000));
        digitalWrite(ledPin, ledValue ? HIGH:LOW);
        ledValue = !ledValue;

        if (ledPin == 16)
        {
            Serial.printf("Led16 %3d\r\n", taskCount++);
        }
        else if (ledPin == 32)
        {

            Serial.printf("Led32 %3d\r\n", taskCount++);
        }
        else
        {
            Serial.printf("LedXX %3d\r\n", taskCount++);
        }
    }
}

void loop()
{
} 

Which gives (varying) results like :

Led32  12
Led32  13
Led32  14
Led32  15
Led32  17
Led32  19
Led32  20
Led32  21

In this case, all output from led16 task is lost.

Found the reason for this behavior :

In SerialUSB there is in the write function :

size_t SerialUSB::write(const uint8_t *buf, size_t length) {
    CoreMutex m(&__usb_mutex, false);
    if (!_running || !m) {
        return 0;
    }

And in CoreMutex.cpp :

CoreMutex::CoreMutex(mutex_t *mutex, uint8_t option) {
    _mutex = mutex;
    _acquired = false;
    _option = option;
    if (__isFreeRTOS) {
        auto m = __get_freertos_mutex_for_ptr(mutex);
        if (_option & FromISR) {
            __freertos_mutex_take_from_isr(m);
        } else {
            if (!__freertos_mutex_try_take(m)) {
                return;
            }
        }
    } else {
        uint32_t owner;
        if (!mutex_try_enter(_mutex, &owner)) {
            if (owner == get_core_num()) { // Deadlock!
                if (_option & DebugEnable) {
                    DEBUGCORE("CoreMutex - Deadlock detected!\n");
                }
                return;
            }
            mutex_enter_blocking(_mutex);
        }
    }
    _acquired = true;
}

For freertos, there is a __freertos_mutex_try_take(m) -> it failes when the mutex is not available. The is no (blocking) wait to get the mutex.

And, there is no check in the FromISR whether the mutex is taken.

Expected something like :

    if (__isFreeRTOS) {
        auto m = __get_freertos_mutex_for_ptr(mutex);
        if (_option & FromISR) {
            if (!__freertos_mutex_take_from_isr(m)) {
                return;
            }
        } else {
            __freertos_mutex_take(m))         }
    }

Running with this code I get the expected output :

Led32  15
Led16  16
Led32  16
Led16  17
Led32  17
Led16  18
Led32  18
Led16  19
Led32  19
Led16  20
Led32  20
Led16  21
Led32  21
Led16  22
Led32  22
Led16  23
Led32  23
Led16  24

Still investigating, but what are your thoughts about this ?

[earlephilhower]

(Note, you should update because this file was changed for the 3.2.1 release to clean it up and handle the IRQ stuff automatically.)

This is basically an artifact of the way FreeRTOS was patched^Whacked in to the core. In the plain SDK case, if the mutex is already locked on this core then we're in some bad state. There's no idea of "waking up" the holding process, because there are no processes. Only core0, core1 and core0irq, core1irq contexts. If the mutex is locked on the other core we can make forward progress so will block until it's available.

--edit start-- In this case it seems like you got a task swapped out while holding the mutex, only to swap in another which tries to grab the mutex but fails, since the swapped-out task has it. I think in the FreeRTOS case this will happen even if they're not on the same core. So actually, this will happen even if the other task is not swapped out. It will be running with the lock and this code will see it and not do the "am I on the same core" check the main core does and immediately bail. So it's actually handling things worse than the basic core stuff at this point... --edit end--

And, there is no check in the FromISR whether the mutex is taken.

I thought the underlying call will block in the SMP FreeRTOS, so there is no need to do anything special. If you try to grab a mutex in an ISR that someone else already has, and you're on the same core, well then that's gonna be a bad time...

--edit 2-- https://www.freertos.org/xSemaphoreTakeFromISR.html Yup, it can fail so your change looks like the start of a good idea. It still needs some work, however, because the GiveFromISR needs to do some magic stuff if there was a prio inversion per: https://www.freertos.org/a00124.html . That's actually a significant change to the core proper because it can't be isolated in the wrapper. The return from ISR itself needs that new call portYIELD_FROM_ISR( xHigherPriorityTaskWoken ); --edit end

Still investigating, but what are your thoughts about this ?

I'll need to think about this, but feel free to do a PR. I made certain assumptions while using this RAII model in the core which I can guarantee are true in the case where FreeRTOS behaves exactly as base SDK. Conceptually, I think you're right and we can use the FreeRTOS priority inversion stuff to handle the case w/o punting.

[earlephilhower]

Re: portYieldFromISR, it looks like this is a nice to have but not 100% necessary call, per the (old) discussion at https://www.freertos.org/FreeRTOS_Support_Forum_Archive/May_2009/freertos_What_if_portYIELD_FROM_ISR_not_used_3282756.html

If there was someone else holding the mutex during an ISR, to me it looks like you could have a lower prio task running until the next task switch. Not the most correct, but not going to crash. And requiring no core changes outside the CoreMutex.

Looks like what you've already done is good enough to go to a PR, I think!

[hreintke]

My primary remarks on Coremutex in ISR were on __freertos_mutex_take_from_isr(m); without checking result. If it returns false, the mutex is not taken but the CoreMutex object sets acquired to true. And on __freertos_mutex_try_take(m)) which only tries and not waits until the mutex is taken.

I am not sure a portYieldFromISR is necessary. In several documentation I read : From FreeRTOS V7.3.0 pxHigherPriorityTaskWoken is an optional parameter and can be set to NULL. Will check what that really means by checking the code. If it needs to check the pxHigherPriorityTaskWoken, there could be something like __freertos_mutex_give_from_isr_yield which does the xSemaphoreGiveFromISR and the 'portYieldFromISR` Or.. add the portYieldFromISR to the __freertos_mutex_give_from_isr anywway. It's the tradeoff between 1/ possible additional portYieldFromISR and 2/ possible lower prio task running until the next task switch

I will make a PR, try to do this weekend.

[earlephilhower]

Closed by #1481 and #1484