LED tubes #123
LED tubes #123
Conversation
Firmware ======== - add esp32-s3-seeed_xiao Backend ======= - IO: configure board preset LuxceoMood1XiaoMod - Effects: all use palette - WIP Backend ======= - All effects use palettes (WIP)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ESP32‑S3 Seeed Xiao support (PlatformIO env, installer manifests, docs), centralizes per‑module persistence into a new SharedFSPersistence wired into startup and module registration, trims palette emojis in docs/effect tags, and shifts many effects from CHSV to ColorFromPalette color sourcing. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main (setup)
participant Shared as SharedFSPersistence
participant Module as Module
participant FS as Filesystem
rect rgba(100,150,240,0.5)
Main->>Shared: allocate SharedFSPersistence\nregisterModule(module, delayed=true)
Shared-->>Main: ack registration
end
rect rgba(120,200,100,0.5)
Main->>Shared: sharedFsPersistence->begin()
Shared->>Module: readFromFS(moduleName) for each registered module
Module->>Shared: writeToFSDelayed(moduleName, data) (on updates)
Shared->>FS: perform eventual writeToFS(moduleName)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
can you review 48e0062 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 32420
🐛 Critical Issue: Image File Extension MismatchThe manifest files reference the wrong image extension: Files: "image": "./images/esp32-s3-seeed_xiao.jpg"But the actual file added is "image": "./images/esp32-s3-seeed_xiao.png"📋 Board Support AdditionThe esp32-s3-seeed_xiao board configuration looks good:
The LuxceoMood1XiaoMod pin assignments look reasonable (pins 1-4 for LEDs, standard I2C/SPI/Serial pins), but please verify these match your hardware schematic. 🎨 Palette MigrationThe conversion from
📝 Documentation UpdatesThe documentation changes are consistent - removing 🎨 emoji references and updating the palette description makes sense since all effects now use palettes by default. ✅ SummaryMust fix: Image extension mismatch in manifest files Overall solid work on both the board addition and palette standardization! 👍 🧠 Learnings used |
pio.ini: httpd stack size 6144 for board presets updates backend ======= - Module: processUpdatedItem: saveNeeded also if oldValue is empty
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai resume |
|
🧠 Learnings used✅ Actions performedReviews resumed. Review triggered.
|
backend ======= - main: enable SharedFSPersistence - Module: remove _fsPersistence - NodeManager: comment _fileManager->read() readFromFS(); - WIP
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonBase/SharedFSPersistence.h (1)
21-21:⚠️ Potential issue | 🔴 Critical
sharedDelayedWriteshas an unprotected data race betweenwriteToFS()andwriteToFSDelayed().The global
inlinevector is accessed unsafely:writeToFS()may push to it from the SvelteKit task (via state update handlers), whilewriteToFSDelayed()iterates and clears it from the httpd task (HTTP POST handlers at/rest/saveConfigand/rest/cancelConfig). Add synchronization (mutex or queue) to protect concurrent access.
🧹 Nitpick comments (3)
src/MoonBase/NodeManager.h (1)
54-54: Track this TODO — disablingreadFromFS()here means file-manager edits to the config JSON won't propagate to runtime state.The comment says "temporary !!!" but there's no tracking issue or clear plan for re-enabling this path under the new
SharedFSPersistenceregime. IfSharedFSPersistenceis meant to handle this, consider wiring the FileManager update handler to callsharedFsPersistence->readFromFS(moduleName)instead, or open an issue so this doesn't get lost.Would you like me to open an issue to track re-enabling FS-triggered state reload under SharedFSPersistence?
src/MoonBase/Module.cpp (1)
280-280: Stale comment on line 287.Line 287 says
"if no data readFromFS"butreadFromFSno longer happens here — it's done externally bySharedFSPersistence::registerModule. Consider updating the comment to reflect the new flow.src/MoonBase/SharedFSPersistence.h (1)
52-52: Inconsistent logging betweenregisterModuleandenableUpdateHandlerhandlers.The update handler registered in
registerModule(line 52) does not logoriginId, while the one inenableUpdateHandler(lines 78-79) does. If a module's handler is never disabled/re-enabled, it will silently write without the log. Consider extracting a shared lambda or helper so both paths behave identically.Proposed fix
// Register update handler - info.updateHandlerId = module->addUpdateHandler([this, module](const String& originId) { writeToFS(module->_moduleName); }, false); + info.updateHandlerId = module->addUpdateHandler([this, module](const String& originId) { + ESP_LOGD(SVK_TAG, "writeToFS %s", originId.c_str()); + writeToFS(module->_moduleName); + }, false);Also applies to: 76-81
|
@coderabbitai, Latest commit gave me this crash during boot (bootloop): [ 898][D][NodeManager.h:126] onUpdate(): [💫] loopTask (4976) [255][255].layer = -> 0 Core 1 register dump: Backtrace: 0x4209da99:0x3fceb580 0x4203b3ee:0x3fceb670 0x4203b3fd:0x3fceb690 0x4203b58a:0x3fceb6b0 0x4203d960:0x3fceb6d0 0x4203db5e:0x3fceb940 0x4203e2d8:0x3fcebbb0 0x4217f46b:0x3fcebc70 0x42083667:0x3fcebc90 0x420a09de:0x3fcebcb0 0x420a0be6:0x3fcebda0 0x420a1c7d:0x3fcebe20 0x420ab54e:0x3fcebe80 0x4037fdbd:0x3fcebea0 ELF file SHA256: f90644e2c Rebooting... What is going on? I worked on this before but paused it, maybe you remember. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20688
Root Cause: Initialization Timing ProblemThe crash is a null pointer dereference of the Old FSPersistence Flow (Working)// Module constructor
Module::Module(...) : _fsPersistence(...) { }
// Later in Module::begin()
_fsPersistence.readFromFS(); // ← Called AFTER module is fully initializedNew SharedFSPersistence Flow (Broken)// In setup()
sharedFsPersistence->registerModule(module, true); // ← Called BEFORE module->begin()
└─→ readFromFS(moduleName); // Line 57: reads immediately!
└─→ updateWithoutPropagation()
└─→ NodeManager::onUpdate()
└─→ nodes->size() // 💥 CRASH: nodes is nullptr!
// Later...
module->begin(); // ← This initializes the nodes vector, but TOO LATEThe ProblemIn void registerModule(Module* module, bool delayedWriting = false) {
// ... setup ModuleInfo ...
_modules[module->_moduleName] = info;
// ❌ TOO EARLY! Module hasn't called begin() yet
readFromFS(module->_moduleName);
}But NodeManager initializes its The SolutionOption 1: Defer all readFromFS() calls (Recommended) void registerModule(Module* module, bool delayedWriting = false) {
ModuleInfo info;
info.module = module;
info.filePath = String("/.config/") + module->_moduleName + ".json";
info.delayedWriting = delayedWriting;
info.hasDelayedWrite = false;
info.updateHandlerId = module->addUpdateHandler(
[this, module](const String& originId) { writeToFS(module->_moduleName); },
false);
_modules[module->_moduleName] = info;
// ✅ DON'T call readFromFS here
}
void begin() {
// ✅ Read all modules AFTER they've all been initialized
for (auto& pair : _modules) {
readFromFS(pair.first);
}
}Option 2: Call readFromFS() explicitly after each module->begin() in main.cpp: for (Module* module : modules) {
module->begin();
}
// Now read persisted state
for (Module* module : modules) {
sharedFsPersistence->readFromFS(module->_moduleName);
}Functional Equivalence CheckOther than this timing issue, the implementations are functionally equivalent:
The only difference causing the crash is when 🏁 Script executed: Length of output: 1147 Perfect! The sequence confirms my diagnosis. Here's the exact fix: Fix for SharedFSPersistence.hRemove the void registerModule(Module* module, bool delayedWriting = false) {
ModuleInfo info;
info.module = module;
info.filePath = String("/.config/") + module->_moduleName + ".json";
info.delayedWriting = delayedWriting;
info.hasDelayedWrite = false;
// Register update handler
info.updateHandlerId = module->addUpdateHandler(
[this, module](const String& originId) { writeToFS(module->_moduleName); },
false);
_modules[module->_moduleName] = info;
// ❌ REMOVE THIS LINE - it's too early!
// readFromFS(module->_moduleName);
}
void begin() {
// All setup happens in registerModule
}
// ✅ ADD THIS NEW METHOD
void readAllFromFS() {
for (auto& pair : _modules) {
readFromFS(pair.first);
}
}Fix for main.cppCall // Begin shared routers (one-time setup)
sharedHttpEndpoint->begin();
sharedWebSocketServer->begin();
sharedEventEndpoint->begin();
sharedFsPersistence->begin();
// MoonBase
`#if` FT_ENABLED(FT_MOONBASE)
fileManager.begin();
for (Module* module : modules) {
module->begin(); // ← Modules initialize their internal state (nodes vector, etc.)
}
// ✅ ADD THIS: Now read persisted state after modules are ready
sharedFsPersistence->readAllFromFS();
// 🌙
`#if` FT_ENABLED(FT_MOONLIGHT)
xTaskCreatePinnedToCore(effectTask, ...Why This WorksThe new sequence becomes:
This matches the old FSPersistence behavior where 🧠 Learnings used |
|
@coderabbitai, cool that worked, but it looks like delayed writes is doing nothing: [ 27685][D][FSPersistence.h:121] writeToFSDelayed(): [🐼] calling 0 writeFuncs from delayedWrites Please check SharedFSPersistence.h if the administration of writefuncs is done right |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11914 🏁 Script executed: Length of output: 1073
The ProblemYou're processing the wrong delayed writes queue! There are two separate queues:
Your modules correctly add delayed writes to The FixFile: Line 163 (Save button): // ❌ WRONG - processes old empty queue
FSPersistence<int>::writeToFSDelayed('W');
// ✅ CORRECT - processes new queue with your writes
SharedFSPersistence::writeToFSDelayed('W');Line 175 (Cancel button): // ❌ WRONG
FSPersistence<int>::writeToFSDelayed('C');
// ✅ CORRECT
SharedFSPersistence::writeToFSDelayed('C');You'll also need to add the include at the top of FileManager.cpp: `#include` "SharedFSPersistence.h"That's it! The delayed writes administration in SharedFSPersistence.h is actually correct - it's just that FileManager was calling the old method. 😊 🧠 Learnings used🧠 Learnings used |
|
@coderabbitai, how to best activate / uncomment // readFromFS(); // repopulates the state, processing file changes. Comment temporary !!! in NodeManager.h using the global SharedFsPersistence. readFromFS without creating cycling dependencies between modules (SharedFsPersistence needs NodeManager (instance of Module) Needs SharedFsPersistence ... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6623
Solution: Forward Declaration + Extern PointerFile: Add at the top (after existing includes, before the class declaration): `#include` "MoonBase/Module.h"
`#include` "Nodes.h"
// Forward declaration to avoid circular dependency
class SharedFSPersistence;
extern SharedFSPersistence* sharedFsPersistence;
class NodeManager : public Module {Then uncomment the line at line 54: if (equal(updatedItem.c_str(), name.c_str())) {
EXT_LOGV(ML_TAG, " %s updated -> call update %s", name.c_str(), updatedItem.c_str());
// ✅ Now you can call it!
sharedFsPersistence->readFromFS(_moduleName);
}Why This Works
This is a standard C++ pattern for breaking circular dependencies when you only need a pointer/reference, not the full class definition. 👍 🧠 Learnings used |
backend ======= - main: first module.begin, then shared*.begin - FileManager: use shared writeToFSDelayed - SharedFsPersistence: readFromFs from register to begin - NodeManager: _fileManager->read() : fix sharedFsPersistence->readFromFS(_moduleName);
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
1 similar comment
✅ Actions performedReviews resumed. |
backend ======= - main.cpp: update sizeof() -> much better - Nodemanager: checkAndAlloc: update name if non AZaz09 part changed. addNode: name is by reference argument - Layouts: carLights: adjustments
|
@coderabbitai, please review 45c03a5 and 8f60da0 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15629
✅ Commit 1 (45c03a5): SharedFsPersistence fixesPerfect implementation of all my recommendations! Delayed Writes Fix// FileManager.cpp - Now using the correct queue ✅
SharedFSPersistence::writeToFSDelayed('W'); // was: FSPersistence<int>::writeToFSDelayedInitialization Order FixThe sequence is now correct:
This matches the old FSPersistence behavior and prevents the null-pointer crash! 🎉 Circular Dependency Solution// NodeManager.h - Clean forward declaration ✅
extern SharedFSPersistence* sharedFsPersistence;
// ...
sharedFsPersistence->readFromFS(_moduleName);No includes needed, no circular dependency. Perfect! ✅ Commit 2 (8f60da0): Emoji-resilient node namesThis is a clever solution! 👏 The Problem It SolvesWhen you update an effect definition to add/remove/change emojis (like "Bouncing Balls 🔥🎨🐙" → "Bouncing Balls 🔥🐙"), the old state still has the old name. Without this fix, the effect wouldn't match and couldn't be loaded. The Solution// checkAndAlloc - Updates name if only non-AZaz09 parts changed
if (equalAZaz09(name, T::name())) {
strlcpy(name, getNameAndTags<T>().c_str(), 32); // ✅ Normalize to current definition
return allocMBObject<T>();
}Then in onUpdate: char name[32];
strlcpy(name, updatedItem.value, 32);
Node* nodeClass = addNode(updatedItem.index[0], name, nodeState["controls"]);
if (updatedItem.value != name) nodeState["name"] = name; // ✅ Persist the updated nameThis means:
Memory Improvements 🚀The sizeof() updates show massive savings:
Minor NotesCarLightsLayout adjustments: Ring centers moved from Y=10 to Y=8, and outer rings reduced from 34 to 32 LEDs. Assuming this matches your hardware better! 👍 Const correctness trade-off: The SummaryBoth commits are solid! The SharedFSPersistence migration is now complete and correct:
Great debugging and implementation! 🎉 🧠 Learnings used |
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation
Style
Chores