Can it support FreeRTOS?

[salyzyn]

When compiling in a FreeRTOS (#define FREERTOS 1) environment, the header build reports:

readerwriterqueue/atomicops.h
581:2: error: #error Unsupported platform! (No semaphore wrapper available)

In an environment that does not support it, but a libc++ is available, we (should) have the following at least:

#ifdef __cplusplus
#if __cplusplus > 202000L
#include <semaphore>
    std::binary_semaphore wakeup_;
#else
#include <condition_variable>
#include <mutex>
    std::condition_variable wakeup_;
    std::mutex lock_;
#endif
#endif

For a direct FreeRTOS implementation, I would expect it to use something like:

#include <FreeRTOS.h>
#include <semphr.h>
#include <task.h>
*__m = xSemaphoreCreateCounting(max,initial);
xSemaphoreTake(,*__m, portMAX_DELAY);
xSemaphoreGive(*__m);
vSemaphoreDelete(*__m);

[cameron314]

At the time, std::counting_semaphore did not exist. Perhaps it's time to add it as a fallback although I'd be surprised if it linked with a typical FreeRTOS build.

I'm tempted to add direct support for FreeRTOS (I've worked a little with the platform in the past). But there doesn't seem to be a standard way to detect it at compile time.

[salyzyn]

Yes, I implemented libc++ for my FreeRTOS build using the Android external/libcxx and external/libcxxabi trees plus some goo of my own since they have an unencumbered license, so not typical. I was just talking generically.

A number of support libraries I've touched recently (I am new to FreeRTOS and the ARM processors I am building for) that need to know require -DFREERTOS=1 on the compiler command line, so I'd use that. I can implement, or be the Guinea pig, either way.

[cameron314]

I've pushed an (untested) implementation. Please let me know if you encounter any bugs. Thanks for volunteering as guinea pig :-)

[salyzyn]

Well, the first test passed (compilation) :-). Compiling using STMCubeF4 SDK, and Nordic Semiconductors NRF51K SDK, with their ports of FreeRTOS, using the latest GCC compiler 10.2.1. My review of the code you added was IMHO perfect and expected.

Unfortunately I spoke too soon, we do not have a running application yet on these platforms, so I will report back once I start breaking things ;-} or if I machinate a specific unit test of the queue.

I assume this will also port to your other project concurrentqueue into the lightweightsemaphore.h.

THANKS!

[cameron314]

so I will report back once I start breaking things ;-}

Excellent :-)

I assume this will also port to your other project concurrentqueue into the lightweightsemaphore.h.

The FreeRTOS implementation will be the same, yes, I'll make a note to bring it over. There's also a fair bit more dependencies on libcxx features for that queue that you may run into when porting it to FreeRTOS, so keep an eye out.

[salyzyn]

Half way into my project, found an initial deficiency in that I could not feed the queue inside an interrupt service routine without panic'ing FreeRTOS. I have not declared success yet since my driver is not working end-to-end yet for a multitude of other reasons, but it now survives a call to signal(), and for the most part appears to communicate the message into the queue. Suggested change below which also proactively deals with allowing a non-blocking try_wait() from within an interrupt service routine as well should the interrupt service routine wish to pull from a queue (untested):

diff --git a/atomicops.h b/atomicops.h
index ffd215a..920001b 100644
--- a/atomicops.h
+++ b/atomicops.h
@@ -613,7 +613,7 @@ namespace moodycamel
 
 			bool try_wait() AE_NO_TSAN
 			{
-				return xSemaphoreTake(m_sema, 0) == pdTRUE;
+				return xSemaphoreTakeFromISR(m_sema, nullptr) == pdTRUE;
 			}
 
 			bool timed_wait(std::uint64_t usecs) AE_NO_TSAN
@@ -625,7 +625,7 @@ namespace moodycamel
 
 			void signal() AE_NO_TSAN
 			{
-				BaseType_t rc = xSemaphoreGive(m_sema);
+				BaseType_t rc = xSemaphoreGiveFromISR(m_sema, nullptr);
 				assert(rc == pdTRUE);
 				AE_UNUSED(rc);
 			}

NB: Older (v7 or older) versions of FreeRTOS make the second argument of the semaphore FromISR routine as mandatory, but the various SDKs I am working with have v8 and above allowing a nullptr to be passed into the optional argument. See https://www.freertos.org/a00124.html

[cameron314]

Ah, didn't realize you needed to use the queue from an ISR context. Is it safe to use the FromISR versions in a non-ISR context? The documentation seems to imply it but doesn't state this explicitly.

