From 6f01896d0443d3cb4029fc4b8c7167bd66d2c3b4 Mon Sep 17 00:00:00 2001 From: Will Miles Date: Sun, 4 Aug 2024 14:02:05 -0400 Subject: [PATCH 1/3] requestJSONBufferLock: Fix locking implementation On ESP8266, it isn't permissible to call delay() in system context; ensure this is legal before waiting. On ESP32, use an operating system mutex to ensure consistent variable state in a multicore environment, and manage the wait without needing to loop. --- wled00/util.cpp | 27 +++++++++++++++++++++++---- wled00/wled.h | 1 + 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/wled00/util.cpp b/wled00/util.cpp index 6bc02234b..d2302a085 100644 --- a/wled00/util.cpp +++ b/wled00/util.cpp @@ -213,15 +213,31 @@ bool requestJSONBufferLock(uint8_t module) DEBUG_PRINTLN(F("ERROR: JSON buffer not allocated!")); return false; } - unsigned long now = millis(); - - while (jsonBufferLock && millis()-now < 250) delay(1); // wait for fraction for buffer lock +#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 +#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. + if (can_yield()) { + unsigned long now = millis(); + while (jsonBufferLock && (millis()-now < 250)) delay(1); // wait for fraction for buffer lock + } +#else + #error Unsupported task framework - fix requestJSONBufferLock +#endif + // If the lock is still held - by us, or by another task if (jsonBufferLock) { DEBUG_PRINT(F("ERROR: Locking JSON buffer failed! (still locked by ")); DEBUG_PRINT(jsonBufferLock); DEBUG_PRINTLN(")"); - return false; // waiting time-outed +#ifdef ARDUINO_ARCH_ESP32 + xSemaphoreGiveRecursive(jsonBufferLockMutex); +#endif + return false; } jsonBufferLock = module ? module : 255; @@ -239,6 +255,9 @@ void releaseJSONBufferLock() DEBUG_PRINT(jsonBufferLock); DEBUG_PRINTLN(")"); jsonBufferLock = 0; +#ifdef ARDUINO_ARCH_ESP32 + xSemaphoreGiveRecursive(jsonBufferLockMutex); +#endif } diff --git a/wled00/wled.h b/wled00/wled.h index f761b970d..d6915e7fb 100644 --- a/wled00/wled.h +++ b/wled00/wled.h @@ -810,6 +810,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()); #else WLED_GLOBAL StaticJsonDocument gDoc; WLED_GLOBAL JsonDocument *pDoc _INIT(&gDoc); From e701b5b5ebddb62e565b74ea2c45da7bc8ef8cc2 Mon Sep 17 00:00:00 2001 From: Will Miles Date: Sun, 4 Aug 2024 14:02:05 -0400 Subject: [PATCH 2/3] util: Print locking module when JSON lock fails For debugging, also log who was trying to lock when it was contended. --- wled00/util.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/wled00/util.cpp b/wled00/util.cpp index d2302a085..5d4428be4 100644 --- a/wled00/util.cpp +++ b/wled00/util.cpp @@ -231,7 +231,9 @@ bool requestJSONBufferLock(uint8_t module) #endif // If the lock is still held - by us, or by another task if (jsonBufferLock) { - DEBUG_PRINT(F("ERROR: Locking JSON buffer failed! (still locked by ")); + DEBUG_PRINT(F("ERROR: Locking JSON buffer (")); + DEBUG_PRINT(module); + DEBUG_PRINT(F(") failed! (still locked by ")); DEBUG_PRINT(jsonBufferLock); DEBUG_PRINTLN(")"); #ifdef ARDUINO_ARCH_ESP32 From 113dbbdf94f9f5b7bc0d007bb4cd7fe7af9b5c2d Mon Sep 17 00:00:00 2001 From: Will Miles Date: Sun, 4 Aug 2024 15:08:46 -0400 Subject: [PATCH 3/3] Use DEBUG_PRINTF_P for jsonBufferLock Tiny code space usage reduction. --- wled00/util.cpp | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/wled00/util.cpp b/wled00/util.cpp index 5d4428be4..86ba5b486 100644 --- a/wled00/util.cpp +++ b/wled00/util.cpp @@ -231,11 +231,7 @@ bool requestJSONBufferLock(uint8_t module) #endif // If the lock is still held - by us, or by another task if (jsonBufferLock) { - DEBUG_PRINT(F("ERROR: Locking JSON buffer (")); - DEBUG_PRINT(module); - DEBUG_PRINT(F(") failed! (still locked by ")); - DEBUG_PRINT(jsonBufferLock); - DEBUG_PRINTLN(")"); + DEBUG_PRINTF_P(PSTR("ERROR: Locking JSON buffer (%d) failed! (still locked by %d)\n"), module, jsonBufferLock); #ifdef ARDUINO_ARCH_ESP32 xSemaphoreGiveRecursive(jsonBufferLockMutex); #endif @@ -243,9 +239,7 @@ bool requestJSONBufferLock(uint8_t module) } jsonBufferLock = module ? module : 255; - DEBUG_PRINT(F("JSON buffer locked. (")); - DEBUG_PRINT(jsonBufferLock); - DEBUG_PRINTLN(")"); + DEBUG_PRINTF_P(PSTR("JSON buffer locked. (%d)\n"), jsonBufferLock); pDoc->clear(); return true; } @@ -253,9 +247,7 @@ bool requestJSONBufferLock(uint8_t module) void releaseJSONBufferLock() { - DEBUG_PRINT(F("JSON buffer released. (")); - DEBUG_PRINT(jsonBufferLock); - DEBUG_PRINTLN(")"); + DEBUG_PRINTF_P(PSTR("JSON buffer released. (%d)\n"), jsonBufferLock); jsonBufferLock = 0; #ifdef ARDUINO_ARCH_ESP32 xSemaphoreGiveRecursive(jsonBufferLockMutex);