Freertosrp2350: use FreeRTOS macros in noInterrupts/interrupts when applicable.

[fietser28]

Related to #2428.

[earlephilhower]

Awesome work! A simple test I thought up while driving home this weekend runs perfectly with this, as well as the more involved malloc stress test. Great digging.

Let me try this out on the RP2040 before merging, but right now it's looking great!

// Released to the public domain
#include <FreeRTOS.h>
#include <task.h>
#include <map>
#define STACK_SIZE 512
#define CORE_0 (1 << 0)
#define CORE_1 (1 << 1)

std::map<eTaskState, const char *> eTaskStateName { {eReady, "Ready"}, { eRunning, "Running" }, {eBlocked, "Blocked"}, {eSuspended, "Suspended"}, {eDeleted, "Deleted"} };
void ps() {
  int tasks = uxTaskGetNumberOfTasks();
  TaskStatus_t *pxTaskStatusArray = new TaskStatus_t[tasks];
  unsigned long runtime;
  tasks = uxTaskGetSystemState( pxTaskStatusArray, tasks, &runtime );
  Serial.printf("# Tasks: %d\n", tasks);
  Serial.printf("%-3s %-16s %-10s %s %s\n", "ID", "NAME", "STATE", "PRIO", "CYCLES");
  for (int i = 0; i < tasks; i++) {
    Serial.printf("%2d: %-16s %-10s %4d %lu\n", i, pxTaskStatusArray[i].pcTaskName, eTaskStateName[pxTaskStatusArray[i].eCurrentState], (int)pxTaskStatusArray[i].uxCurrentPriority, pxTaskStatusArray[i].ulRunTimeCounter);
  }
  delete[] pxTaskStatusArray;
}

static TaskHandle_t l[16];

void loop() {
  ps();
  delay(1000);
}

#define LOOP(z) \
void loop##z(void *params) {\
  (void) params;\
  while (true) {\
    srand(z);\
    int sum = 0;\
    for (int i = 0; i < 500000; i++) sum+= rand();\
    Serial.printf("L%d: %08x\n", z, sum);\
    delay(1000 + z * 10);\
  }\
}

LOOP(0);
LOOP(1);
LOOP(2);
LOOP(3);


void setup() {
  xTaskCreate(loop0, "loop0", STACK_SIZE, NULL, 1, &l[0]);
  vTaskCoreAffinitySet(l[0], CORE_0);
  xTaskCreate(loop1, "loop1", STACK_SIZE, NULL, 1, &l[1]);
  vTaskCoreAffinitySet(l[1], CORE_0);
  xTaskCreate(loop2, "loop2", STACK_SIZE, NULL, 1, &l[2]);
  vTaskCoreAffinitySet(l[2], CORE_0);
  xTaskCreate(loop3, "loop3", STACK_SIZE, NULL, 1, &l[3]);
  vTaskCoreAffinitySet(l[3], CORE_1);
}

[earlephilhower]

I had a chance to run this in the debugger to check and it's actually not correct. It's just disabling IRQ enable/disable completely. There is no taskENTER/EXIT_CRITICAL defined and the call is optimized out. You can load in GDB and look at the assembly, it's just a no-op.

FreeRTOS calls need to be defined extern "C" and this defines a weak C++ call with the same name (but different linkage due to the C++ name mangling).

Unfortunately that's not enough to actually get it to do anything. There's also no extern "C" taskEXIT_CRITICAL() call. It's a #define (of potentially another #define) in task.h. :(

[earlephilhower]

portENTER/EXIT_CRITITCAL is actually another set of #defines...

earle@amd:~/Arduino/hardware/pico/rp2040/libraries/FreeRTOS/lib/FreeRTOS-Kernel/portable/ThirdParty/GCC/RP2040$ grep -r portENTER_CRITICAL
include/portmacro.h:    #define portENTER_CRITICAL()    vPortEnterCritical()
include/portmacro.h:    #define portENTER_CRITICAL()               vTaskEnterCritical()
include/portmacro.h:    #define portENTER_CRITICAL_FROM_ISR()      vTaskEnterCriticalFromISR()
earle@amd:~/Arduino/hardware/pico/rp2040/libraries/FreeRTOS/lib/FreeRTOS-Kernel/portable/ThirdParty/GCC/RP2040$ cd ..

earle@amd:~/Arduino/hardware/pico/rp2040/libraries/FreeRTOS/lib/FreeRTOS-Kernel/portable/ThirdParty/GCC$ cd RP2350_ARM_NTZ/
earle@amd:~/Arduino/hardware/pico/rp2040/libraries/FreeRTOS/lib/FreeRTOS-Kernel/portable/ThirdParty/GCC/RP2350_ARM_NTZ$ grep -r portENTER_CRITICAL
non_secure/portmacro.h:    #define portENTER_CRITICAL()                      vPortEnterCritical()
non_secure/portmacro.h:    #undef portENTER_CRITICAL
non_secure/portmacro.h:    #define portENTER_CRITICAL()               vTaskEnterCritical()
non_secure/portmacro.h:    #define portENTER_CRITICAL_FROM_ISR()      vTaskEnterCriticalFromISR()
non_secure/portmacrocommon.h:#define portENTER_CRITICAL()                      vPortEnterCritical()

If we look at RP2350 vPortEnterCritical we see it's:

void vPortEnterCritical( void ) /* PRIVILEGED_FUNCTION */
{
    portDISABLE_INTERRUPTS();
#if configNUMBER_OF_CORES == 1
    ulCriticalNesting++;
#else
    ulCriticalNestings[portGET_CORE_ID()]++;
#endif

    /* Barriers are normally not required but do ensure the code is
     * completely within the specified behaviour for the architecture. */
    __asm volatile ( "dsb" ::: "memory" );
    __asm volatile ( "isb" );
}

And, portDisableInterrupts === noInterrupts() === infinite loop if we do actually unwind to the proper name and get it hooked it. Basically, disabling IRQs is a subset of a critical section on FreeRTOS...

[fietser28]

Oeps, missed the macro part. I've changed it now similar to other FreeRTOS macro calls.

Als had a look with the debugger and it now really calls the FreeRTOS taskENTER/EXIT_CRITICAL. These calls also temporarily disable the FreeRTOS scheduler. This might be a bit of overkill?

Thanks for your patience with me trying to find the way around in all this. There is a good reason I like to use Arduino-pico: It

[earlephilhower]

No worries, and I do appreciate your persistence! It's a subtle thing, and you might just have cracked it. Still puzzling me is how adjusting noInterrupts and interruptsaffects FreeRTOS scheduling. Those calls never happen in the FreeRTOS variant.cpp wrapper or FreeRTOS core files.

I traced this through and I was wrong about the portDISABLE_INTERRUPTS ending up in an infinite loop. For some reason I thought that was #defined (yet again) to noInterrupts but it is instead natively implemented meaning no infinite loop. Looks good now, I see IRQs disabled as appropriate.

noInterrupts/interrupts are used in the core pretty sparingly. As long as this preserves the "no interrupts" semantic, additional "no task switch" shouldn't be an issue

• The malloc-class functions all do this to avoid issues with user LWIP callbacks also wanting to do memory allocs. This is a short lifetime thing and seems OK to say "swapping tasks while performing surgery on the heap is a bad thing."
• GPIO, SPI, and other peripheral IRQ attach/detach, also very short lived
• Flash read/write protection (again, short lived)

I'll throw some more tests at this on the 2040 and the 2350 and see where we end up. But so far basic sanity and code inspection seem good!

[earlephilhower]

I can't break this in my own testing, and the change makes logical sense, too! Nice work!