From b54c153fc3a547a546a444c0936a2a5f80f59743 Mon Sep 17 00:00:00 2001 From: Will Miles Date: Sun, 3 Aug 2025 13:43:13 -0400 Subject: [PATCH] POC implementation for deferred settings Add a basic function queue to the main loop, and have settings-set operations defer operation to it. Rejigger the JSON lock as a binary semaphrore, as it needs to pass ownership from one task to another. First step towards a global fix for #4779. --- wled00/fcn_declare.h | 2 +- wled00/handler_queue.cpp | 49 +++++++++++++++++++++++++++ wled00/handler_queue.h | 13 ++++++++ wled00/json.cpp | 2 +- wled00/set.cpp | 2 +- wled00/util.cpp | 16 ++++----- wled00/wled.cpp | 5 +++ wled00/wled.h | 2 +- wled00/wled_server.cpp | 72 ++++++++++++++++++++++------------------ wled00/ws.cpp | 43 +++++++++++++----------- 10 files changed, 141 insertions(+), 65 deletions(-) create mode 100644 wled00/handler_queue.cpp create mode 100644 wled00/handler_queue.h diff --git a/wled00/fcn_declare.h b/wled00/fcn_declare.h index d19f89b27d..44413cdf8e 100644 --- a/wled00/fcn_declare.h +++ b/wled00/fcn_declare.h @@ -508,7 +508,7 @@ size_t printSetFormIndex(Print& settingsScript, const char* key, int index); size_t printSetClassElementHTML(Print& settingsScript, const char* key, const int index, const char* val); void prepareHostname(char* hostname); [[gnu::pure]] bool isAsterisksOnly(const char* str, byte maxLen); -bool requestJSONBufferLock(uint8_t moduleID=255); +bool requestJSONBufferLock(uint8_t moduleID=255, bool wait = true); void releaseJSONBufferLock(); uint8_t extractModeName(uint8_t mode, const char *src, char *dest, uint8_t maxLen); uint8_t extractModeSlider(uint8_t mode, uint8_t slider, char *dest, uint8_t maxLen, uint8_t *var = nullptr); diff --git a/wled00/handler_queue.cpp b/wled00/handler_queue.cpp new file mode 100644 index 0000000000..2d50b803b1 --- /dev/null +++ b/wled00/handler_queue.cpp @@ -0,0 +1,49 @@ +#include "handler_queue.h" +#include +#include + +#if defined(ARDUINO_ARCH_ESP32) + +static StaticSemaphore_t handlerQueueMutexBuffer; +static SemaphoreHandle_t handlerQueueMutex = xSemaphoreCreateMutexStatic(&handlerQueueMutexBuffer); +struct guard_type { + SemaphoreHandle_t _mtx; + guard_type(SemaphoreHandle_t m) : _mtx(m) { + xSemaphoreTake(_mtx, portMAX_DELAY); // todo: error check + } + ~guard_type() { + xSemaphoreGive(_mtx); + } +}; +#define guard() const guard_type guard(handlerQueueMutex) +#else +#define guard() +#endif + +static std::deque> handler_queue; + +// Enqueue a function on the main task +bool HandlerQueue::callOnMainTask(std::function func) { + guard(); + handler_queue.push_back(std::move(func)); + return true; // TODO: queue limit +} + +// Run the next task in the queue, if any +void HandlerQueue::runNext() { + std::function f; + { + guard(); + if (handler_queue.size()) { + f = std::move(handler_queue.front()); + handler_queue.pop_front(); + } + } + if (f) { + auto t1 = millis(); + f(); + auto t2 = millis(); + Serial.printf("Handler took: %d\n", t2-t1); + } +} + diff --git a/wled00/handler_queue.h b/wled00/handler_queue.h new file mode 100644 index 0000000000..3667452065 --- /dev/null +++ b/wled00/handler_queue.h @@ -0,0 +1,13 @@ +// handler_queue.h +// deferred execution handler queue. Used for making access to WLED globals safe. + +#include + +namespace HandlerQueue { + + // Enqueue a function on the main task + bool callOnMainTask(std::function func); + + // Run the next task in the queue, if any + void runNext(); +} diff --git a/wled00/json.cpp b/wled00/json.cpp index 4414681023..3035eda302 100644 --- a/wled00/json.cpp +++ b/wled00/json.cpp @@ -1123,7 +1123,7 @@ void serveJson(AsyncWebServerRequest* request) return; } - if (!requestJSONBufferLock(17)) { + if (!requestJSONBufferLock(17, false)) { request->deferResponse(); return; } diff --git a/wled00/set.cpp b/wled00/set.cpp index 0ae6fe898d..29bfed101a 100644 --- a/wled00/set.cpp +++ b/wled00/set.cpp @@ -652,7 +652,7 @@ void handleSettingsSet(AsyncWebServerRequest *request, byte subPage) //USERMODS if (subPage == SUBPAGE_UM) { - if (!requestJSONBufferLock(5)) { + if (!requestJSONBufferLock(5, false)) { request->deferResponse(); return; } diff --git a/wled00/util.cpp b/wled00/util.cpp index 6ff7b05dfc..a249ec0b96 100644 --- a/wled00/util.cpp +++ b/wled00/util.cpp @@ -151,7 +151,7 @@ bool isAsterisksOnly(const char* str, byte maxLen) //threading/network callback details: https://github.com/wled-dev/WLED/pull/2336#discussion_r762276994 -bool requestJSONBufferLock(uint8_t moduleID) +bool requestJSONBufferLock(uint8_t moduleID, bool wait) { if (pDoc == nullptr) { DEBUG_PRINTLN(F("ERROR: JSON buffer not allocated!")); @@ -159,10 +159,11 @@ bool requestJSONBufferLock(uint8_t moduleID) } #if defined(ARDUINO_ARCH_ESP32) - // Use a recursive mutex type in case our task is the one holding the JSON buffer. - // This can happen during large JSON web transactions. In this case, we continue immediately - // and then will return out below if the lock is still held. - if (xSemaphoreTakeRecursive(jsonBufferLockMutex, 250) == pdFALSE) return false; // timed out waiting + if (xSemaphoreTake(jsonBufferLockMutex, wait ? 250 : 0) == pdFALSE) { + // TODO, coaelesce with below + DEBUG_PRINTF_P(PSTR("ERROR: Locking JSON buffer mutex (%d) failed! (locked by %d??)\n"), moduleID, jsonBufferLock); + return false; // timed out waiting + } #elif defined(ARDUINO_ARCH_ESP8266) // If we're in system context, delay() won't return control to the user context, so there's // no point in waiting. @@ -176,9 +177,6 @@ bool requestJSONBufferLock(uint8_t moduleID) // If the lock is still held - by us, or by another task if (jsonBufferLock) { DEBUG_PRINTF_P(PSTR("ERROR: Locking JSON buffer (%d) failed! (still locked by %d)\n"), moduleID, jsonBufferLock); -#ifdef ARDUINO_ARCH_ESP32 - xSemaphoreGiveRecursive(jsonBufferLockMutex); -#endif return false; } @@ -194,7 +192,7 @@ void releaseJSONBufferLock() DEBUG_PRINTF_P(PSTR("JSON buffer released. (%d)\n"), jsonBufferLock); jsonBufferLock = 0; #ifdef ARDUINO_ARCH_ESP32 - xSemaphoreGiveRecursive(jsonBufferLockMutex); + xSemaphoreGive(jsonBufferLockMutex); #endif } diff --git a/wled00/wled.cpp b/wled00/wled.cpp index 5826b9ac38..6cccb8e251 100644 --- a/wled00/wled.cpp +++ b/wled00/wled.cpp @@ -1,6 +1,7 @@ #define WLED_DEFINE_GLOBAL_VARS //only in one source file, wled.cpp! #include "wled.h" #include "wled_ethernet.h" +#include "handler_queue.h" #ifdef WLED_ENABLE_AOTA #define NO_OTA_PORT #include @@ -184,6 +185,9 @@ void WLED::loop() heapTime = millis(); } + // Run next deferred request + HandlerQueue::runNext(); + //LED settings have been saved, re-init busses //This code block causes severe FPS drop on ESP32 with the original "if (busConfigs[0] != nullptr)" conditional. Investigate! if (doInitBusses) { @@ -320,6 +324,7 @@ void WLED::setup() #ifdef ARDUINO_ARCH_ESP32 pinMode(hardwareRX, INPUT_PULLDOWN); delay(1); // suppress noise in case RX pin is floating (at low noise energy) - see issue #3128 + xSemaphoreGive(jsonBufferLockMutex); // Init JSON buffer lock #endif #ifdef WLED_BOOTUPDELAY delay(WLED_BOOTUPDELAY); // delay to let voltage stabilize, helps with boot issues on some setups diff --git a/wled00/wled.h b/wled00/wled.h index 3772fcd1f6..cda81c3c2d 100644 --- a/wled00/wled.h +++ b/wled00/wled.h @@ -970,7 +970,7 @@ WLED_GLOBAL int8_t spi_sclk _INIT(SPISCLKPIN); // global ArduinoJson buffer #if defined(ARDUINO_ARCH_ESP32) WLED_GLOBAL JsonDocument *pDoc _INIT(nullptr); -WLED_GLOBAL SemaphoreHandle_t jsonBufferLockMutex _INIT(xSemaphoreCreateRecursiveMutex()); +WLED_GLOBAL SemaphoreHandle_t jsonBufferLockMutex _INIT(xSemaphoreCreateBinary()); #else WLED_GLOBAL StaticJsonDocument gDoc; WLED_GLOBAL JsonDocument *pDoc _INIT(&gDoc); diff --git a/wled00/wled_server.cpp b/wled00/wled_server.cpp index f12b00ab27..6d888c68bf 100644 --- a/wled00/wled_server.cpp +++ b/wled00/wled_server.cpp @@ -1,4 +1,5 @@ #include "wled.h" +#include "handler_queue.h" #ifndef WLED_DISABLE_OTA #ifdef ESP8266 @@ -299,7 +300,7 @@ void initServer() }); server.on(F("/settings"), HTTP_POST, [](AsyncWebServerRequest *request){ - serveSettings(request, true); + HandlerQueue::callOnMainTask([=]{ serveSettings(request, true); }); }); const static char _json[] PROGMEM = "/json"; @@ -308,10 +309,7 @@ void initServer() }); AsyncCallbackJsonWebHandler* handler = new AsyncCallbackJsonWebHandler(FPSTR(_json), [](AsyncWebServerRequest *request) { - bool verboseResponse = false; - bool isConfig = false; - - if (!requestJSONBufferLock(14)) { + if (!requestJSONBufferLock(14, false)) { request->deferResponse(); return; } @@ -325,37 +323,42 @@ void initServer() } if (root.containsKey("pin")) checkSettingsPIN(root["pin"].as()); - const String& url = request->url(); - isConfig = url.indexOf(F("cfg")) > -1; - if (!isConfig) { - /* - #ifdef WLED_DEBUG - DEBUG_PRINTLN(F("Serialized HTTP")); - serializeJson(root,Serial); - DEBUG_PRINTLN(); - #endif - */ - verboseResponse = deserializeState(root); - } else { - if (!correctPIN && strlen(settingsPIN)>0) { - releaseJSONBufferLock(); - serveJsonError(request, 401, ERR_DENIED); - return; - } - verboseResponse = deserializeConfig(root); //use verboseResponse to determine whether cfg change should be saved immediately - } - releaseJSONBufferLock(); + HandlerQueue::callOnMainTask([=](){ + bool verboseResponse = false; + bool isConfig = false; - if (verboseResponse) { + const String& url = request->url(); + isConfig = url.indexOf(F("cfg")) > -1; if (!isConfig) { - lastInterfaceUpdate = millis(); // prevent WS update until cooldown - interfaceUpdateCallMode = CALL_MODE_WS_SEND; // schedule WS update - serveJson(request); return; //if JSON contains "v" + /* + #ifdef WLED_DEBUG + DEBUG_PRINTLN(F("Serialized HTTP")); + serializeJson(root,Serial); + DEBUG_PRINTLN(); + #endif + */ + verboseResponse = deserializeState(root); } else { - configNeedsWrite = true; //Save new settings to FS + if (!correctPIN && strlen(settingsPIN)>0) { + releaseJSONBufferLock(); + serveJsonError(request, 401, ERR_DENIED); + return; + } + verboseResponse = deserializeConfig(root); //use verboseResponse to determine whether cfg change should be saved immediately } - } - request->send(200, CONTENT_TYPE_JSON, F("{\"success\":true}")); + releaseJSONBufferLock(); + + if (verboseResponse) { + if (!isConfig) { + lastInterfaceUpdate = millis(); // prevent WS update until cooldown + interfaceUpdateCallMode = CALL_MODE_WS_SEND; // schedule WS update + serveJson(request); return; //if JSON contains "v" + } else { + configNeedsWrite = true; //Save new settings to FS + } + } + request->send(200, CONTENT_TYPE_JSON, F("{\"success\":true}")); + }); }, JSON_BUFFER_SIZE); server.addHandler(handler); @@ -509,7 +512,10 @@ void initServer() return; } - if(handleSet(request, request->url())) return; + if (request->url().indexOf("win") >= 0) { + HandlerQueue::callOnMainTask([=]() { handleSet(request, request->url()); }); + return; + } #ifndef WLED_DISABLE_ALEXA if(espalexa.handleAlexaApiCall(request)) return; #endif diff --git a/wled00/ws.cpp b/wled00/ws.cpp index 45640b68ce..cb1212c50e 100644 --- a/wled00/ws.cpp +++ b/wled00/ws.cpp @@ -1,4 +1,5 @@ #include "wled.h" +#include "handler_queue.h" /* * WebSockets server for bidirectional communication @@ -34,8 +35,7 @@ void wsEvent(AsyncWebSocket * server, AsyncWebSocketClient * client, AwsEventTyp client->text(F("pong")); return; } - - bool verboseResponse = false; + if (!requestJSONBufferLock(11)) { client->text(F("{\"error\":3}")); // ERR_NOBUF return; @@ -47,26 +47,31 @@ void wsEvent(AsyncWebSocket * server, AsyncWebSocketClient * client, AwsEventTyp releaseJSONBufferLock(); return; } - if (root["v"] && root.size() == 1) { - //if the received value is just "{"v":true}", send only to this client - verboseResponse = true; - } else if (root.containsKey("lv")) { - wsLiveClientId = root["lv"] ? client->id() : 0; - } else { - verboseResponse = deserializeState(root); - } - releaseJSONBufferLock(); - if (!interfaceUpdateCallMode) { // individual client response only needed if no WS broadcast soon - if (verboseResponse) { - sendDataWs(client); + // Handle the rest on the main task + HandlerQueue::callOnMainTask([=](){ + bool verboseResponse = false; + if (root["v"] && root.size() == 1) { + //if the received value is just "{"v":true}", send only to this client + verboseResponse = true; + } else if (root.containsKey("lv")) { + wsLiveClientId = root["lv"] ? client->id() : 0; } else { - // we have to send something back otherwise WS connection closes - client->text(F("{\"success\":true}")); + verboseResponse = deserializeState(root); } - // force broadcast in 500ms after updating client - //lastInterfaceUpdate = millis() - (INTERFACE_UPDATE_COOLDOWN -500); // ESP8266 does not like this - } + releaseJSONBufferLock(); + + if (!interfaceUpdateCallMode) { // individual client response only needed if no WS broadcast soon + if (verboseResponse) { + sendDataWs(client); + } else { + // we have to send something back otherwise WS connection closes + client->text(F("{\"success\":true}")); + } + // force broadcast in 500ms after updating client + //lastInterfaceUpdate = millis() - (INTERFACE_UPDATE_COOLDOWN -500); // ESP8266 does not like this + } + }); } } else { //message is comprised of multiple frames or the frame is split into multiple packets