Added multithreading support for ESP32s #3310

Phando · 2023-08-09T01:32:48Z

Added optional multithreading support to core WLED classes. WLED, UsermodManager and Usermod. The actual multithreading is done in custom usermods. Any usermod with a backgroundLoop, will be executed on the background thread.

softhack007

Hi, thanks for this interesting proposal.
Below please find some first comments - I'll need some more time to go through everything...

Please also study how multi-tasking is managed in four-line-display(ALT) and in the audioreactive usermod.

softhack007 · 2023-08-09T08:00:09Z

usermods/usermod_v2_background/usermod_v2_background.h

+    void doBackgroundWork(){
+      backgroundIndex++;
+      Serial.println("B: " + String(backgroundIndex));
+    }


Please generally(!) avoid fastled EVERY_N_MILLIS as it does a very bad job .... for example, if the N_MILLIS time is missed by a single microsecond, it does nothing.

You might be able to utilize vTaskDelayUntil(). There are examples in the fourLineDisplay_ALT or audioreactive usermods.

I thought EVERY_N... was clean, I will move to vTaskDelayUntil, thank you for that.

The benefit of every_n_... is that it does not block as it avoids calling delay().
But you can easily substitute it with a few lines like this:

const unsigned long task1_period = 40; // to run every 40 millisecond static unsigned long task1_last_run = 0; // "static" needed so the value is preserved if ((millis() - task1_last_run) >= task1_period) { // activate after at least 40 millis have passed task1_last_run = millis(); // remember last runtime task1_do_work(); // do work } // do something else, or just wait

In contrast to 8266 or other small arduino platforms, on esp32 millis() and delay() do work as expected even when you are outside of the main looptask (i.e. in a seperate task). They are still "forbidden" for interrupt service routines (ISR).
vTaskDelayUntil() will suspend your main task until a specific time, its for "nothing to do" situations.

softhack007 · 2023-08-09T08:01:44Z

usermods/usermod_v2_background/usermod_v2_background.h

+    }
+
+    void loop() {
+      EVERY_N_MILLISECONDS(1000) { doMainWork(); }


See my previous comment

softhack007 · 2023-08-09T08:07:13Z

platformio.ini

@@ -11,7 +11,7 @@

 # CI binaries
 ; default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, esp32dev, esp32_eth # ESP32 variant builds are temporarily excluded from CI due to toolchain issues on the GitHub Actions Linux environment
-default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, esp32dev, esp32_eth, lolin_s2_mini, esp32c3dev, esp32s3dev_8MB, esp32s3dev_8MB_PSRAM_opi
+; default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, esp32dev, esp32_eth, lolin_s2_mini, esp32c3dev, esp32s3dev_8MB, esp32s3dev_8MB_PSRAM_opi


Please revert this

I am happy to revert this part. I usually use platform_overrides. Is that the preferred method for adding new functionality?

blazoncek · 2023-08-09T08:13:01Z

I am not a RTOS expert or have deep knowledge about task scheduling on ESP but these crossed my mind nevertheless:

why are you inventing new API for task loops? why not just schedule each usermod's loop as a separate task instead?
did it cross your mind that not all libraries used in usermods are thread safe?
setting 10k for task stack is quite liberal and IMO a lot
you should not modify platformio.ini

I wonder what @softhack007 will say as he is better at low level stuff.

softhack007 · 2023-08-09T08:16:20Z

did it cross your mind that not all libraries used in usermods are thread safe?

I fully support this point. Not all libraries are thread-safe, and the typical usermod is not thread-safe either.

softhack007 · 2023-08-09T11:10:59Z

wled00/wled.cpp

@@ -285,6 +285,31 @@ DEBUG_PRINTLN(F("Watchdog: disabled"));
 #endif
 }

+#if defined(WLED_ENABLE_BACKGROUND)


Can you move these two functions (background main loop, and creation of the new task) into your main usermod?
I think its not strictly needed that this is added into the WLED core. You can achieve the same thing in the "setup" function of your main UsermodBackground::Setup(). Similar to what usermods/audioreactive does.

WLED/usermods/audioreactive/audio_reactive.h

Line 1173 in 45d7e66

if (enabled) onUpdateBegin(false); // create FFT task

WLED/usermods/audioreactive/audio_reactive.h

Line 1356 in 45d7e66

void onUpdateBegin(bool init)

WLED/usermods/audioreactive/audio_reactive.h

Lines 1379 to 1405 in 45d7e66

if (init && FFT_Task) {

vTaskSuspend(FFT_Task); // update is about to begin, disable task to prevent crash

if (udpSyncConnected) { // close UDP sync connection (if open)

udpSyncConnected = false;

fftUdp.stop();

}

} else {

// update has failed or create task requested

if (FFT_Task) {

vTaskResume(FFT_Task);

connected(); // resume UDP

} else

// xTaskCreatePinnedToCore(

xTaskCreate( // no need to "pin" this task to core #0

FFTcode, // Function to implement the task

"FFT", // Name of the task

5000, // Stack size in words

NULL, // Task input parameter

1, // Priority of the task

&FFT_Task // Task handle

// , 0 // Core where the task should run

);

}

micDataReal = 0.0f; // just to be sure

if (enabled) disableSoundProcessing = false;

}

I will look at audio reactive for sure. I am not trying to reinvent the wheel, but make writing usermods easier. By adding backgroundLoop to wled.cpp and the other core files, all devs need to do is implement the backgroundLoop function. Also this strategy can support multiple usermods all with main and background loops. That said, it is never my goal to reinvent the wheel, just make the usermod experience easier for new devs.

The stack size is large, but settable via the BACKGROUND_STACK_SIZE setting.

softhack007 · 2023-08-09T11:32:03Z

wled00/wled.cpp