[salyzyn]

I feel like the above is more of a heads up as I am still working and testing.

IDK, I am still a neophyte in this specific environment. I will have to read the FreeRTOS code and then I will wonder if some ports create a problem. The main problem of course would be that one would need to take a critical section if running in mainline.

I already do something like this in my executive (that uses your readwriterqueue as well):

#ifdef FREERTOS
    auto isr = xPortIsInsideInterrupt();
    if (!isr) {
            portENTER_CRITICAL();
    }
#endif
    auto ret = fastQ_.try_enqueue({function, arg1, arg2, arg3});
#ifdef FREERTOS
    if (!isr) {
            portEXIT_CRITICAL();
    }
#endif

which is making sure when I enqueue entries into the readerwriterqueue we are safe. But this is cargo cult paranoia to prevent multiple writers (interrupt service, or a thread switch on this single processor mcu stuffing functions) just happens to make sure that the call to xSemaphoreGiveFromISR is inside a critical section regardless.

This would not be true of a dedicated queue from a single interrupt service and a single mainline.

Feel free to use a call to xPortIsInsideInterrupt() to switch between the FromISR and the regular form for now until we have a definitive answer?

[salyzyn]

Confirmed that the following continued to work. I am further along, but I am expecting I can not remove the xPortIsInsideInterrupt() cargo cult until I can dig deeper. I looked (a bit) at the FreeRTOS code and it got too loopy and redirected for me to immediately figure out the detailed differences between xSemaphoreTakeFromISR and xSemaphoreTake were, they did seem to call completely different handling (xSemaphoreTakeFromISR is restricted in which kinds of semaphores it works on as documented).

diff --git a/atomicops.h b/atomicops.h
index ffd215a..a94e13a 100644
--- a/atomicops.h
+++ b/atomicops.h
@@ -613,7 +613,9 @@ namespace moodycamel
 
 			bool try_wait() AE_NO_TSAN
 			{
-				return xSemaphoreTake(m_sema, 0) == pdTRUE;
+				return ((xPortIsInsideInterrupt())
+				  ? xSemaphoreTakeFromISR(m_sema, nullptr)
+				  : xSemaphoreTake(m_sema, 0)) == pdTRUE;
 			}
 
 			bool timed_wait(std::uint64_t usecs) AE_NO_TSAN
@@ -625,7 +627,9 @@ namespace moodycamel
 
 			void signal() AE_NO_TSAN
 			{
-				BaseType_t rc = xSemaphoreGive(m_sema);
+				BaseType_t rc = (xPortIsInsideInterrupt())
+				  ? xSemaphoreGiveFromISR(m_sema, nullptr)
+				  : xSemaphoreGive(m_sema);
 				assert(rc == pdTRUE);
 				AE_UNUSED(rc);
 			}

[cameron314]

On further reflection, it doesn't make sense to block in an ISR context. Looking at the FreeRTOS source, it seems to simply return failure in this case when asked to wait on a semaphore.

It seems that the ISR versions might be safe to call from a non-ISR context, but the major difference is that any unblocked task is not immediately switched to -- this would have to be done by the caller, but again, in a real ISR context it's not desirable for the queue to randomly switch out of the interrupt handler.

[salyzyn]

I am satisfied with the second approach (check xPortIsInsideInterrupt) because for non-ISR context code arrangements are made to switch to the woke tasks as you state. Whereas for ISR routines the LightWeightSemaphore is too abstract to communicate the wake variable to request at the end of the ISR to also trigger an immediate thread switch to the woke task, so it is what it is. I am not sure how one could even modify the LightWeightSemaphore or readerwriterqueue to communicate the request to wake the thread instead of waiting for Idle or a timer tick should there be another active thread, this would be very application-specific since the wake has to be at the end of the ISR routine that is stuffing the readwriterqueue. Am I right?

I am personally and currently satisfied with the performance in my application because I am not running multiple threads, just the one executive loop, so I see no woke thread issues as the transition is quick from IDLE after the interrupt, the RTOS is working for me!

[salyzyn]

Issue can not be closed until the above change that adds 'BaseType_t rc = (xPortIsInsideInterrupt()) ? xSemaphoreGiveFromISR(m_sema, nullptr) : xSemaphoreGive(m_sema);' is added as noted above. I have been testing it extensively from ISR and non-ISR context and am satisfied with the performance and reliability.

[cameron314]

I'll have a look at this again in a few days. Thanks for your patience.

[salyzyn]

Confirmed works like a charm, THANKS!