Conversation
pio.ini: Latest FastLED Backend ======= - IO: add pin_PIR and use in board_LuxceoMood1XiaoMod (pin 4) - LightsControl: support PIR
|
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 PIR sensor support and per-source debounce; refactors module lifecycle to central loop hooks ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as "main.cpp"
participant Routers as "Shared Routers\n(HTTP/WebSocket/Event/FS)"
participant ModuleIO as "ModuleIO"
participant Lights as "ModuleLightsControl"
participant HW as "Hardware GPIO / I2C / PIR"
Main->>Routers: create shared routers
Main->>ModuleIO: create & registerWithRouters(Routers)
Main->>Lights: create & registerWithRouters(Routers)
Main->>ModuleIO: init()
Main->>Lights: init()
loop every 20ms
Main->>Lights: loop20ms()
Lights->>ModuleIO: request pin mapping / readPins (event-driven)
ModuleIO->>HW: digitalRead(pinPIR)
HW-->>ModuleIO: level (HIGH/LOW)
ModuleIO->>Lights: emit IO update (I2CReady / pin states)
Lights->>Lights: debounce & update lastPIRPinState
Lights->>Lights: set lightsOn based on PIR/push/toggle
Lights->>Routers: emit state/event if changed
end
loop every 1s
Main->>ModuleIO: loop1s()
Main->>Lights: loop1s()
Lights->>Routers: update shared status (fps/clients/connection)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/MoonBase/Modules/ModuleIO.h`:
- Around line 72-73: The enum now includes pin_PIR but setupDefinition() never
registers a matching UI entry, so update setupDefinition() to call
addControlValue(control, "PIR") (or "PIR ♨️") for the usage select after the
existing "Reserved" entry; locate the usage select registration in
setupDefinition and add addControlValue(control, "PIR") for the pin_PIR enum
value so pins mapped to pin_PIR show correctly in the web UI.
In `@src/MoonLight/Modules/ModuleLightsControl.h`:
- Around line 406-408: The debounce state is shared across independent inputs
(lastDebounceTime, lastButtonPinState), causing interference between the push
button, toggle button and PIR handlers; add per-input debounce variables (for
example lastDebounceTimePush, lastDebounceTimeToggle, lastDebounceTimePIR and
lastButtonPinStatePush, lastButtonPinStateToggle, lastButtonPinStatePIR) and
keep debounceDelay as-is or make a per-input constant if needed, then update
each handler (push button handler, toggle button handler, PIR handler) to
reference its corresponding lastDebounceTime* and lastButtonPinState* variables
instead of the shared ones so each input debounces independently.
- Around line 488-500: The PIR handler in ModuleLightsControl.h currently forces
_state.data["lightsOn"] to match pinPIR on every debounce interval, preventing
manual overrides; change the logic in the block that reads pinPIR to act on edge
transitions or respect a PIR-enable/timeout mode instead of absolute level:
introduce and maintain a persistent previous PIR state (e.g., lastPirState) and
only call update(...) when the PIR input transitions (rising edge -> optionally
set lightsOn true; falling edge -> start/complete a configurable auto-off
timeout before setting lightsOn false), or add a boolean state flag (pirEnabled)
that the UI can toggle and guard the PIR handling with it; keep use of millis(),
lastDebounceTime, debounceDelay and still call update(newState,
ModuleState::update, _moduleName) only when you actually want to change
lightsOn.
backend ======= main: use loopxx() virtual functions - modules: use loop20ms instead of loopms, add loop1s and loop10s - Module effects: loop1s: set sharedData - Lights control: PIR ffixes
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/MoonBase/Modules/ModuleDevices.h (1)
119-125: Note:Module::loop20ms()is not called here.Unlike
ModuleIOandModuleLightsControlwhich callModule::loop20ms(), this override skips it. This meansrequestUIUpdateflag processing is bypassed. This appears intentional sinceModuleDevicesusesupdate()directly, but it diverges from the documented contract on Line 113 ofModule.h: "any Module that overrides loop20ms() must continue to call Module::loop20ms()".If
requestUIUpdateis never set on this module, there's no functional issue — just a consistency concern.src/main.cpp (1)
389-423: Consider removing commented-out sizeof diagnostics instead of keeping them as dead code.This block of commented-out
sizeof(...)expressions adds visual clutter without providing runtime value. If these are useful for future debugging, consider moving them to a separate header/doc or behind a#if 0/ dedicated build flag so the intent is explicit.
|
@coderabbitai, I am using the updated firmware manager (see d5a1806) but firmware installs are not working. See logging below. Can you explain what is happening? Look at E (200994) boot_comm: Image requires efuse blk rev >= v16.0, but chip is v1.3 especially. Logging: [133358][I][DownloadFirmwareService.cpp:220] downloadUpdate(): [🐼] Starting OTA from: https://github.com/MoonModules/MoonLight/releases/download/v0.8.0/MoonLight_esp32-s3-n16r8v_0-8-0_webflash.bin [134426][D][NetworkManager.cpp:83] hostByName(): Clearing DNS cache [135407][D][HTTPClient.cpp:390] disconnect(): tcp is closed [135414][D][HTTPClient.cpp:290] beginInternal(): protocol: https, host: release-assets.githubusercontent.com port: 443 url: /github-production-release-asset/944519235/aed33b2b-6f64-4d40-90aa-f540e83fc79c?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-02-16T14%3A30%3A33Z&rscd=attachment%3B+filename%3DMoonLight_esp32-s3-n16r8v_0-8-0_webflash.bin&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-02-16T13%3A29%3A42Z&ske=2026-02-16T14%3A30%3A33Z&sks=b&skv=2018-11-09&sig=%2BG1bdJR5VmeppVTKv7tsMplb5SvvjUpeARV56RR%2FWFs%3D&jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmVsZWFzZS1hc3NldHMuZ2l0aHVidXNlcmNvbnRlbnQuY29tIiwia2V5Ijoia2V5MSIsImV4cCI6MTc3MTI0OTMzNiwibmJmIjoxNzcxMjQ5MDM2LCJwYXRoIjoicmVsZWFzZWFzc2V0cHJvZHVjdGlvbi5ibG9iLmNvcmUud2luZG93cy5uZXQifQ.KiOWJig_81pbOs6TZf_oEthlb5ptraAbWjOK7SWfJ2I&response-content-disposition=attachment%3B%20filename%3DMoonLight_esp32-s3-n16r8v_0-8-0_webflash.bin&response-content-type=application%2Foctet-stream [135530][D][NetworkManager.cpp:127] hostByName(): DNS found IPv4 185.199.110.133 [135973][D][HTTPUpdate.cpp:265] handleUpdate(): Header read fin. [135980][D][HTTPUpdate.cpp:266] handleUpdate(): Server header: [135986][D][HTTPUpdate.cpp:267] handleUpdate(): - code: 200 [135993][D][HTTPUpdate.cpp:268] handleUpdate(): - len: 2663872 [136000][D][HTTPUpdate.cpp:280] handleUpdate(): ESP32 info: [136007][D][HTTPUpdate.cpp:281] handleUpdate(): - free Space: 3145728 [136014][D][HTTPUpdate.cpp:282] handleUpdate(): - current Sketch Size: 2899392 [136253][I][DownloadFirmwareService.cpp:72] update_started(): [🐼] HTTP Update started [136376][D][Updater.cpp:239] begin(): OTA Partition: app1 [201137][E][HTTPUpdate.cpp:402] handleUpdate(): Update failed [201145][D][HTTPClient.cpp:390] disconnect(): tcp is closed [201160][E][DownloadFirmwareService.cpp:156] updateTask(): [🐼] HTTP Update failed with error (9): Update error: Could Not Activate The Firmware |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2109 🏁 Script executed: Length of output: 6403 🏁 Script executed: Length of output: 1548 🌐 Web query: 💡 Result: That message usually means ESP-IDF’s image compatibility check is rejecting the new OTA image because the chip’s eFuse block revision is higher than what the image says it supports. What “eFuse blk rev v16.0 vs v1.3” is
So if your chip reports eFuse block rev = v16.0, but the OTA image was built with a max supported eFuse block revision of v1.3, the update will fail. Why it happensMost commonly: you built the firmware (or the OTA binary) with an older ESP-IDF / older project config whose What to do (recommended order)
Key point: don’t try to “fix” the chip from Sources: 🏁 Script executed: Length of output: 5749
What's HappeningThe error
The firmware downloads successfully and writes to flash, but activation is blocked by the bootloader's hardware compatibility check. Why This HappenedThe v0.8.0 release binaries were likely built with the newer ESP-IDF 5.5.x platform (you're currently using SolutionsOption 1: Build firmware locally (Recommended for your device) # Your local build will automatically match your chip revision
pio run -e esp32-s3-n16r8vThe locally built firmware will be compatible with your v1.3 chip revision. Option 2: Adjust release build configuration [esp32-s3-base]
build_flags =
${env.build_flags}
${moonlight.build_flags}
${livescripts.build_flags}
${HP_ALL_DRIVERS.build_flags}
-D CONFIG_IDF_TARGET_ESP32S3=1
-D CONFIG_ESP32S3_REV_MIN_0=y
; Add this to support older chip revisions in releases:
-D CONFIG_ESP_EFUSE_BLOCK_REV_MAX_FULL=199This would make release binaries compatible with a wider range of ESP32-S3 hardware revisions, though it may disable some newer hardware features. The commit d5a1806 you mentioned didn't cause this issue - the firmware manager is functioning properly. This is purely a hardware compatibility issue with the pre-built binary. 🧠 Learnings used |
|
@coderabbitai, regarding
When compiling I get: Users/ewoud/.platformio/packages/framework-arduinoespressif32-libs/esp32s3/qio_opi/include/sdkconfig.h:1010:9: warning: "CONFIG_ESP32S3_REV_MIN_0" redefined |
|
Tip For best results, initiate chat on the files or code changes.
The Real IssueThe problem isn't about chip revision settings - it's about max_efuse_blk_rev_full, which is determined by CONFIG_ESP_EFUSE_BLOCK_REV_MAX_FULL and reflects whether the current IDF version supports this efuse block format or not, and should not be changed by the user. Important distinction:
These are two separate versioning systems. Why You Can't Fix This with Build FlagsCONFIG_ESP_EFUSE_BLOCK_REV_MAX_FULL cannot be changed by user - only Espressif can change it when a new version will be supported in ESP-IDF. This value is baked into the precompiled Arduino framework libraries you're using. SolutionsOption 1: Build Locally ✅ (Already works for you) Option 2: Fix Release Binaries Option 3: Document Hardware Requirements The root cause is that ESP-IDF 5.5.x (used in Recommendation: Stick with local builds for your hardware, or consider providing multiple release variants built with different ESP-IDF versions if you need to support a wide range of ESP32-S3 chip ages. 🧠 Learnings used |
|
@coderabbitai, are the DebounceTime's for the buttons/pir still needed in ModuleLightsControl now the loop runs every 20ms instead of always? Would save a few bytes if not |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22713
Why 20ms loop interval doesn't eliminate debounce needsMechanical switch bounce physics haven't changed:
What would happen without debounce: With your current 50ms debounce, at a single button press: Without debounce (reading every 20ms during bounce): Result: Lights flicker 5 times for a single button press! 🎇 Current implementationYour current 50ms debounce acts as rate-limiting - it samples the button every ~50-60ms instead of every 20ms. This gives mechanical contacts time to settle between samples, drastically reducing false triggers. Memory considerationThe debounce variables use:
Total: 24 bytes - negligible for ESP32 with hundreds of KB of RAM. VerdictKeep the debounce! It's preventing a significant user experience bug. Those 24 bytes are buying you reliable button behavior. 👍 🧠 Learnings used |
firmware ======== - pio.ini: latest FastLED Frontend ======== - Firmware manager: confirmGithubUpdate clearer message if no matching firmware found backend ======= - IO: callUpdateHandlers once in loop - Module Driver: addUpdateHandler in constructor, no initial readPins (see IO)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
interface/src/routes/system/update/GithubFirmwareManager.svelte (1)
156-156: Case-sensitive "Nightly" check.
release.name.includes("Nightly")won't match"nightly"or"NIGHTLY BUILD". Consider a case-insensitive check if naming conventions aren't strictly enforced:-|| release.name.includes("Nightly")} +|| release.name.toLowerCase().includes("nightly")}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/system/update/GithubFirmwareManager.svelte` at line 156, The conditional that checks for Nightly builds is case-sensitive: update the check around release.name.includes("Nightly") used with compareVersions(page.data.features.firmware_version, release.tag_name) so it performs a case-insensitive match (e.g., normalize release.name to lower/upper or use a case-insensitive regex) to catch "nightly", "NIGHTLY BUILD", etc.; keep the overall logic (install if versions differ OR it's a nightly) but replace the includes("Nightly") part with a case-insensitive test on release.name.src/main.cpp (1)
389-457: Deadsizeofdiagnostics block is duplicated and unreachable.This
#if 0block contains two near-identical copies of the samesizeoflisting (lines 391–424 and 425–456). It's also placed aftervTaskDelete(NULL)on most builds, making it doubly unreachable. Consider removing the duplicate or the entire block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 389 - 457, The duplicated, unreachable diagnostic block guarded by "#if 0" (containing repeated sizeof(...) lines) should be removed or collapsed; locate the "#if 0" / "#endif" block that lists sizeof(esp32sveltekit), sizeof(Module), sizeof(moduleDevices), etc. (it's also placed after vTaskDelete(NULL)), and delete the duplicate copy (or remove the entire block) so only one diagnostic listing remains if needed or none at all; ensure you remove both repeated series between the matching "#if 0" and "#endif" and keep surrounding code intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/routes/system/update/GithubFirmwareManager.svelte`:
- Around line 59-70: In confirmGithubUpdate(release: any) guard against missing
or empty release.assets by checking if release.assets is truthy and has length
and return early (or show an error) if not; then replace the loose substring
check that uses
release.assets[i].name.includes(page.data.features.firmware_built_target) with a
stricter match (for example use a regex with word boundaries anchored around
page.data.features.firmware_built_target or split the asset name into segments
and compare the exact segment) so you only pick the correct .bin asset, and keep
the existing .bin check and browser_download_url assignment.
In `@lib/framework/SystemStatus.cpp`:
- Line 200: The hardcoded root["lps"] = 100 is misleading; either measure real
loops-per-second or make it a named constant/target. Fix by replacing the magic
100: if you want real LPS, add a counter (e.g., lpsCounter) incremented in the
main loop hook (e.g., loop20ms or mainLoop) and compute a rate over a short
interval (resetting the counter or using a rolling window) then assign that
computed value to root["lps"]; alternatively, if it is a target, introduce a
descriptive constant (e.g., constexpr int TARGET_LPS) and set root["target_lps"]
= TARGET_LPS and remove the misleading root["lps"] assignment. Ensure you modify
SystemStatus.cpp where root["lps"] is set and update any callers/consumers
accordingly.
---
Nitpick comments:
In `@interface/src/routes/system/update/GithubFirmwareManager.svelte`:
- Line 156: The conditional that checks for Nightly builds is case-sensitive:
update the check around release.name.includes("Nightly") used with
compareVersions(page.data.features.firmware_version, release.tag_name) so it
performs a case-insensitive match (e.g., normalize release.name to lower/upper
or use a case-insensitive regex) to catch "nightly", "NIGHTLY BUILD", etc.; keep
the overall logic (install if versions differ OR it's a nightly) but replace the
includes("Nightly") part with a case-insensitive test on release.name.
In `@src/main.cpp`:
- Around line 389-457: The duplicated, unreachable diagnostic block guarded by
"#if 0" (containing repeated sizeof(...) lines) should be removed or collapsed;
locate the "#if 0" / "#endif" block that lists sizeof(esp32sveltekit),
sizeof(Module), sizeof(moduleDevices), etc. (it's also placed after
vTaskDelete(NULL)), and delete the duplicate copy (or remove the entire block)
so only one diagnostic listing remains if needed or none at all; ensure you
remove both repeated series between the matching "#if 0" and "#endif" and keep
surrounding code intact.
|
@coderabbitai, Using callUpdateHandlers in Module IO and not using readPins() // initially on modules which subscribe to the IO update handler |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11867
✅ What's Good1. Centralized Initialization Pattern // ModuleIO.h - Single source of truth for initial pin reads
void loop20ms() override {
if (!called) {
callUpdateHandlers(_moduleName); // triggers readPins() for all subscribers
called = true;
}
}This is much cleaner than having each module manage its own 2. Event-Driven Consistency // ModuleDrivers constructor - subscribe early, react when notified
_moduleIO->addUpdateHandler([this](const String& originId) { readPins(); }, false);Modules now purely react to IO updates rather than mixing reactive + polling approaches. 3. Code Reduction
💡 Minor Suggestions1. Flag naming clarity bool called = false; // Generic - could be clearerConsider renaming to 2. Comment precision // during boot, the IO module is unchanged , not triggering updates, so need to do it manuallyExcellent comment! Minor typo: remove space before comma. 3. Handler registration timing 🎯 Architecture Analysis
🔍 Edge Cases Handled✅ Boot scenario where IO state doesn't change → manual trigger in Unrelated ChangepinMode(pinPIR, INPUT); // was INPUT_PULLUPThis is a functional change for PIR sensors - correct if your PIR modules have external pull-ups or active output drivers. VerdictApproved! 🎉 This is a textbook example of refactoring toward better separation of concerns. The pattern is now consistent, maintainable, and eliminates redundant initialization logic. 🧠 Learnings used |
🧩 Analysis chain🏁 Script executed: Length of output: 266234 🏁 Script executed: Length of output: 2287
Verdict
Two small nits to lock it down
A tiny tweak that covers both // ModuleIO.h/.cpp
class ModuleIO : public Module {
// ...
bool _bootPinsPublished = false;
void loop20ms() override {
Module::loop20ms();
if (!_bootPinsPublished) {
// 1) Update IO’s own pin state first
readPins();
// 2) Now notify subscribers (Drivers, LightsControl, etc.)
callUpdateHandlers(_moduleName);
_bootPinsPublished = true;
}
}
}Why this helps
Also advisable
Boot sequence (expected) sequenceDiagram
participant Main as main()
participant IO as ModuleIO
participant Drivers as ModuleDrivers
participant Lights as ModuleLightsControl
Main->>IO: construct + addUpdateHandler(readPins)
Main->>Drivers: construct + IO.addUpdateHandler(Drivers::readPins)
Main->>Lights: construct + IO.addUpdateHandler(Lights::readPins)
Main->>Main: start loop20ms tick
Main->>IO: loop20ms()
IO->>IO: readPins() // IO internal state ready
IO->>Drivers: callUpdateHandlers("io") -> Drivers::readPins()
IO->>Lights: callUpdateHandlers("io") -> Lights::readPins()
Overall, the change looks solid. With the one-shot gating + “IO reads first” ordering, the event-driven init should be robust, consistent, and free from double-initialization across all subscribers. If you want, I can follow up with a minimal patch once you confirm where you prefer to place the _bootPinsPublished flag (header vs. TU) and whether loop20ms is your standardized 20 ms hook across modules. 🧠 Learnings used✅ Actions performedReview triggered.
|
|
@coderabbitai, I am testing I2S bus and IMU driver (MPU6050) on esp32-D0 and it is not working, see logging. Can you investigate what is going on? I would expect that it would not say: Found 0 device(s) as I connected the MPU6050 the right way on pin 21 and 22 [ 2587][I][D_IMU.h:40] initBoard(): [💫] AppDrivers (2288) Starting board 0 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9784 🏁 Script executed: Length of output: 3364
Timeline of EventsRoot CauseThe new event-driven architecture has a race condition:
The IMU tries to use Wire 1.6 seconds before it's initialized! Why the I2CReady Check FailsYour code at D_IMU.h line 43 does check
Looking at your code more carefully: moduleIO->read([&](ModuleState& state) {
if (state.data["I2CReady"].as<bool>()) {
mpu.initialize(); // This still executes!The FixIMU should subscribe to ModuleIO updates and only initialize when I2C becomes ready, not poll state directly. Similar to how ModuleDrivers now works: In D_IMU.h, modify the setup() or constructor: void setup() override {
addControl(gyro, "gyro", "coord3D");
addControl(accell, "accell", "coord3D");
addControl(board, "board", "select");
addControlValue("MPU6050");
addControlValue("BMI160");
// Subscribe to IO updates to detect when I2C becomes ready
moduleIO->addUpdateHandler([this](const String& originId) {
moduleIO->read([&](ModuleState& state) {
// Only try to init if we want to AND I2C is actually ready now
if (on && requestInitBoard && state.data["I2CReady"].as<bool>()) {
initBoard();
requestInitBoard = false;
}
}, name());
}, false);
}And modify loop20ms() to remove the immediate init: void loop20ms() override {
// Remove this block - let the update handler do it:
// if (requestInitBoard) {
// initBoard();
// requestInitBoard = false;
// }
// Rest of the loop code...
if (!motionTrackingReady) return;
// ... existing FIFO reading code ...
}Alternative Quick FixIf you don't want to restructure, you can add a more robust check in void loop20ms() override {
if (requestInitBoard) {
// Only init if I2C is ACTUALLY ready RIGHT NOW
bool i2cActuallyReady = false;
moduleIO->read([&](ModuleState& state) {
i2cActuallyReady = state.data["I2CReady"].as<bool>();
}, name());
if (i2cActuallyReady) {
initBoard();
requestInitBoard = false;
}
// else: keep requestInitBoard true and try again next loop
}
// ... rest of loop ...
}This ensures IMU waits until I2C is actually initialized before trying to use Wire. Would you like me to prepare a complete patch for the event-driven approach? 🧠 Learnings used |
- IMU driver: add i2cActuallyReady and set in handler
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Drivers/D_IMU.h (1)
120-124: No retry on failed initialization — intentional?If
initBoard()fails (e.g., MPU6050 not physically present or DMP init fails),requestInitBoardis set tofalseon line 123 regardless of outcome. The driver won't retry unless the user togglesonor changesboard. This is reasonable to avoid infinite retries on bad hardware, but if transient I2C errors are expected, a bounded retry mechanism could improve robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_IMU.h` around lines 120 - 124, The code clears requestInitBoard unconditionally after calling initBoard(), so failed/partial inits (e.g., MPU6050/DMP or transient I2C errors) will suppress further retries; change init flow so requestInitBoard is only cleared when initBoard() reports success (e.g., return bool or throw on failure) and implement a bounded retry/backoff: add a retry counter (e.g., imuInitRetries) and/or timestamp for next attempt, increment retries on failure and retry while retries < MAX_INIT_RETRIES, reset counter on success, and ensure i2cActuallyReady is still checked before attempts; update references to initBoard(), requestInitBoard, i2cActuallyReady and add imuInitRetries/MAX_INIT_RETRIES symbols to locate and implement this logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonBase/Modules/ModuleIO.h`:
- Around line 720-735: The one-shot boot handler is firing twice because
loop20ms() calls setBoardPresetDefaults(newBoardID) which itself triggers
callUpdateHandlers(), then your subsequent guarded
callUpdateHandlers(_moduleName) runs again; to fix, rename the flag from called
to a clearer _initialPinReadDone and either (A) set _initialPinReadDone = true
before invoking setBoardPresetDefaults(newBoardID) so the explicit
callUpdateHandlers(_moduleName) is skipped when a preset change occurs, or (B)
move the one-shot callUpdateHandlers(_moduleName) into the branch guarded by
newBoardID == UINT8_MAX so it only runs when no preset change is pending; update
all references to the renamed flag accordingly.
In `@src/MoonLight/Nodes/Drivers/D_IMU.h`:
- Around line 37-39: Register the update handler with moduleIO->addUpdateHandler
as you currently do, but immediately perform an eager read to capture the
initial I2C state (call moduleIO->read(...) once after registering) so
i2cActuallyReady is set if the event already fired during ModuleIO boot; also
reformat the nested lambdas across multiple lines and change the inner lambda
capture from [&] to [this] for clarity and safety (refer to moduleIO,
addUpdateHandler, read, i2cActuallyReady and name() in D_IMU.h).
---
Nitpick comments:
In `@src/MoonLight/Nodes/Drivers/D_IMU.h`:
- Around line 120-124: The code clears requestInitBoard unconditionally after
calling initBoard(), so failed/partial inits (e.g., MPU6050/DMP or transient I2C
errors) will suppress further retries; change init flow so requestInitBoard is
only cleared when initBoard() reports success (e.g., return bool or throw on
failure) and implement a bounded retry/backoff: add a retry counter (e.g.,
imuInitRetries) and/or timestamp for next attempt, increment retries on failure
and retry while retries < MAX_INIT_RETRIES, reset counter on success, and ensure
i2cActuallyReady is still checked before attempts; update references to
initBoard(), requestInitBoard, i2cActuallyReady and add
imuInitRetries/MAX_INIT_RETRIES symbols to locate and implement this logic.
|
@coderabbitai, applied your fixes above, not committed yet but still no MPU6050 connected. It is still strange I found 0 devices, see the log: [ 4184][D][ModuleIO.h:900] readPins(): [💫] ESP32 SvelteKit (4084) I2CSDA changed 21 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8359
The Remaining IssueLooking at your code and logs, I found the root cause:
Current (Broken) Sequence:The ProblemLine 1023 in ModuleIO.h sets This means:
The FixSet In ModuleIO.h readPins(), around line 916: // Current code:
Wire.begin(pinSDA, pinSCL, I2CFreq);
EXT_LOGI(ML_TAG, "initI2C Wire sda:%d scl:%d freq:%d kHz (%d)",
pinSDA, pinSCL, I2CFreq / 1000, I2CFreq);
_triggerUpdateI2C = 1;
// Add this:
update([&](ModuleState& state) {
state.data["I2CReady"] = true; // Set immediately!
}, ModuleState::update, _moduleName);Then in _updateI2CDevices(), change line 1023: // Remove this line (redundant now):
// newState["I2CReady"] = _triggerUpdateI2C == 1;
// Keep the scan logic:
if (_triggerUpdateI2C == 1) {
JsonArray i2cDevices = newState["i2cBus"].to<JsonArray>();
// ... scan loop ...
}This ensures:
Hardware CheckThe scan found 0 devices - this could still be a hardware issue:
TestAfter the code fix:
Let me know if you want me to provide a complete patch! 🧠 Learnings used |
|
@coderabbitai, regarding
it's set not immediately after Wire.begin but a bit later. Why is that a problem? I think it is only important 4190][I][esp32-hal-i2c-ng.c:112] i2cInit(): Initializing I2C Master: num=0 sda=21 scl=22 freq=100000 runs before the IMU driver does something, also keep in mind the IMU driver can also be added much later as it is a Node which can be added and deleted anytime in runtime, so even if there is no IMU driver, scanning I2C bus should find a device isn't it ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5062 🏁 Script executed: Length of output: 3762
Your Analysis is Correct ✓The sequence is fine:
The scan happens after Wire.begin() succeeds, so the I2C peripheral is ready. And you're right that the IMU driver being a runtime-addable Node means the scan should work independently. The Real IssueThe scan finding 0 devices means the MPU6050 isn't responding at all. This is a hardware issue, not software timing. The ESP_ERR_INVALID_STATE (259) that happens later is actually a symptom, not the root cause. The new Hardware TroubleshootingSince your board (esp32-D0) has no pin conflicts (ledPins are 16,3,1,4 - not 21/22), please check: 1. Wiring verification: 2. Module quality:
3. Pull-up resistors:
4. Power quality:
5. Test with i2c_scanner sketch: Wire.begin(21, 22);
for(byte addr = 1; addr < 127; addr++) {
Wire.beginTransmission(addr);
byte error = Wire.endTransmission();
if(error == 0) Serial.printf("Device at 0x%02X\n", addr);
}If this also finds 0 devices, it confirms hardware issue, not MoonLight code. My apologies for the red herring about I2CReady timing - you correctly identified that as non-issue! 🎯 🧠 Learnings used |
docs ==== - add IMU driver and I2S in module IO in docs backend ======= Module IO: - add name and id to i2cBus - onUpdate: boardPreset also at boot, if not modded, modded also true if index changed - rename called to _initialPinReadDone - loop20ms: add _updateI2CDevices (from loop1s) - _updateI2CDevices: address in 0xFF format IMU driver - setup: add IO read I2CReady and requestInitBoard - initBoard: update i2cBus name and ID if found
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/moonbase/inputoutput.md (1)
40-52:⚠️ Potential issue | 🟡 MinorStale "Planned later" entry and a spelling error in the new section.
- Line 42 —
* I2Cis still listed under "Planned later" but is now documented in the new## I2C peripheralssection added directly below. Remove it from the planned list.- Line 51 — "initalized" → "initialized".
✏️ Proposed fix
* Planned later * I2S for microphone and line in - * I2C-* Name and ID: Unknown if the device is not initalized. If a driver (e.g. [IMU driver](../../moonlight/drivers/#driver-nodes)) is added, it will provide name and ID +* Name and ID: Unknown if the device is not initialized. If a driver (e.g. [IMU driver](../../moonlight/drivers/#driver-nodes)) is added, it will provide name and ID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/moonbase/inputoutput.md` around lines 40 - 52, Remove the redundant "* I2C" bullet from the "Planned later" list (since the new "## I2C peripherals" section now documents it) and correct the spelling of "initalized" to "initialized" in the "Name and ID" description; update the bullet list and the sentence under the "## I2C peripherals" heading accordingly.
🧹 Nitpick comments (1)
interface/src/lib/components/moonbase/RowRenderer.svelte (1)
267-267: LGTM — symmetrical divider mirrors line 160.The closing divider creates a clean visual frame around the component's content, matching the existing top divider at line 160 with identical classes.
One optional nit: both
<div class="divider">elements are purely decorative. Addingaria-hidden="true"would prevent screen readers from announcing the empty separator element.♻️ Optional accessibility tweak
-<div class="divider mb-2 mt-0"></div> +<div class="divider mb-2 mt-0" aria-hidden="true"></div>(Same applies to the matching divider at line 160.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/lib/components/moonbase/RowRenderer.svelte` at line 267, Add aria-hidden="true" to the decorative divider elements in RowRenderer.svelte so screen readers ignore them; specifically update the two <div class="divider ..."> elements (the top divider near the opening of the component and the matching closing divider) to include aria-hidden="true" to mark them as non‑semantic decoration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/moonlight/drivers.md`:
- Line 42: The table row for "IMU Driver" contains two <img> tags missing alt
attributes and a typo ("inertion"); update the two image tags in that row to
include descriptive alt text (e.g., alt="MPU-6050 IMU" and alt="IMU Driver
Controls") and fix the remark text to read "Receive inertial data from an IMU /
I2C peripheral" (or "Receive inertia" if you prefer), keeping the rest of the
markdown and links intact; target the line containing the "IMU Driver" row in
docs/moonlight/drivers.md to make these edits.
In `@src/MoonLight/Nodes/Drivers/D_IMU.h`:
- Around line 57-79: The read callback is modifying the shared state by
assigning newState = state.data (JsonObject reference) which bypasses
update/diff logic; fix by deep-copying state.data into the local JsonDocument
first (use doc.set(state.data) or JsonDocument::set(JsonObject)) then obtain
newState = doc.to<JsonObject>() and perform the i2cDevice["name"]/["id"]
mutations on that local copy; after moduleIO->read return, call moduleIO->update
with the local copied JsonObject (doc.to<JsonObject() or newState) so
compareRecursive sees a real change and update handlers/persistence run.
---
Outside diff comments:
In `@docs/moonbase/inputoutput.md`:
- Around line 40-52: Remove the redundant "* I2C" bullet from the "Planned
later" list (since the new "## I2C peripherals" section now documents it) and
correct the spelling of "initalized" to "initialized" in the "Name and ID"
description; update the bullet list and the sentence under the "## I2C
peripherals" heading accordingly.
---
Duplicate comments:
In `@src/MoonLight/Nodes/Drivers/D_IMU.h`:
- Around line 37-42: The inner lambda passed into moduleIO->addUpdateHandler
currently uses a [&] capture and is written as a single-line nested lambda;
change the inner lambda capture to [this] (not [&]) and expand the nested
lambdas into multi-line blocks for readability and to avoid accidental
captures—update the lambda passed to moduleIO->addUpdateHandler and the nested
moduleIO->read call that assign i2cActuallyReady (and the similar eager
moduleIO->read) to use [this] and block-style bodies while leaving
requestInitBoard and name() usage unchanged.
---
Nitpick comments:
In `@interface/src/lib/components/moonbase/RowRenderer.svelte`:
- Line 267: Add aria-hidden="true" to the decorative divider elements in
RowRenderer.svelte so screen readers ignore them; specifically update the two
<div class="divider ..."> elements (the top divider near the opening of the
component and the matching closing divider) to include aria-hidden="true" to
mark them as non‑semantic decoration.
|
@coderabbitai, MPU6050 works now on an ESP32-D0, so the code is correct now. But still not on ESP32-S3 (speed Xiao with Pin 5 = SDA and [in 6 = SCL.
Please re-anaylise the problem with latest commit and this in mind This is the logging: [ 3307][D][ModuleIO.h:667] setBoardPresetDefaults(): [💫] ESP32 SvelteKit (3320) boardID 17 [ 9916][D][SharedFSPersistence.h:121] writeToFS(): [🐼] delayedWrites: Add /.config/tasks.json |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
Let me search for ESP32-S3 I2C pin restrictions and Xiao board specifics. 🌐 Web query: 💡 Result: GPIO5 and GPIO6 on ESP32‑S3 are not strapping pins, so they do not have boot‑strap level requirements and can be used as I²C SCL/SDA like normal GPIOs. The ESP32‑S3 strapping pins are GPIO0, GPIO3, GPIO45, and GPIO46. [1][2] What to watch when using GPIO5/6 for I²C:
Bottom line: GPIO5 + GPIO6 are safe choices for I²C on ESP32‑S3; the strapping‑pin restrictions apply only to GPIO0/3/45/46. [1][2] Sources: 🌐 Web query: 💡 Result: For the Seeed Studio XIAO ESP32-S3, the commonly used / “default” I²C pin mapping is:
This matches the board pin mapping documentation [1] and is also what Seeed forum helpers typically point to when I²C isn’t working due to wrong GPIO selection [2]. In Arduino, you can be explicit: `#include` <Wire.h>
Wire.begin(5, 6); // SDA=GPIO5, SCL=GPIO6Sources: [1] [2] 🌐 Web query: 💡 Result: On the ESP32-S3 you can route I2C (SDA/SCL) to “almost any” GPIO via the GPIO matrix/IO MUX, but some pins have hard/boot-time limitations that make I2C “not work” in Arduino. [1][2] Pins that commonly break I2C on ESP32‑S3 (Arduino
|
|
@coderabbitai, I am about to merge this PR back to main. Please do a thorough review of everything done in this commit and come with recommendations to merge and possible future actions |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7756 🏁 Script executed: Length of output: 5266 🏁 Script executed: Length of output: 5314
✅ APPROVED FOR MERGEThe code is production-ready with solid architecture improvements. 🎯 Key AchievementsArchitecture
Features
Documentation
🔍 Code Quality Findings✅ No TODOs/FIXMEs in critical paths
|
| Category | Lines Changed | Files |
|---|---|---|
| Core Architecture | +74/-58 | main.cpp, Module.h |
| I/O & Sensors | +55/-36 | ModuleIO.h, ModuleLightsControl.h |
| IMU Driver | +78/-50 | D_IMU.h |
| Module Timing | +18/-18 | ModuleDevices, Effects, Tasks, LiveScripts |
| Documentation | +13/-3 | inputoutput.md, drivers.md, effects.md |
| Total | +238/-165 | 15 files |
🎬 Merge Checklist
- All
loop()methods migrated to timed variants - No TODOs/FIXMEs in production code
- Thread safety verified (mutex usage correct)
- Documentation updated for new features
- No breaking API changes
- Hardware issues documented (conversation log)
- Event-driven initialization pattern working
- I2C readiness prevents race conditions
💡 Verdict
Merge with confidence! This PR represents a significant architectural improvement with minimal risk. The centralized initialization pattern is more maintainable, and the I2C/IMU handling is robust.
The minor issues noted above can be addressed in follow-up commits - they don't block the merge.
Great engineering work! 🚀
✏️ Learnings added
Learnt from: ewowi
URL: https://github.com/MoonModules/MoonLight/pull/128
Timestamp: 2026-02-18T11:15:32.720Z
Learning: On ESP32-S3 boards like Seeed Studio Xiao ESP32-S3, I2C pins (typically GPIO5=SDA, GPIO6=SCL) do not have onboard pull-up resistors. ESP32-S3 internal pull-ups (~tens of kΩ) are insufficient for reliable I2C operation. External pull-up resistors (4.7kΩ for 100kHz, 2.2kΩ for 400kHz, to 3.3V) are required for I2C peripherals like MPU6050. Symptoms without pull-ups: I2C scan timeouts (6+ seconds instead of <500ms), "Found 0 device(s)", ESP_ERR_INVALID_STATE (259) errors. This differs from ESP32-D0 and other boards that include onboard I2C pull-ups.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-05T13:20:46.001Z
Learning: In MoonLight with FastLED 3.11 on ESP-IDF 5.x+ (Arduino core 3.x): FastLED defaults to RMT5 driver (wraps Espressif's led_strip component), which has known ISR handler lifecycle issues causing repeated "Handler detached" messages every ~500ms and severe performance degradation (e.g., 2 FPS). RMT5 is more prone to Wi-Fi/interrupt jitter than legacy RMT4. Solution: force legacy RMT4 by adding `-D FASTLED_RMT5=0` to build flags in platformio.ini, or switch to FastLED's parallel I/O drivers which don't use RMT. RMT4 and RMT5 cannot coexist in the same binary.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-16T15:53:11.236Z
Learning: For ESP32-PICO-MINI-02 (and similar ESP32 boards with PSRAM), the critical configuration to enable PSRAM is `board_build.arduino.memory_type = qio_qspi` in platformio.ini. The board JSON may already include `-DBOARD_HAS_PSRAM` and `flash_mode: qio`, but without the memory_type setting, PSRAM won't be properly initialized, causing LoadStoreError crashes when code tries to access PSRAM addresses.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-28T18:06:23.868Z
Learning: ESP32-P4 observed lower headroom for httpd task stack in MoonLight: NodeManager rename (create new node + rebuild controls + remove old) can overflow httpd stack when onUpdate runs in httpd. Mitigation: defer only NodeManager rename heavy work to SvelteKit loop via a small pendingRenames queue; keep identity short-circuit in compareRecursive and equality-guarded controls updates. Increasing httpd stack_size for P4 builds is an optional, broader fix.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-26T14:59:22.091Z
Learning: In MoonModules/MoonLight PR `#114`: ModuleDevices deadlock caused by: (1) StatefulService.h line 204-205 bug - after 500ms mutex timeout, continues WITHOUT acquiring mutex, causing race conditions and JSON corruption. (2) receiveUDP() while-loop processes own UDP broadcasts via loopback, creating circular call chain: sendUDP→broadcast→receiveUDP→updateDevices→update→handlers→sendUDP. (3) updateDevices() holds _accessMutex during expensive sort/JSON operations with 15 devices, can exceed 500ms with nested calls. Solution: Fix StatefulService to wait forever or reboot on timeout; skip own broadcasts by checking message.name == ownHostname; optimize updateDevices to sort outside mutex.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-14T19:58:39.117Z
Learning: ESP32-P4/httpd: Heavy onUpdate cascades (preset apply, rename, controls rebuild) can exhaust 6 KB httpd stack; 8192 bytes is the safe default. Defer heavy work from onUpdate (httpd) to the SvelteKit loop and avoid large StaticJsonDocument locals in onUpdate to preserve stack headroom.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-25T10:45:58.923Z
Learning: In MoonLight ModuleDevices: Variable Length Arrays (VLAs) like `char buffer[packetSize]` on ESP32 task stack are unsafe because tasks have limited stack space (~8KB). When packetSize exceeds buffer size or stack limit, causes stack overflow and crashes (including WebSocket failures). Solution: use `char buffer[sizeof(UDPMessage)]` or static/heap allocation, and always validate packetSize before reading: `if (packetSize > sizeof(UDPMessage)) { deviceUDP.flush(); continue; }`.
</learning]>
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-08T11:39:52.878Z
Learning: In MoonLight WebSocket monitor: When browser tabs are hidden, browsers throttle JavaScript (including WebSocket onmessage handlers) but keep TCP connections open. Monitor data (48KB frames every 20-300ms) continues arriving at TCP level but isn't processed. This fills ESP32's httpd_ws_send_frame_async() buffers, causing ESP_FAIL. When tab becomes visible, hundreds of queued frames overwhelm the JavaScript main thread, freezing the UI. Solution: use Page Visibility API (client_visibility event) to stop emitting monitor data to hidden clients, preventing buffer overflow and backlog buildup.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-04T09:52:08.096Z
Learning: MoonLight ModuleDevices: To reduce missed packets when many devices send at once, add a 0–20 ms non-blocking jitter only to non-control (status) UDP broadcasts (post-control apply and optionally the 10s heartbeat). Implement by scheduling the send in loop20ms instead of delaying inside addUpdateHandler/httpd context; keep control packets immediate.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-30T20:14:11.929Z
Learning: In MoonModules/MoonLight ModuleDevices group sync: Use boolean `isOriginalSender` flag instead of sequence number for deduplication. When originator broadcasts control to group (isOriginalSender=true), members apply locally but don't re-broadcast (pass non-numeric originId to prevent addUpdateHandler from triggering sendUDP). This prevents infinite broadcast loops with zero memory overhead (no state cache needed), versus sequence number approach which requires caching last state and processing 250× more packets (each member broadcasts duplicates).
</add_learning>
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 114
File: src/MoonBase/Modules/ModuleDevices.h:39-49
Timestamp: 2026-01-26T12:58:47.381Z
Learning: In MoonModules/MoonLight: Do not add null checks for essential components like `moduleControl` in ModuleDevices or similar core dependencies. A null value for these essential components indicates a programming error (bug in initialization/calling code) that should fail-fast rather than be masked with defensive checks. This aligns with the "don't mask programming errors" philosophy used throughout the codebase.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T12:49:07.297Z
Learning: In MoonLight channel reordering architecture: Keep rgbwBufferMapping in driver task (not effect task) because: (1) Driver has spare CPU cycles waiting for DMA/I2S/Art-Net timing; effect task is CPU-bound and starvation-prone. (2) Contiguous RGB writes in effects are cache-friendly; scattered writes at final offsets are cache-hostile for non-RGB orders (GRB, BGR, etc.). (3) forEachLightIndex already handles sparse mapping iteration; moving reordering to effects adds complexity without benefit since drivers must send full frames anyway (can't skip iteration). (4) rgbwBufferMapping does brightness/gamma LUT + white extraction + reordering—moving just reordering creates code duplication. (5) Effects should express color intent (CRGB), not hardware details (offsetRed/Green/Blue). Driver-side transformation maintains separation of concerns and task load balance for scaling to 98K+ pixels.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-13T19:05:38.712Z
Learning: In MoonModules/MoonLight SharedFSPersistence (src/MoonBase/SharedFSPersistence.h): To prevent stale default values from being written to filesystem during initialization, update handlers must be enabled AFTER files are read. Implementation: (1) registerModule() stores module info without enabling handler, (2) begin() first reads all files via readFromFS(), then enables handlers only for modules with delayedWriting=true. This matches FSPersistence behavior where files are loaded before handlers become active, preventing defaults from overwriting persisted state.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR `#84`: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:34:40.097Z
Learning: For the MoonLight LED system: Effects task can run on Core 0 (PRO_CPU) since it only performs computation (calculating colors, writing to channels array) without timing-critical hardware interaction. WiFi preemption is acceptable for pure computation tasks. Only the driver task (using DMA, I2S, LCD, PARLIO) requires Core 1 (APP_CPU) for uninterrupted timing-critical hardware operations.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:33.598Z
Learning: In MoonLight GameOfLifeEffect (src/MoonLight/Nodes/Effects/E_MoonModules.h): The effect's nested loops (especially lines 257-323) iterate through all cells (e.g., 128×128 = 16,384) with nested neighbor checks (18 per cell = 294,912 operations) without yielding. This starves effectTask (priority 3) when higher-priority tasks like HTTP server (priority 5) run, causing the effect to disappear. UI actions (save, refresh) temporarily show one frame because HTTP completes and briefly releases CPU back to effectTask. Solution: Add taskYIELD() inside outer X loop to prevent task starvation. This is the same root cause as issue `#102` (Starsky glitches with many driver nodes).
1 participant
Summary by CodeRabbit