+  for (;;)
+  {
+    // instance->usermods.backgroundLoop();
+    usermods.backgroundLoop();


Sorry, but this way of "running" stuff in the background is very problematic.

Together with the activation conditions (every_n_milliseconds) in each of your background usermods, this basically implements a busy-wait-loop that will compete for 100% CPU cycles with all other tasks on core#0 (e.g. audioreactive, system tasks (like wifi or tcp), and ISRs).

Your code would even compile on -C3 and -S2 (that only have core #0), but it will cause instabilities and may even heat up cheaper boards (=fire hazard).

Normally in FreeRTOS a task only uses the CPU when it has something to do, and the task goes into "waiting" state (vTaskDelay, waiting on semaphores or queues, etc) when nothing is scheduled. This allows other tasks at the same priority to activate in time to meat their task deadlines.

To improve, I would suggest that each backgroundLoop() should return the number of millis until it wants to be activated again, similar to return FRAMETIME; used in effects (FX.cpp). This would allow your scheduler to identify "nothing to do" situations, and free the CPU by vTaskDelay() if no background loop is due for running.

Think this is going to change how I write multithreaded apps for the foreseeable future. Thank you

Phando · 2023-08-09T15:57:58Z

Thank you for all the comments. I was not aware that the four line display and audioreactive implemented multithreading. Your comments are all insightful and grounded in reality. There are some bits in there I wish I knew years ago. I'll go back to the drawing board and see if there is need for this at all.

softhack007 · 2023-08-09T16:11:09Z

don't give up .... its still an interesting feature :-) Maybe just the final details will be different from your first design.
Could still be something for 0.15.0, as there are plans already to evolve/improve the usermods framework into similar direction.

cheers 🥇

Phando · 2023-08-09T16:14:49Z

@softhack007 I am not giving up, I was stoked for the feedback. Given all the comments, I need to go back and rework my solution and try again.

blazoncek · 2023-08-09T17:45:49Z

I would suggest a complete re-think of how usermods are called.
Your approach is in the right direction IMO except perhaps a bit too simple ATM.

I think modifying add() function and adding a few semaphores would do the trick.

jandolina added 2 commits August 8, 2023 18:27

Sync up with fork

88d0be6

Commenting out the ttgo display

7aebb78

softhack007 self-requested a review August 9, 2023 07:51

softhack007 requested changes Aug 9, 2023

View reviewed changes

softhack007 reviewed Aug 9, 2023

View reviewed changes

Phando closed this Aug 9, 2023

Phando deleted the multithread branch August 24, 2023 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added multithreading support for ESP32s #3310

Added multithreading support for ESP32s #3310

Phando commented Aug 9, 2023

softhack007 left a comment •

edited

Loading

softhack007 Aug 9, 2023 •

edited

Loading

Phando Aug 9, 2023

softhack007 Aug 9, 2023 •

edited

Loading

softhack007 Aug 9, 2023

softhack007 Aug 9, 2023

Phando Aug 9, 2023

blazoncek commented Aug 9, 2023

softhack007 commented Aug 9, 2023

softhack007 Aug 9, 2023 •

edited

Loading

Phando Aug 9, 2023

softhack007 Aug 9, 2023 •

edited

Loading

softhack007 Aug 9, 2023 •

edited

Loading

Phando Aug 9, 2023

Phando commented Aug 9, 2023

softhack007 commented Aug 9, 2023 •

edited

Loading

Phando commented Aug 9, 2023

blazoncek commented Aug 9, 2023

	if (init && FFT_Task) {
	vTaskSuspend(FFT_Task); // update is about to begin, disable task to prevent crash
	if (udpSyncConnected) { // close UDP sync connection (if open)
	udpSyncConnected = false;
	fftUdp.stop();
	}
	} else {
	// update has failed or create task requested
	if (FFT_Task) {
	vTaskResume(FFT_Task);
	connected(); // resume UDP
	} else
	// xTaskCreatePinnedToCore(
	xTaskCreate( // no need to "pin" this task to core #0
	FFTcode, // Function to implement the task
	"FFT", // Name of the task
	5000, // Stack size in words
	NULL, // Task input parameter
	1, // Priority of the task
	&FFT_Task // Task handle
	// , 0 // Core where the task should run
	);
	}
	micDataReal = 0.0f; // just to be sure
	if (enabled) disableSoundProcessing = false;
	}

Added multithreading support for ESP32s #3310

Added multithreading support for ESP32s #3310

Conversation

Phando commented Aug 9, 2023

softhack007 left a comment • edited Loading

Choose a reason for hiding this comment

softhack007 Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Phando Aug 9, 2023

Choose a reason for hiding this comment

softhack007 Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

softhack007 Aug 9, 2023

Choose a reason for hiding this comment

softhack007 Aug 9, 2023

Choose a reason for hiding this comment

Phando Aug 9, 2023

Choose a reason for hiding this comment

blazoncek commented Aug 9, 2023

softhack007 commented Aug 9, 2023

softhack007 Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Phando Aug 9, 2023

Choose a reason for hiding this comment

softhack007 Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

softhack007 Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Phando Aug 9, 2023

Choose a reason for hiding this comment

Phando commented Aug 9, 2023

softhack007 commented Aug 9, 2023 • edited Loading

Phando commented Aug 9, 2023

blazoncek commented Aug 9, 2023

softhack007 left a comment •

edited

Loading

softhack007 Aug 9, 2023 •

edited

Loading

softhack007 Aug 9, 2023 •

edited

Loading

softhack007 Aug 9, 2023 •

edited

Loading

softhack007 Aug 9, 2023 •

edited

Loading

softhack007 Aug 9, 2023 •

edited

Loading

softhack007 commented Aug 9, 2023 •

edited

Loading