Conversation
pio.ini: Latest FastLED Backend ======= - IO: add pin_PIR and use in board_LuxceoMood1XiaoMod (pin 4) - LightsControl: support PIR
WalkthroughAdds PIR (motion) sensor support, per-source debounce for inputs, and centralizes periodic module loops: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as "main.cpp"
participant Router as "Shared Routers\n(HTTP/WebSocket/Event/FS)"
participant Module as "ModuleLightsControl"
participant HW as "Hardware GPIO / PIR"
Main->>Router: create shared routers
Main->>Module: create & registerWithRouters(Router)
Main->>Module: init()
loop every 20ms
Main->>Module: loop20ms()
Module->>HW: digitalRead(pinPIR)
HW-->>Module: level (HIGH/LOW)
Module->>Module: debounce & update lastPIRPinState
Module->>Module: set lightsOn based on PIR
Module->>Router: emit state/event if changed
end
loop every 1s
Main->>Module: loop1s()
Module->>Router: 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 (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)
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.
| bool called = false; | ||
|
|
||
| void loop20ms() override { | ||
| // run in sveltekit task | ||
| Module::loop(); | ||
| Module::loop20ms(); | ||
|
|
||
| if (newBoardID != UINT8_MAX) { | ||
| setBoardPresetDefaults(newBoardID); // run from sveltekit task | ||
| newBoardID = UINT8_MAX; | ||
| } | ||
|
|
||
| // during boot, the IO module is unchanged , not triggering updates, so need to do it manually | ||
| if (!called) { | ||
| callUpdateHandlers(_moduleName); // calls readPins for all subscribed handlers | ||
| called = true; | ||
| } |
There was a problem hiding this comment.
Double handler fan-out on boot when a board preset is active.
When newBoardID != UINT8_MAX on the first loop20ms() call (normal path — the saved board preset triggers onUpdate → sets newBoardID), setBoardPresetDefaults → update() already fires callUpdateHandlers internally. Then the explicit callUpdateHandlers on line 733 fires them all a second time.
Consider either:
- Moving the one-shot
callUpdateHandlersinside thenewBoardID == UINT8_MAXbranch (i.e., only when no preset change is pending), or - Setting
called = truebeforesetBoardPresetDefaultsso the explicit call is skipped when the preset path already triggered handlers.
Also, called is a very generic name for a class-level flag — something like _initialPinReadDone would be self-documenting.
Proposed fix
- bool called = false;
+ bool _initialPinReadDone = false;
void loop20ms() override {
// run in sveltekit task
Module::loop20ms();
if (newBoardID != UINT8_MAX) {
setBoardPresetDefaults(newBoardID); // run from sveltekit task
newBoardID = UINT8_MAX;
+ _initialPinReadDone = true; // setBoardPresetDefaults→update already notified handlers
}
// during boot, the IO module is unchanged , not triggering updates, so need to do it manually
- if (!called) {
+ if (!_initialPinReadDone) {
callUpdateHandlers(_moduleName); // calls readPins for all subscribed handlers
- called = true;
+ _initialPinReadDone = true;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/Modules/ModuleIO.h` around lines 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.
|
|
||
| // Subscribe to IO updates to detect when I2C becomes ready | ||
| moduleIO->addUpdateHandler([this](const String& originId) { moduleIO->read([&](ModuleState& state) { i2cActuallyReady = state.data["I2CReady"]; }, name()); }, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ModuleIO class definition and addUpdateHandler implementation
find . -type f \( -name "*.h" -o -name "*.cpp" \) | xargs grep -l "class ModuleIO" 2>/dev/null | head -5Repository: MoonModules/MoonLight
Length of output: 98
🏁 Script executed:
# Search for addUpdateHandler definition
rg -n "addUpdateHandler" --type=cpp --type=hpp -B 2 -A 5 | head -80Repository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
# Check when setup() is called and initialization order
rg -n "void setup\(\)" --type=cpp --type=hpp -A 3 | head -60Repository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
# Look for ModuleIO begin() or initialization
rg -n "begin\(\)" --type=cpp --type=hpp | grep -i "moduleio\|module" | head -20Repository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
# Read ModuleIO.h to find addUpdateHandler signature and implementation
cat -n ./src/MoonBase/Modules/ModuleIO.h | head -200Repository: MoonModules/MoonLight
Length of output: 7913
🏁 Script executed:
# Search for D_IMU.h and read it to see the full context
cat -n ./src/MoonLight/Nodes/Drivers/D_IMU.h | head -150Repository: MoonModules/MoonLight
Length of output: 6395
🏁 Script executed:
# Search for when setup() is called on drivers
rg -n "setup\(\)" -t c -B 2 -A 2 | grep -A 5 -B 5 "IMU\|Driver" | head -40Repository: MoonModules/MoonLight
Length of output: 2421
🏁 Script executed:
# Find Module class definition
rg -n "class Module" -t c | head -10Repository: MoonModules/MoonLight
Length of output: 878
🏁 Script executed:
# Search for addUpdateHandler implementation in Module or base classes
rg -n "addUpdateHandler" -t c -A 10 | head -100Repository: MoonModules/MoonLight
Length of output: 8417
🏁 Script executed:
# Look for initialization order - when begin() and setup() are called
rg -n "\.begin\(\)" -t c | grep -i "module\|driver" | head -20Repository: MoonModules/MoonLight
Length of output: 263
🏁 Script executed:
# Find Module base class to understand addUpdateHandler
fd -t f "Module.h" | head -5Repository: MoonModules/MoonLight
Length of output: 125
🏁 Script executed:
# Read Module.h to understand addUpdateHandler signature
cat "./src/MoonBase/Module.h" | grep -A 15 "addUpdateHandler"Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Look for all drivers that call handlers manually after registration
rg -n "addUpdateHandler.*false" -A 1 -t c | grep -B 1 "readPins\|read\(\["Repository: MoonModules/MoonLight
Length of output: 88
🏁 Script executed:
# Read Module.h to find addUpdateHandler
cat -n "./src/MoonBase/Module.h" | head -300Repository: MoonModules/MoonLight
Length of output: 7471
🏁 Script executed:
# Search for drivers calling readPins manually after addUpdateHandler with simpler grep
rg "addUpdateHandler.*false" -A 2 -t c | head -50Repository: MoonModules/MoonLight
Length of output: 2337
🏁 Script executed:
# Check ModuleIO to see if it calls initial read/updates
rg -n "readPins" "./src/MoonBase/Modules/ModuleIO.h" -B 3 -A 3Repository: MoonModules/MoonLight
Length of output: 801
🏁 Script executed:
# Check how drivers are registered/initialized
rg -n "IMUDriver\|IMU" --type=c | grep -E "new|register|add" | head -20Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Check ModuleDrivers to see when drivers are added
cat -n "./src/MoonLight/Modules/ModuleDrivers.h" | head -100Repository: MoonModules/MoonLight
Length of output: 4879
Add eager I2C-ready read to prevent missing initial state if module loads after ModuleIO's boot-time handler dispatch.
D_Infrared.h already handles this pattern (line 107): after registering an IO handler with false, it manually calls readPins() to capture the initial state. D_IMU should follow the same approach—register the handler, then eagerly read the current I2C state in case that event already fired during boot.
Also improve readability: the nested lambdas on a single line are difficult to parse. Break them across lines and use explicit [this] capture for the inner lambda rather than [&].
Suggested fix (matches D_Infrared pattern)
// Subscribe to IO updates to detect when I2C becomes ready
- moduleIO->addUpdateHandler([this](const String& originId) { moduleIO->read([&](ModuleState& state) { i2cActuallyReady = state.data["I2CReady"]; }, name()); }, false);
+ moduleIO->addUpdateHandler([this](const String& originId) {
+ moduleIO->read([this](ModuleState& state) {
+ i2cActuallyReady = state.data["I2CReady"];
+ }, name());
+ }, false);
+
+ // Read current I2C state in case boot-time handler dispatch already occurred
+ moduleIO->read([this](ModuleState& state) {
+ i2cActuallyReady = state.data["I2CReady"];
+ }, name());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Subscribe to IO updates to detect when I2C becomes ready | |
| moduleIO->addUpdateHandler([this](const String& originId) { moduleIO->read([&](ModuleState& state) { i2cActuallyReady = state.data["I2CReady"]; }, name()); }, false); | |
| // Subscribe to IO updates to detect when I2C becomes ready | |
| moduleIO->addUpdateHandler([this](const String& originId) { | |
| moduleIO->read([this](ModuleState& state) { | |
| i2cActuallyReady = state.data["I2CReady"]; | |
| }, name()); | |
| }, false); | |
| // Read current I2C state in case boot-time handler dispatch already occurred | |
| moduleIO->read([this](ModuleState& state) { | |
| i2cActuallyReady = state.data["I2CReady"]; | |
| }, name()); |
🤖 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 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).
|
@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 |
1 participant
Summary by CodeRabbit