v0.7.0 preparations #76
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -199,8 +199,11 @@ class StatefulService | |||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| // 🌙 adding semaphore wait too long logging | ||||||||||||||||||||||||||||||||||
| if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) { | ||||||||||||||||||||||||||||||||||
| ESP_LOGI("🐼", "_accessMutex wait 100ms"); | ||||||||||||||||||||||||||||||||||
| if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) { | ||||||||||||||||||||||||||||||||||
| ESP_LOGW("🐼", "_accessMutex waited 500ms and continues 🤷♂️"); | ||||||||||||||||||||||||||||||||||
| // xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
201
to
207
There was a problem hiding this comment. Critical: Mutex acquisition failure leads to unprotected access and semaphore corruption. If both
Consider one of these solutions: Option 1 (Recommended): Track acquisition success and conditionally release inline void beginTransaction()
{
+ _mutexAcquired = false;
// 🌙 adding semaphore wait too long logging
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) {
ESP_LOGI("🐼", "_accessMutex wait 100ms");
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
- ESP_LOGW("🐼", "_accessMutex waited 500ms and continues 🤷♂️");
- // xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
+ ESP_LOGW("🐼", "_accessMutex waited 500ms, blocking indefinitely");
+ xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
}
}
+ _mutexAcquired = true;
}
inline void endTransaction()
{
- xSemaphoreGiveRecursive(_accessMutex);
+ if (_mutexAcquired) {
+ xSemaphoreGiveRecursive(_accessMutex);
+ _mutexAcquired = false;
+ }
}(Add Option 2: Restore indefinite blocking if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
- ESP_LOGW("🐼", "_accessMutex waited 500ms and continues 🤷♂️");
- // xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
+ ESP_LOGW("🐼", "_accessMutex waited 500ms, blocking indefinitely");
+ xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. I agree this is a risk 2 processes mutating the same, however due to recursiveness, the same process can try to take the mutex twice, at the moment it is too much overhead to track if the same process has acquired it already, but you may suggest an easy fix for that. There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: Length of output: 4107 🌐 Web query: 💡 Result: Short answer: if you call xSemaphoreGiveRecursive() on a recursive mutex that the calling task does not hold, the call will fail (return pdFALSE) — it does not silently decrement an internal count or hand ownership to the caller. FreeRTOS documents that xSemaphoreGiveRecursive returns pdTRUE when the semaphore is successfully given and pdFALSE on error (for example when the semaphore was not previously obtained), and community reports describe xSemaphoreGiveRecursive returning FALSE when the mutex is not owned by the caller. [1][2] Sources:
@ewowi, I need to clarify a few points about FreeRTOS recursive mutexes: About Recursive TrackingGood news: There's no overhead! FreeRTOS recursive mutexes (
This is built into FreeRTOS - you don't need to implement any tracking yourself. The recursive mutex exists specifically to handle same-task re-entry without deadlock. About xSemaphoreGiveRecursive FailureYour assumption is incorrect: The Real ProblemThe critical issue isn't recursion tracking - it's that if both Easy FixThe simplest solution is to restore the indefinite blocking you commented out: if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) {
ESP_LOGI("🐼", "_accessMutex wait 100ms");
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
ESP_LOGW("🐼", "_accessMutex waited 500ms, blocking indefinitely");
xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); // ← uncomment this
}
}This ensures the mutex is always acquired before accessing 🧬 Code Graph Analysis Results[StatefulService.h:38-44] Constructor and recursive mutex initialization
[StatefulService.h:100-115] beginTransaction
[StatefulService.h:116-123] endTransaction
[StatefulService.h:125-133] private members
Collaborator
Author
There was a problem hiding this comment. Okay, correction: the thing is not that the same process tries to acquire it, I think the effect task processing a state change, hands over to the SvelteKit task to recursively call begin transaction, apparently before the effect task has freed it. Or something like this What I can tell you that occasionally infinite locks happen. So can you check the code to find the root cause for this locking between multiple processes? And provide a solution. Maybe some tasks need to explicitly give the mutex if they are done with whatever they are doing ? There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: Length of output: 13480 🏁 Script executed: Length of output: 850 🏁 Script executed: Length of output: 3687 🏁 Script executed: Length of output: 4064 🏁 Script executed: Length of output: 4292 🏁 Script executed: Length of output: 532 🏁 Script executed: Length of output: 4972 🏁 Script executed: Length of output: 3465 🏁 Script executed: Length of output: 1190 @ewowi, I've found the root cause of your deadlock issue! It's a classic circular dependency deadlock between multiple tasks, not a recursive lock issue. Root Cause AnalysisYour system has multiple tasks that can update modules concurrently:
The deadlock occurs because update handlers immediately acquire mutexes of other modules: Example Deadlock ChainFrom _fileManager->addUpdateHandler([this](const String& originId) {
// Handler runs in context of whatever task called FileManager.update()
_fileManager->read([&](FilesState& filesState) { // ← Re-acquires FileManager mutex!
_moduleEffects->read([&](ModuleState& effectsState) { // ← Also acquires Effects mutex!Deadlock scenario:
Similar chains exist for ModuleIO → ModuleDrivers/ModuleLightsControl. Why Recursive Mutex Doesn't HelpRecursive mutexes only allow the same task to re-enter. Different tasks (effectTask vs SvelteKit loop) cannot share ownership, so the recursive property provides no protection against cross-task deadlocks. The Current Code ProblemYour commented-out
SolutionsImmediate Fix: Restore Blocking (Required)This prevents unprotected access and semaphore corruption: if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) {
ESP_LOGI("🐼", "_accessMutex wait 100ms");
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
ESP_LOGW("🐼", "_accessMutex waited 500ms, blocking indefinitely");
xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); // ← REQUIRED
}
}However, this will cause permanent hangs on deadlock instead of unprotected access. You need the architectural fix below. Architectural Fix: Defer Cross-Module CallsThe proper solution is to never call Option 1: Task Notification Pattern (Recommended) Add a "dirty flag" mechanism to modules that need cross-module data: class NodeManager {
private:
volatile bool _fileManagerDirty = false;
public:
void begin() {
// Set flag instead of immediate read
_fileManager->addUpdateHandler([this](const String& originId) {
_fileManagerDirty = true; // Atomic flag set
});
}
void loop() {
// Check and process in your own task context
if (_fileManagerDirty) {
_fileManagerDirty = false;
_fileManager->read([&](FilesState& filesState) {
// Safe: runs in NodeManager's task, not handler context
for (auto updatedItem : filesState.updatedItems) {
// ... process updates
}
});
}
// ... rest of loop
}
};Option 2: Pass Data in Handler (Alternative) Modify handlers to receive necessary data as parameters: // In StatefulService.h, create a variant that passes state copy
typedef std::function<void(const String &originId, const T& stateCopy)> StateUpdateCallbackWithData;
void callUpdateHandlers(const String &originId) {
T stateCopy = _state; // Copy outside mutex
for (const StateUpdateHandlerInfo_t &updateHandler : _updateHandlers) {
updateHandler._cb(originId, stateCopy); // Pass copy
}
}Then handlers don't need to call _fileManager->addUpdateHandler([this](const String& originId, const FilesState& filesState) {
// No read() call needed - data passed in
for (auto updatedItem : filesState.updatedItems) {
// ... process
}
});Specific Code Changes NeededLooking at your codebase, these handlers need refactoring:
All of these should either:
Would you like me to generate a detailed refactoring for one of these as an example? 🧬 Code Graph Analysis Results<filename=lib/framework/StatefulService.h> <filename=lib/framework/StatefulService.h> <filename=lib/framework/StatefulService.h> <filename=lib/framework/StatefulService.h> <no_relevant_code_snippets>
Collaborator
Author
There was a problem hiding this comment. @coderabbitai,regarding
Yes for all 5 files please There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: Length of output: 8344 🏁 Script executed: Length of output: 497 🏁 Script executed: Length of output: 956 @ewowi, here are the detailed refactoring examples for all 5 files. The pattern is consistent: set flags in handlers, check flags in loop(). 1. NodeManager.hAdd private member variable: private:
volatile bool _fileManagerDirty = false;BEFORE (lines 41-61): _fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s", originId.c_str());
// read the file state (read all files and folders on FS and collect changes)
_fileManager->read([&](FilesState& filesState) {
// loop over all changed files (normally only one)
for (auto updatedItem : filesState.updatedItems) {
// if file is the current live script, recompile it (to do: multiple live effects)
EXT_LOGV(ML_TAG, "updateHandler updatedItem %s", updatedItem.c_str());
Char<32> name;
name.format("/.config/%s.json", _moduleName);
if (equal(updatedItem.c_str(), name.c_str())) {
EXT_LOGV(ML_TAG, " %s updated -> call update %s", name.c_str(), updatedItem.c_str());
readFromFS(); // repopulates the state, processing file changes
}
}
});
});AFTER: _fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s (deferred)", originId.c_str());
_fileManagerDirty = true; // Set flag instead of immediate read
});Add loop() method: void loop() override {
Module::loop(); // Call parent if NodeManager inherits from Module
// Process deferred FileManager updates
if (_fileManagerDirty) {
_fileManagerDirty = false;
_fileManager->read([&](FilesState& filesState) {
for (auto updatedItem : filesState.updatedItems) {
EXT_LOGV(ML_TAG, "Processing updatedItem %s", updatedItem.c_str());
Char<32> name;
name.format("/.config/%s.json", _moduleName);
if (equal(updatedItem.c_str(), name.c_str())) {
EXT_LOGV(ML_TAG, " %s updated -> call readFromFS %s", name.c_str(), updatedItem.c_str());
readFromFS(); // Safe: runs in NodeManager's task context
}
}
});
}
}2. ModuleLiveScripts.hAdd private member variables: private:
volatile bool _fileManagerDirty = false;BEFORE (lines 40-65): _fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s", originId.c_str());
// read the file state (read all files and folders on FS and collect changes)
_fileManager->read([&](FilesState& filesState) {
// loop over all changed files (normally only one)
for (auto updatedItem : filesState.updatedItems) {
// if file is the current live script, recompile it (to do: multiple live effects)
uint8_t index = 0;
_moduleEffects->read([&](ModuleState& effectsState) {
for (JsonObject nodeState : effectsState.data["nodes"].as<JsonArray>()) {
if (updatedItem == nodeState["name"]) {
EXT_LOGD(ML_TAG, "updateHandler equals current item -> livescript compile %s", updatedItem.c_str());
LiveScriptNode* liveScriptNode = (LiveScriptNode*)_moduleEffects->findLiveScriptNode(nodeState["name"]);
if (liveScriptNode) {
liveScriptNode->compileAndRun();
_moduleEffects->requestUIUpdate = true; // update the Effects UI
}
EXT_LOGD(ML_TAG, "update due to new node %s done", nodeState["name"].as<const char*>());
}
index++;
}
});
}
});
});AFTER: _fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s (deferred)", originId.c_str());
_fileManagerDirty = true; // Set flag instead of nested reads
});Add loop() method: void loop() override {
Module::loop();
// Process deferred FileManager updates
if (_fileManagerDirty) {
_fileManagerDirty = false;
// Safe: both reads execute in ModuleLiveScripts task context
_fileManager->read([&](FilesState& filesState) {
for (auto updatedItem : filesState.updatedItems) {
uint8_t index = 0;
_moduleEffects->read([&](ModuleState& effectsState) {
for (JsonObject nodeState : effectsState.data["nodes"].as<JsonArray>()) {
if (updatedItem == nodeState["name"]) {
EXT_LOGD(ML_TAG, "Compiling livescript %s", updatedItem.c_str());
LiveScriptNode* liveScriptNode = (LiveScriptNode*)_moduleEffects->findLiveScriptNode(nodeState["name"]);
if (liveScriptNode) {
liveScriptNode->compileAndRun();
_moduleEffects->requestUIUpdate = true;
}
EXT_LOGD(ML_TAG, "Update for node %s done", nodeState["name"].as<const char*>());
}
index++;
}
});
}
});
}
}3. ModuleLightsControl.h - FileManager HandlerAdd private member variables: private:
volatile bool _fileManagerDirty = false;
volatile bool _moduleIODirty = false;BEFORE (lines 48-67): _fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s", originId.c_str());
// read the file state (read all files and folders on FS and collect changes)
_fileManager->read([&](FilesState& filesState) {
// loop over all changed files (normally only one)
bool presetChanged = false;
for (auto updatedItem : filesState.updatedItems) {
// if file is the current live script, recompile it (to do: multiple live effects)
EXT_LOGV(ML_TAG, "updateHandler updatedItem %s", updatedItem.c_str());
if (strstr(updatedItem.c_str(), "/.config/presets")) {
EXT_LOGV(ML_TAG, " preset.json updated -> call update %s", updatedItem.c_str());
presetChanged = true;
}
}
if (presetChanged) {
EXT_LOGV(ML_TAG, "setPresetsFromFolder");
setPresetsFromFolder(); // update the presets from the folder
}
});
});AFTER: _fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s (deferred)", originId.c_str());
_fileManagerDirty = true; // Set flag instead of immediate read
});4. ModuleLightsControl.h - ModuleIO HandlerBEFORE (line 68): moduleIO.addUpdateHandler([this](const String& originId) { readPins(); }, false);
readPins(); // initiallyAFTER: moduleIO.addUpdateHandler([this](const String& originId) {
_moduleIODirty = true; // Set flag instead of immediate readPins()
}, false);
readPins(); // initially - still safe as it's in setup contextBEFORE readPins() (lines 72-88): void readPins() {
moduleIO.read([&](ModuleState& state) {
pinRelayLightsOn = UINT8_MAX;
pinButtonLightsOn = UINT8_MAX;
for (JsonObject pinObject : state.data["pins"].as<JsonArray>()) {
uint8_t usage = pinObject["usage"];
uint8_t gpio = pinObject["GPIO"];
if (usage == pin_Relay_LightsOn) {
if (GPIO_IS_VALID_OUTPUT_GPIO(gpio)) {
pinRelayLightsOn = gpio;
pinMode(pinRelayLightsOn, OUTPUT);
uint8_t newBri = _state.data["lightsOn"] ? _state.data["brightness"] : 0;
digitalWrite(pinRelayLightsOn, newBri > 0 ? HIGH : LOW);
EXT_LOGD(ML_TAG, "pinRelayLightsOn found %d", pinRelayLightsOn);
} else
EXT_LOGE(MB_TAG, "gpio %d not valid", pinRelayLightsOn);
}
// ... rest of pin processing
}
});
}AFTER readPins() (no changes needed - method stays the same): // readPins() stays unchanged - just call it from loop() instead of handler
void readPins() {
moduleIO.read([&](ModuleState& state) {
// ... same implementation ...
});
}Modify existing loop() (line 271): void loop() override {
Module::loop();
// Process deferred FileManager updates
if (_fileManagerDirty) {
_fileManagerDirty = false;
_fileManager->read([&](FilesState& filesState) {
bool presetChanged = false;
for (auto updatedItem : filesState.updatedItems) {
EXT_LOGV(ML_TAG, "Processing updatedItem %s", updatedItem.c_str());
if (strstr(updatedItem.c_str(), "/.config/presets")) {
EXT_LOGV(ML_TAG, "preset.json updated");
presetChanged = true;
}
}
if (presetChanged) {
EXT_LOGV(ML_TAG, "setPresetsFromFolder");
setPresetsFromFolder();
}
});
}
// Process deferred ModuleIO updates
if (_moduleIODirty) {
_moduleIODirty = false;
readPins(); // Safe: runs in ModuleLightsControl task context
}
// ... existing preset loop code ...
uint8_t presetLoop = _state.data["presetLoop"];
if (presetLoop && millis() - lastPresetTime > presetLoop * 1000) {
lastPresetTime = millis();
// ... rest of existing loop code ...
}
}5. ModuleDrivers.hAdd private member variable: private:
volatile bool _moduleIODirty = false;
bool initPins = false; // Existing variableBEFORE (line 76): _moduleIO->addUpdateHandler([this](const String& originId) { readPins(); }, false);AFTER: _moduleIO->addUpdateHandler([this](const String& originId) {
_moduleIODirty = true; // Set flag instead of immediate readPins()
}, false);Modify existing loop() (lines 178-188): BEFORE: void loop() override {
NodeManager::loop();
if (!initPins) {
readPins(); // initially
initPins = true;
}
}AFTER: void loop() override {
NodeManager::loop();
// Process deferred ModuleIO updates
if (_moduleIODirty) {
_moduleIODirty = false;
readPins(); // Safe: runs in ModuleDrivers task context
}
// Initial pin read (keeps existing behavior)
if (!initPins) {
readPins();
initPins = true;
}
}Summary of ChangesFor each file, the refactoring follows this pattern:
This completely eliminates the circular dependency deadlock because:
The Would you like me to also show how to implement this with a task notification pattern instead of polling flags, which can be more efficient? 🧬 Code Graph Analysis Resultslib/framework/MqttSettingsService.h, lines around 109-120 lib/framework/MqttSettingsService.h, lines around 96-107 lib/framework/WiFiSettingsService.h, lines around 90-224 lib/framework/APSettingsService.h, lines around 115-144 lib/framework/APSettingsService.h, lines around 102-113 |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix list indentation per markdown standards.
Line 99 uses 4-space indentation instead of the standard 2-space indentation for nested list items.
📝 Committable suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
99-99: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents