Skip to content

Super sync part 1 #114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ewowi wants to merge 7 commits into
base: main
Choose a base branch
from
+11,841 −11,688
Open

Super sync part 1 #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1990708
Module Devices sends UDP message when lightcontrol changes
ewowi Jan 24, 2026
ced9a76
Increase udp responsetime (20ms) and process all messages
ewowi Jan 24, 2026
a967c12
Send updates to devices from devices module
ewowi Jan 25, 2026
d7998a5
Devices Control messages (UDP, not httpclient)
ewowi Jan 25, 2026
2fdf976
Devices: onUpdate send fix, preset action fix
ewowi Jan 26, 2026
4e689d7
Add originId argument to update functions
ewowi Jan 26, 2026
386a340
Devices tweaks
ewowi Jan 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions .coderabbit.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
language: "en-US"

review:
enable_review: true
enable_fix_suggestions: true
enable_tests_generation: false

context:
timeout_ms: 900000 # 15 minutes max
early_access: true
reviews:
profile: "chill"
request_changes_workflow: true
high_level_summary: true
poem: true
review_status: true
collapse_walkthrough: false
auto_review:
enabled: true
drafts: true
tools:
github-checks:
timeout_ms: 900000 # 15 minutes max
chat:
auto_reply: true
2 changes: 1 addition & 1 deletion interface/src/routes/menu.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@
title: 'Devices',
icon: CPU,
href: '/moonbase/module?group=moonbase&module=devices',
feature: page.data.features.moonbase
feature: page.data.features.moonlight // In MoonLight for the time being, should move to MoonBase using moduleControlCenter ...
},
{
title: 'Tasks',
Expand Down
4 changes: 2 additions & 2 deletions lib/framework/StatefulService.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,9 @@ class StatefulService
{
// 🌙 adding semaphore wait too long logging
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) {
ESP_LOGI("🐼", "_accessMutex %s wait 100ms", originId.c_str());
// ESP_LOGI("🐼", "_accessMutex %s wait 100ms", originId.c_str());
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
ESP_LOGW("🐼", "_accessMutex %s waited 500ms and continues", originId.c_str());
ESP_LOGW("🐼", "%s _accessMutex %s waited 500ms and continues", pcTaskGetName(xTaskGetCurrentTaskHandle()), originId.c_str());
// xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
Comment on lines 201 to 205
Copy link

@coderabbitai coderabbitai bot Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Block or abort on mutex timeout — continuing without the lock is unsafe.

If both waits fail, the function proceeds without owning _accessMutex, so callers mutate _state concurrently and endTransaction() gives an unowned mutex. This matches the corruption issues observed in this repo. Please keep the warning (no auto‑restart), but do ensure the lock is acquired or fail the transaction.

✅ Safer fix (block after warning) 
-            if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
-                ESP_LOGW("🐼", "%s _accessMutex %s waited 500ms and continues", pcTaskGetName(xTaskGetCurrentTaskHandle()), originId.c_str());
-                // xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
-            }
+            if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
+                ESP_LOGW("🐼", "%s _accessMutex %s waited 500ms; blocking until acquired", pcTaskGetName(xTaskGetCurrentTaskHandle()), originId.c_str());
+                xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
+            }

Based on learnings, the warning should remain but execution must not proceed without the mutex.

🤖 Prompt for AI Agents 
In `@lib/framework/StatefulService.h` around lines 201 - 205, The current code
warns when xSemaphoreTakeRecursive(_accessMutex,...) times out twice but then
continues without owning _accessMutex, risking concurrent mutation of _state and
an unowned mutex in endTransaction(); update the failure path so after logging
the ESP_LOGW (keeping the warning) you either block until the mutex is acquired
by calling xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY) or explicitly
abort the transaction by returning an error/setting a failure state and ensuring
callers of the surrounding function (and endTransaction()) handle this abort;
refer to _accessMutex, xSemaphoreTakeRecursive, endTransaction(), _state,
originId, pcTaskGetName and xTaskGetCurrentTaskHandle() to locate and modify the
code.

}
}
Expand Down
23,116 changes: 11,558 additions & 11,558 deletions lib/framework/WWWData.h
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion platformio.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ build_flags =
-D BUILD_TARGET=\"$PIOENV\"
-D APP_NAME=\"MoonLight\" ; 🌙 Must only contain characters from [a-zA-Z0-9-_] as this is converted into a filename
-D APP_VERSION=\"0.8.1\" ; semver compatible version string
-D APP_DATE=\"20260122\" ; 🌙
-D APP_DATE=\"20260126\" ; 🌙

