Merged
Conversation
pio.ini: add MPU6050 driver backend ======= - NodeManager: send onUpdate to node when on/off changed - Nodes: add sharedData.gravity, add D_MPU6050.h - Module Drivers: add D_MPU6050.h - D_MPU6050.h: New - Effects: particles: add gyro
|
Caution Review failedThe pull request is closed. 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 an MPU6050-based IMU driver and shared gravity state, synchronizes node onUpdate with layer mutex, consolidates particle gravity selection, introduces I2C scanning/init, implements delayed FS-write queueing and handler control, wires ModuleLightsControl into ModuleEffects, and schedules 20ms driver loops. Changes
Sequence DiagramsequenceDiagram
participant NM as NodeManager
participant MD as ModuleDrivers
participant IMU as IMUDriver
participant I2C as Wire/I2C
participant SD as SharedData
participant Eff as ParticlesEffect
NM->>MD: request node allocation / enable IMU node
MD->>IMU: instantiate & setup IMUDriver
IMU->>I2C: init / probe MPU6050 (Wire)
I2C-->>IMU: ACK / responses
IMU->>IMU: DMP init & calibration
loop every 20ms
IMU->>I2C: read FIFO packet
I2C-->>IMU: sensor packet
IMU->>IMU: compute quaternion -> gravity / YPR
IMU->>SD: update sharedData.gravity
end
NM->>IMU: nodeClass->onUpdate(...) -- locked via node->layerMutex
Eff->>SD: read sharedData.gravity (if gravityType==Gyro)
Eff->>Eff: apply gravity to particles
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 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: 4
🤖 Fix all issues with AI agents
In `@src/MoonBase/NodeManager.h`:
- Line 267: The call to nodeClass->onUpdate currently passes the full nodeState
without holding layerMutex and thus is inconsistent with the controls-value
path; wrap the on-toggle path's nodeClass->onUpdate call with layerMutex (same
scope used in the controls-value path) and normalize the argument semantics by
passing a consistent control object (e.g., construct a single-control JSON
containing only the on field or an explicit {"on": ...} object) instead of the
full nodeState so onUpdate implementations always receive the same JSON shape.
In `@src/MoonLight/Nodes/Drivers/D_MPU6050.h`:
- Line 46: The onUpdate method declaration lacks the override specifier which
risks silent signature drift; update the declaration of void onUpdate(const
Char<20>& oldValue, const JsonObject& control) in class D_MPU6050 to add the
override keyword so it matches the other overridden methods (e.g., setup,
hasOnLayout, onLayout, loop20ms) and lets the compiler validate the override.
- Around line 37-44: The method initI2S incorrectly initializes I2C (Wire.begin)
so rename the function to initI2C (and update all callers/usages) and update the
log message (EXT_LOGI) to reflect "initI2C" to avoid protocol confusion; while
here also remove or replace the hardcoded pins (Wire.begin(5, 6)) by using the
existing pin allocation mechanism or configurable pin variables (e.g., use
allocatePin or pass sda/scl parameters or read configured pins) so the function
no longer hardcodes 5 and 6 and will work across boards.
- Around line 83-84: The hasOnLayout() override currently returns true while
onLayout() is an empty override, causing unnecessary mapping-system calls;
either remove both overrides (delete the hasOnLayout() and onLayout() methods so
the node uses the base-class default of no layout handling) or implement real
layout initialization/update logic inside onLayout() and keep hasOnLayout()
true; locate the D_MPU6050 class methods hasOnLayout() and onLayout() and apply
one of these two changes (prefer removing both overrides if the driver does not
need to react to layout changes).
🧹 Nitpick comments (3)
src/MoonLight/Nodes/Drivers/D_MPU6050.h (2)
26-26: Dead code:gravityVector(public) anddevStatus(private member) are unused.
gravityVector(line 26) is declared but never written or read. The privategravity(line 135) is the actual gravity vector used inloop20ms. Either removegravityVectoror use it.- The member
devStatus(line 127) is shadowed by a local variable inonUpdate(line 56) and never referenced elsewhere.Remove dead declarations
Coord3D gyro; // in degrees (not radians) Coord3D accell; - VectorFloat gravityVector;private: MPU6050 mpu; // MPU control/status vars - uint8_t devStatus; // return status after each device operation (0 = success, !0 = error) uint8_t fifoBuffer[64]; // FIFO storage bufferAlso applies to: 127-127
46-81: No handling when node is turned off — MPU6050 DMP stays enabled.
onUpdateinitializes and enables the DMP whencontrol["on"] == true, but there's no branch forcontrol["on"] == false. When the user disables this driver node, the DMP keeps running andloop20msjust returns early due tomotionTrackingReadystayingtrue. Consider disabling DMP and resettingmotionTrackingReadyon turn-off to save power/bus bandwidth and zero outsharedData.gravity.Proposed off-handling
void onUpdate(const Char<20>& oldValue, const JsonObject& control) override { if (!control["on"].isNull()) { if (control["on"] == true) { if (initI2C()) { // ... existing init code ... } + } else { + // Disable DMP and reset state + if (motionTrackingReady) { + mpu.setDMPEnabled(false); + motionTrackingReady = false; + sharedData.gravity = Coord3D(); + } } } }platformio.ini (1)
144-145: MPU6050 dependency is added to[env](global) — consider scoping to[moonlight]if only MoonLight builds use it.Placing
ElectronicCats/MPU6050 @ 1.3.0under[env]lib_depsmeans it's pulled and linked for every board variant, even those without MPU6050 hardware or theFT_MOONLIGHTfeature flag. If this library is only used in MoonLight builds, moving it to the[moonlight]section (as done with FastLED and other specialized dependencies) would avoid unnecessary binary bloat for other firmware variants.Also note: line 144 uses 2 spaces for indentation while line 143 uses a tab — consider standardizing to match surrounding entries.
| if (nodeClass != nullptr) { | ||
| nodeClass->on = updatedItem.value.as<bool>(); // set nodeclass on/off | ||
| // EXT_LOGD(ML_TAG, " nodeclass 🔘:%d 🚥:%d 💎:%d", nodeClass->on, nodeClass->hasOnLayout(), nodeClass->hasModifier()); | ||
| nodeClass->onUpdate(updatedItem.oldValue, nodeState); // custom onUpdate for the node |
There was a problem hiding this comment.
Inconsistent mutex handling and argument semantics for onUpdate.
Two observations:
- No mutex: The controls-value path (line 282–285) holds
layerMutexaroundonUpdate, but this newon-toggle path does not. For MPU6050Driver the hardware init may not need it, but other nodes'onUpdatecould mutate shared layer state. Consider wrapping consistently:
+ xSemaphoreTake(*nodeClass->layerMutex, portMAX_DELAY);
nodeClass->onUpdate(updatedItem.oldValue, nodeState); // custom onUpdate for the node
+ xSemaphoreGive(*nodeClass->layerMutex);- Argument type mismatch: Here
nodeState(full node JSON withname,on,controls) is passed as thecontrolparameter, while at line 284 a single control object is passed. The MPU6050Driver relies on checkingcontrol["on"]to distinguish, but this convention is implicit and fragile — a future node'sonUpdatecould misinterpret the JSON shape.
🤖 Prompt for AI Agents
In `@src/MoonBase/NodeManager.h` at line 267, The call to nodeClass->onUpdate
currently passes the full nodeState without holding layerMutex and thus is
inconsistent with the controls-value path; wrap the on-toggle path's
nodeClass->onUpdate call with layerMutex (same scope used in the controls-value
path) and normalize the argument semantics by passing a consistent control
object (e.g., construct a single-control JSON containing only the on field or an
explicit {"on": ...} object) instead of the full nodeState so onUpdate
implementations always receive the same JSON shape.
Comment on lines
37
to
44
| bool initI2S() { | ||
| // tbd: set pins in ui!! | ||
| // allocatePin(21, "Pins", "I2S SDA"); | ||
| // allocatePin(22, "Pins", "I2S SCL"); | ||
| bool success = Wire.begin(5, 6); | ||
| EXT_LOGI(ML_TAG, "initI2S Wire begin %s", success ? "success" : "failure"); | ||
| return success; | ||
| } |
There was a problem hiding this comment.
Method is named initI2S but initializes I2C (Wire/Wire.begin).
I²S (Inter-IC Sound) and I²C (Inter-Integrated Circuit / Wire) are different protocols. This should be initI2C to avoid confusion.
Also, I2C pins 5 and 6 are hardcoded — acknowledged by the TODO, but worth noting this will fail on many boards.
Proposed rename
- bool initI2S() {
- // tbd: set pins in ui!!
- // allocatePin(21, "Pins", "I2S SDA");
- // allocatePin(22, "Pins", "I2S SCL");
+ bool initI2C() {
+ // tbd: set pins in ui!!
+ // allocatePin(21, "Pins", "I2C SDA");
+ // allocatePin(22, "Pins", "I2C SCL");
bool success = Wire.begin(5, 6);
- EXT_LOGI(ML_TAG, "initI2S Wire begin %s", success ? "success" : "failure");
+ EXT_LOGI(ML_TAG, "initI2C Wire begin %s", success ? "success" : "failure");
return success;
}📝 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.
Suggested change
| bool initI2S() { | |
| // tbd: set pins in ui!! | |
| // allocatePin(21, "Pins", "I2S SDA"); | |
| // allocatePin(22, "Pins", "I2S SCL"); | |
| bool success = Wire.begin(5, 6); | |
| EXT_LOGI(ML_TAG, "initI2S Wire begin %s", success ? "success" : "failure"); | |
| return success; | |
| } | |
| bool initI2C() { | |
| // tbd: set pins in ui!! | |
| // allocatePin(21, "Pins", "I2C SDA"); | |
| // allocatePin(22, "Pins", "I2C SCL"); | |
| bool success = Wire.begin(5, 6); | |
| EXT_LOGI(ML_TAG, "initI2C Wire begin %s", success ? "success" : "failure"); | |
| return success; | |
| } |
🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Drivers/D_MPU6050.h` around lines 37 - 44, The method
initI2S incorrectly initializes I2C (Wire.begin) so rename the function to
initI2C (and update all callers/usages) and update the log message (EXT_LOGI) to
reflect "initI2C" to avoid protocol confusion; while here also remove or replace
the hardcoded pins (Wire.begin(5, 6)) by using the existing pin allocation
mechanism or configurable pin variables (e.g., use allocatePin or pass sda/scl
parameters or read configured pins) so the function no longer hardcodes 5 and 6
and will work across boards.
| return success; | ||
| } | ||
|
|
||
| void onUpdate(const Char<20>& oldValue, const JsonObject& control) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing override keyword on onUpdate.
All other overridden methods in this class (setup, hasOnLayout, onLayout, loop20ms, destructor) use override, but onUpdate does not. Without override, the compiler won't catch signature drift if the base class changes.
Fix
- void onUpdate(const Char<20>& oldValue, const JsonObject& control) {
+ void onUpdate(const Char<20>& oldValue, const JsonObject& control) override {📝 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.
Suggested change
| void onUpdate(const Char<20>& oldValue, const JsonObject& control) { | |
| void onUpdate(const Char<20>& oldValue, const JsonObject& control) override { |
🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Drivers/D_MPU6050.h` at line 46, The onUpdate method
declaration lacks the override specifier which risks silent signature drift;
update the declaration of void onUpdate(const Char<20>& oldValue, const
JsonObject& control) in class D_MPU6050 to add the override keyword so it
matches the other overridden methods (e.g., setup, hasOnLayout, onLayout,
loop20ms) and lets the compiler validate the override.
Collaborator
Author
|
@coderabbitai, the mcu6050 is not working, I wired the scanner/scl/gnd/vcc correctly. got the following logging, what is wrong? [ 56027][D][NodeManager.h:262] onUpdate(): [💫] httpd (1524) MPU6050 driver ☸️ on: true (#2) ransmit_receive failed: [259] ESP_ERR_INVALID_STATE |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6271
The ProblemWhen The SolutionYou need to end the existing I2C bus before reinitializing it with your custom pins. Modify the bool initI2S() {
// tbd: set pins in ui!!
// allocatePin(21, "Pins", "I2S SDA");
// allocatePin(22, "Pins", "I2S SCL");
// End any existing I2C bus before reinitializing
Wire.end();
delay(10); // Small delay to ensure bus is fully released
bool success = Wire.begin(5, 6);
EXT_LOGI(ML_TAG, "initI2S Wire begin %s", success ? "success" : "failure");
return success;
}The Additional ChecksAlso verify:
Try the |
Collaborator
Author
|
@coderabbitai, regarding
Improved it a bit but still errors, please investigate: [ 69729][D][NodeManager.h:262] onUpdate(): [💫] httpd (1524) MPU6050 driver ☸️ on: true (#2) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5209 🌐 Web query: 💡 Result:
Sources: Citations:
The Real ProblemBased on the logs and research, the issue is likely that ESP_ERR_INVALID_STATE occurs when the I2C driver isn't properly installed or was deleted/deinitialized. When you call Solution: Remove Wire.end() and Add Proper DelayTry this updated bool initI2S() {
// tbd: set pins in ui!!
// allocatePin(21, "Pins", "I2S SDA");
// allocatePin(22, "Pins", "I2S SCL");
// Don't call Wire.end() - it deletes the driver and causes INVALID_STATE
bool success = Wire.begin(5, 6);
EXT_LOGI(ML_TAG, "initI2S Wire begin %s", success ? "success" : "failure");
if (success) {
delay(100); // Give I2C bus time to stabilize
}
return success;
}Verify Hardware with I2C ScannerBefore testing again, let's verify the MPU6050 is actually present and at the correct address. Add this temporary diagnostic function: void scanI2C() {
EXT_LOGI(ML_TAG, "Scanning I2C bus...");
byte count = 0;
for (byte i = 1; i < 127; i++) {
Wire.beginTransmission(i);
if (Wire.endTransmission() == 0) {
EXT_LOGI(ML_TAG, "Found I2C device at address 0x%02X", i);
count++;
}
}
EXT_LOGI(ML_TAG, "Found %d device(s)", count);
}Call it in if (initI2S()) {
scanI2C(); // Diagnostic - remove after testing
mpu.initialize();
// ... rest of codeHardware ChecklistThe MPU6050 should appear at 0x68 or 0x69 (depending on AD0 pin). Verify:
Let me know what addresses the scanner finds! |
Collaborator
Author
|
@coderabbitai, regarding
[ 18026][I][esp32-hal-i2c-ng.c:112] i2cInit(): Initializing I2C Master: num=0 sda=5 scl=6 freq=100000 |
|
Tip For best results, initiate chat on the files or code changes.
Critical Issue: GPIO Pin ChoiceESP32 allows any GPIO pin to be configured as I2C SDA and SCL, but the default I2C pins for ESP32 with Arduino IDE are GPIO 22 (SCL) and GPIO 21 (SDA). These are the most reliable pins for I2C communication. Immediate Solution: Use Standard I2C PinsChange your bool initI2C() {
// Use standard ESP32 I2C pins
// GPIO 21 = SDA, GPIO 22 = SCL
bool success = Wire.begin(21, 22);
EXT_LOGI(ML_TAG, "initI2C Wire begin %s", success ? "success" : "failure");
if (success) {
delay(100); // Give I2C bus time to stabilize
}
return success;
}Rewire your MPU6050:
Additional TroubleshootingIf the scanner still finds 0 devices after switching pins:
The fact that the scanner took 6+ seconds and then failed with Try the pin change first and let me know what the scanner finds! |
Collaborator
Author
|
@coderabbitai, Which has pin 5 and 6, please check their website and let me know if 5,6 is indeed right |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: For the Seeed Studio XIAO ESP32S3 / XIAO ESP32S3 Sense, the default I2C pins are:
Note: the Sense camera module uses a different I2C pair:
Sources: [1] [2] [3] [4]
The Seeed XIAO ESP32S3 uses GPIO 5 (SDA) and GPIO 6 (SCL) as the default I2C pins, so your code is using the right pins. Since the pins are correct but the scanner found 0 devices, the issue is likely one of the following: Hardware Issues to Check
Software Issue: Arduino-ESP32 Core BugThe Try adding a small delay after bool initI2C() {
bool success = Wire.begin(5, 6); // Correct pins for XIAO ESP32S3
EXT_LOGI(ML_TAG, "initI2C Wire begin %s", success ? "success" : "failure");
if (success) {
delay(200); // Longer delay for I2C bus stabilization
Wire.setClock(100000); // Explicitly set to 100kHz
}
return success;
}What Arduino-ESP32 core version are you using? (Check |
|
This is my mcu6050 board , do I need to short wire anything?
I am on arduino v3.3.5 |
|
Tip For best results, initiate chat on the files or code changes.
What I See in the ImageThe Comment image shows a blue GY-521 MPU6050 breakout board with:
The AD0 Pin - Important!Yes, you should connect the AD0 pin! Here's why: The AD0 pin controls the I2C address:
Right now, your AD0 pin appears to be floating (not connected), which can cause unreliable I2C communication. Solution: Connect the AD0 pin to GND for the standard 0x68 address. Arduino-ESP32 3.0.5 I2C IssuesYou're on Arduino-ESP32 3.0.5, which has known I2C bugs causing Updated Code to TryUpdate your bool initI2C() {
bool success = Wire.begin(5, 6); // XIAO ESP32S3 I2C pins
EXT_LOGI(ML_TAG, "initI2C Wire begin %s", success ? "success" : "failure");
if (success) {
Wire.setClock(50000); // Lower clock speed to 50kHz for stability
delay(200); // Longer stabilization delay
}
return success;
}
void onUpdate(const Char<20>& oldValue, const JsonObject& control) {
EXT_LOGI(ML_TAG, "onUpdate %s", oldValue.charArr);
if (control["n"].as<Char<20>>() == "on" && nodeState[control["n"].as<Char<20>>()].as<bool>()) {
if (initI2C()) {
scanI2C(); // Keep the scanner for now
// Explicitly set MPU6050 address to 0x68
mpu.setAddr(MPU6050_ADDRESS_AD0_LOW); // or just use default
mpu.initialize();
delay(100);
if (mpu.testConnection()) {
EXT_LOGI(ML_TAG, "MPU6050 connection successful at address 0x68");
// ... rest of DMP setup
} else {
EXT_LOGI(ML_TAG, "MPU6050 connection failed - check AD0 wiring");
}
}
}
}Wiring ChecklistXIAO ESP32S3 → MPU6050:
Next Steps
If it still doesn't work after grounding AD0, we may need to try Arduino-ESP32 v2.x or use a workaround for the 3.x I2C driver bug. Let me know what the scanner shows after connecting AD0! |
Change MPU6050 driver to IMU driver and add board control (MPU6050 and BMI160) I2S -< I2C, including safety delays - WIP add scanI2C
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/MoonBase/Nodes.h`:
- Around line 347-348: The field Coord3D gravity likely uses integer components
and will truncate the MPU6050 float gravity vector; update the type or handling:
replace Coord3D gravity with a float-based struct (e.g., VectorFloat or a new
Coord3DFloat) or store gravity as an array of floats and ensure any code using
gravity performs the proper float reads/conversions (see related D_IMU.h
handling), or if you must keep Coord3D, add clear documentation and explicit
scaling/quantization conversions where gravity is assigned so values in
[−1.0,1.0] are preserved correctly.
In `@src/MoonLight/Modules/ModuleDrivers.h`:
- Line 109: The build fails because the code calls getNameAndTags<IMUDriver>()
in addControlValue but the actual class in D_IMU.h is defined as MPU6050Driver;
update the template type references to the real class name (replace IMUDriver
with MPU6050Driver) wherever getNameAndTags<IMUDriver>() and related uses appear
(e.g., the addControlValue(control, getNameAndTags<...>()) calls around the IMU
driver registrations), or alternatively rename the class in D_IMU.h to
IMUDriver—ensure consistency between the class declaration (MPU6050Driver) and
all template/type usages (getNameAndTags and addControlValue).
In `@src/MoonLight/Nodes/Drivers/D_IMU.h`:
- Around line 41-56: In initI2C(), fix the comment typos (replace "I2S" with
"I2C"), remove the hardcoded Wire.begin(5, 6) and instead read user-configurable
I2C SDA/SCL pins (use the existing UI control pattern such as
addControl/allocatePin or the pin config variables you have) and pass those pin
values into Wire.begin, and correct the clock mismatch by either setting
Wire.setClock(100000) to match the "100kHz" comment or update the comment to
"50kHz" if 50000 is intended; ensure references to Wire.begin and Wire.setClock
and the initI2C() function are updated accordingly.
- Around line 71-112: onUpdate is performing long blocking initialization (delay
calls, mpu.dmpInitialize, CalibrateAccel, CalibrateGyro) on the HTTP/UI thread
and logging errors with EXT_LOGI; move the heavy work out of onUpdate into a
background task/thread or schedule a non-blocking state-machine so onUpdate only
enqueues/requests initialization and returns immediately, call a new InitIMUTask
(or similar) that runs initI2C(), scanI2C(), mpu.initialize(),
mpu.dmpInitialize(), CalibrateAccel(6), CalibrateGyro(6) and sets
motionTrackingReady when done; also change the two incorrect log calls that
currently use EXT_LOGI for failures (the mpu.dmpInitialize error and the
mpu.testConnection failure) to use EXT_LOGW or EXT_LOGE so errors are logged at
the proper severity.
- Around line 1-20: The class is named MPU6050Driver but other code expects
IMUDriver; rename the class MPU6050Driver to IMUDriver (and rename its
destructor ~MPU6050Driver to ~IMUDriver) so the symbol matches ModuleDrivers.h
references (IMUDriver), and update the file header `@file` from D_MPU6050.h to
D_IMU.h to reflect the real filename; alternatively you may add a type alias
(using IMUDriver = MPU6050Driver) but be sure the destructor/name usages match
across the module.
- Around line 126-153: The gravity VectorFloat components (gravity.x, gravity.y,
gravity.z) are being assigned to sharedData.gravity which is a Coord3D with int
fields, causing truncation; update the code so gravity is stored with proper
precision—either change the SharedData.gravity type from Coord3D (int) to a
float-based struct (e.g., VectorFloat/float fields) or scale the float values
before assignment (e.g., multiply gravity.x/y/z by a factor like 1000 and
convert to int) and document the unit change; locate assignments in the block
handling board == 0 (mpu.dmpGetGravity -> sharedData.gravity.x/y/z) and apply
the chosen fix consistently wherever sharedData.gravity is used.
🧹 Nitpick comments (2)
src/MoonLight/Nodes/Drivers/D_IMU.h (2)
86-86: LocaldevStatusshadows the private member on Line 162.Line 86 declares
uint8_t devStatusas a local variable, which shadows the privatedevStatusmember (Line 162). The private member is never used elsewhere. Either remove the private member or reuse it here by dropping the localuint8_tdeclaration.Proposed fix: remove unused private member
// MPU control/status vars - uint8_t devStatus; // return status after each device operation (0 = success, !0 = error) uint8_t fifoBuffer[64]; // FIFO storage buffer
156-156: Destructor doesn't clean up I2C or DMP state.When the driver node is removed, the destructor doesn't call
mpu.setDMPEnabled(false)orWire.end(). If the node is destroyed and recreated, this could leave the I2C bus in an inconsistent state — particularly relevant given the I2C initialization issues discussed in the PR comments.Proposed cleanup
- ~MPU6050Driver() override {}; // e.g. to free allocated memory + ~MPU6050Driver() override { + if (motionTrackingReady) { + mpu.setDMPEnabled(false); + } + }
Comment on lines
+347
to
+348
|
|
||
| Coord3D gravity; |
There was a problem hiding this comment.
Coord3D gravity may not be the right type for gravity data.
As noted in the D_IMU.h review, the MPU6050 gravity vector contains float values in the range [–1.0, 1.0]. If Coord3D uses integer fields (e.g., int16_t), this will silently truncate to 0/±1. Consider using a float-based struct (e.g., VectorFloat) or documenting the expected scale/encoding if you intend to pre-scale the values.
🤖 Prompt for AI Agents
In `@src/MoonBase/Nodes.h` around lines 347 - 348, The field Coord3D gravity
likely uses integer components and will truncate the MPU6050 float gravity
vector; update the type or handling: replace Coord3D gravity with a float-based
struct (e.g., VectorFloat or a new Coord3DFloat) or store gravity as an array of
floats and ensure any code using gravity performs the proper float
reads/conversions (see related D_IMU.h handling), or if you must keep Coord3D,
add clear documentation and explicit scaling/quantization conversions where
gravity is assigned so values in [−1.0,1.0] are preserved correctly.
src/MoonLight/Nodes/Drivers/D_IMU.h
Outdated
Comment on lines
41
to
56
| bool initI2C() { | ||
| // tbd: set pins in ui!! | ||
| // allocatePin(21, "Pins", "I2S SDA"); | ||
| // allocatePin(22, "Pins", "I2S SCL"); | ||
| Wire.end(); | ||
| delay(10); | ||
| bool success = Wire.begin(5, 6); | ||
| EXT_LOGI(ML_TAG, "initI2C Wire begin %s", success ? "success" : "failure"); | ||
|
|
||
| if (success) { | ||
| delay(200); // Give I2C bus time to stabilize | ||
| Wire.setClock(50000); // Explicitly set to 100kHz | ||
| } | ||
|
|
||
| return success; | ||
| } |
There was a problem hiding this comment.
Hardcoded I2C pins and comment/code mismatch on clock speed.
Three items here:
- Lines 43–44: Comments reference "I2S" — should be "I2C".
- Line 47:
Wire.begin(5, 6)hardcodes GPIO5/GPIO6 (XIAO ESP32S3). This will break on other boards. Consider making the pins configurable via UI controls (the commented-outaddControlapproach on lines 42–44 is the right direction). - Line 52: Comment says "Explicitly set to 100kHz" but the code sets
50000(50 kHz).
Fix the comment typos
- // tbd: set pins in ui!!
- // allocatePin(21, "Pins", "I2S SDA");
- // allocatePin(22, "Pins", "I2S SCL");
+ // tbd: set pins in ui!!
+ // allocatePin(21, "Pins", "I2C SDA");
+ // allocatePin(22, "Pins", "I2C SCL");- Wire.setClock(50000); // Explicitly set to 100kHz
+ Wire.setClock(50000); // Explicitly set to 50kHz (lower for reliability)🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Drivers/D_IMU.h` around lines 41 - 56, In initI2C(), fix
the comment typos (replace "I2S" with "I2C"), remove the hardcoded Wire.begin(5,
6) and instead read user-configurable I2C SDA/SCL pins (use the existing UI
control pattern such as addControl/allocatePin or the pin config variables you
have) and pass those pin values into Wire.begin, and correct the clock mismatch
by either setting Wire.setClock(100000) to match the "100kHz" comment or update
the comment to "50kHz" if 50000 is intended; ensure references to Wire.begin and
Wire.setClock and the initI2C() function are updated accordingly.
ModuleIO - Include Wire - Add controls i2cFreq and i2cBus - default assign sda to pin 21 and scl to pin 22 - readPins: add I2C pin assignment - add updateDevices: Scanning I2C bus and send frequency and addresses to UI IMU driver - remove initI2C and scan I2C - gravity xyz float to int correction - WIP
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/MoonBase/Modules/ModuleIO.h`:
- Line 222: The i2cFreq control created via addControl(controls, "i2cFreq", ...)
uses a max of 65534 which is below typical Wire.getClock() values (100000,
400000); update the max bound on the "i2cFreq" control in ModuleIO.h to a larger
value (e.g., 1000000 or UINT32_MAX) so real I2C clock frequencies aren't
clamped, ensuring the chosen max fits the addControl parameter type (int vs.
unsigned) and adjusting the type if necessary.
- Around line 928-929: Remove the sentinel fake I2C device being appended with
address 255: stop creating the JsonObject via
newState["i2cBus"].as<JsonArray>().add<JsonObject>() and assigning
i2cDevice["address"] = 255 so the i2cBus array only contains real scan results;
if a non-empty-array indicator is needed, handle that in the UI layer instead of
adding a sentinel entry (look for usages of i2cDevice/newState["i2cBus"] in
ModuleIO.h and related scan/serialize functions to ensure no other code depends
on the sentinel).
- Around line 819-826: readPins() can call Wire.begin() repeatedly without
cleanup causing "Bus already started in Master Mode" on ESP32; before calling
Wire.begin(pinI2CSDA, pinI2CSCL) ensure you call Wire.end() if Wire is already
initialized (check Wire.isEnabled() or track a local/field flag), then call
Wire.begin(...) and log via EXT_LOGI/EXT_LOGE as before; also re-enable the
stabilization delay and explicit clock set (uncomment the delay(200) and
Wire.setClock(...) lines and set the clock to the recommended value for
Arduino-ESP32 3.x) so initI2C sequence (pins pinI2CSDA/pinI2CSCL, Wire.begin/
Wire.end, delay, Wire.setClock) is safe when readPins() runs repeatedly.
- Around line 905-908: Loop1s() is calling updateDevices() every second which
triggers a full I2C bus scan and causes blocking/traffic; change this so
updateDevices() is not invoked from loop1s(); instead call updateDevices() once
after a successful Wire.begin() (e.g., in readPins() after initializing I2C when
pinI2CSCL and pinI2CSDA are valid) and cache the discovered devices, and provide
an explicit on-demand way to rescan if needed; update references to pinI2CSCL
and pinI2CSDA checks so the scan only happens once after initialization and
remove the per-second call in loop1s().
🧹 Nitpick comments (4)
src/MoonLight/Nodes/Drivers/D_IMU.h (4)
22-27:gravityVector(Line 26) is declared but never used.The public
VectorFloat gravityVectormember is never read or written in this file. The privategravitymember (Line 139) is what's actually used for DMP calculations. Either removegravityVectoror wire it up to serve its intended purpose.
55-55: LocaldevStatuson Line 55 shadows the private member on Line 131.The
uint8_t devStatusdeclared insideonUpdateshadows the class member of the same name, leaving the member uninitialized and unused. Remove the member declaration on Line 131 since the variable is only needed locally.Proposed fix
// MPU control/status vars - uint8_t devStatus; // return status after each device operation (0 = success, !0 = error) uint8_t fifoBuffer[64]; // FIFO storage bufferAlso applies to: 131-131
100-105:gyrovalues also suffer float→int truncation (fractional degrees lost).
ypr[i] * 180 / M_PIproduces floats (e.g., 45.73°) that truncate to integers when assigned toCoord3D(int fields). This loses sub-degree resolution. The gravity scaling viaINT16_MAX(Lines 103–105) addresses the equivalent issue for gravity — consider a similar scaling approach for gyro, or document that integer-degree precision is intentional.
96-121: Redundant re-parsing of quaternion, accel, and gravity (Lines 111–113) with only a comment as justification.The comment "needed to repeat the following 3 lines (yes if you look at the output: otherwise not 0)" suggests an empirical workaround. The DMP packet is deterministic — re-parsing the same
fifoBuffershould yield the same results. This likely masks an issue with FIFO alignment or a stale-buffer read. Consider investigating the root cause rather than doubling the parsing work every 20ms cycle.
src/MoonBase/Modules/ModuleIO.h
Outdated
Comment on lines
928
to
929
| JsonObject i2cDevice = newState["i2cBus"].as<JsonArray>().add<JsonObject>(); | ||
| i2cDevice["address"] = 255; |
There was a problem hiding this comment.
Sentinel entry with address 255 pollutes the device list.
A fake device at address 255 is appended after the real scan results. This is not a valid I2C address (7-bit range is 0–127) and will appear as a spurious device in the UI. If this is meant to ensure the i2cBus array is non-empty or signal end-of-list, consider handling that in the UI layer instead.
Proposed fix: remove the sentinel
EXT_LOGI(ML_TAG, "Found %d device(s)", count);
- JsonObject i2cDevice = newState["i2cBus"].as<JsonArray>().add<JsonObject>();
- i2cDevice["address"] = 255;📝 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.
Suggested change
| JsonObject i2cDevice = newState["i2cBus"].as<JsonArray>().add<JsonObject>(); | |
| i2cDevice["address"] = 255; | |
| // Lines 928-929 removed - sentinel entry deleted |
🤖 Prompt for AI Agents
In `@src/MoonBase/Modules/ModuleIO.h` around lines 928 - 929, Remove the sentinel
fake I2C device being appended with address 255: stop creating the JsonObject
via newState["i2cBus"].as<JsonArray>().add<JsonObject>() and assigning
i2cDevice["address"] = 255 so the i2cBus array only contains real scan results;
if a non-empty-array indicator is needed, handle that in the UI layer instead of
adding a sentinel entry (look for usages of i2cDevice/newState["i2cBus"] in
ModuleIO.h and related scan/serialize functions to ensure no other code depends
on the sentinel).
Module IO - setup, onUpdate: i2cFreq is modifyable (in kHz) - setBoardPresetDefaults: i2c default 21,22 on esp32d0 and 8, 9 on others - wip - readPins: reinit Wire after i2c pins changed, using freq, and updateDevices(not every s) IMU module - remove gravityVector
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 813-836: Reset pinI2CSDA and pinI2CSCL to UINT8_MAX before
scanning the _state.data["pins"] array so stale GPIO values don't persist when a
preset has no I2C pins; in the function/block that iterates over pins (the loop
referencing pinObject["usage"] and setting pinI2CSDA/pinI2CSCL), explicitly set
pinI2CSDA = UINT8_MAX and pinI2CSCL = UINT8_MAX immediately prior to the for
loop, then keep the existing conditional Wire.begin(...) logic that checks for
UINT8_MAX.
In `@src/MoonLight/Nodes/Drivers/D_IMU.h`:
- Around line 40-80: Replace the hardcoded i2cInited = true in onUpdate with a
real I2C readiness check: query the ModuleIO state (e.g. moduleIO->pinI2CSDA
and/or moduleIO->pinI2CSCL != UINT8_MAX or a Wire.begin() success flag) to
determine if I2C is initialized before proceeding with MPU6050 initialization;
update the i2cInited variable accordingly and short-circuit the MPU setup (and
log a warning) when the check fails (references: onUpdate, i2cInited,
moduleIO->pinI2CSDA, Wire.begin).
- Around line 90-117: The code calls mpu.dmpGetLinearAccel(&aaReal, &aa,
&gravity) using aa which is never populated; restore or add a call to
mpu.dmpGetAccel(&aa, fifoBuffer) after obtaining the FIFO packet/quaternion
(e.g., after mpu.dmpGetQuaternion(&q, fifoBuffer)) so aa is filled before
dmpGetLinearAccel, and remove/adjust the commented duplicate lines accordingly
to ensure accell.x/y/z come from a valid aaReal.
🧹 Nitpick comments (1)
src/MoonBase/Modules/ModuleIO.h (1)
839-840:pinI2CSDA/pinI2CSCLare in the public section — consider moving toprivate.These members are declared between the public
readPins()/updateDevices()methods and the explicitprivate:label on line 943. If external access from the IMU driver is needed (e.g., to verify pin config), keep them public intentionally; otherwise move them below line 943.
Collaborator
Author
|
@coderabbitai,
Also make a recommendation how to do the same with SPI (generic in Module IO, other modules can use it). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 61352 🌐 Web query: 💡 Result: On ESP32-family SoCs there aren’t “fixed” I2C pins in hardware—the I2C signals are routed through the GPIO matrix, so you can map SDA/SCL to many usable GPIOs (subject to each pin’s constraints). [7] What people call “default I2C pins” usually means the defaults chosen by a software platform/board variant (most commonly Arduino-ESP32 Arduino-ESP32 (
|
| SoC | Default SDA | Default SCL | Notes / source |
|---|---|---|---|
| ESP32 | GPIO21 | GPIO22 | Arduino-ESP32 docs (Generic ESP32). [1] |
| ESP32-S2 | GPIO8 | GPIO9 | Mentioned as coming from pins_arduino.h for ESP32-S2. [2] |
| ESP32-S3 | GPIO8 | GPIO9 | Shown from variants/esp32s3/pins_arduino.h. [3] |
| ESP32-C3 | GPIO8 | GPIO9 | Stated as default for “ESP32C3 Dev Module” in Arduino-ESP32. [4] |
| ESP32-C6 | GPIO23 | GPIO22 | Shown as variants/esp32c6/pins_arduino.h defaults; also notes LP I2C pins SDA1=GPIO6, SCL1=GPIO7. [5] |
| ESP32-P4 | (no single universal default) | (no single universal default) | Boards commonly pick something like SDA=GPIO7 / SCL=GPIO8 (example ESP32-P4 boards/docs). [6] |
Reminder (all chips above)
If you don’t want the defaults, set pins explicitly (Arduino: Wire.begin(sda, scl) / Wire.setPins(sda, scl); ESP-IDF: configure sda_io_num / scl_io_num). ESP-IDF’s I2C setup is explicitly pin-config driven. [8]
🌐 Web query:
BMI160 Arduino ESP32 library IMU accelerometer gyroscope
💡 Result:
Good Arduino/ESP32 libraries for the Bosch BMI160 (accel + gyro)
- hanyazou / BMI160-Arduino (API header:
BMI160Gen.h)
- Supports SPI and I2C; example shows
BMI160.begin(...SPI_MODE...)and...I2C_MODE, 0x68)and basicreadGyro()usage. [1] - I2C note: tie SDO/SA0 to GND for the default address (example uses 0x68). [1]
- DFRobot_BMI160 (easy I2C-focused library; includes step counter helpers)
- Provides
I2cInit()andgetSensorData()methods; documents 0x68 when SDIO is GND and 0x69 as the alternate address. [2] - README lists FireBeetle-ESP32 as “Work Well” (so it’s known to run on ESP32-class boards). [2]
- EmotiBit_BMI160 (Arduino Library Manager listing exists)
- Published as “EmotiBit BMI160”; supports BMI160 (+ BMM150) and is based on the CurieIMU-style API. [3]
Minimal wiring (I2C, typical ESP32)
- VCC → 3.3V, GND → GND, SDA/SCL → ESP32 SDA/SCL
- Set address with SDO/SA0: low → 0x68, high → 0x69 (per DFRobot docs). [2]
Minimal code (hanyazou / BMI160Gen.h, I2C)
`#include` <Wire.h>
`#include` <BMI160Gen.h>
const int i2c_addr = 0x68;
void setup() {
Serial.begin(115200);
Wire.begin(); // (On ESP32 you can also pass SDA,SCL pins if needed)
BMI160.begin(BMI160GenClass::I2C_MODE, i2c_addr); // shown in repo example [1]
}
void loop() {
int gx, gy, gz;
BMI160.readGyro(gx, gy, gz); // shown in repo example [1]
Serial.printf("g: %d %d %d\n", gx, gy, gz);
delay(200);
}Sources: [1] [2] [3]
🌐 Web query:
Arduino unified IMU library MPU6050 BMI160 LSM6DS3 multi-sensor support
💡 Result:
- Closest thing to a “unified” Arduino IMU API: Adafruit’s Unified Sensor abstraction (
Adafruit_Sensor) which standardizes reads intosensors_event_tacross many Adafruit drivers. (arduinolibraries.info) - MPU6050:
Adafruit_MPU6050uses Adafruit’s stack (BusIO + Unified Sensor) and exposes accel/gyro (and temp). (learn.adafruit.com) - LSM6DS3 family: Adafruit’s
Adafruit_LSM6DSlibrary supports multiple LSM6DS variants (including LSM6DS33 and others) and also uses Unified Sensor. (learn.adafruit.com) - BMI160: There isn’t a commonly-used Arduino library for BMI160 that plugs into Adafruit Unified Sensor the same way; typical options are vendor/community drivers like DFRobot_BMI160 or EmotiBit_BMI160, with their own APIs. (arduinolibraries.info)
Practical multi-sensor approach (MPU6050 + LSM6DS3 + BMI160):
- Use Adafruit Unified Sensor for MPU6050 + LSM6DS3*, so your code reads both via a similar
getEvent()pattern. (learn.adafruit.com) - Wrap your BMI160 driver in a small adapter that outputs the same fields (accel/gyro/temp) in a struct compatible with your unified path (or mimic Adafruit’s
sensors_event_t). (arduinolibraries.info)
If you tell me which BMI160 breakout you’re using (DFRobot? Bosch? other) and whether you need I2C or SPI, I can point you to the best-maintained driver + sketch an adapter interface.
Citations:
- 1: https://www.arduinolibraries.info/libraries/adafruit-unified-sensor?utm_source=openai
- 2: https://learn.adafruit.com/mpu6050-6-dof-accelerometer-and-gyro/arduino?utm_source=openai
- 3: https://learn.adafruit.com/lsm6dsox-and-ism330dhc-6-dof-imu/arduino?utm_source=openai
- 4: https://www.arduinolibraries.info/libraries/df-robot_bmi160?utm_source=openai
- 5: https://learn.adafruit.com/mpu6050-6-dof-accelerometer-and-gyro/arduino?utm_source=openai
- 6: https://www.arduinolibraries.info/libraries/df-robot_bmi160?utm_source=openai
@ewowi, I've completed a thorough analysis of the I2C centralization approach, researched ESP32 pin defaults, and explored BMI160 and generic IMU library options. Here's my comprehensive review:
✅ Architectural Review: I2C Centralization in ModuleIO.h
Is the centralized approach correct?
Yes, this is an excellent architectural decision! The centralization of I2C in ModuleIO.h provides:
- ✅ Single Source of Truth: One place manages Wire initialization, pin configuration, and bus state
- ✅ Prevents conflicts: Avoids multiple drivers calling Wire.begin() with different pins
- ✅ Runtime reconfiguration: Pin changes trigger automatic Wire reinitialization
- ✅ Bus discovery: Centralized
updateDevices()scans all I2C devices once - ✅ Frequency control: Single i2cFreq parameter affects all I2C devices
Current Implementation Quality
ModuleIO.h I2C Management:
- ✅ Pin reading from UI configuration (pin_I2C_SDA, pin_I2C_SCL)
- ✅ Wire.end() before reinit to clean up previous state
- ✅ Frequency configuration with Wire.setClock()
- ✅ Device scanning with updateDevices()
⚠️ Issue: i2cBus only shows addresses, not device names
D_IMU.h Integration:
- ✅ Removed I2C initialization logic (no longer calls Wire.begin)
- ✅ Relies on Module IO for bus setup
⚠️ Issue:bool i2cInited = true; // todo: check in moduleIO if successfull- hardcoded, should actually check ModuleIO state
📌 Recommendation 1: Default I2C Pins for All ESP32 Variants
Based on Arduino-ESP32 platform defaults, update the default pin configuration in ModuleIO.h:
// In setBoardPresetDefaults() for board_none (default case):
`#ifdef` CONFIG_IDF_TARGET_ESP32
pinAssigner.assignPin(21, pin_I2C_SDA); // ESP32 classic
pinAssigner.assignPin(22, pin_I2C_SCL);
`#elif` defined(CONFIG_IDF_TARGET_ESP32S2)
pinAssigner.assignPin(8, pin_I2C_SDA); // ESP32-S2
pinAssigner.assignPin(9, pin_I2C_SCL);
`#elif` defined(CONFIG_IDF_TARGET_ESP32S3)
pinAssigner.assignPin(8, pin_I2C_SDA); // ESP32-S3
pinAssigner.assignPin(9, pin_I2C_SCL);
`#elif` defined(CONFIG_IDF_TARGET_ESP32C3)
pinAssigner.assignPin(8, pin_I2C_SDA); // ESP32-C3
pinAssigner.assignPin(9, pin_I2C_SCL);
`#elif` defined(CONFIG_IDF_TARGET_ESP32C6)
pinAssigner.assignPin(23, pin_I2C_SDA); // ESP32-C6
pinAssigner.assignPin(22, pin_I2C_SCL);
`#elif` defined(CONFIG_IDF_TARGET_ESP32P4)
pinAssigner.assignPin(7, pin_I2C_SDA); // ESP32-P4 (common board default)
pinAssigner.assignPin(8, pin_I2C_SCL);
`#else`
pinAssigner.assignPin(21, pin_I2C_SDA); // Fallback
pinAssigner.assignPin(22, pin_I2C_SCL);
`#endif`Note: Your current code already handles ESP32 vs others, but could be more explicit for C6 and P4.
📌 Recommendation 2: Device Naming Strategy for i2cBus
Problem: i2cBus control only shows numeric addresses (e.g., 0x68), not device names.
Solution: Implement a device identification registry
Step 1: Add device name field to i2cBus control
In ModuleIO.h:
control = addControl(controls, "i2cBus", "rows");
control["crud"] = "r";
rows = control["n"].to<JsonArray>();
{
addControl(rows, "address", "number", 0, 255, true); // ro
addControl(rows, "name", "text", 0, 32, true); // ro - NEW
}Step 2: Create I2C device registry in ModuleIO
// In ModuleIO.h private section:
struct I2CDeviceInfo {
uint8_t address;
const char* name;
Node* owner; // Which node claimed this device
};
std::vector<I2CDeviceInfo> i2cDeviceRegistry;
public:
// Allow drivers to register their I2C devices
void registerI2CDevice(uint8_t address, const char* name, Node* owner) {
for (auto& dev : i2cDeviceRegistry) {
if (dev.address == address) {
dev.name = name;
dev.owner = owner;
return;
}
}
i2cDeviceRegistry.push_back({address, name, owner});
}Step 3: Update updateDevices() to use registry
void updateDevices() {
JsonDocument doc;
doc["i2cBus"].to<JsonArray>();
JsonObject newState = doc.as<JsonObject>();
EXT_LOGI(ML_TAG, "Scanning I2C bus...");
byte count = 0;
for (byte i = 1; i < 127; i++) {
Wire.beginTransmission(i);
if (Wire.endTransmission() == 0) {
JsonObject i2cDevice = newState["i2cBus"].as<JsonArray>().add<JsonObject>();
i2cDevice["address"] = i;
// Look up device name in registry
const char* deviceName = "Unknown";
for (const auto& dev : i2cDeviceRegistry) {
if (dev.address == i) {
deviceName = dev.name;
break;
}
}
i2cDevice["name"] = deviceName;
EXT_LOGI(ML_TAG, "Found I2C device at address 0x%02X: %s", i, deviceName);
count++;
}
}
EXT_LOGI(ML_TAG, "Found %d device(s)", count);
doc["i2cFreq"] = Wire.getClock() / 1000;
update(newState, ModuleState::update, _moduleName);
}Step 4: IMU driver registers itself
In D_IMU.h onUpdate():
if (control["on"] == true) {
bool i2cInited = true; // TODO: get from ModuleIO
if (i2cInited) {
if (board == 0) { // MPU6050
// Register this device with Module IO
ModuleIO* io = modules->getModuleOfClass<ModuleIO>();
if (io) {
io->registerI2CDevice(0x68, "MPU6050", this); // or 0x69 if AD0 high
}
mpu.initialize();
// ... rest of code
}
}
}Alternative Simpler Approach: Known I2C address database in ModuleIO:
const char* getDeviceNameByAddress(uint8_t addr) {
switch(addr) {
case 0x68: return "MPU6050/MPU9250"; // Could be either
case 0x69: return "MPU6050/MPU9250 (AD0=HIGH)";
case 0x76: return "BME280/BMP280";
case 0x77: return "BME280/BMP280 (ALT)";
case 0x3C: return "OLED SSD1306";
case 0x3D: return "OLED SSD1306 (ALT)";
case 0x40: return "PCA9685/INA219";
// ... add more as needed
default: return "Unknown";
}
}This is simpler but less accurate (address collisions between different chips). I recommend the registry approach for accuracy.
📌 Recommendation 3: BMI160 IMU Support
Library Recommendation
Use hanyazou/BMI160-Arduino (BMI160Gen.h API) - supports both I2C and SPI with simple API.
Why this library:
- ✅ I2C and SPI support
- ✅ I2C addresses: 0x68 (SDO=GND) or 0x69 (SDO=VCC)
- ✅ Similar API structure to MPU6050 libraries
- ✅ Actively maintained
Add to platformio.ini:
lib_deps =
ElectronicCats/MPU6050 @ 1.3.0 ; for MPU6050
hanyazou/BMI160-Arduino @ ^1.0.0 ; for BMI160Implementation in D_IMU.h
`#if` FT_MOONLIGHT
`#include` <MPU6050_6Axis_MotionApps20.h>
`#include` <BMI160Gen.h> // NEW
class IMUDriver : public Node {
// ... existing code ...
void onUpdate(const Char<20>& oldValue, const JsonObject& control) override {
if (!control["on"].isNull()) {
if (control["on"] == true) {
bool i2cInited = true; // TODO: check ModuleIO
if (i2cInited) {
if (board == 0) { // MPU6050
// Register with Module IO
ModuleIO* io = modules->getModuleOfClass<ModuleIO>();
if (io) io->registerI2CDevice(0x68, "MPU6050", this);
mpu.initialize();
// ... existing MPU6050 code ...
} else if (board == 1) { // BMI160 - NEW
// Register with Module IO
ModuleIO* io = modules->getModuleOfClass<ModuleIO>();
if (io) io->registerI2CDevice(0x68, "BMI160", this);
BMI160.begin(BMI160GenClass::I2C_MODE, 0x68);
if (BMI160.getDeviceID() == 0xD1) { // BMI160 device ID
EXT_LOGI(ML_TAG, "BMI160 connection successful");
motionTrackingReady = true;
} else {
EXT_LOGW(ML_TAG, "BMI160 connection failed");
}
}
}
}
}
}
void loop20ms() override {
if (!motionTrackingReady) return;
if (board == 0) { // MPU6050
// ... existing MPU6050 loop code ...
} else if (board == 1) { // BMI160 - NEW
int gx, gy, gz, ax, ay, az;
BMI160.readGyro(gx, gy, gz);
BMI160.readAccelerometer(ax, ay, az);
// Convert raw values to degrees (BMI160 gyro: 16.4 LSB/°/s at ±2000°/s range)
gyro.x = gx / 16.4f;
gyro.y = gy / 16.4f;
gyro.z = gz / 16.4f;
// Convert raw accel values (BMI160 accel: 16384 LSB/g at ±2g range)
accell.x = ax;
accell.y = ay;
accell.z = az;
// Calculate gravity vector from accelerometer
float norm = sqrt(ax*ax + ay*ay + az*az);
if (norm > 0) {
sharedData.gravity.x = (ax / norm) * INT16_MAX;
sharedData.gravity.y = (ay / norm) * INT16_MAX;
sharedData.gravity.z = (az / norm) * INT16_MAX;
}
}
}
private:
MPU6050 mpu;
// BMI160 is accessed via static BMI160 object from library
};
`#endif`Note: BMI160 uses I2C address 0x68 when SDO is grounded, same as MPU6050, so they can't coexist on the same bus without address changes.
📌 Recommendation 4: Generic IMU Library Strategy
Reality Check
Adafruit Unified Sensor is the closest to a unified Arduino IMU API, but BMI160 doesn't have Adafruit Unified Sensor support.
Recommended Approach: Internal Abstraction Layer
Don't try to use a single external library - instead, create your own thin abstraction:
// In D_IMU.h - add this interface
struct IMUSensorData {
Coord3D gyro; // degrees/sec
Coord3D accell; // raw or m/s²
Coord3D gravity; // normalized
bool valid;
};
class IMUDriver : public Node {
// ... existing code ...
private:
// Unified read method
IMUSensorData readSensor() {
IMUSensorData data = {};
if (board == 0) { // MPU6050
if (mpu.dmpGetCurrentFIFOPacket(fifoBuffer)) {
mpu.dmpGetQuaternion(&q, fifoBuffer);
mpu.dmpGetGravity(&gravity, &q);
mpu.dmpGetYawPitchRoll(ypr, &q, &gravity);
data.gyro.x = ypr[1] * 180 / M_PI;
data.gyro.y = ypr[0] * 180 / M_PI;
data.gyro.z = ypr[2] * 180 / M_PI;
mpu.dmpGetLinearAccel(&aaReal, &aa, &gravity);
data.accell.x = aaReal.x;
data.accell.y = aaReal.y;
data.accell.z = aaReal.z;
data.gravity.x = gravity.x * INT16_MAX;
data.gravity.y = gravity.y * INT16_MAX;
data.gravity.z = gravity.z * INT16_MAX;
data.valid = true;
}
} else if (board == 1) { // BMI160
int gx, gy, gz, ax, ay, az;
BMI160.readGyro(gx, gy, gz);
BMI160.readAccelerometer(ax, ay, az);
data.gyro.x = gx / 16.4f;
data.gyro.y = gy / 16.4f;
data.gyro.z = gz / 16.4f;
data.accell.x = ax;
data.accell.y = ay;
data.accell.z = az;
float norm = sqrt(ax*ax + ay*ay + az*az);
if (norm > 0) {
data.gravity.x = (ax / norm) * INT16_MAX;
data.gravity.y = (ay / norm) * INT16_MAX;
data.gravity.z = (az / norm) * INT16_MAX;
}
data.valid = true;
}
return data;
}
public:
void loop20ms() override {
if (!motionTrackingReady) return;
IMUSensorData data = readSensor();
if (data.valid) {
gyro = data.gyro;
accell = data.accell;
sharedData.gravity = data.gravity;
EXT_LOGD(ML_TAG, "Gyro: %f %f %f", gyro.x, gyro.y, gyro.z);
}
}
};Benefits:
- ✅ Single loop20ms() code path
- ✅ Easy to add new IMU chips (just add another
else ifin readSensor()) - ✅ No dependency on unified external library
- ✅ Keep using best-in-class chip-specific libraries
Future IMUs to add:
- LSM6DS3 / LSM6DSO (common on newer boards)
- ICM-20948 (9-axis with magnetometer)
- BNO055/BNO085 (fusion-ready IMUs)
📌 Recommendation 5: SPI Centralization (Similar to I2C)
Current State
- ✅ SPI pin types defined in ModuleIO (pin_SPI_SCK, pin_SPI_MISO, pin_SPI_MOSI)
⚠️ SPI.begin() called inEthernetSettingsService.cpp(not centralized)
Proposed Changes
1. Add SPI management to ModuleIO.h:
// In setupDefinition():
addControl(controls, "spiFreq", "number", 0, 80000, false, "kHz");
control = addControl(controls, "spiBus", "rows");
control["crud"] = "r";
rows = control["n"].to<JsonArray>();
{
addControl(rows, "cs", "number", 0, SOC_GPIO_PIN_COUNT-1, true); // ro
addControl(rows, "name", "text", 0, 32, true); // ro
}
// Private members:
uint8_t pinSPISCK = UINT8_MAX;
uint8_t pinSPIMISO = UINT8_MAX;
uint8_t pinSPIMOSI = UINT8_MAX;
std::vector<SPIDeviceInfo> spiDeviceRegistry;
struct SPIDeviceInfo {
uint8_t csPin;
const char* name;
Node* owner;
};
// Public methods:
void registerSPIDevice(uint8_t csPin, const char* name, Node* owner);
bool isSPIInitialized() const { return pinSPISCK != UINT8_MAX && pinSPIMISO != UINT8_MAX && pinSPIMOSI != UINT8_MAX; }2. Initialize SPI in readPins():
void readPins() {
// ... existing I2C code ...
// SPI pin detection
for (JsonObject pinObject : _state.data["pins"].as<JsonArray>()) {
uint8_t usage = pinObject["usage"];
if (usage == pin_SPI_SCK) {
pinSPISCK = pinObject["GPIO"];
EXT_LOGD(ML_TAG, "SPISCK found %d", pinSPISCK);
}
if (usage == pin_SPI_MISO) {
pinSPIMISO = pinObject["GPIO"];
EXT_LOGD(ML_TAG, "SPIMISO found %d", pinSPIMISO);
}
if (usage == pin_SPI_MOSI) {
pinSPIMOSI = pinObject["GPIO"];
EXT_LOGD(ML_TAG, "SPIMOSI found %d", pinSPIMOSI);
}
}
if (pinSPISCK != UINT8_MAX && pinSPIMISO != UINT8_MAX && pinSPIMOSI != UINT8_MAX) {
SPI.end(); // Clean up any previous SPI initialization
SPI.begin(pinSPISCK, pinSPIMISO, pinSPIMOSI);
uint16_t frequency = _state.data["spiFreq"];
if (frequency > 0) {
// SPI frequency is set per-transaction, not globally like I2C
EXT_LOGI(ML_TAG, "SPI bus initialized: sck:%d miso:%d mosi:%d", pinSPISCK, pinSPIMISO, pinSPIMOSI);
}
}
}3. Update EthernetSettingsService to use ModuleIO's SPI:
// In EthernetSettingsService.cpp, replace SPI.begin() with:
ModuleIO* io = /* get ModuleIO reference */;
if (io && io->isSPIInitialized()) {
// SPI already initialized by ModuleIO, just use it
ETH.begin(v_ETH_PHY_TYPE, v_ETH_PHY_ADDR, v_ETH_PHY_CS, v_ETH_PHY_IRQ, v_ETH_PHY_RST, SPI);
} else {
EXT_LOGW("Ethernet", "SPI not initialized by ModuleIO");
}Benefits:
- ✅ Consistent architecture (I2C and SPI both in ModuleIO)
- ✅ Single source of truth for bus configuration
- ✅ Easy to add SPI-based drivers (displays, SD cards, etc.)
- ✅ Prevents pin conflicts
📋 Summary of Follow-up Actions
High Priority:
- ✅ Fix i2cInited check in D_IMU.h - currently hardcoded to
true - ✅ Add device naming to i2cBus - implement registry approach
- ✅ Update ESP32-C6 and ESP32-P4 default pins - explicit per-variant defaults
- ✅ Add BMI160 support - use hanyazou/BMI160-Arduino library
Medium Priority:
- ✅ Create internal IMU abstraction - unified readSensor() method
- ✅ Centralize SPI in ModuleIO - mirror I2C architecture
- ✅ Document I2C/SPI device registration - for future driver developers
Low Priority:
- Add more IMU chips (LSM6DS3, ICM-20948, BNO055)
- Add SPI device scanning (enumerate CS pins with known devices)
- Add I2C pull-up resistor strength configuration
Would you like me to generate code for any specific recommendation, or shall I create a GitHub issue with this analysis as a follow-up task?
🧠 Learnings used
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.
Collaborator
Author
|
@coderabbitai, I also want to add my-sr602 pir and digital microphones like the ics43432/4 or inmp441 How to add these, generic solution like for i2c and spi needed ? And do i2s microphones interfere with i2s FastLED drivers ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 114151 🌐 Web query: 💡 Result: For INMP441 and ICS-43432/ICS43432 on ESP32 + Arduino, you typically don’t need a mic-specific library—just the Arduino-ESP32 I2S API (or the underlying ESP-IDF I2S driver). (docs.espressif.com) Recommended (current) option: Arduino-ESP32
|
| Concern | FastLED RMT + I2S Mic | FastLED Old I2S LCD + I2S Mic |
|---|---|---|
| Direct peripheral conflict | ✅ No conflict | ❌ Conflict! Don't use! |
| DMA bandwidth competition | ||
| Recommended? | ✅ Safe combination | ❌ Avoid - use RMT instead |
📋 Action Items Summary
For PIR Sensors:
- ✅ Add
pin_PIRusage type to ModuleIO.h - ✅ Create
D_PIR.hdriver (simple GPIO input reading) - ✅ Register in ModuleDrivers.h
For I2S Microphones:
- ✅ Centralize I2S bus management in ModuleIO.h (similar to I2C/SPI)
- ✅ Initialize I2S bus when pins are assigned
- ✅ Create
D_Microphone.hdriver - ✅ Add audio level to SharedData for effects to use
- ✅ Consider adding FFT analysis if you want frequency-reactive effects
Interference Mitigation:
- ✅ Verify you're using FastLED RMT mode (not I2S LCD mode)
- ✅ Allocate DMA buffers in internal RAM
- ✅ Minimize flash/WiFi activity during LED updates
- ✅ Consider dual-core task pinning for heavy loads
Would you like me to generate the complete implementation code for either the PIR driver or the I2S microphone driver?
🧠 Learnings used
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: 2026-02-05T13:20:45.988Z
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.
ModuleIO: add I2C pins for default boards per MCU type, reset pins before update IMU Driver: fix accell calculation, BMI160 code (commented ATM)
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/MoonBase/Modules/ModuleIO.h`:
- Around line 578-601: Remove the redundant initial I2C pin assignment block
that calls pinAssigner.assignPin(21, pin_I2C_SDA)/assignPin(22, pin_I2C_SCL)
(the block guarded by `#ifdef` CONFIG_IDF_TARGET_ESP32 at the top) because
setBoardPresetDefaults() already performs target-specific assignments; keep the
later granular conditional block (the `#ifdef` / `#elif` chain that includes
CONFIG_IDF_TARGET_ESP32, ESP32S2/S3/C3, ESP32C6, ESP32P4, and the fallback) and
delete the earlier duplicate assignments so pinAssigner.assignPin is only
invoked once per board preset.
- Around line 848-859: The i2cFreq control currently defaults to 0 and leads to
calling Wire.begin(pinI2CSDA, pinI2CSCL, 0) and showing "0 kHz" in the UI;
update the setupDefinition to provide a sensible default (e.g., 100) for i2cFreq
and add a guard in the code that reads _state.data["i2cFreq"] (the
readPins/initI2C area) to treat 0 as the default value (replace with 100 or
another chosen default) before multiplying by 1000 and passing to
Wire.begin(pinI2CSDA, pinI2CSCL, frequency * 1000); ensure any logged frequency
uses the adjusted value so the UI shows a nonzero kHz and maintain behavior that
falls back to a safe clock when frequency is absent or zero.
In `@src/MoonLight/Nodes/Drivers/D_IMU.h`:
- Around line 29-35: The board selection control (added in setup via
addControl(board,...)) can flip at runtime without clearing state, so update
handling must reset motion-tracking state; add a guard in onUpdate (or a change
callback for the "board" control) that detects when board changes and sets
motionTrackingReady = false and disables/re-initializes motion tracking (e.g.,
set isEnabled = false or call the MPU6050 teardown/init sequence) so loop20ms's
BMI160 branch cannot run while MPU6050 state remains valid; reference
functions/vars: setup, addControl(board,...), board, onUpdate, loop20ms,
motionTrackingReady, isEnabled (or the MPU init/teardown helpers) to locate
where to implement this reset.
- Around line 38-88: Add a cleanup path for when the driver is turned off by
modifying onUpdate in D_IMU: when control["on"] == false (or control["on"] is
present and false) call the MPU shutdown steps—disable the DMP via
mpu.setDMPEnabled(false) (or equivalent), clear any interrupt/status flags, stop
any periodic reads, and set motionTrackingReady = false; also ensure any
board==1 BMI160 cleanup is mirrored. Then implement the same teardown in the
D_IMU destructor (~D_IMU) to disable DMP/interrupts and reset
motionTrackingReady so loop20ms stops reading after node removal. Ensure you
reference the existing symbols onUpdate, motionTrackingReady, mpu.setDMPEnabled
(and ~D_IMU) when making changes.
🧹 Nitpick comments (2)
src/MoonLight/Nodes/Drivers/D_IMU.h (1)
109-118: Stale comments around the accelerometer read — clean up for clarity.
- Line 109: "display real acceleration, adjusted to remove gravity" is an orphaned comment (no display code follows it).
- Line 113: "needed to repeat the following 3 lines" is misleading since only
dmpGetAccel(line 115) is active; the other two remain commented out.Consider removing these leftover comments to avoid confusing future readers about why certain calls are or aren't needed.
src/MoonBase/Modules/ModuleIO.h (1)
862-863: I2C pin members are public — verify this is intentional.
pinI2CSDAandpinI2CSCLare declared in thepublicsection, presumably soD_IMU.hand other drivers can query them. If external read access is the intent, this is fine; consider adding a brief comment to document the intended usage.
Collaborator
Author
|
@coderabbitai, something went wrong in saving files (delayedWrite?) between commit 03509aa and 45c03a5 in which I switched from FsPersistence to SharedFsPersistence. In the logging I see for both commits when I save: [ 32284][D][SharedFSPersistence.h:165] writeToFSDelayed(): [🐼] calling 6 writeFuncs from delayedWrites [ 32338][D][FSPersistence.h:121] writeToFSDelayed(): [🐼] calling 6 writeFuncs from delayedWrites If I update the effect from Game of Life to VU meter and save and check the files in the file system I see in both cases : {"name":"VU Meter ⏹️ ♫💫📺","on":true,"controls":[{"name":"speed","value":255,"type":"slider","default":255,"p":1207978129,"valid":true,"size":8}, But when I restart the device in case of the newer commit in which I implemented SharedFsPersistence (45c03a5) I see after restart the old Game of life is restored. So it looks like even the new effect is saved, somewhere along the way another save to the filesystem is done restoring old? (As during reboot, the file system contents is used to build up the state. What is the problem? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20087 🏁 Script executed: Length of output: 28912 🏁 Script executed: Length of output: 35050
Root CauseThe SharedFSPersistence change altered the order of operations during initialization, causing update handlers to fire during the initial state setup, which writes stale/default data to the delayed writes queue before the correct data is loaded from the filesystem. What Happens in the Broken Version (45c03a5):What Happened in the Working Version (03509aa):The FixYou have two options: Option 1: Disable Handlers During Initial Read (Recommended)Modify void begin() {
// Disable all update handlers before reading
for (auto& pair : _modules) {
disableUpdateHandler(pair.first);
}
// Read initial state from filesystem
for (auto& pair : _modules) {
readFromFS(pair.first);
}
// Re-enable update handlers after all reads complete
for (auto& pair : _modules) {
enableUpdateHandler(pair.first);
}
}Option 2: Register Handlers After All Reads (Alternative)Change the initialization sequence in // Register modules WITHOUT handlers first
for (Module* module : modules) {
sharedHttpEndpoint->registerModule(module);
sharedWebSocketServer->registerModule(module);
sharedEventEndpoint->registerModule(module);
sharedFsPersistence->registerModule(module, false); // ← false: no delayed writing yet
}
// ... module begin() calls ...
// Read from filesystem
sharedFsPersistence->begin();
// NOW enable delayed writing and handlers
for (Module* module : modules) {
auto it = sharedFsPersistence->_modules.find(module->_moduleName);
if (it != sharedFsPersistence->_modules.end()) {
it->second.delayedWriting = true;
sharedFsPersistence->enableUpdateHandler(module->_moduleName);
}
}But this requires making Why This is SubtleThe bug is hard to spot because:
Recommendation: Implement Option 1 by modifying Would you like me to generate the complete fixed 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
I want a solution which is as close as possible to (not shared) FSPersistence as code and functionality should not diverge. Is this the case with option 1, in other words , is this realigning with how FSPersistence works ? |
|
Tip For best results, initiate chat on the files or code changes.
How FSPersistence Works (Original)Looking at the code flow: // In Module::begin()
void begin() {
_fsPersistence = std::make_unique<FSPersistence<ModuleState>>(this, _moduleName);
// ↑ FSPersistence constructor runs here:
// 1. Register update handler
// 2. readFromFS() immediately ← File data loaded FIRST
// 3. Uses updateWithoutPropagation (no handler trigger)
setupData(); // ← Runs AFTER file is already loaded
// Default values are compared against file data, so only actual changes trigger handlers
}Timeline: Register handler → Read file → Setup defaults How SharedFSPersistence Currently Works (Broken)// In main.cpp
sharedFsPersistence->registerModule(module, true);
// ↑ If delayedWriting=true, immediately calls enableUpdateHandler()
// Later...
module->begin();
setupData(); // ← Triggers handlers with DEFAULTS before file is read! ❌
// Even later...
sharedFsPersistence->begin();
readFromFS(); // ← Reads file data, but too late - defaults already queuedTimeline: Register handler → Setup defaults (triggers handler) → Read file The Right Fix (Matching FSPersistence)Option 1 doesn't perfectly match FSPersistence because it disables/re-enables handlers, which FSPersistence never does. The correct fix is to delay handler enabling until after files are read, matching the FSPersistence order: Modified SharedFSPersistence::registerModule()void registerModule(Module* module, bool delayedWriting) {
const char* moduleName = module->_moduleName;
if (_modules.find(moduleName) != _modules.end()) {
return;
}
ModuleInfo moduleInfo;
moduleInfo.service = module;
moduleInfo.delayedWriting = delayedWriting;
// Don't enable handler here - remember the setting for later
_modules[moduleName] = moduleInfo;
EXT_LOGD(ML_TAG, "Module %s registered (delayed writing: %s)",
moduleName, delayedWriting ? "enabled" : "disabled");
}Modified SharedFSPersistence::begin()void begin() {
EXT_LOGI(ML_TAG, "SharedFSPersistence begin - reading state from filesystem");
// FIRST: Read all files before any handlers are active
for (auto& pair : _modules) {
readFromFS(pair.first);
}
// SECOND: Now enable handlers for modules that requested delayed writing
for (auto& pair : _modules) {
if (pair.second.delayedWriting) {
enableUpdateHandler(pair.first);
EXT_LOGD(ML_TAG, "Enabled update handler for %s after file read", pair.first);
}
}
EXT_LOGI(ML_TAG, "SharedFSPersistence initialization complete");
}Why This Matches FSPersistence
Result: Both ensure file data is loaded before handlers can queue writes. ImplementationThe fix is actually simpler than Option 1 - just move the Would you like me to generate the complete updated 🐰✨ 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes.
✅ Analysis Confirms Platform BugYour observation is correct:
The checkReOrderSwap activity (163502-171036ms) is normal:
🎯 ConclusionThis is definitively a platform regression bug in 55.03.37 that doesn't exist in 55.03.35. The crash is in the bootloader's WiFi cleanup code during ✅ Next Steps
The issue template I provided earlier is ready to copy-paste to: Your application code is working correctly - the ESP32-P4 support in 55.03.37 has a critical bootloader bug that causes crashes during reboot after filesystem operations. 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, see also in the logging the repeatingly change in reorder swap in the devices module. Can you find the root cause and a solution?: [242884][W][Module.cpp:250] update(): [💫] ESP32 SvelteKit (2616) checkReOrderSwap changed, compareRecursive also changed? devices |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11778
🐛 Root Cause: Unstable Sort ComparisonThe infinite flip-flop between positions 14 and 15 is caused by an unstable sort in std::sort(devicesVector.begin(), devicesVector.end(),
[](JsonObject a, JsonObject b) { return a["name"] < b["name"]; });The Problem
✅ Solution: Stable Comparison with Tie-BreakerReplace the unstable comparison with a stable one that uses IP address as a tie-breaker: // In ModuleDevices.h line 190, replace:
std::sort(devicesVector.begin(), devicesVector.end(),
[](JsonObject a, JsonObject b) { return a["name"] < b["name"]; });
// With:
std::sort(devicesVector.begin(), devicesVector.end(),
[](JsonObject a, JsonObject b) {
// Primary sort: by name
int nameCompare = strcmp(
a["name"].as<const char*>(),
b["name"].as<const char*>()
);
if (nameCompare != 0) return nameCompare < 0;
// Tie-breaker: by IP address (ensures stable sort)
return strcmp(
a["ip"].as<const char*>(),
b["ip"].as<const char*>()
) < 0;
}
);Alternative Solution: Use
|
pio.ini: latest FastLED backend ======= Devices: - sort on IP as tie breaker Module IO: - i2cFreq default 100 - add state.data["I2CReady"] - SharedFSPersistence: writeToFSDelayed: cache sharedDelayedWrites IMU driver: - add initBoard and stopBoard and onUpdate - use ModuleIO.state.data.I2CReady - stopBoard at destructor - Particles effect: gravityType (none, random, gyro)
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/MoonBase/Modules/ModuleIO.h`:
- Around line 641-642: The multiplication for setting I2C clock overflows
because updatedItem.value.as<uint16_t>() * 1000 is done in 16-bit arithmetic;
modify the Wire.setClock call (the i2cFreq branch that checks updatedItem.name
== "i2cFreq") to cast the value to a wider type (e.g., uint32_t) before
multiplying so the multiplication is performed in 32-bit arithmetic and passes
the correct clock value to Wire.setClock.
- Around line 842-855: The else after Wire.begin(...) is missing braces so
EXT_LOGE(ML_TAG, "initI2C Wire failed") executes unconditionally and
_state.data["I2CReady"] is not properly set to false on failure; add braces
around the else block to include both setting _state.data["I2CReady"] = false
and the EXT_LOGE call. Also prevent uint16_t overflow when computing frequency *
1000 by promoting frequency to a wider type (e.g., uint32_t or cast before
multiplication) when passing to Wire.begin(pinI2CSDA, pinI2CSCL, frequency *
1000) so values ≥66 won't wrap.
In `@src/MoonLight/Nodes/Drivers/D_IMU.h`:
- Around line 37-90: The calibration calls
mpu.CalibrateAccel(6)/mpu.CalibrateGyro(6) inside the moduleIO->read() lambda
which can hold the module IO mutex for seconds; change initBoard() so the lambda
only reads state.data["I2CReady"] and, when ready and board==0, sets a
short-lived flag (e.g., needMPUInit) or posts an async task, then returns
immediately; perform the heavy work (mpu.initialize(), mpu.testConnection(),
mpu.dmpInitialize(), mpu.CalibrateAccel, mpu.CalibrateGyro, mpu.setDMPEnabled
and setting motionTrackingReady) outside the moduleIO->read() callback (either
directly after the read returns if synchronous or in a background thread/task)
so moduleIO->_accessMutex is not held during long I2C operations.
- Around line 102-114: The handler in onUpdate runs stopBoard() and initBoard()
unconditionally when control["name"] == "board"; guard that branch so it only
restarts hardware if the node is currently enabled—check the node's on state
(e.g., using the on member or this->on) and wrap the stopBoard()/initBoard()
calls in if (on) { stopBoard(); initBoard(); } so you don't initialize the MPU
when the node is off.
🧹 Nitpick comments (6)
src/MoonBase/Modules/ModuleDevices.h (1)
190-199: Good fix for the sort instability — minor null-safety note on the IP tie-breaker.The IP tie-breaker resolves the flip-flop issue nicely. One small concern:
as<const char*>()returnsnullptrwhen the key is missing or not a string, andstrcmp(nullptr, …)is undefined behavior. This is unlikely given line 174 always sets"ip", but a defensive guard is cheap:Proposed defensive fix
- return strcmp(a["ip"].as<const char*>(), b["ip"].as<const char*>()) < 0; + const char* ipA = a["ip"].as<const char*>(); + const char* ipB = b["ip"].as<const char*>(); + return strcmp(ipA ? ipA : "", ipB ? ipB : "") < 0;Also, consider removing the commented-out
strcmp/nameComparelines (193-194) to keep the code clean.src/MoonBase/SharedFSPersistence.h (3)
21-21: Global mutablesharedDelayedWriteshas no synchronization.This
inlineglobal vector is mutated fromwriteToFS(push_back) and consumed fromwriteToFSDelayed(move + iterate). If these can execute on different tasks/cores or re-entrantly (e.g., the cancel path inwriteToFSDelayedcallsreadFromFS→module->update→ handler →writeToFS→ push_back ontosharedDelayedWrites), you risk a data race.The
std::moveon line 175 mitigates re-entrancy during the iteration loop, but push_back from another task whilewriteToFSDelayedis between lines 175–179 is still unprotected.Worth confirming that all callers run on the same task, or consider guarding with a mutex.
35-51: Map keyed on rawconst char*— ensure pointers remain valid for the map's lifetime.
_modulesstoresmodule->_moduleNameas the key by pointer value. TheCStrComparatorcorrectly compares by string content, but the pointer itself must remain valid as long as the entry exists. If_moduleNameis a string literal or a stable member (common for module names), this is fine. If it could ever point to freed/moved memory, lookups will dereference a dangling pointer.Consider using
std::stringas the key type if module names aren't guaranteed to be string literals, or add a comment documenting the lifetime requirement.
118-140: Delayed write dedup flaghasDelayedWriteis never reset if the queue is cleared externally.
hasDelayedWriteis set totrueon line 139 and only reset tofalseinside the lambda on line 136. IfsharedDelayedWriteswere ever cleared without invoking the lambdas (e.g., a future code path or error recovery), the flag staystrueand that module can never queue another delayed write.This is fine today given the current call patterns, but it's fragile. Consider resetting
hasDelayedWritefor all modules inwriteToFSDelayedafter processing, or documenting the invariant that every queued lambda must be invoked.src/MoonBase/Modules/ModuleIO.h (1)
858-859:pinI2CSDA/pinI2CSCLare declared in thepublicsection but are internal state.These tracking variables are implementation details of pin management and should be in the
privatesection alongsidecurrent_board_id.src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
1254-1262: Axis swaps for gyro gravity look intentional but warrant a brief comment.The mapping
gravity[0] = -sharedData.gravity.x,gravity[1] = sharedData.gravity.z,gravity[2] = -sharedData.gravity.ytransforms from IMU orientation to display orientation. A short inline comment explaining the coordinate convention (e.g., "IMU Z-up → display Y-up") would help future maintainers understand the swap rationale.
Collaborator
Author
|
@coderabbitai, regarding
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8687
✅ Spinlock Solution (4 bytes overhead)In // ADDED: Global delayed writes queue (matches templated version)
inline portMUX_TYPE sharedDelayedWritesMux = portMUX_INITIALIZER_UNLOCKED;
inline std::vector<std::function<void(char)>> sharedDelayedWrites;Protect if (info.delayedWriting) {
if (!info.hasDelayedWrite) {
ESP_LOGD(SVK_TAG, "delayedWrites: Add %s", info.filePath.c_str());
portENTER_CRITICAL(&sharedDelayedWritesMux);
sharedDelayedWrites.push_back([this, module = info.module](char writeOrCancel) {
auto it = _modules.find(module->_moduleName);
if (it == _modules.end()) return;
ESP_LOGD(SVK_TAG, "delayedWrites: %c %s", writeOrCancel, it->second.filePath.c_str());
if (writeOrCancel == 'W') {
this->writeToFSNow(module->_moduleName);
} else {
// Cancel: read old state back from FS
this->readFromFS(module->_moduleName);
// Update UI with restored state
it->second.module->update([](ModuleState& state) { return StateUpdateResult::CHANGED; }, SVK_TAG);
}
it->second.hasDelayedWrite = false;
});
info.hasDelayedWrite = true;
portEXIT_CRITICAL(&sharedDelayedWritesMux);
}
}Protect static void writeToFSDelayed(char writeOrCancel) {
// Move pending writes under lock
std::vector<std::function<void(char)>> pending;
portENTER_CRITICAL(&sharedDelayedWritesMux);
pending = std::move(sharedDelayedWrites);
sharedDelayedWrites.clear(); // leave in valid-but-empty state
portEXIT_CRITICAL(&sharedDelayedWritesMux);
ESP_LOGD(SVK_TAG, "calling %u writeFuncs from delayedWrites", pending.size());
// Process outside of lock
for (auto& writeFunc : pending) {
writeFunc(writeOrCancel);
}
}📊 Comparison
Verdict: 🎯 Why This Works
This is the leanest thread-safe solution possible on ESP32! 🐰✨ 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
There exists double entries in the devices module, same name, same ip, can you check the devices module and avoid double entries to be added |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20178
🐛 The ProblemIn for (JsonObject dev : devices) {
if (dev["name"] == message.name.c_str()) { // ← Only checks name!
device = dev;
newDevice = false;
break;
}
}Then in the sort phase (lines 183-207), there's NO deduplication: if (newDevice) { // sort devices in vector and add to a new document
// ...
for (JsonObject device : devicesVector) {
doc2["devices"].add(device); // ← Adds ALL devices, including duplicates!
}
}How duplicates accumulate:
✅ The Fix: Deduplicate During SortReplace lines 183-207 with this: if (newDevice) { // sort devices in vector and add to a new document and update
JsonDocument doc2;
// Build deduplication map: key = "name|ip", value = device
std::map<String, JsonObject> uniqueDevices;
for (JsonObject dev : devices) {
if (time(nullptr) - dev["lastSync"].as<time_t>() < 86400) { // max 1 day
String key = String(dev["name"].as<const char*>()) + "|" + String(dev["ip"].as<const char*>());
// Only keep the most recent entry for each name+ip combination
if (uniqueDevices.find(key) == uniqueDevices.end() ||
dev["lastSync"].as<time_t>() > uniqueDevices[key]["lastSync"].as<time_t>()) {
uniqueDevices[key] = dev;
}
}
}
// Extract unique devices into vector for sorting
std::vector<JsonObject> devicesVector;
for (auto& pair : uniqueDevices) {
devicesVector.push_back(pair.second);
}
std::sort(devicesVector.begin(), devicesVector.end(), [](JsonObject a, JsonObject b) {
// Primary sort: by name
if (a["name"] != b["name"]) return a["name"] < b["name"];
// Tie-breaker: by IP address (ensures stable sort)
return strcmp(a["ip"].as<const char*>(), b["ip"].as<const char*>()) < 0;
});
doc2["devices"].to<JsonArray>();
for (JsonObject device : devicesVector) {
doc2["devices"].add(device);
}
JsonObject newState = doc2.as<JsonObject>();
update(newState, ModuleState::update, _moduleName);
}🎯 What This Fixes
📋 Additional RecommendationAlso improve the device lookup to check both name AND IP: // Replace lines 156-166 with:
JsonObject device = JsonObject();
bool newDevice = true;
for (JsonObject dev : devices) {
if (dev["name"] == message.name.c_str() && dev["ip"] == ip.toString()) { // Check name AND IP
device = dev;
newDevice = false;
break;
// EXT_LOGD(ML_TAG, "updated ...%d %s", ip[3], name);
}
}This prevents duplicates from being added in the first place if a device changes its name but keeps the same IP (or vice versa). Would you like me to generate the complete patched 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
Can this be done simpler? we now have a map and a vector as local copies, can only one of the constructs be used? |
|
Tip For best results, initiate chat on the files or code changes.
✅ Simplified Version: Map OnlyThe key insight: Replace lines 183-207 with this simplified version: if (newDevice) { // sort devices and deduplicate
JsonDocument doc2;
// Build deduplication map: key = "name|ip", value = device
// std::map automatically keeps entries sorted by key (name|ip)
std::map<String, JsonObject> uniqueDevices;
for (JsonObject dev : devices) {
if (time(nullptr) - dev["lastSync"].as<time_t>() < 86400) { // max 1 day
String key = String(dev["name"].as<const char*>()) + "|" + String(dev["ip"].as<const char*>());
// Only keep the most recent entry for each name+ip combination
if (uniqueDevices.find(key) == uniqueDevices.end() ||
dev["lastSync"].as<time_t>() > uniqueDevices[key]["lastSync"].as<time_t>()) {
uniqueDevices[key] = dev;
}
}
}
// Map is already sorted by key (name|ip), just iterate and add
doc2["devices"].to<JsonArray>();
for (auto& pair : uniqueDevices) {
doc2["devices"].add(pair.second);
}
JsonObject newState = doc2.as<JsonObject>();
update(newState, ModuleState::update, _moduleName);
}🎯 Why This WorksLexicographic string comparison automatically gives correct ordering: Benefits:
Perfect for ESP32-D0's tight memory constraints! 🐰✨ 🧠 Learnings used |
pio.ini: - pioarduino back from 55.03.37 to 55.03.35 (p4 nano reboot crash) - FastLED: back from 02-14 to 02-12 (c3 compile error) - issues logged backend ======= - (Devices: use map, deduplicate list) - Module IO: Wire.setClock uint32_t, private variables - Shared FS Persistence: add sharedDelayedWrites mutex - IMU driver: add requestInitBoard to get it out of onUpdate
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MoonBase/SharedFSPersistence.h (2)
123-142:⚠️ Potential issue | 🟠 MajorHeap allocation inside
portENTER_CRITICALspinlock is unsafe.
portENTER_CRITICALdisables interrupts on the current core.sharedDelayedWrites.push_back(...)may trigger astd::vectorreallocation (heap allocation), and the heap allocator itself uses internal locks. Allocating memory with interrupts disabled can deadlock or cause a panic on ESP-IDF.Use a FreeRTOS mutex (
SemaphoreHandle_twithxSemaphoreTake/xSemaphoreGive) instead, which allows preemption and safe heap operations:Proposed fix: replace spinlock with FreeRTOS mutex
Replace the global spinlock (line 22) with a FreeRTOS mutex:
-inline portMUX_TYPE sharedDelayedWritesMux = portMUX_INITIALIZER_UNLOCKED; +inline SemaphoreHandle_t sharedDelayedWritesMux = xSemaphoreCreateMutex();Then in
writeToFSandwriteToFSDelayed, replace critical sections:- portENTER_CRITICAL(&sharedDelayedWritesMux); + xSemaphoreTake(sharedDelayedWritesMux, portMAX_DELAY); sharedDelayedWrites.push_back([this, module = info.module](char writeOrCancel) { ... }); info.hasDelayedWrite = true; - portEXIT_CRITICAL(&sharedDelayedWritesMux); + xSemaphoreGive(sharedDelayedWritesMux);Note:
xSemaphoreCreateMutex()at globalinlinescope won't work before FreeRTOS scheduler starts. Initialize it in a function (e.g., a static helper withstd::call_onceor inSharedFSPersistenceconstructor).
36-52:⚠️ Potential issue | 🟡 MinorMap keyed on
const char*— document assumptions about_moduleNamelifetime.
_modulesuses rawconst char*as keys viaCStrComparator. This is safe only ifmodule->_moduleNamealways points to long-lived storage (string literals, static strings, or memory never freed). While the current code appears to work (likely because modules are rarely deleted and_moduleNamepoints to stable strings), this design is fragile. Consider documenting the lifetime guarantee or refactoring to useModule*as the key instead.
🤖 Fix all issues with AI agents
In `@src/MoonBase/Modules/ModuleIO.h`:
- Around line 932-955: updateDevices() currently always writes the runtime I2C
clock from Wire.getClock() into doc["i2cFreq"], which can overwrite the
user-configured frequency; change the behavior to only update doc["i2cFreq"]
when the measured Wire.getClock()/1000 differs from the configured value by a
meaningful tolerance (e.g., a few kHz) or omit writing it entirely and leave the
user-configured value as source of truth — locate the doc["i2cFreq"] assignment
and the update(newState, ModuleState::update, _moduleName) call and modify the
logic to compare Wire.getClock()/1000 against the existing configured value
before setting it (or skip setting), ensuring any update only happens when the
difference exceeds the chosen tolerance.
- Around line 844-845: The Wire.end() call is causing intermittent
ESP_ERR_INVALID_STATE when immediately followed by Wire.begin(); remove the
Wire.end() and the subsequent delay(100) and simply call Wire.begin(sda, scl)
where the I2C is initialized (replace the Wire.end(); delay(100); sequence). Use
Wire.begin(sda, scl) directly — it will reinitialize the bus when pins change
and is a no-op if the bus is already running with the same pins.
- Around line 843-857: The updateDevices() call from readPins() indirectly
triggers update() which fires callUpdateHandlers and re-enters readPins(),
causing a redundant Wire.end()/Wire.begin() cycle; fix this by adding a re-entry
guard (e.g., a boolean member like _inUpdateDevices) around the body of
updateDevices() and/or around readPins() so that if the guard is set you skip
running the I2C teardown/scan/initialization (the
Wire.end()/delay()/Wire.begin() path and the subsequent updateDevices() call),
set the guard at start and clear it at exit to prevent recursive calls, or
alternatively have updateDevices() write _state.data["I2CReady"] and other scan
results directly instead of calling update() so callUpdateHandlers won't be
invoked.
🧹 Nitpick comments (3)
src/MoonBase/Modules/ModuleDevices.h (1)
210-232: Remove or enable the commented-out deduplication block.This 22-line block is dead code. If the map-based dedup approach isn't ready, consider removing it and tracking it in an issue instead. Leaving large commented blocks hampers readability and risks going permanently stale.
Note: without deduplication, a device that changes IP (e.g., DHCP lease renewal) will appear twice in the list until the 86400s staleness check at Line 188 evicts the old entry—the name-only lookup at Line 160 will match the first occurrence and stop updating the second.
src/MoonLight/Nodes/Drivers/D_IMU.h (1)
102-102:requestInitBoardis in the public section but is internal state.Move it to the
privatesection (after line 184) alongside the other internal members.src/MoonBase/SharedFSPersistence.h (1)
178-179: Nit:sharedDelayedWrites.clear()afterstd::moveis redundant.After
std::move, the source vector is in a valid-but-unspecified state — but per the C++ standard,std::vector's move constructor leaves the source empty. The.clear()is harmless but unnecessary.
Collaborator
Author
|
@coderabbitai, I have again issues with writing to fs. similar to above: I change an effect, press the save button, check files and see the effects.json file is updated with the new effect. Then I press restart and it restarts with the old effect. How on earth is this possible? If it is on fs it is pretty persistent I would say. Is the old effect preserved somewhere and written back to fs at the moment I press restart? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 113456
What’s happening now is different: there’s still an out‑of‑band writer to /.config/effects.json that bypasses SharedFSPersistence.
This means:
So the reversion isn’t caused by SharedFSPersistence; it’s caused by ModuleLightsControl writing effects.json directly after Save. Recommended fixes (pick one): Option A — Keep Save semantics (best UX)
Patch sketch (inside ModuleLightsControl::onUpdate preset click branch): // Replace: copyFile(presetFile, "/.config/effects.json"); _fileManager->update(...effects.json...);
File f = ESPFS.open(presetFile.c_str(), "r");
if (f) {
JsonDocument presetDoc;
if (deserializeJson(presetDoc, f) == DeserializationError::Ok) {
f.close();
// Apply preset to effects module state without propagation (no handler write)
if (auto* effects = modules::find<ModuleEffects>()) {
effects->updateWithoutPropagation(presetDoc.as<JsonObject>(), ModuleState::update, "effects");
// expose unsaved change in UI (whatever your save flag/UI mechanism is)
_sveltekit->getFileManager()->setSaveNeeded(true);
}
} else {
f.close();
}
}Option B — Persist immediately on click (maintain current “click = commit” behavior, but route through persistence)
Patch sketch: // 1) Apply to state (as above)
effects->updateWithoutPropagation(presetDoc.as<JsonObject>(), ModuleState::update, "effects");
// 2) Persist through SharedFSPersistence (no delayed queue for this single write)
sharedFsPersistence->disableUpdateHandler("effects"); // avoid feedback
sharedFsPersistence->writeToFSNow("effects"); // single, atomic write
sharedFsPersistence->enableUpdateHandler("effects");Why this helps
Nice-to-have hardening
Key takeaways
If you want, I can prep a small PR plan to switch preset handling to Option A (save semantics) or Option B (immediate persist via SharedFSPersistence). 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, the issue with not saving new state in filesystem had to do with presets overruling, new commit to solve that will follow. Also found out that httpd stack size must be increased to 8192 (from 6K) to deal with heavy updates. The stack size can decrease very fast in some situations. Could you do an analysis in the onUpdate functions where large amounts of stack size is used (and not freed, e.g. due to lots of onUpdates Called in a hierarchy). Where can I save on stack? I guess Strings are candidates. I use Char a lot , which saves all on stacked, should we change some of them to String (which also uses heap)? It's a difficult choice as heap on non-psram devices is sparse . So any ideas for smart solutions welcome |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
While that runs, here are immediate, low-risk wins you can apply (even before we pinpoint exact lines): Quick, surgical mitigations
Once the scan returns, I’ll point to the biggest stack consumers (files/lines) and propose precise patches (swap StaticJsonDocument to pooled, shrink Char, or defer work). Also happy to suggest where increasing httpd to 8192 is still necessary (e.g., deep effect graph updates) and where we can claw back 1–2 KB with the changes above. 🏁 Script executed: Length of output: 99006
What’s actually using stack
Top hotspots to address first
Example patch // main.cpp
static portMUX_TYPE logMux = portMUX_INITIALIZER_UNLOCKED;
static int custom_vprintf(const char* fmt, va_list args) {
static char s_buffer[256]; // moved off stack
portENTER_CRITICAL(&logMux);
int len = vsnprintf(s_buffer, sizeof(s_buffer), fmt, args);
// ... existing sink (UART / ring / whatever) using s_buffer ...
portEXIT_CRITICAL(&logMux);
return len;
}
Example // before (allocates a String)
updatedItem.value.as<String>().c_str()
// after (no heap; bounded stack)
char vbuf[48];
size_t n = serializeJson(updatedItem.value, vbuf, sizeof(vbuf));
vbuf[MIN(n, sizeof(vbuf)-1)] = 0; // ensure NUL
EXT_LOGD(ML_TAG, "%s[%d]%s[%d].%s = %s -> %s",
updatedItem.parent[0].c_str(), updatedItem.index[0],
updatedItem.parent[1].c_str(), updatedItem.index[1],
updatedItem.name.c_str(), updatedItem.oldValue.c_str(), vbuf);
Tiny job queue pattern // A small lock-free ring or a guarded vector of std::function<void()>
struct PendingJob { void (*fn)(void*), *arg; };
volatile uint8_t head, tail;
PendingJob jobs[8];
// in onUpdate (httpd ctx): enqueue light job
enqueueJob([](void* a){ static_cast<NodeClass*>(a)->applyPreset(); }, nodePtr);
// in SvelteKit loop task:
while (dequeueJob(&job)) job.fn(job.arg);
Example (header-private helper) static JsonDocument& taskDoc() {
static JsonDocument doc; // one per task due to FreeRTOS TLS? If not available, make it file-static in this TU
doc.clear();
return doc;
}
// usage
auto& doc = taskDoc();
JsonObject root = doc.to<JsonObject>();Note: If multiple clients handled concurrently in the same task, guard with a small portMUX around serialize+send.
Where Char vs String makes sense
Add a lightweight StackProbe to find remaining culprits
struct StackProbe {
const char* tag; UBaseType_t hi{};
StackProbe(const char* t): tag(t), hi(uxTaskGetStackHighWaterMark(nullptr)) {}
~StackProbe() {
UBaseType_t now = uxTaskGetStackHighWaterMark(nullptr);
if (now < hi) EXT_LOGW("STACK", "[%s] used %u bytes (HWM now %u words)", tag,
(hi - now) * sizeof(StackType_t), now);
}
};
`#define` STACK_PROBE(tag) StackProbe _sp_##__LINE__{tag}Usage: void onUpdate(...) override {
STACK_PROBE("NodeManager::onUpdate");
...
}Quick wins summary (ordered by impact vs. effort)
If you want, I can prep tiny diffs for:
Also, good call on moving httpd stack to 8192. With the above, it should stay well clear of the cliff even during bursty updates. ✏️ Learnings added
🧠 Learnings used |
pio.ini: http stack to 8192 for heavy updates Front end ======== Field Renderer: Pad: if the same button selected, set selected to 255 Backend ======= - general remove as<String> in logging - Main: Module Effects add moduleLightsControl argument, add loop20msDrivers - Module IO: remove Wire.end() - Module lights control: remove preset selection if pressed again - Module effects: add onUpdate to clear preset if update happens
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/MoonBase/Modules/ModuleIO.h`:
- Line 849: The direct assignment to _state.data["I2CReady"] bypasses the
module's update() → persistence → UI sync pipeline; replace the direct mutation
with a call to update() (or the module's state setter) to set "I2CReady" so
subscribers and persistence are notified, e.g., use update({"I2CReady": true})
or the equivalent state API used elsewhere in ModuleIO, and if you intended to
avoid triggering handlers during init, add a concise comment next to the line
explaining that behavior and why notifications are suppressed.
In `@src/MoonLight/Layers/PhysicalLayer.cpp`:
- Around line 143-150: loop20msDrivers currently calls node->loop20ms() without
synchronization or yielding; this risks concurrent access to sharedData and task
starvation. Update PhysicalLayer::loop20msDrivers so each node->loop20ms() call
is wrapped with the node's layerMutex (use xSemaphoreTake(*node->layerMutex,
portMAX_DELAY) before and xSemaphoreGive(*node->layerMutex) after) and call
addYield(10) after releasing the mutex, matching the pattern used in loopDrivers
to prevent data races and watchdog starvation.
In `@src/MoonLight/Modules/ModuleEffects.h`:
- Around line 301-319: The onUpdate override currently triggers a full
_moduleLightsControl read and recreates a stack JsonDocument on every UI update;
change onUpdate (and its NodeManager::onUpdate call) to early-return unless the
UpdatedItem indicates a meaningful change (e.g., name or on toggle) to scope the
deselect, then inside the _moduleLightsControl->read lambda check the existing
preset selected value (state.data["preset"]["selected"]) and skip calling
_moduleLightsControl->update if it is already 255 to avoid redundant cascades;
also avoid copying the entire preset/list into a stack JsonDocument by
constructing a minimal object containing only preset.select and preset.selected
(or use ModuleState update helpers) and change EXT_LOGI("remove preset") to a
verbose/debug level (EXT_LOGV/EXT_LOGD); finally remove the pointless if
(newState.size()) guard.
In `@src/MoonLight/Nodes/Drivers/D_FastLED.h`:
- Around line 347-352: The destructor ~FastLEDDriver() currently calls
events.onChannelCreated.clear() and events.onChannelEnqueued.clear() and invokes
FastLED.reset(...), which removes all listeners and assumes a singleton; to fix,
either (A) track the listener handles returned when adding listeners to
FastLED.channelEvents() (store them as members, e.g.
onChannelCreatedHandle/onChannelEnqueuedHandle) and in ~FastLEDDriver() call the
corresponding remove/unregister using those handles instead of clear(), or (B)
enforce singleton semantics on FastLEDDriver by deleting copy/move constructors
and adding a static instance guard (and document it) so
checkAndAlloc<FastLEDDriver>(name) cannot create multiple instances; choose one
approach and update the destructor and class declaration accordingly, leaving
FastLED.reset(ResetFlags::CHANNELS) only if singleton is guaranteed.
🧹 Nitpick comments (3)
src/MoonBase/Modules/ModuleIO.h (2)
580-595: Misleading comment on line 584: says "ESP32-C3" but the condition also covers S2 and S3.The
#elifcoversESP32S2,ESP32S3, andESP32C3, but the inline comment only mentions C3. This could mislead someone picking different default pins for S2 vs S3.Proposed fix
`#elif` defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C3) - pinAssigner.assignPin(8, pin_I2C_SDA); // ESP32-C3 + pinAssigner.assignPin(8, pin_I2C_SDA); // ESP32-S2/S3/C3 pinAssigner.assignPin(9, pin_I2C_SCL);
930-954:updateDevices()is public and performs a full 127-address I2C bus scan — consider guarding against concurrent or repeated calls.If
updateDevices()is ever called from httpd context (e.g., via an update handler chain), the blocking scan (~127Wire.beginTransmission/endTransmissionround-trips) can stall the calling task. Combined with the re-entry issue fromreadPins(), this can double the scan cost. Consider:
- Making it
private(only called internally fromreadPins).- Adding a debounce or "already scanned" guard if it should remain public.
src/main.cpp (1)
192-196:loop20msDrivers()is gated bynewFrameReady— sensor drivers won't tick when no frames are produced.The 20ms driver loop only runs inside the
if (newFrameReady)branch (line 183). During normal operation frames flow continuously so this is fine, but if effects are paused or during extended layout/mapping phases (isPositions != 0), sensor nodes like the IMU won't get theirloop20ms()calls.If future drivers need guaranteed periodic polling regardless of frame state, consider moving the 20ms block outside the
if (newFrameReady)guard — or at least outside theif (layerP.lights.header.isPositions == 0)check at line 182.Possible restructuring to decouple 20ms from frame readiness
esp32sveltekit.lps++; layerP.loopDrivers(); - - if (millis() - last20ms >= 20) { - last20ms = millis(); - layerP.loop20msDrivers(); - } } } + // Run 20ms driver tasks regardless of frame state + if (millis() - last20ms >= 20) { + last20ms = millis(); + layerP.loop20msDrivers(); + } + if (!mutexGiven) xSemaphoreGive(swapMutex); // not double buffer or if conditions not met vTaskDelay(1);
Comment on lines
347
to
352
| ~FastLEDDriver() override { | ||
| auto& events = FastLED.channelEvents(); | ||
| events.onChannelCreated.clear(); | ||
| events.onChannelEnqueued.clear(); | ||
| FastLED.reset(ResetFlags::CHANNELS); // global: ensure only one FastLEDDriver instance exists. If multiple driver nodes are possible, this destructor will tear down channels for all of them. If singleton is guaranteed by design, consider documenting that assumption at the class level. | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the FastLEDDriver class definition to check for singleton enforcement
# (deleted copy constructor, private constructor, etc.)
grep -n "class FastLEDDriver" src/MoonLight/Nodes/Drivers/D_FastLED.hRepository: MoonModules/MoonLight
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Check if there are explicit singleton guards or design patterns in the class
head -100 src/MoonLight/Nodes/Drivers/D_FastLED.h | tail -50Repository: MoonModules/MoonLight
Length of output: 1977
🏁 Script executed:
#!/bin/bash
# Look at the setup() method where listeners are added to understand the pattern
sed -n '60,100p' src/MoonLight/Nodes/Drivers/D_FastLED.hRepository: MoonModules/MoonLight
Length of output: 1707
🏁 Script executed:
#!/bin/bash
# Search for how many FastLEDDriver instances are created in the codebase
rg "FastLEDDriver" --type=cpp -B2 -A2 | head -50Repository: MoonModules/MoonLight
Length of output: 2211
🏁 Script executed:
#!/bin/bash
# Check the full destructor context and any comments about singleton design
sed -n '340,360p' src/MoonLight/Nodes/Drivers/D_FastLED.hRepository: MoonModules/MoonLight
Length of output: 680
.clear() removes all listeners and offers no protection against multiple driver instances.
Lines 349–350 clear every subscriber on onChannelCreated and onChannelEnqueued. While the comment on line 351 acknowledges the singleton assumption for FastLED.reset(), the same caveat applies to the event clearing above it. However, the class provides no enforcement of singleton semantics—there are no deleted copy/move constructors, and the allocation pattern in ModuleDrivers.h uses checkAndAlloc<FastLEDDriver>(name), which permits multiple instances.
If a second FastLEDDriver is instantiated and then destroyed, its destructor will wipe all event listeners (including those from the first instance), breaking the first driver's operation.
Store the listener handles returned by .add() and remove only those in the destructor, or enforce singleton semantics explicitly (deleted copy/move constructors, or static instance guard).
🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Drivers/D_FastLED.h` around lines 347 - 352, The
destructor ~FastLEDDriver() currently calls events.onChannelCreated.clear() and
events.onChannelEnqueued.clear() and invokes FastLED.reset(...), which removes
all listeners and assumes a singleton; to fix, either (A) track the listener
handles returned when adding listeners to FastLED.channelEvents() (store them as
members, e.g. onChannelCreatedHandle/onChannelEnqueuedHandle) and in
~FastLEDDriver() call the corresponding remove/unregister using those handles
instead of clear(), or (B) enforce singleton semantics on FastLEDDriver by
deleting copy/move constructors and adding a static instance guard (and document
it) so checkAndAlloc<FastLEDDriver>(name) cannot create multiple instances;
choose one approach and update the destructor and class declaration accordingly,
leaving FastLED.reset(ResetFlags::CHANNELS) only if singleton is guaranteed.
Firmware - pio.ini: latest FastLED backend ======= - IO: I2S: maintain assigned pins and only change if updated -> _triggerUpdateI2S -> _updateI2SDevices - Physical layer: loop20ms in mutex - Module effects: refactor onUpdate preset reset, calling triggerResetPreset - Artnet out: yield and delay tweaks - Effects: VU meter: circles within size
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 830-861: The code uses I2S-named variables for I2C logic causing
confusion; rename pinsI2SChanged to pinsI2CChanged, _triggerUpdateI2S to
_triggerUpdateI2C, and any methods like _updateI2SDevices to _updateI2CDevices
(and adjust related log messages such as "initI2C" checks) throughout the
ModuleIO class so identifiers reflect I2C usage (e.g., references in the loop
over _state.data["pins"], the Wire.begin block, and any other places that
read/update _pinI2CSDA/_pinI2CSCL); update all references/usages and tests
accordingly to preserve behavior while fixing names.
In `@src/MoonLight/Nodes/Drivers/D_ArtnetOut.h`:
- Line 186: The packet-sending loop in D_ArtnetOut (the for loop that calls
writePackage() for each light/universe) currently has addYield(10) commented
out, which can starve the IDLE task on large setups; restore yielding by
re-enabling addYield(10) immediately after the writePackage() calls (or
alternatively insert a vTaskDelay(1) or addYield(10) every N packets) so the
loop yields frequently enough while still using the existing vTaskDelayUntil
scheduling; modify the loop around writePackage() inside the ArtNet driver
(D_ArtnetOut) to reinstate yielding.
In `@src/MoonLight/Nodes/Effects/E_MoonLight.h`:
- Around line 1254-1258: The code in the gravity assignment block (when
gravityType == 2) uses integer division because sharedData.gravity.{x,y,z} and
INT16_MAX are ints, causing outputs to be only -1,0,1; change the division to
floating-point (e.g. cast numerator or divisor to float or use 32767.0f) when
assigning gravity[0], gravity[1], and gravity[2] so the calculations produce
fractional values (keep the existing sign flips and Y/Z swap logic intact).
🧹 Nitpick comments (4)
src/MoonLight/Modules/ModuleEffects.h (2)
298-316: Good: preset reset deferred from httpd to SvelteKit loop — addresses stack and frequency concerns.Moving the preset-deselect logic out of
onUpdate(httpd task) intoloop()with a coalescing boolean flag is a solid improvement: it avoids httpd stack pressure fromJsonDocument+ cascadingupdate()calls, and naturally deduplicates rapid slider-drag updates into a single reset per loop iteration. Theselected != 255guard at line 305 prevents redundant cross-module updates.One remaining concern: line 310 copies the entire
state.data["preset"]object (including thelistarray) into a stack-localJsonDocument. If the preset list grows large, this is unnecessary memory pressure. Consider constructing a minimal object with onlyselectandselected:Suggested minimal state construction
- JsonDocument doc; - JsonObject newState = doc.to<JsonObject>(); - - EXT_LOGD(ML_TAG, "remove preset"); - newState["preset"] = state.data["preset"]; - newState["preset"]["select"] = 255; + JsonDocument doc; + JsonObject newState = doc.to<JsonObject>(); + + EXT_LOGD(ML_TAG, "remove preset"); + newState["preset"]["select"] = 255; + newState["preset"]["selected"] = 255;This avoids deep-copying the preset list and only sends the fields that
compareRecursiveneeds to act on. Based on learnings, heavyonUpdatecascades can exhaust stack; keeping the payload minimal is good hygiene even in the loop context.
319-325:triggerResetPresetis a plainboolshared across tasks — benign here but worth a note.
onUpdateruns in httpd;loop()runs in the SvelteKit task. The flag is a simple set-true / read-and-clear pattern, so the worst case is one extra or one missed reset cycle — both harmless. If you ever need stricter guarantees,std::atomic<bool>is zero-cost on ESP32 and makes the cross-task intent explicit.Also, every UI-originated
onUpdate(including high-frequency slider drags) sets this flag. Thanks to the coalescing inloop()and the!= 255guard, the actual cross-module update fires at most once, which is fine. Just confirming the design is intentional — if only specific control changes (e.g., node name, on/off) should deselect the preset, scoping the trigger would further reduce unnecessaryread()calls.src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
1589-1593: Potential out-of-bounds access onsharedData.bandswhenbandexceedsNUM_GEQ_CHANNELS / 2.The loop runs
nHorizontal * nVertical = 8iterations, and each iteration accessessharedData.bands[2 * (band++)], giving indices 0, 2, 4, 6, 8, 10, 12, 14. This works whenNUM_GEQ_CHANNELSis 16, but the user-facingbandsslider (line 1574) is not consulted—changing it has no effect on this loop. IfNUM_GEQ_CHANNELSwere ever reduced or ifnHorizontal/nVerticalare made configurable, this would read out of bounds.Consider using the
bandscontrol variable to cap the iteration or adding a bounds guard on the index.src/MoonBase/Modules/ModuleIO.h (1)
951-955:_I2CFreqis declared but never read or written.This member is unused —
i2cFreqis read directly from_state.data["i2cFreq"]on line 850 and fromWire.getClock()in_updateI2SDevices. Remove it to avoid confusion.uint8_t _pinI2CSDA = UINT8_MAX; uint8_t _pinI2CSCL = UINT8_MAX; - uint8_t _I2CFreq = UINT8_MAX;
Collaborator
Author
|
@coderabbitai, regarding
I like the idea but I think that will not work as the whole preset object will be copied, if part of this object is missing than the compare recursive will remove that missing part, please verify and confirm and if you have another proposal , let me know |
|
Tip For best results, initiate chat on the files or code changes.
Two low-stack patterns you can use:
JsonDocument patch; // small, heap-backed; no large locals
JsonObject p = patch["preset"].to<JsonObject>();
p["select"] = newSelect; // e.g., 255
p["selected"] = newSelected; // e.g., 255 or an index
update(patch.as<JsonObject>(), ModuleState::update, _moduleName);
update([&](ModuleState& s) {
s.data["preset"]["select"] = newSelect;
s.data["preset"]["selected"] = newSelected;
return StateUpdateResult::CHANGED; // notify without walking compareRecursive
}, _moduleName);
Caveats and tips:
If you want, I can turn this into a tiny helper like setPresetSelection(select, selected) that uses option (1) internally, so all call sites stay safe and lightweight. ✏️ Learnings added
🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
I am not convinced so will leave it as is. I am about to merge the PR back to main. Can you do a thorough review if all the changes in this PR. Make sure the description summarizes all what is done in this PR and give an advice if it is okay to merge back to main. |
|
High-level summary of what this PR delivers
Readiness to merge to main
If those are acceptable as post-merge follow-ups, I recommend merge. The SharedFSPersistence fix and the I2C centralization are valuable improvements and unblock the new IMU/pir/mic workstreams. Potential open issues to be aware of
Suggested follow-ups (next PRs)
Release notes draft (you can paste in PR description)
Recommendation
If you want, I can turn the follow-ups above into individual GitHub issues with checklists and backlinks to this PR. 🧠 Learnings used✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Behavior Changes
Bug Fixes
Chores