-D PLATFORM_VERSION=\"pioarduino-55.03.35\" ; 🌙 make sure it matches with above plaftform

Expand Down
30 changes: 14 additions & 16 deletions src/MoonBase/Module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ void setDefaults(JsonObject controls, JsonArray definition) {

// shared static variables
SemaphoreHandle_t ModuleState::updateMutex = nullptr;
UpdatedItem ModuleState::updatedItem;
Char<20> ModuleState::updateOriginId;
UpdatedItem ModuleState::mutexedUpdatedItem;
String ModuleState::mutexedOriginId;

void ModuleState::setupData() {
// only if no file ...
Expand All @@ -57,7 +57,7 @@ void ModuleState::setupData() {
// assign the new defaults to state and run onUpdate
data.clear(); //->to<JsonObject>(); //clear data
UpdatedItem updatedItem;
compareRecursive("", data, doc.as<JsonObject>(), updatedItem); // fill data with doc and calls onUpdates
compareRecursive("", data, doc.as<JsonObject>(), updatedItem, "module"); // fill data with doc and calls onUpdates
}

// to do: check if the file matches the definition
Expand All @@ -69,7 +69,7 @@ void ModuleState::read(ModuleState& state, JsonObject& stateJson) {
stateJson.set(state.data); // copy
}

bool ModuleState::checkReOrderSwap(const JsonString& parent, const JsonVariant& stateData, const JsonVariant& newData, UpdatedItem& updatedItem, uint8_t depth, uint8_t index) {
bool ModuleState::checkReOrderSwap(const JsonString& parent, const JsonVariant& stateData, const JsonVariant& newData, UpdatedItem& updatedItem, const String& originId, uint8_t depth, uint8_t index) {
bool changed = false;
// check if newData is a reordering of state
// if so reorder state, no comparison with updates needed
Expand Down Expand Up @@ -115,7 +115,7 @@ bool ModuleState::checkReOrderSwap(const JsonString& parent, const JsonVariant&
updatedItem.name = "swap";
updatedItem.index[0] = stateIndex;
updatedItem.index[1] = newIndex;
postUpdate(updatedItem);
postUpdate(updatedItem, originId);
}

if (parkedFromIndex == UINT8_MAX) parkedFromIndex = newIndex; // the index of value in the array stored in the parking spot
Expand All @@ -129,7 +129,7 @@ bool ModuleState::checkReOrderSwap(const JsonString& parent, const JsonVariant&
return changed;
}

bool ModuleState::compareRecursive(const JsonString& parent, const JsonVariant& stateData, const JsonVariant& newData, UpdatedItem& updatedItem, uint8_t depth, uint8_t index) {
bool ModuleState::compareRecursive(const JsonString& parent, const JsonVariant& stateData, const JsonVariant& newData, UpdatedItem& updatedItem, const String& originId, uint8_t depth, uint8_t index) {
bool changed = false;
for (JsonPair newControl : newData.as<JsonObject>()) {
if (stateData[newControl.key()].isNull()) {
Expand Down Expand Up @@ -175,7 +175,7 @@ bool ModuleState::compareRecursive(const JsonString& parent, const JsonVariant&
// String xxx;
// serializeJson(newArray[i], xxx);
// EXT_LOGD(ML_TAG, "before cr %s", xxx.c_str());
changed = compareRecursive(key, stateArray[i], newArray[i], updatedItem, depth + 1, i) || changed;
changed = compareRecursive(key, stateArray[i], newArray[i], updatedItem, originId, depth + 1, i) || changed;
} else if (i >= newArray.size()) { // newArray has deleted a row
// newArray.add<JsonObject>(); //add dummy row
changed = true; // compareRecursive(key, stateArray[i], newArray[i], updatedItem, depth+1, i) || changed;
Expand Down Expand Up @@ -208,7 +208,7 @@ bool ModuleState::compareRecursive(const JsonString& parent, const JsonVariant&
updatedItem.value = JsonVariant(); // Assign an empty JsonVariant
stateArray[i].remove(control.key()); // remove the control from the state row so onUpdate see it as empty

postUpdate(updatedItem);
postUpdate(updatedItem, originId);
}

// String dbg;
Expand Down Expand Up @@ -246,7 +246,7 @@ bool ModuleState::compareRecursive(const JsonString& parent, const JsonVariant&
// String xxx;
// serializeJson(newArray[i], xxx);
// EXT_LOGD(ML_TAG, "before cr %s", xxx.c_str());
changed = compareRecursive(key, stateArray[i], newArray[i], updatedItem, depth + 1, i) || changed;
changed = compareRecursive(key, stateArray[i], newArray[i], updatedItem, originId, depth + 1, i) || changed;
}
}
} else { // if control is key/value
Expand All @@ -260,7 +260,7 @@ bool ModuleState::compareRecursive(const JsonString& parent, const JsonVariant&
// EXT_LOGD(MB_TAG, "kv %s.%s v: %s d: %d", parent.c_str(), key.c_str(), newValue.as<String>().c_str(), depth);
// EXT_LOGD(MB_TAG, "kv %s[%d]%s[%d].%s = %s -> %s", updatedItem.parent[0].c_str(), updatedItem.index[0], updatedItem.parent[1].c_str(), updatedItem.index[1], updatedItem.name.c_str(), updatedItem.oldValue.c_str(), updatedItem.value.as<String>().c_str());

postUpdate(updatedItem);
postUpdate(updatedItem, originId);
}
// else {
// EXT_LOGD(MB_TAG, "do not update %s", key.c_str());
Expand All @@ -274,8 +274,6 @@ bool ModuleState::compareRecursive(const JsonString& parent, const JsonVariant&
StateUpdateResult ModuleState::update(JsonObject& newData, ModuleState& state, const String& originId) {
// if (state.data.isNull()) EXT_LOGD(ML_TAG, "state data is null %d %d", newData.size(), newData != state.data); // state.data never null here

updateOriginId = originId;

if (newData.size() != 0) { // in case of empty file

// check which controls have updated
Expand All @@ -284,7 +282,7 @@ StateUpdateResult ModuleState::update(JsonObject& newData, ModuleState& state, c

// bool isNew = state.data.isNull(); // device is starting , not useful as state.data never null here

bool changed = state.checkReOrderSwap("", state.data, newData, updatedItem);
bool changed = state.checkReOrderSwap("", state.data, newData, updatedItem, originId);

// if (originId != "devicesserver" && originId != "tasksserver") {
// String ss;
Expand All @@ -296,7 +294,7 @@ StateUpdateResult ModuleState::update(JsonObject& newData, ModuleState& state, c
// serializeJson(state.data, Serial);Serial.println();
// serializeJson(newData, Serial);Serial.println();

if (state.compareRecursive("", state.data, newData, updatedItem)) {
if (state.compareRecursive("", state.data, newData, updatedItem, originId)) {
if (changed) EXT_LOGW(ML_TAG, "checkReOrderSwap changed, compareRecursive also changed? %s", originId.c_str());
changed = true;
}
Expand All @@ -319,8 +317,8 @@ Module::Module(const char* moduleName, PsychicHttpServer* server, ESP32SvelteKit
EXT_LOGV(MB_TAG, "constructor %s", moduleName);
_server = server;

_state.processUpdatedItem = [&](const UpdatedItem& updatedItem) {
processUpdatedItem(updatedItem); // Ensure updatedItem is of type UpdatedItem&
_state.processUpdatedItem = [&](const UpdatedItem& updatedItem, const String& originId) {
processUpdatedItem(updatedItem, originId); // Ensure updatedItem is of type UpdatedItem&
};
}

Expand Down
42 changes: 22 additions & 20 deletions src/MoonBase/Module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ class ModuleState {
public:
JsonObject data = JsonObject(); // isNull()

static UpdatedItem updatedItem;
// for post/getUpdate
static UpdatedItem mutexedUpdatedItem;
static String mutexedOriginId;
static SemaphoreHandle_t updateMutex;
bool updatePending = false; // should not be static as each module needs to keep track of it's own status

static Char<20> updateOriginId; // static, written by ModuleState::update, no mutex needed as written by one process at a time (http mostly, sveltekit sometimes recursively)
bool mutexedUpdatePending = false; // should not be static as each module needs to keep track of it's own status
Comment on lines +47 to +51
Copy link

@coderabbitai coderabbitai bot Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the Module.h file around lines 47-51 to see the actual code
head -60 src/MoonBase/Module.h | tail -20

Repository: MoonModules/MoonLight

Length of output: 745

🏁 Script executed:

# Get more context from Module.h - check the class definition and static members
sed -n '40,160p' src/MoonBase/Module.h

Repository: MoonModules/MoonLight

Length of output: 4742

🏁 Script executed:

# Check Module.cpp for static member definitions
fd -e cpp -x grep -l "ModuleState::" {} \; | head -5

Repository: MoonModules/MoonLight

Length of output: 90

🏁 Script executed:

# Search for ModuleState instantiations to understand usage patterns
rg "ModuleState" --type cpp --type h -C3 | head -100

Repository: MoonModules/MoonLight

Length of output: 5174

🏁 Script executed:

# Search for Module/ModuleState instantiation to understand usage patterns
rg "new Module|Module(" --type cpp --type h -B2 -A2 | head -60

Repository: MoonModules/MoonLight

Length of output: 151

🏁 Script executed:

# Check for any guard or synchronization that might prevent multiple concurrent instances
rg "ModuleState" --type cpp -A5 -B5 | grep -E "(static|singleton|instance)" | head -20

Repository: MoonModules/MoonLight

Length of output: 668

🏁 Script executed:

# Look at the template usage - how many StatefulService<ModuleState> instances are created
rg "StatefulService<ModuleState>" --type cpp --type h -C3

Repository: MoonModules/MoonLight

Length of output: 352

🏁 Script executed:

# Search for Module instantiation in main and other files
rg "Module\(" --type cpp -B2 -A2 | head -80

Repository: MoonModules/MoonLight

Length of output: 5646

🏁 Script executed:

# Count the number of module types/instances being created
fd -e cpp | xargs grep -l "Module(" | head -10

Repository: MoonModules/MoonLight

Length of output: 101

🏁 Script executed:

# Look at main.cpp to see how many modules are instantiated
grep -n "Module" src/main.cpp | head -30

Repository: MoonModules/MoonLight

Length of output: 2360

🏁 Script executed:

# Check the Module constructor to understand instantiation
sed -n '1,50p' src/MoonBase/Module.cpp

Repository: MoonModules/MoonLight

Length of output: 2036

Race condition confirmed: mixed static/non-static queue variables with multiple Module instances.

With 9+ different Module instances in the system (ModuleLightsControl, ModuleDevices, ModuleEffects, ModuleDrivers, ModuleLiveScripts, ModuleChannels, etc.), the current design creates a race condition:

  • mutexedUpdatedItem and mutexedOriginId are static (shared across all modules)
  • mutexedUpdatePending is non-static (per-instance)

When Module A calls postUpdate(), it sets the shared static queue and its own instance flag. When Module B calls postUpdate() before Module A's getUpdate() runs, it overwrites the static queue data. Module A's getUpdate() then reads Module B's data from the corrupted queue.

Either make all three static (single shared queue) or all three non-static (per-module queues).

🔧 Option 1: Make mutexedUpdatePending static for a single shared queue 
-  bool mutexedUpdatePending = false;  // should not be static as each module needs to keep track of it's own status
+  static bool mutexedUpdatePending;

And in Module.cpp:

bool ModuleState::mutexedUpdatePending = false;
🔧 Option 2: Make all three non-static for per-module queues 
-  static UpdatedItem mutexedUpdatedItem;
-  static String mutexedOriginId;
+  UpdatedItem mutexedUpdatedItem;
+  String mutexedOriginId;

Remove the corresponding static definitions from Module.cpp.

🤖 Prompt for AI Agents 
In `@src/MoonBase/Module.h` around lines 47 - 51, The static vs non-static mix
causes cross-instance races; make the queue per-module by converting
mutexedUpdatedItem, mutexedOriginId, and updateMutex to non-static instance
members (so they match the existing non-static mutexedUpdatePending), remove
their static definitions from Module.cpp, and initialize/create the updateMutex
in the ModuleState constructor and free it in the destructor; also update
postUpdate() and getUpdate() usages to refer to the instance members
(mutexedUpdatedItem, mutexedOriginId, updateMutex, mutexedUpdatePending) so each
Module instance has its own queue and semaphore.


ModuleState() {
EXT_LOGD(MB_TAG, "ModuleState constructor");
Expand Down Expand Up @@ -97,31 +97,32 @@ class ModuleState {
void setupData();

// called from ModuleState::update and ModuleState::setupData()
bool compareRecursive(const JsonString& parent, const JsonVariant& oldData, const JsonVariant& newData, UpdatedItem& updatedItem, uint8_t depth = UINT8_MAX, uint8_t index = UINT8_MAX);
bool compareRecursive(const JsonString& parent, const JsonVariant& oldData, const JsonVariant& newData, UpdatedItem& updatedItem, const String& originId, uint8_t depth = UINT8_MAX, uint8_t index = UINT8_MAX);

// called from ModuleState::update
bool checkReOrderSwap(const JsonString& parent, const JsonVariant& oldData, const JsonVariant& newData, UpdatedItem& updatedItem, uint8_t depth = UINT8_MAX, uint8_t index = UINT8_MAX);
bool checkReOrderSwap(const JsonString& parent, const JsonVariant& oldData, const JsonVariant& newData, UpdatedItem& updatedItem, const String& originId, uint8_t depth = UINT8_MAX, uint8_t index = UINT8_MAX);

std::function<void(const UpdatedItem&)> processUpdatedItem = nullptr;
std::function<void(const UpdatedItem&, const String&)> processUpdatedItem = nullptr;

static void read(ModuleState& state, JsonObject& stateJson);
static StateUpdateResult update(JsonObject& newData, ModuleState& state, const String& originId); //, const String& originId

ReadHook readHook = nullptr; // called when the UI requests the state, can be used to update the state before sending it to the UI

void postUpdate(const UpdatedItem& updatedItem) {
void postUpdate(const UpdatedItem& updatedItem, const String& originId) {
const char* taskName = pcTaskGetName(xTaskGetCurrentTaskHandle());

if (contains(taskName, "SvelteKit") || contains(taskName, "loopTask")) { // at boot, the loopTask starts, after that the loopTask is destroyed
if (processUpdatedItem) processUpdatedItem(updatedItem);
if (processUpdatedItem) processUpdatedItem(updatedItem, originId);
} else {
// Wait until previous update is processed
while (true) {
if (xSemaphoreTake(updateMutex, pdMS_TO_TICKS(10)) == pdTRUE) {
if (!updatePending) {
if (!mutexedUpdatePending) {
// EXT_LOGD(ML_TAG, "%s[%d]%s[%d].%s = %s -> %s", updatedItem.parent[0].c_str(), updatedItem.index[0], updatedItem.parent[1].c_str(), updatedItem.index[1], updatedItem.name.c_str(), updatedItem.oldValue.c_str(), updatedItem.value.as<String>().c_str());
this->updatedItem = updatedItem;
updatePending = true;
mutexedUpdatedItem = updatedItem;
mutexedOriginId = originId;
mutexedUpdatePending = true;
xSemaphoreGive(updateMutex);
break;
}
Expand All @@ -135,14 +136,15 @@ class ModuleState {
void getUpdate() {
// Try to acquire mutex without blocking
if (xSemaphoreTake(updateMutex, pdMS_TO_TICKS(10)) == pdTRUE) {
if (updatePending) {
if (mutexedUpdatePending) {
// Copy update data
UpdatedItem localCopy = updatedItem;
updatePending = false;
UpdatedItem localCopy = mutexedUpdatedItem;
const String localOriginId = mutexedOriginId;
mutexedUpdatePending = false;
xSemaphoreGive(updateMutex);

// Process OUTSIDE the mutex (no lock held during callback)
if (processUpdatedItem) processUpdatedItem(localCopy);
if (processUpdatedItem) processUpdatedItem(localCopy, localOriginId);
return;
}
xSemaphoreGive(updateMutex);
Expand Down Expand Up @@ -179,17 +181,17 @@ class Module : public StatefulService<ModuleState> {
}
}

void processUpdatedItem(const UpdatedItem& updatedItem) {
void processUpdatedItem(const UpdatedItem& updatedItem, const String& originId) {
if (updatedItem.name == "swap") {
onReOrderSwap(updatedItem.index[0], updatedItem.index[1]);
saveNeeded = true;
} else {
if (updatedItem.oldValue != "" && updatedItem.name != "channel") { // todo: fix the problem at channel, not here...
if (!_state.updateOriginId.contains("server")) { // only triggered by updates from front-end
if (!originId.endsWith("server")) { // only triggered by updates from front-end
saveNeeded = true;
}
}
onUpdate(updatedItem);
onUpdate(updatedItem, originId);
}
}

Expand All @@ -205,7 +207,7 @@ class Module : public StatefulService<ModuleState> {

// called in compareRecursive->execOnUpdate
// called from compareRecursive
virtual void onUpdate(const UpdatedItem& updatedItem) {};
virtual void onUpdate(const UpdatedItem& updatedItem, const String& originId) {};
virtual void onReOrderSwap(uint8_t stateIndex, uint8_t newIndex) {};

protected:
Expand Down
Loading