Board preset add presets #70
Conversation
Back end ======== - Ethernet: compiler directives to variables , explicitly for CONFIG_IDF_TARGET_ESP32S3 - Devices: add delete option - IO: add SPI pins, add readPins (for ethernet), add jumper1 (switch IR / ethernet for SE16), setup dig uno - Node manager: on default on
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Ethernet settings & init lib/framework/EthernetSettingsService.h, lib/framework/EthernetSettingsService.cpp |
Expose ESP32S3 SPI/PHY config fields (SPI pins, phy type/addr/cs/irq/rst) and change ETH.begin logic to use SPI-based init when SPI pins provided (ESP32S3). Add ESP32P4 placeholder; keep default built-in MAC/PHY path for other ESP32 variants. |
Module IO, pin mapping & runtime reinit src/MoonBase/Modules/ModuleIO.h |
Add _sveltekit member, readPins() and nested PinAssigner; refactor board preset pin assignment to use per-pin index/usage and call readPins to map UI pins to Ethernet settings, triggering Ethernet reinit when a complete SPI/PHY set is defined (ESP32S3-guarded). |
Infrared driver reinit src/MoonLight/Nodes/Drivers/D_Infrared.h |
Reset infrared pin sentinel, fully teardown existing RMT RX channel and recreate RX channel/queue/callback when pin changes; use active callback path and capture this in update handler. |
Virtual/Physical layer & mapping refactor src/MoonLight/Layers/VirtualLayer.h, src/MoonLight/Layers/VirtualLayer.cpp, src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonLight/Modules/ModuleMoonLightInfo.h |
Replace mappingTable vector with raw pointer + mappingTableSize; add constructor/destructor changes, realloc/zeroing logic, update bounds checks and JSON reporting to use mappingTableSize, and switch cleanup to freeMB. |
Drivers and LED pin assignment changes src/MoonLight/Nodes/Drivers/D_FastLED.h, src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h, src/MoonLight/Modules/ModuleDrivers.h |
Make preprocessor target guards robust using defined(...), and change LED assignment to read an explicit per-pin index (1–20) for pin_LED usage into ledPins[index-1]. |
Node/Devices UI defaults & CRUD src/MoonBase/Modules/ModuleDevices.h, src/MoonBase/NodeManager.h |
Allow device-row deletion by changing CRUD permission "r" → "rd". Set node on control default to true. Change some lambdas to capture [this]. |
Utilities & guard normalization src/MoonBase/Utilities.cpp, src/MoonBase/Nodes.h, src/MoonBase/Utilities.h |
Normalize target checks to defined(...), add final return false; in isInPSRAM, add explicit fallback driver declaration for non-ESP32 targets, and change freeMB/freeMBObject to take pointer-by-reference so freed pointers are nulled. |
ModuleLightsControl pin & palettes src/MoonLight/Modules/ModuleLightsControl.h |
Adjust isInPSRAM argument usage, rename pin checks (Relay/OnOff → LightsOn), change default brightness, add new palette options (Quin, Orange) and handling branches, and change some lambdas to [this] capture. |
Misc drivers & effects tweaks src/MoonLight/Nodes/Effects/E_MoonModules.h, src/MoonLight/Nodes/Effects/E_MoonLight.h |
Add null-pointer guard in GameOfLifeEffect startup; change RandomEffect color sourcing to palette-based and update tags. |
Docs & UI changes docs/moonbase/inputoutput.md, docs/gettingstarted/installer.md, interface/src/lib/components/moonbase/MultiRow.svelte |
Add Jumper1 preset and board details, merge installer table columns, and show first three fields or any with show===true in MultiRow. |
Build config cleanup firmware/esp32-p4.ini, firmware/esp32-s3.ini, platformio.ini |
Remove commented FT_ETHERNET lines (cosmetic) and update APP_DATE define in platformio.ini. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant UI as Web UI / Settings
participant ModuleIO as ModuleIO::readPins
participant Svelte as ESP32SvelteKit
participant EthSvc as EthernetSettingsService
participant ETH_HW as ETH Driver (ESP-IDF)
UI->>ModuleIO: user maps pins / sets jumper1
ModuleIO->>ModuleIO: assign per-pin usage/index (PinAssigner)
ModuleIO->>Svelte: write `v_ETH_SPI_*` / `v_ETH_PHY_*` settings
Svelte->>EthSvc: request Ethernet init/update
alt SPI pins provided (ESP32S3)
EthSvc->>ETH_HW: init SPI bus (SCK/MISO/MOSI)
EthSvc->>ETH_HW: ETH.begin(SPI, phyType, addr, cs, irq, rst)
else default built-in path
EthSvc->>ETH_HW: ETH.begin(default MAC/PHY)
end
ETH_HW-->>EthSvc: init result
EthSvc-->>Svelte: report status
Svelte-->>ModuleIO: update UI/state
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Focus review on:
src/MoonBase/Modules/ModuleIO.h— PinAssigner logic, readPins correctness,_sveltekitwiring, and Ethernet restart trigger.lib/framework/EthernetSettingsService.cpp/ .h — ESP32S3 SPI init branches and ETH.begin overload usage.src/MoonLight/Nodes/Drivers/D_Infrared.h— RMT channel/queue teardown/recreate and callback handling.src/MoonLight/Layers/VirtualLayer.*&src/MoonLight/Modules/ModuleMoonLightInfo.h— allocation/resizing, bounds checks,freeMBusage, and JSON reporting.src/MoonBase/Utilities.h— changed freeMB/freeMBObject signatures and allocator deallocate call.
Possibly related PRs
- Board presets tuning #66 — overlaps ModuleIO pin enums, wiring/preset changes and readPins integration.
- d0 tuning #63 — modifies Ethernet settings/header paths and is relevant to EthernetSettingsService changes.
- add heap-optimization: request heap optimization review #62 — related to VirtualLayer mappingTable representation/size handling.
Suggested labels
enhancement
Poem
🐇 I jiggle jumper1 — pins click, wires hum,
SPI wakes the PHY beneath the moonlit drum,
Rows can vanish when the rabbit taps delete,
Nodes wake on by default, the network finds its beat,
I hop off happy — Ethernet purrs, snug and neat.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 18.52% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
|
| Title check | ❓ Inconclusive | The title 'Board preset add presets' is vague and does not clearly convey the main changes; it uses generic phrasing that doesn't specify what presets are being added or what the primary functional goal is. | Revise the title to be more specific about the key changes, such as 'Add Ethernet SPI configuration and pin assignment for board presets' or 'Support ESP32S3 Ethernet SPI setup and jumper presets'. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
board-preset-add-presets
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/MoonBase/Modules/ModuleIO.h (2)
147-148: Consider adding a default value forjumper1.Unlike the
moddedcheckbox (line 142),jumper1lacks an explicit default. When_state.data["jumper1"]is unset (null), the conditional at line 293 may behave unexpectedly depending on ArduinoJson's null-to-bool coercion (typicallyfalse).control = addControl(controls, "jumper1", "checkbox"); +control["default"] = false;
479-486: Fragile pointer validation using magic address.The check
(uintptr_t)ess < 0x3FC00000relies on ESP32S3 memory layout specifics. This magic number may not be portable across ESP32 variants or future SDK versions.Consider using
nullptrcheck as the primary guard, or document why this specific address boundary is necessary:- if ((uintptr_t)ess < 0x3FC00000) { + if (ess == nullptr) { EXT_LOGW(MB_TAG, "EthernetSettingsService not available"); - EXT_LOGW(MB_TAG, "ESS PTR = %p\n", (void*)ess); return; }If the address check is intentional (e.g., detecting uninitialized pointers), consider adding a comment explaining the rationale.
lib/framework/EthernetSettingsService.h (2)
36-36: Consider migration or fallback for the new Ethernet settings file pathChanging
ETHERNET_SETTINGS_FILEto"/.config/ethernetSettings.json"will cause existing deployments to ignore any settings stored at the old path and silently fall back to defaults. If nodes are typically configured over Ethernet, this could temporarily break remote access after an upgrade.Consider either:
- Reading from the legacy path as a fallback when the new file is missing, or
- Providing a one‑time migration that moves/renames the old file into
/.config, or- At minimum, documenting this as a breaking change in release notes.
113-124: ESP32S3 SPI/PHY runtime members: confirm initialization order and encapsulationMoving compiler directives into instance members for ESP32S3 is a good step toward runtime configurability, but a couple of points are worth double‑checking:
- These are initialized with board‑specific defaults and are public. Ensure that
ModuleIO::readPins(or equivalent) updates them beforeconfigureNetworkis first called, otherwiseinitEthernet()will use the baked‑in defaults instead of the board/UI‑selected values.- Using
UINT8_MAXas the sentinel forv_ETH_SPI_SCKis fine on ESP32, but it depends onuint8_tmapping to the type underlying that macro. If you want to make this more explicit/robust, a smallstatic constexpr uint8_t kSpiPinUnset = 0xFF;(and using that constant in both the member initializer and the check) would clarify intent.- Longer‑term, consider wrapping these in a small config struct or getter/setter API instead of exposing raw public members so callers don’t accidentally put the service into an inconsistent state.
Overall the approach is sound; the main ask is to verify the configuration lifecycle and maybe tighten the API a bit. Based on learnings, this follows the project’s pattern of moving ESP32 board specifics into configurable C++ members.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/framework/ESP32SvelteKit.cpp(1 hunks)lib/framework/EthernetSettingsService.cpp(1 hunks)lib/framework/EthernetSettingsService.h(2 hunks)src/MoonBase/Modules/ModuleDevices.h(1 hunks)src/MoonBase/Modules/ModuleIO.h(8 hunks)src/MoonBase/NodeManager.h(1 hunks)src/MoonLight/Layers/VirtualLayer.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/factory_settings.ini : Configure default or factory-set ESP32 parameters in `factory_settings.ini`
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
lib/framework/EthernetSettingsService.hlib/framework/EthernetSettingsService.cpp
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/lib/PsychicHttp/**/*.{h,cpp} : Use the custom PsychicHttp library for HTTP server capabilities, request/response handling, WebSockets, and file serving
Applied to files:
lib/framework/EthernetSettingsService.h
🧬 Code graph analysis (3)
src/MoonBase/Modules/ModuleIO.h (1)
src/MoonLight/Modules/ModuleDrivers.h (1)
readPins(34-66)
src/MoonBase/NodeManager.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonBase/Modules/ModuleDevices.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
src/MoonBase/Modules/ModuleDevices.h (1)
38-41: LGTM! Delete capability added for device management.The change from
"r"to"rd"enables users to remove stale device entries from the list. This is a sensible addition for maintaining a clean device registry.lib/framework/ESP32SvelteKit.cpp (1)
84-85: Documentation-only change.The comment update reflects endpoint count changes. No functional impact.
src/MoonBase/NodeManager.h (1)
96-98: LGTM! Sensible default for node activation.Setting the "on" checkbox default to
trueimproves UX by having nodes active by default. This follows the established pattern used by other controls in this function.src/MoonLight/Layers/VirtualLayer.cpp (2)
372-372: Formatting-only change.Whitespace adjustment before the inline comment. No functional impact.
453-453: Formatting-only change.Same whitespace adjustment as in
drawLine. No functional impact.src/MoonBase/Modules/ModuleIO.h (4)
77-82: LGTM! New SPI and PHY pin types added.The new
pin_SPI_SCK,pin_SPI_MISO,pin_SPI_MOSI,pin_PHY_CS, andpin_PHY_IRQenum values extend the pin taxonomy to support ESP32S3 Ethernet configuration.
292-301: LGTM! Clean jumper-based pin mode switching.The
jumper1toggle correctly switches between IR mode (single pin) and Ethernet mode (SPI + PHY pins). This aligns with the PR objective for SE16 board configuration.
487-513: LGTM! Robust Ethernet pin configuration.The implementation correctly:
- Resets all pins to
UINT8_MAXbefore scanning- Only initializes Ethernet when all 5 required pins are configured
- Handles the unconnected reset pin (
v_ETH_PHY_RST = -1)The conditional guard at line 506 prevents partial/invalid Ethernet initialization.
303-320: Documentation and pin cleanup for QuinLED Dig Uno V3.The added comments document board-specific setup requirements. The commented-out pin assignments preserve knowledge of alternative configurations while simplifying the default preset.
|
@coderabbitai, the code crashes on this, on executing ess->v_ETH_SPI_SCK = UINT8_MAX: void readPins(ESP32SvelteKit* sveltekit) { crash code: [ 1128][D][ModuleIO.h:480] readPins(): [🌙] loopTask (3984) Try to configure ethernet Guru Meditation Error: Core 1 panic'ed (StoreProhibited). Exception was unhandled. Core 1 register dump: PC : 0x4208367c PS : 0x00060f30 A0 : 0x82083770 A1 : 0x3fceb450 A2 : 0x3fca3678 A3 : 0x00000000 A4 : 0x00000000 A5 : 0x001139aa A6 : 0x00000300 A7 : 0x00000f90 A8 : 0xffffffff A9 : 0x3fceb3d0 A10 : 0x3c19376e A11 : 0x00000468 A12 : 0x3c19375a A13 : 0x000001e0 A14 : 0x42083677 A15 : 0x3fceb40c SAR : 0x00000011 EXCCAUSE: 0x0000001d EXCVADDR: 0x00000328 LBEG : 0x40056fc5 LEND : 0x40056fe7 LCOUNT : 0xffffffff Backtrace: 0x42083679:0x3fceb450 0x4208376d:0x3fceb4d0 0x4208151b:0x3fceb4f0 0x42081536:0x3fceb510 0x42081bf9:0x3fceb530 0x4209328a:0x3fceb560 0x4203a7ca:0x3fceb660 0x4203a7d9:0x3fceb680 0x4203a8bb:0x3fceb6a0 0x4203a92a:0x3fceb6c0 0x4203d38d:0x3fceb6e0 0x4203d53f:0x3fceb950 0x4203da97:0x3fcebbc0 0x4216c08b:0x3fcebc80 0x4203ac31:0x3fcebca0 0x4203e381:0x3fcebcc0 0x4203e411:0x3fcebdc0 0x42094611:0x3fcebe20 0x4209c9a2:0x3fcebe80 0x4037fd21:0x3fcebea0 #0 0x42083679 in ModuleIO::readPins(ESP32SvelteKit*) at src/MoonBase/Modules/ModuleIO.h:487 #1 0x4208376d in ModuleIO::ModuleIO(PsychicHttpServer*, ESP32SvelteKit*)::{lambda(String const&)#1}::operator()(String const&) const at src/MoonBase/Modules/ModuleIO.h:114 (inlined by) void std::__invoke_impl<void, ModuleIO::ModuleIO(PsychicHttpServer*, ESP32SvelteKit*)::{lambda(String const&)#1}&, String const&>(std::__invoke_other, ModuleIO::ModuleIO(PsychicHttpServer*, ESP32SvelteKit*)::{lambda(String const&)#1}&, String const&) at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/invoke.h:61 (inlined by) std::enable_if<is_invocable_r_v<void, ModuleIO::ModuleIO(PsychicHttpServer*, ESP32SvelteKit*)::{lambda(String const&)#1}&, String const&>, void>::type std::__invoke_r<void, ModuleIO::ModuleIO(PsychicHttpServer*, ESP32SvelteKit*)::{lambda(String const&)#1}&, String const&>(ModuleIO::ModuleIO(PsychicHttpServer*, ESP32SvelteKit*)::{lambda(String const&)#1}&, String const&) at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/invoke.h:111 (inlined by) std::_Function_handler<void (String const&), ModuleIO::ModuleIO(PsychicHttpServer*, ESP32SvelteKit*)::{lambda(String const&)#1}>::_M_invoke(std::_Any_data const&, String const&) at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/std_function.h:290 #2 0x4208151b in std::function<void (String const&)>::operator()(String const&) const at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/std_function.h:591 #3 0x42081536 in StatefulService::callUpdateHandlers(String const&) at lib/framework/StatefulService.h:183 #4 0x42081bf9 in StatefulService::update(ArduinoJson::V742PB22::JsonObject&, std::function<StateUpdateResult (ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&)>, String const&) at lib/framework/StatefulService.h:152 #5 0x4209328a in ModuleIO::onUpdate(UpdatedItem const&) at src/MoonBase/Modules/ModuleIO.h:449 (discriminator 4) #6 0x4203a7ca in Module::processUpdatedItem(UpdatedItem const&) at src/MoonBase/Module.h:178 #7 0x4203a7d9 in std::_Function_handler<void (UpdatedItem const&), Module::Module(String const&, PsychicHttpServer*, ESP32SvelteKit*)::{lambda(UpdatedItem const&)#1}>::_M_invoke(std::_Any_data const&, UpdatedItem const&) at src/MoonBase/Module.cpp:318 (inlined by) __invoke_impl<void, Module::Module(const String&, PsychicHttpServer*, ESP32SvelteKit*)::<lambda(const UpdatedItem&)>&, const UpdatedItem&> at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/invoke.h:61 (inlined by) __invoke_r<void, Module::Module(const String&, PsychicHttpServer*, ESP32SvelteKit*)::<lambda(const UpdatedItem&)>&, const UpdatedItem&> at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/invoke.h:111 (inlined by) _M_invoke at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/std_function.h:290 #8 0x4203a8bb in std::function<void (UpdatedItem const&)>::operator()(UpdatedItem const&) const at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/std_function.h:591 #9 0x4203a92a in ModuleState::postUpdate(UpdatedItem const&) at src/MoonBase/Module.h:120 (discriminator 1) #10 0x4203d38d in ModuleState::compareRecursive(ArduinoJson::V742PB22::JsonString const&, ArduinoJson::V742PB22::JsonVariant const&, ArduinoJson::V742PB22::JsonVariant const&, UpdatedItem&, unsigned char, unsigned char) at src/MoonBase/Module.cpp:260 #11 0x4203d53f in ModuleState::compareRecursive(ArduinoJson::V742PB22::JsonString const&, ArduinoJson::V742PB22::JsonVariant const&, ArduinoJson::V742PB22::JsonVariant const&, UpdatedItem&, unsigned char, unsigned char) at src/MoonBase/Module.cpp:246 (discriminator 8) #12 0x4203da97 in ModuleState::update(ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&) at src/MoonBase/Module.cpp:290 (discriminator 3) #13 0x4216c08b in StateUpdateResult std::__invoke_impl<StateUpdateResult, StateUpdateResult (&)(ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&), ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&>(std::__invoke_other, StateUpdateResult (&)(ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&), ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&) at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/invoke.h:61 (inlined by) std::enable_if<is_invocable_r_v<StateUpdateResult, StateUpdateResult (&)(ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&), ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&>, StateUpdateResult>::type std::__invoke_r<StateUpdateResult, StateUpdateResult (&)(ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&), ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&>(StateUpdateResult (&)(ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&), ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&) at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/invoke.h:114 (inlined by) std::_Function_handler<StateUpdateResult (ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&), StateUpdateResult ()(ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&)>::_M_invoke(std::_Any_data const&, ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&) at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/std_function.h:290 #14 0x4203ac31 in std::function<StateUpdateResult (ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&)>::operator()(ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&) const at /Users/ewoud/.platformio/packages/toolchain-xtensa-esp-elf/xtensa-esp-elf/include/c++/14.2.0/bits/std_function.h:591 (inlined by) StatefulService::updateWithoutPropagation(ArduinoJson::V742PB22::JsonObject&, std::function<StateUpdateResult (ArduinoJson::V742PB22::JsonObject&, ModuleState&, String const&)>, String const&) at lib/framework/StatefulService.h:160 #15 0x4203e381 in FSPersistence::readFromFS() at lib/framework/FSPersistence.h:55 (discriminator 2) #16 0x4203e411 in Module::begin() at src/MoonBase/Module.cpp:325 #17 0x42094611 in setup() at src/main.cpp:302 (discriminator 4) #18 0x4209c9a2 in loopTask(void*) at /Users/ewoud/.platformio/packages/framework-arduinoespressif32/cores/esp32/main.cpp:67 Why is this? looks pretty standard to me |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 650 🏁 Script executed: Length of output: 2573 🏁 Script executed: Length of output: 942 @ewowi The crash is caused by incorrect preprocessor directive syntax. The issue is subtle but critical: Root CauseYou're using: #if CONFIG_IDF_TARGET_ESP32S3This should be: #ifdef CONFIG_IDF_TARGET_ESP32S3
// or
#if defined(CONFIG_IDF_TARGET_ESP32S3)Why This Causes the CrashWhen you use
However, ESP-IDF macros can have subtle differences in how they're defined across compilation units. The result is:
The FixUpdate all three locations to use consistent, safe preprocessor checks: lib/framework/EthernetSettingsService.h (line 114): #ifdef CONFIG_IDF_TARGET_ESP32S3
uint8_t v_ETH_SPI_SCK = UINT8_MAX;
// ... rest of members
#endiflib/framework/EthernetSettingsService.cpp (line 87): #ifdef CONFIG_IDF_TARGET_ESP32S3
if (v_ETH_SPI_SCK != UINT8_MAX) {
// ...
}
#elif defined(CONFIG_IDF_TARGET_ESP32P4)
// ...
#else
// ...
#endifsrc/MoonBase/Modules/ModuleIO.h (line 487): #ifdef CONFIG_IDF_TARGET_ESP32S3
ess->v_ETH_SPI_SCK = UINT8_MAX;
ess->v_ETH_SPI_MISO = UINT8_MAX;
ess->v_ETH_SPI_MOSI = UINT8_MAX;
ess->v_ETH_PHY_CS = UINT8_MAX;
ess->v_ETH_PHY_IRQ = UINT8_MAX;
#endifThis ensures all compilation units see the same class layout, preventing the ODR (One Definition Rule) violation that's causing your crash. |
|
@coderabbitai, still got: [ 1136][W][ModuleIO.h:483] readPins(): [🌙] loopTask (3936) EthernetSettingsService not available And later a crash Also got [ 1137][W][ModuleIO.h:484] readPins(): [🌙] loopTask (3936) ESS PTR = 0x37c one time I will submit a commit in a second, please comment on above and commit in one comment. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3925 🏁 Script executed: Length of output: 3346 🏁 Script executed: Length of output: 285 @ewowi I found the root cause! The issue is that FT_ETHERNET is disabled in your build configuration. Root Cause AnalysisThe invalid pointer addresses (0x268, 0x37c) you're seeing are caused by calling a non-existent method:
SolutionsOption 1: Enable FT_ETHERNET (recommended if you need Ethernet) build_flags =
${common.build_flags}
-D FT_ETHERNET=1Option 2: Guard the call (if Ethernet is optional) void readPins(ESP32SvelteKit* sveltekit) {
#if FT_ENABLED(FT_ETHERNET)
EXT_LOGD(MB_TAG, "Try to configure ethernet");
EthernetSettingsService* ess = sveltekit->getEthernetSettingsService();
// ... rest of the logic
#else
EXT_LOGD(MB_TAG, "Ethernet feature not enabled, skipping readPins");
#endif
}I'm ready to review your upcoming commit once you push it. Please tag me when it's ready! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/MoonLight/Nodes/Drivers/D_FastLED.h (1)
395-529: S3 guard update is fine; consider normalizing guard style laterUsing
#ifdef CONFIG_IDF_TARGET_ESP32S3here matches the pattern used elsewhere in the PR and avoids depending on the macro’s numeric value. The surrounding S2/C3 blocks still use#if CONFIG_IDF_TARGET_...; that’s valid but you might later normalize all of them to#if defined(...)/#ifdeffor consistency.src/MoonBase/Modules/ModuleIO.h (2)
147-148: jumper1 control and SE16V1 preset mapping are coherentThe new
jumper1checkbox, combined with the SE16V1 preset logic:
- Routes GPIO 5 to Infrared when
jumper1is true.- Otherwise assigns GPIO 5/6/7 to
SPI_MISO/SPI_MOSI/SPI_SCKand GPIO 15/18 toPHY_CS/PHY_IRQ.This matches the new enum values and provides a clear IR vs. Ethernet split for SE16V1 without affecting other boards.
You might want to set an explicit default for
jumper1(control["default"] = false;) for clarity, even if it currently behaves as false by default.Also applies to: 283-303
479-513: readPins wiring is correct; consider replacing the magic pointer heuristicFunctionally,
readPins()does what you want:
- Obtains
EthernetSettingsService*fromESP32SvelteKit.- On ESP32S3, clears
v_ETH_SPI_*,v_ETH_PHY_CS, andv_ETH_PHY_IRQtoUINT8_MAX, scans_state.data["pins"]for the new usages, and fills the correspondingv_ETH_*fields.- Calls
ess->initEthernet()once all five required pins are found, forcing a clean Ethernet restart with the new SPI/PHY wiring.The only questionable part is:
if ((uintptr_t)ess < 0x3FC00000) { EXT_LOGW(MB_TAG, "EthernetSettingsService not available"); EXT_LOGW(MB_TAG, "ESS PTR = %p\n", (void*)ess); return; }Using a hard‑coded address threshold as a validity check is very ESP32‑layout‑specific and may surprise future maintainers. If possible, it would be more robust to:
- Have
getEthernetSettingsService()returnnullptrwhen Ethernet is disabled or not constructed, and simply checkif (!ess).- Or, guard the entire
readPins()body with the existing Ethernet feature macro (e.g.,#if FT_ENABLED(FT_ETHERNET)), so this code is compiled out when Ethernet is not part of the build.The current heuristic does mitigate the previously seen low-address garbage pointers, but tightening the contract at the API/feature level would be safer and clearer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/framework/EthernetSettingsService.cpp(1 hunks)lib/framework/EthernetSettingsService.h(2 hunks)src/MoonBase/Modules/ModuleIO.h(8 hunks)src/MoonBase/Nodes.h(1 hunks)src/MoonBase/Utilities.cpp(1 hunks)src/MoonLight/Nodes/Drivers/D_FastLED.h(1 hunks)src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/MoonBase/Nodes.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.hsrc/MoonBase/Utilities.cpplib/framework/EthernetSettingsService.cppsrc/MoonLight/Nodes/Drivers/D_FastLED.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/lib/PsychicHttp/**/*.{h,cpp} : Use the custom PsychicHttp library for HTTP server capabilities, request/response handling, WebSockets, and file serving
Applied to files:
lib/framework/EthernetSettingsService.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (11)
src/MoonBase/Utilities.cpp (1)
106-122: LGTM! Preprocessor directive fix correctly applied.The change from
#if CONFIG_IDF_TARGET_*to#if defined(CONFIG_IDF_TARGET_*)is correct and addresses the ODR violation issue identified in the PR discussion. Using#if defined(...)ensures the preprocessor checks whether the macro is defined (rather than evaluating its value), which guarantees consistent class layouts across translation units and prevents the crash described in the PR objectives.src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (1)
149-174: ESP32 target guard change looks correct and consistentSwitching to
#if defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32)keeps behavior while aligning with the other files’ pattern and avoiding surprises if some target macros aren’t defined.lib/framework/EthernetSettingsService.cpp (1)
85-100: Multi-target Ethernet init matches new SPI configuration fieldsThe new block cleanly separates:
- ESP32S3: SPI-based
ETH.begin(...)whenv_ETH_SPI_SCK != UINT8_MAX, else fallback to plainETH.begin().- ESP32P4: explicit
ETH.begin()branch.- Other targets: existing
ETH.begin()path.This aligns with the new
v_ETH_*members in the header and avoids the previous layout/guard pitfalls while finally wiring up the ESP32P4 branch.lib/framework/EthernetSettingsService.h (2)
36-37: Settings path comment change is safeOnly the comment is updated; the actual
ETHERNET_SETTINGS_FILEpath remains unchanged and consistent with existing persistence logic.
113-125: ESP32S3 Ethernet configuration fields are consistent with cpp usageThe new
v_ETH_SPI_*andv_ETH_PHY_*members, guarded by#ifdef CONFIG_IDF_TARGET_ESP32S3, line up with the initialization logic inconfigureNetwork()andModuleIO::readPins. UsingUINT8_MAXas the “not configured” sentinel for pins is clear and keeps the S3-only layout consistent across translation units.src/MoonBase/Modules/ModuleIO.h (6)
12-18: Header/feature guards are correctly nested
#ifndef ModuleIO_hcombined with#if FT_MOONBASE == 1and the two closing#endifs at the bottom form a consistent wrapper, so the module is only compiled when MoonBase is enabled and the header is protected from multiple inclusion.Also applies to: 516-517
19-83: New IO_PinUsage entries stay aligned with the UI select valuesAdding
pin_SPI_SCK,pin_SPI_MISO,pin_SPI_MOSI,pin_PHY_CS, andpin_PHY_IRQjust beforepin_count, and appending the correspondingaddControlValue(...)strings, preserves all existing numeric values (includingpin_Button_OnOff) and keeps persistedusageindices backward compatible while enabling the new Ethernet-related usages.Also applies to: 216-221
114-115: Update handler correctly hooks Ethernet pin re-evaluationRegistering
addUpdateHandler([&](const String&){ readPins(sveltekit); }, false);ensures Ethernet pin discovery and re-init runs whenever IO state changes, without creating ownership issues forsveltekit(captured by reference but owned externally).
258-263: C3-specific guard around rtc_gpio_get_level is a sensible compatibility fixSkipping
rtc_gpio_get_level()under#ifndef CONFIG_IDF_TARGET_ESP32C3avoids calling a non-existent C3 API while still using it on other targets, matching the inline “todo find c3 alternative” note.
304-321: QuinLED Dig Uno v3 preset additions are localized and safeThe added comments and explicit
object["maxPower"] = 50;plus pin assignments for LED and Relay keep changes scoped to the Dig Uno v3 preset and don’t interfere with other board layouts.
469-477: Deferred boardPreset reset via newBoardID still behaves as beforeChecking
newBoardID != UINT8_MAXinloop()and then callingsetBoardPresetDefaults(newBoardID)preserves the previous “apply preset on next loop iteration” behavior, and is compatible with the new Ethernet-pin auto-configuration that runs insidesetBoardPresetDefaultsvia the update handler.
|
@coderabbitai, FT_ETHERNET is defined in features.ini for all boards, which is included in platformio.ini, therefor it is enabled (otherwise I would get compile errors. The the magic pointer heuristic is only temporary to test on pointers |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3360 🏁 Script executed: Length of output: 5116 🏁 Script executed: Length of output: 9665 🏁 Script executed: Length of output: 2026 @ewowi Thank you for the clarification about FT_ETHERNET being enabled. I apologize for the incorrect analysis earlier. I've found the actual root cause of the invalid pointer issue: Root Cause: Dangling Reference in LambdaLook at line 114 in ModuleIO.h: addUpdateHandler([&](const String& originId) { readPins(sveltekit); }, false);The problem:
SolutionsOption 1: Store sveltekit as a member variable (recommended) class ModuleIO : public Module {
private:
ESP32SvelteKit* _sveltekit; // Add member
public:
ModuleIO(PsychicHttpServer* server, ESP32SvelteKit* sveltekit)
: Module("inputoutput", server, sveltekit), _sveltekit(sveltekit) { // Initialize member
EXT_LOGV(MB_TAG, "constructor");
addUpdateHandler([&](const String& originId) {
readPins(_sveltekit); // Use member, not parameter
}, false);
}Option 2: Capture sveltekit by value in lambda addUpdateHandler([this, sveltekit](const String& originId) {
readPins(sveltekit); // Captures pointer by value
}, false);Option 1 is cleaner and safer for long-term maintenance. The dangling reference also explains why your pointer heuristic check is catching these invalid values—they're not real addresses at all. |
|
@coderabbitai, also check these memory addresses , is EXCVADDR: 0xa5a5a8cd okay? [ 3743][D][ModuleIO.h:481] readPins(): [🌙] ESP32 SvelteKit (5344) Try to configure ethernet Core 0 register dump: Backtrace: 0x420836d3:0x3fcbc200 0x420837e5:0x3fcbc280 0x420814a7:0x3fcbc2a0 0x420814c2:0x3fcbc2c0 0x42081b85:0x3fcbc2e0 0x4208e4b1:0x3fcbc310 0x4208e560:0x3fcbc420 0x42094e1e:0x3fcbc440 0x420288ee:0x3fcbc470 0x420288fd:0x3fcbc4b0 0x4037fd21:0x3fcbc4d0 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2225 @ewowi No, EXCVADDR: 0xa5a5a8cd is NOT okay – it's a clear sign of a use-after-free bug! What 0xa5a5a5a5 MeansThe pattern you're seeing is highly diagnostic:
This confirms you're accessing memory that has been freed. Root Cause ConfirmedLooking at line 114 in your ModuleIO.h: addUpdateHandler([&](const String& originId) { readPins(sveltekit); }, false);The bug:
The FixAdd sveltekit as a member variable: class ModuleIO : public Module {
private:
ESP32SvelteKit* _sveltekit; // Add this member
public:
ModuleIO(PsychicHttpServer* server, ESP32SvelteKit* sveltekit)
: Module("inputoutput", server, sveltekit), _sveltekit(sveltekit) { // Initialize it
EXT_LOGV(MB_TAG, "constructor");
// Use the member variable, not the parameter
addUpdateHandler([this](const String& originId) {
readPins(_sveltekit); // Safe: accesses member, not dangling reference
}, false);
}This ensures |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
42-49: Add explicit type conversion for JSON value extraction.Line 46 assigns
pinObject["GPIO"]directly touint8_t pinInfraredwithout an explicit cast. While ArduinoJson supports implicit conversion, using.as<uint8_t>()improves type safety and consistency with line 44 which does use explicit type access.- pinInfrared = pinObject["GPIO"]; + pinInfrared = pinObject["GPIO"].as<uint8_t>();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
firmware/esp32-p4.ini(0 hunks)firmware/esp32-s3.ini(0 hunks)src/MoonBase/Modules/ModuleIO.h(8 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(2 hunks)
💤 Files with no reviewable changes (2)
- firmware/esp32-s3.ini
- firmware/esp32-p4.ini
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
🧬 Code graph analysis (1)
src/MoonBase/Modules/ModuleIO.h (2)
src/MoonBase/Module.h (1)
Module(139-210)src/MoonBase/Module.cpp (5)
Module(308-320)setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
50-82: RMT channel reinitialization logic looks correct.The cleanup sequence (unregister callback → disable → delete → null) followed by fresh initialization is the proper approach for dynamic pin reconfiguration. Using
ESP_ERROR_CHECKenforces fail-fast behavior which is appropriate here since partial RMT state could cause undefined behavior.src/MoonBase/Modules/ModuleIO.h (4)
107-118: Dangling reference issue addressed - sveltekit stored as member.The fix correctly stores
sveltekitas the_sveltekitmember before registering the update handler. This resolves the use-after-free issue identified in PR comments where the lambda previously captured a stack parameter that became invalid after constructor return.
77-82: Enum values properly extended for SPI/PHY pins.The new pin usage enums are correctly placed before
pin_countand align with the corresponding control value strings added at lines 220-224.
296-306: Jumper-based feature switching implemented correctly.The conditional logic properly maps pin 5 to either Infrared or SPI_MISO based on
jumper1state, enabling hardware-level switching between IR receiver and Ethernet functionality for the SE16V1 board.
483-514: Preprocessor guard consistency verified—no ODR violation risk.EthernetSettingsService.h guards the
v_ETH_*members with#ifdef CONFIG_IDF_TARGET_ESP32S3(line 114), which matches the guard used in ModuleIO.h (line 487). Both files use identical preprocessor guard style, ensuring consistent struct layouts across translation units and eliminating the potential for ODR violations.
Back end ======= - IO: jumper1 default false, set Dig-Quad-V3 - Physical layer: nrOfAssignedPins=0 not in monitorpass Virtual layer: mappingTable from vector to array, mappingTableIndexes.clear bugfix, onLayoutPre: realloc mappingTable
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/moonbase/inputoutput.md (1)
65-65: Consider consistent casing for jumper1 references.The documentation uses both lowercase "jumper1" (referring to the UI control) and capitalized "Jumper1" in the same sentence. For clarity, consider using "jumper1" consistently when referring to the control/setting, and reserve "Jumper1" only when referring to the physical hardware component.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/moonbase/inputoutput.md(2 hunks)src/MoonBase/Modules/ModuleIO.h(8 hunks)src/MoonLight/Layers/PhysicalLayer.cpp(2 hunks)src/MoonLight/Layers/VirtualLayer.cpp(10 hunks)src/MoonLight/Layers/VirtualLayer.h(3 hunks)src/MoonLight/Modules/ModuleMoonLightInfo.h(2 hunks)src/MoonLight/Nodes/Effects/E_MoonModules.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
🧬 Code graph analysis (3)
src/MoonLight/Layers/VirtualLayer.h (1)
src/MoonLight/Layers/VirtualLayer.cpp (2)
VirtualLayer(24-41)VirtualLayer(43-61)
src/MoonBase/Modules/ModuleIO.h (2)
src/MoonBase/Module.h (1)
Module(139-210)src/MoonBase/Module.cpp (3)
Module(308-320)addControl(361-370)addControl(361-361)
src/MoonLight/Layers/VirtualLayer.cpp (1)
src/MoonLight/Layers/VirtualLayer.h (1)
VirtualLayer(57-282)
🪛 LanguageTool
docs/moonbase/inputoutput.md
[grammar] ~50-~50: Ensure spelling is correct
Context: ... into (Or shield or controller board or interfae board) - Device (DVC): All of the above...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~65-~65: Ensure spelling is correct
Context: ...hen flash, Nice !!! ### SE16 v1 * Set jumper1 the same as you set it on the board: Ju...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~65-~65: Ensure spelling is correct
Context: ...r1 the same as you set it on the board: Jumper1: on: Infrared, off: Ethernet
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
147-148: LGTM! Critical safety guard prevents crash on allocation failure.The null pointer check prevents dereferencing invalid pointers when memory allocation fails in
onSizeChanged. This is consistent with the existing guard inloop()(line 202) and aligns with the PR's broader safety improvements.src/MoonLight/Layers/PhysicalLayer.cpp (2)
135-139: LGTM!The refactoring to move
nrOfAssignedPins = 0inside theif (!monitorPass)block aligns with the adjacent memset operations and ensures consistent state reset behavior.
210-210: LGTM!The log message is now more descriptive by explicitly naming the variable (
indexP:) rather than using positional formatting.src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
66-66: LGTM!The changes correctly adopt
mappingTableSizeto align with the mappingTable refactor in VirtualLayer.h (vector → raw pointer + explicit size tracking).Also applies to: 87-87
src/MoonLight/Layers/VirtualLayer.h (1)
70-71: LGTM!The mappingTable refactor (vector → raw pointer with explicit size tracking) enables finer control over memory allocation and is consistently applied across the header. The constructor relocation to the .cpp file and the updated bounds check in
isMapped()align with the new memory management approach.Also applies to: 85-85, 212-212
src/MoonBase/Modules/ModuleIO.h (2)
107-107: LGTM!The addition of the
_sveltekitmember and its initialization addresses the dangling reference issue mentioned in the PR objectives. The lambda at line 118 now safely accesses the member throughthisrather than capturing a stack-allocated constructor parameter.Also applies to: 116-116, 118-118
151-152: Past review concern addressed.The
jumper1control now includes an explicit default value (control["default"] = false), addressing the previous review comment about undefined state.src/MoonLight/Layers/VirtualLayer.cpp (5)
24-41: LGTM!The constructor implementation defers mappingTable allocation (commented provisioning code) to lazy initialization during layout passes, reducing boot-time memory pressure.
54-60: LGTM!The destructor correctly uses
freeMB(mappingTable)instead of vector-based cleanup, and loops by reference to avoid unnecessary copies during cleanup.
144-144: LGTM!Bounds checks in
setLight()andgetLight()now usemappingTableSize, consistent with the pointer-based refactor.Also applies to: 176-176
323-340: LGTM!The dynamic reallocation logic correctly resizes
mappingTablewhen the layer size increases, usesreallocMBfor safe reallocation, and initializes new elements with thePhysMap()constructor.
352-355: LGTM!The bound check using
mappingTableSizeensuresaddIndexP()is only invoked for valid indices, preventing out-of-bounds writes.
| void readPins() { | ||
| #if FT_ENABLED(FT_ETHERNET) | ||
| EXT_LOGD(MB_TAG, "Try to configure ethernet"); | ||
| EthernetSettingsService* ess = _sveltekit->getEthernetSettingsService(); | ||
| #ifdef CONFIG_IDF_TARGET_ESP32S3 | ||
| ess->v_ETH_SPI_SCK = UINT8_MAX; | ||
| ess->v_ETH_SPI_MISO = UINT8_MAX; | ||
| ess->v_ETH_SPI_MOSI = UINT8_MAX; | ||
| ess->v_ETH_PHY_CS = UINT8_MAX; | ||
| ess->v_ETH_PHY_IRQ = UINT8_MAX; | ||
| // if ethernet pins change | ||
| // find the pins needed | ||
| for (JsonObject pinObject : _state.data["pins"].as<JsonArray>()) { | ||
| uint8_t usage = pinObject["usage"]; | ||
| uint8_t gpio = pinObject["GPIO"]; | ||
| if (usage == pin_SPI_SCK) ess->v_ETH_SPI_SCK = gpio; | ||
| if (usage == pin_SPI_MISO) ess->v_ETH_SPI_MISO = gpio; | ||
| if (usage == pin_SPI_MOSI) ess->v_ETH_SPI_MOSI = gpio; | ||
| if (usage == pin_PHY_CS) ess->v_ETH_PHY_CS = gpio; | ||
| if (usage == pin_PHY_IRQ) ess->v_ETH_PHY_IRQ = gpio; | ||
| } | ||
|
|
||
| // allocate the pins found | ||
| if (ess->v_ETH_SPI_SCK != UINT8_MAX && ess->v_ETH_SPI_MISO != UINT8_MAX && ess->v_ETH_SPI_MOSI != UINT8_MAX && ess->v_ETH_PHY_CS != UINT8_MAX && ess->v_ETH_PHY_IRQ != UINT8_MAX) { | ||
| // ess->v_ETH_PHY_TYPE = ETH_PHY_W5500; | ||
| // ess->v_ETH_PHY_ADDR = 1; | ||
| ess->v_ETH_PHY_RST = -1; // not wired | ||
| ess->initEthernet(); // restart ethernet | ||
| } | ||
| #endif | ||
| #endif | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Add null-pointer guard for getEthernetSettingsService().
The PR objectives indicate that this code previously crashed with invalid pointer values (e.g., ESS PTR = 0x268). Although getEthernetSettingsService() is now accessed through the stored _sveltekit member (fixing the dangling reference), there's no null-check before dereferencing ess at lines 492–496 and 502–506. If the service is unavailable or FT_ETHERNET is misconfigured, this will crash.
void readPins() {
#if FT_ENABLED(FT_ETHERNET)
EXT_LOGD(MB_TAG, "Try to configure ethernet");
EthernetSettingsService* ess = _sveltekit->getEthernetSettingsService();
+ if (!ess) {
+ EXT_LOGW(MB_TAG, "Ethernet settings service not available");
+ return;
+ }
#ifdef CONFIG_IDF_TARGET_ESP32S3
ess->v_ETH_SPI_SCK = UINT8_MAX;🤖 Prompt for AI Agents
In src/MoonBase/Modules/ModuleIO.h around lines 487 to 518, the call to
_sveltekit->getEthernetSettingsService() can return null but ess is dereferenced
immediately; add a null-pointer guard after obtaining ess (e.g., if (!ess)
return; or skip the ethernet configuration block) so you don't access members or
call initEthernet() when the service is unavailable, ensuring all subsequent
uses of ess are inside the guarded branch.
There was a problem hiding this comment.
This is a typical case the developer should solve. Afaik this will never happen, please check as well 🐰
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the implementation of getEthernetSettingsService
echo "=== Searching for getEthernetSettingsService implementation ==="
rg -nP -A5 'getEthernetSettingsService\s*\(' --type=cpp --type=h
echo ""
echo "=== Searching for ESP32SvelteKit class definition ==="
rg -nP -A10 'class\s+ESP32SvelteKit' --type=cpp --type=h
echo ""
echo "=== Searching for EthernetSettingsService member in ESP32SvelteKit ==="
rg -nP 'EthernetSettingsService.*_ethernet|ethernet.*Service' --type=cpp --type=h | head -30
Length of output: 2374
@ewowi You're absolutely right! 🐰
I've verified the implementation and getEthernetSettingsService() cannot return null:
// lib/framework/ESP32SvelteKit.h:149-152
EthernetSettingsService *getEthernetSettingsService()
{
return &_ethernetSettingsService; // Returns address of member object
}The key point is that _ethernetSettingsService is a direct member object (not a pointer) of ESP32SvelteKit:
// lib/framework/ESP32SvelteKit.h:241
EthernetSettingsService _ethernetSettingsService;Since it returns the address of a member object (&_ethernetSettingsService), the pointer can only be null if the parent ESP32SvelteKit object itself doesn't exist. But _sveltekit is stored in your constructor and is guaranteed to be valid when readPins() executes.
Additionally, both the getter method and your readPins() implementation are guarded by #if FT_ENABLED(FT_ETHERNET), so the code won't compile if the feature is disabled.
The previous crashes (ESS PTR = 0x268, 0x37c) were due to the dangling lambda capture, which you've now fixed by storing _sveltekit as a member. The null-check I suggested isn't necessary here. Thanks for catching this! 👍
🧬 Code Graph Analysis Results
[src/MoonBase/Modules/ModuleDrivers.h] (lines 34-66)
void readPins() {
_moduleIO->read([&](ModuleState& state) {
// find the pins in board definitions
memset(layerP.ledPins, UINT8_MAX, sizeof(layerP.ledPins));
layerP.maxPower = state.data["maxPower"];
EXT_LOGD(ML_TAG, "maxPower %d", layerP.maxPower);
// assign pins (valid only)
for (JsonObject pinObject : state.data["pins"].as<JsonArray>()) {
uint8_t usage = pinObject["usage"];
if (usage >= pin_LED_01 && usage <= pin_LED_20 && GPIO_IS_VALID_OUTPUT_GPIO(pinObject["GPIO"].as<uint8_t>())) {
layerP.ledPins[usage - 1] = pinObject["GPIO"];
}
}
// Remove all UINT8_MAX values by compacting the array
layerP.nrOfLedPins = 0;
for (int readPos = 0; readPos < sizeof(layerP.ledPins); readPos++) {
if (layerP.ledPins[readPos] != UINT8_MAX) { // only pins which have a nrOfLedPins // && layerP.ledsPerPin[layerP.nrOfLedPins] != UINT16_MAX && layerP.nrOfLedPins] != 0
layerP.ledPins[layerP.nrOfLedPins++] = layerP.ledPins[readPos];
}
}
// log pins
for (int i = 0; i < layerP.nrOfLedPins; i++) {
EXT_LOGD(ML_TAG, "ledPins[%d-%d] = %d (#%d)", i, layerP.nrOfLedPins, layerP.ledPins[i], layerP.ledsPerPin[i]);
}
layerP.requestMapPhysical = true;
layerP.requestMapVirtual = true;
}
[src/MoonBase/Modules/ModuleLightsControl.h] (lines 72-86)
void readPins() {
moduleIO.read([&](ModuleState& state) {
pinRelayBrightness = UINT8_MAX;
pinToggleOnOff = UINT8_MAX;
for (JsonObject pinObject : state.data["pins"].as<JsonArray>()) {
uint8_t usage = pinObject["usage"];
if (usage == pin_Relay_Brightness) {
pinRelayBrightness = pinObject["GPIO"];
EXT_LOGD(ML_TAG, "pinRelayBrightness found %d", pinRelayBrightness);
} else if (usage == pin_Button_OnOff) {
pinToggleOnOff = pinObject["GPIO"];
pinMode(pinToggleOnOff, INPUT_PULLUP);
EXT_LOGD(ML_TAG, "pinToggleOnOff found %d", pinToggleOnOff);
}
}
[src/MoonBase/Nodes/Drivers/D_Infrared.h] (lines 40-84)
void readPins() {
moduleIO->read([&](ModuleState& state) {
pinInfrared = UINT8_MAX;
for (JsonObject pinObject : state.data["pins"].as<JsonArray>()) {
uint8_t usage = pinObject["usage"];
if (usage == pin_Infrared) {
pinInfrared = pinObject["GPIO"];
EXT_LOGD(ML_TAG, "pin_Infrared found %d", pinInfrared);
}
}
if (pinInfrared != UINT8_MAX) {
EXT_LOGI(IR_DRIVER_TAG, "Changing to pin #%d", pinInfrared);
if (rx_channel) {
EXT_LOGI(IR_DRIVER_TAG, "Removing callback");
ESP_ERROR_CHECK(rmt_rx_register_event_callbacks(rx_channel, &cbs_empty, receive_queue));
EXT_LOGI(IR_DRIVER_TAG, "Stopping RMT reception");
ESP_ERROR_CHECK(rmt_disable(rx_channel));
EXT_LOGI(IR_DRIVER_TAG, "Deleting old RX channel");
ESP_ERROR_CHECK(rmt_del_channel(rx_channel));
rx_channel = NULL;
}
if (receive_queue) {
vQueueDelete(receive_queue);
receive_queue = NULL;
}
rx_channel_cfg.gpio_num = (gpio_num_t)pinInfrared;
EXT_LOGI(IR_DRIVER_TAG, "create RMT RX channel");
ESP_ERROR_CHECK(rmt_new_rx_channel(&rx_channel_cfg, &rx_channel));
EXT_LOGI(IR_DRIVER_TAG, "Enable RMT RX channel");
ESP_ERROR_CHECK(rmt_enable(rx_channel));
EXT_LOGI(IR_DRIVER_TAG, "Register RX done callback");
receive_queue = xQueueCreate(1, sizeof(rmt_rx_done_event_data_t));
assert(receive_queue);
ESP_ERROR_CHECK(rmt_rx_register_event_callbacks(rx_channel, &cbs, receiveQueue));
EXT_LOGI(IR_DRIVER_TAG, "Arm receive");
ESP_ERROR_CHECK(rmt_receive(rx_channel, raw_symbols, sizeof(raw_symbols), &receive_config));
}
// for (int i = 0; i < sizeof(pins); i++) EXT_LOGD(ML_TAG, "pin %d = %d", i, pins[i]);
}
[src/MoonBase/Modules/ModuleIO.h] (context)
- This file defines IO_PinUsage, IO_Boards, and the ModuleIO class, including the setup for board presets and readPins logic that interacts with UI and board configurations. It uses a Board preset mechanism to assign pin usages for different boards. The code references readPins(), setBoardPresetDefaults(), and onUpdate() to handle updates from the UI and apply new board configurations accordingly.
|
@coderabbitai, please do a review of my change of mappingTable from vector to array |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 58459 @ewowi I've completed a thorough review of your mappingTable vector-to-array conversion. Overall, the change is well-implemented with good memory safety, but I found one bug and a few areas for potential improvement. ✅ What's Working Well1. Memory Management Pattern
2. Bounds Checking Improvements // Before: checked against "used" count (could cause buffer overruns)
if (indexV < mappingTableSizeUsed) { ... }
// After: checks against allocated size (safer)
if (indexV < mappingTableSize) { ... }3. Null-Safety
4. Lazy Allocation Strategy 🐛 Bug Foundsrc/MoonLight/Modules/ModuleLightsControl.h:43 — Incorrect pointer usage: // ❌ Current (takes address of the pointer variable, not the allocated memory)
EXT_LOGI(ML_TAG, "isInPSRAM: mt:%d ...", isInPSRAM(&layerP.layers[0]->mappingTable), ...);
// ✅ Should be (checks the allocated memory)
EXT_LOGI(ML_TAG, "isInPSRAM: mt:%d ...", isInPSRAM(layerP.layers[0]->mappingTable), ...);This will always report the pointer variable's location (stack/data segment), not whether the allocated array is in PSRAM.
|
Back-end ======== Virtual layer: mappingTable realloc: always if size changes, if failed, keep old, init using memset
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/MoonLight/Layers/VirtualLayer.cpp (1)
323-342: GuardingmemsetonmappingTableis a small robustness winThe realloc-and-zero pattern is reasonable, and with your invariants
mappingTable == nullptrimpliesmappingTableSize == 0, somemset(mappingTable, 0, mappingTableSize * sizeof(PhysMap))is safe in practice.To make this intent explicit and silence static analyzers, consider guarding the memset:
- memset(mappingTable, 0, mappingTableSize * sizeof(PhysMap)); // set mappingTable to default PhysMap + if (mappingTable && mappingTableSize) { + memset(mappingTable, 0, mappingTableSize * sizeof(PhysMap)); // set mappingTable to default PhysMap + }This doesn’t change behavior but documents the invariant.
src/MoonLight/Modules/ModuleLightsControl.h (1)
41-44: Fix toisInPSRAMargument formappingTableis correct; optional tweak for indexesSwitching from taking the address of the variable to passing the buffer pointer itself:
isInPSRAM(layerP.layers[0]->mappingTable)now matches the
bool isInPSRAM(void* ptr)signature and actually checks where the mapping table lives. This fixes the earlier bug where&mappingTablereported PSRAM status of the pointer variable, not the data.If you also care about the PSRAM location of
mappingTableIndexes’ underlying storage (rather than the vector object), you could consider:isInPSRAM(layerP.layers[0]->mappingTableIndexes.data())but that’s optional and only affects the debug log.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/moonbase/inputoutput.md(2 hunks)src/MoonLight/Layers/VirtualLayer.cpp(10 hunks)src/MoonLight/Modules/ModuleLightsControl.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/MoonLight/Layers/VirtualLayer.cpp (2)
src/MoonLight/Layers/VirtualLayer.h (1)
VirtualLayer(57-282)src/main.cpp (4)
size(27-45)size(27-27)size(58-61)size(58-58)
src/MoonLight/Modules/ModuleLightsControl.h (1)
src/MoonBase/Utilities.cpp (2)
isInPSRAM(103-126)isInPSRAM(103-103)
🪛 LanguageTool
docs/moonbase/inputoutput.md
[grammar] ~65-~65: Ensure spelling is correct
Context: ...hen flash, Nice !!! ### SE16 v1 * Set jumper1 the same as you set it on the board: on...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
docs/moonbase/inputoutput.md (2)
15-15: Clear and informative Jumper1 description.The addition of the Jumper1 preset is well-documented and provides helpful context for users who need to switch between Infrared and Ethernet modes.
48-51: Terminology section is clear and properly formatted.The naming convention definitions are well-structured and provide helpful clarity on the hardware hierarchy. The past typo fix ("interfae" → "interface") is correctly applied.
src/MoonLight/Layers/VirtualLayer.cpp (4)
24-41: Ctor/dtor ownership ofmappingTablelooks consistentLeaving
mappingTableasnullptrin the constructor and doing all allocation inonLayoutPre, then freeing withfreeMB(mappingTable)in the destructor, forms a clean ownership story. ClearingmappingTableIndexesby reference avoids copies and keeps capacity for reuse, which is good for fragmentation.No functional issues spotted here.
Also applies to: 53-61
92-119:addIndexPstate transitions remain correctThe clarified comments for the
m_zeroLights → m_oneLight → m_moreLightstransitions match the existing logic, and the reuse ofmappingTableIndexesviamappingTableIndexesSizeUsedis still sound.No changes in behavior or correctness concerns here.
143-177: Confirm intent of usingmappingTableSizefor bounds instead of “used” size
setLight/getLightandaddLightnow all guard access withindexV < mappingTableSize, wheremappingTableSizeissize.x * size.y * size.z, andonLayoutPrezero-initializes the full table.This means:
- All virtual voxels in the 3D grid are considered “in-table”, defaulting to
m_zeroLightssemantics untiladdLightmaps them.- The “no mapping” fallback branch in
setLight/getLight(direct channel indexing) will effectively never be used for indices within the virtual grid.If that’s the intended model (full grid backing store, with
m_zeroLightsrepresenting “no physical LED but keep virtual color”), then this is coherent. If you still rely on the direct-channel fallback for some layouts, you may want to double-check call sites that pass indices ≥nrOfLightsbut <mappingTableSize.No hard bug here; just worth explicitly confirming.
Also applies to: 351-356
401-401: Comment-only tweaks are fineThe “single pixel” comments in
drawLine/drawLine3Dwere reformatted only; no behavioral change and nothing problematic.Also applies to: 482-482
docs/moonbase/inputoutput.md
Outdated
| * erase flash first as MoonLight uses a partition scheme with 3MB of flash and no ota. | ||
| * Lolin Wifi fix no -> add as board preset | ||
| * you might need to reset your router if you first run WLED on the same MCU | ||
| * remove fuse then flash, Nice !!! |
There was a problem hiding this comment.
Reduce informality and excessive punctuation in technical documentation.
The phrase "remove fuse then flash, Nice !!!" is too casual and uses excessive exclamation marks. Reword to match the professional tone of technical documentation.
Apply this diff to improve clarity and formality:
-* remove fuse then flash, Nice !!!
+* Remove fuse, then flash📝 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.
| * remove fuse then flash, Nice !!! | |
| * Remove fuse, then flash |
🤖 Prompt for AI Agents
In docs/moonbase/inputoutput.md around line 61, the sentence "remove fuse then
flash, Nice !!!" is informal and uses excessive punctuation; replace it with a
concise, professional phrase such as "Remove the fuse, then flash the device."
or "Remove the fuse before flashing the device." to match the technical
documentation tone and remove extra exclamation marks. Ensure punctuation and
capitalization are correct and consistent with surrounding content.
Installer: updates board presets Front-end ======== Multirow: support property show Back-end ======= - IO: board presets refactor: usage + index, add ButtonToggle (🚧) - Drivers: readPins, use usage + index
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/MoonLight/Layers/VirtualLayer.cpp (2)
143-172: Add null check before accessingmappingTable.The bounds check
indexV < mappingTableSizedoes not guard againstmappingTablebeingnullptr. If allocation fails inonLayoutPre,mappingTableSizecould be non-zero whilemappingTableis null (based on lines 336-338 where size is preserved on realloc failure).void VirtualLayer::setLight(const uint16_t indexV, const uint8_t* channels, uint8_t offset, uint8_t length) { - if (indexV < mappingTableSize) { + if (mappingTable && indexV < mappingTableSize) {
174-207: Add null check before accessingmappingTableingetLight.Same issue as
setLight- accessingmappingTable[indexV]without verifyingmappingTableis non-null.template <typename T> T VirtualLayer::getLight(const uint16_t indexV, uint8_t offset) const { - if (indexV < mappingTableSize) { + if (mappingTable && indexV < mappingTableSize) {
♻️ Duplicate comments (2)
docs/moonbase/inputoutput.md (2)
59-59: Clarify ambiguous phrasing.The phrase "Lolin Wifi fix no -> add as board preset" remains unclear. Please clarify whether this is a TODO, a configuration instruction, or a note about the Lolin WiFi module.
61-61: Improve documentation tone.The phrase "remove fuse then flash" could be more formal for technical documentation. Consider: "Remove fuse, then flash".
🧹 Nitpick comments (2)
src/MoonLight/Layers/VirtualLayer.cpp (2)
24-41: Consider removing or documenting the commented-out allocation code.The commented-out pre-allocation logic (lines 28-40) adds noise. If this is intended as a future reference for claiming more space at boot, consider moving it to a code comment explaining the rationale, or removing it entirely if it's no longer relevant.
59-60: Consider nullifyingmappingTableafter freeing.After calling
freeMB(mappingTable), the pointer becomes dangling. While the destructor runs once during normal object lifetime, settingmappingTable = nullptris a defensive practice that prevents double-free issues if the code is refactored later.// clear mapping table // mappingTable.clear(); freeMB(mappingTable); + mappingTable = nullptr; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/gettingstarted/installer.md(1 hunks)docs/moonbase/inputoutput.md(2 hunks)interface/src/lib/components/moonbase/MultiRow.svelte(2 hunks)platformio.ini(1 hunks)src/MoonBase/Modules/ModuleIO.h(11 hunks)src/MoonLight/Layers/VirtualLayer.cpp(10 hunks)src/MoonLight/Modules/ModuleDrivers.h(1 hunks)src/MoonLight/Modules/ModuleLightsControl.h(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Applied to files:
platformio.ini
🧬 Code graph analysis (2)
src/MoonLight/Layers/VirtualLayer.cpp (2)
src/MoonLight/Layers/VirtualLayer.h (1)
VirtualLayer(57-282)src/main.cpp (4)
size(27-45)size(27-27)size(58-61)size(58-58)
src/MoonLight/Modules/ModuleLightsControl.h (1)
src/MoonBase/Utilities.cpp (2)
isInPSRAM(103-126)isInPSRAM(103-103)
🪛 LanguageTool
docs/gettingstarted/installer.md
[style] ~15-~15: Using many exclamation marks might seem excessive (in this case: 19 exclamation marks for a text that’s 7457 characters long)
Context: ...--------|
| esp32-s3-devkitc-1-n8r8v | 
{: style="width:100px"} | | SE-16p
{: style="width:100px"} |
| esp32-s3-devkitc-1-n16r8v | {: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-s3-atoms3r | 
{: style="width:100px"} | | M5Stack store{:target="_blank"} |
| esp32-s3-zero-n4r2 | 
{: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-d0-16mb | 
{: style="width:100px"} | | Serg74{:target="_blank"}/i/93057/products/2021-08-14T14%3A44%3A14.418Z-shield_v3-1.jpg?1628927139)
| esp32-d0 | 
{: style="width:100px"} | | Dig Uno:
Dig Quad:
| esp32-p4-nano | 
{: style="width:100px"} | esp32-c6-firmware-update *
| Waveshare{:target="_blank"}
ESP32-P4 shield:
| esp32-d0-wrover | 
{: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-d0-wrover-moonbase | {: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-d0-moonbase | {: style="width:100px"} | | |
| esp32-p4-olimex 🚧 | 
{: style="width:100px"} | | Olimex{:target="_blank"} |
| esp32-c3-devkitm-1 | 
{: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-c3-supermini | 
{: style="width:100px"} | | Ali*{:target="_blank"} |
| others | 
docs/moonbase/inputoutput.md
[grammar] ~65-~65: Ensure spelling is correct
Context: ...ove fuse then flash ### SE16 v1 * Set jumper1 the same as you set it on the board: on...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/gettingstarted/installer.md
19-19: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
23-23: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
platformio.ini (1)
59-59: Routine build metadata update.The APP_DATE change from "2025120417" to "2025120714" is a benign, compile-time versioning update with no functional impact. The format is consistent with the existing convention and aligns with the PR development timeline.
interface/src/lib/components/moonbase/MultiRow.svelte (1)
198-200: Non-functional reformatting of dragDisabled condition.The condition has been reformatted but remains logically equivalent. The drag functionality will behave identically.
docs/gettingstarted/installer.md (1)
13-28: LGTM!The table restructuring consolidates the "Shop" and "Board presets" columns effectively, improving readability. The static analysis hints about exclamation marks and list indentation are false positives—the exclamation marks are markdown image syntax (
![...]), and the indentation warnings reference table rows, not actual lists.docs/moonbase/inputoutput.md (3)
15-15: LGTM!Good addition of the Jumper1 preset documentation, which aligns with the new jumper1 control in ModuleIO.h.
48-51: LGTM!The naming convention section is well-structured with clear abbreviations (MCU, MCB, CRB, DVC).
63-65: LGTM!The SE16 v1 section with jumper1 guidance is helpful and aligns with the new board preset logic in ModuleIO.h.
src/MoonLight/Modules/ModuleLightsControl.h (2)
43-43: Good fix for theisInPSRAMcall.This correctly passes the pointer value (
mappingTable) instead of the address of the pointer (&mappingTable). The previous code would have checked if the pointer variable itself (on the stack or in the object) was in PSRAM, not the memory it points to.
78-85: LGTM!The pin usage enum references are correctly updated to match the renamed enum values (
pin_Relay_LightsOn,pin_Button_LightsOn) in theIO_PinUsageenum.src/MoonBase/Modules/ModuleIO.h (5)
87-98: Good fix for the dangling reference issue.Storing
sveltekitas the_sveltekitmember (line 96) and using it in the lambda viathiscapture (line 98) correctly fixes the use-after-free bug described in the PR objectives. The lambda no longer captures a reference to the constructor's stack parameter.
201-219: LGTM!The
PinAssignerhelper class cleanly encapsulates the index-based pin assignment logic. The counter correctly resets to 1 when usage changes, producing 1-based indices that align with the consumer code inModuleDrivers.h.
275-293: Good implementation of jumper1-conditional pin assignment.The conditional logic correctly assigns either Infrared (jumper1=true) or SPI/Ethernet pins (jumper1=false) for the SE16V1 board, aligning with the documentation in
inputoutput.md.
476-507: LGTM!The
readPins()implementation correctly:
- Uses
#ifdef CONFIG_IDF_TARGET_ESP32S3(avoiding the#ifvs#ifdefinconsistency noted in PR discussions)- Accesses
_sveltekitsafely (now a member, not a dangling reference)- Only initializes Ethernet when all required pins are configured
131-133: LGTM!Good that the
jumper1checkbox now has an explicit default value offalse, addressing the earlier review feedback.src/MoonLight/Modules/ModuleDrivers.h (1)
46-49: LGTM!The LED pin assignment logic correctly transitions from the old enum-range approach to the new explicit index-based approach. The bounds check (
index >= 1 && index <= 20) and array access (index-1) properly align with the 1-based indexing produced byPinAssignerin ModuleIO.h.src/MoonLight/Layers/VirtualLayer.cpp (1)
92-121: LGTM!The added state-transition comments (zero→one, one→more, more→more) improve code readability and clarify the mapping logic.
| if (mappingTableSize != size.x * size.y * size.z) { | ||
| PhysMap* newTable = reallocMB<PhysMap>(mappingTable, size.x * size.y * size.z); | ||
| if (newTable) { | ||
| mappingTable = newTable; | ||
| EXT_LOGD(ML_TAG, "realloc mappingTable %d -> %dx%dx%d", mappingTableSize, size.x, size.y, size.z); | ||
| mappingTableSize = size.x * size.y * size.z; | ||
| } else { | ||
| EXT_LOGW(ML_TAG, "realloc mappingTable failed keeping oldSize %d", mappingTableSize); | ||
| } | ||
| } | ||
| if (mappingTable && mappingTableSize) memset(mappingTable, 0, mappingTableSize * sizeof(PhysMap)); // set mappingTable to default PhysMap |
There was a problem hiding this comment.
Realloc failure handling and unconditional memset need attention.
Two issues:
-
Memset placement: Line 340 runs unconditionally after the realloc block. If realloc fails (lines 336-338),
mappingTableSizeretains its old value, but you still memset the old table - potentially clearing valid data you intended to preserve. -
Potential overflow:
size.x * size.y * size.zis computed withuint16_tarithmetic. For dimensions like 256×256×256 this overflows. Consider casting tosize_tbefore multiplication.
if (mappingTableSize != size.x * size.y * size.z) {
- PhysMap* newTable = reallocMB<PhysMap>(mappingTable, size.x * size.y * size.z);
+ size_t newSize = (size_t)size.x * size.y * size.z;
+ PhysMap* newTable = reallocMB<PhysMap>(mappingTable, newSize);
if (newTable) {
mappingTable = newTable;
EXT_LOGD(ML_TAG, "realloc mappingTable %d -> %dx%dx%d", mappingTableSize, size.x, size.y, size.z);
- mappingTableSize = size.x * size.y * size.z;
+ mappingTableSize = newSize;
+ memset(mappingTable, 0, mappingTableSize * sizeof(PhysMap)); // only memset on successful realloc
} else {
EXT_LOGW(ML_TAG, "realloc mappingTable failed keeping oldSize %d", mappingTableSize);
}
}
- if (mappingTable && mappingTableSize) memset(mappingTable, 0, mappingTableSize * sizeof(PhysMap)); // set mappingTable to default PhysMap
+ else if (mappingTable && mappingTableSize) {
+ memset(mappingTable, 0, mappingTableSize * sizeof(PhysMap)); // reset existing table
+ }
}| if (position.x != UINT16_MAX) { // can be set to UINT16_MAX by modifier todo: check multiple modifiers | ||
| uint16_t indexV = XYZUnModified(position); | ||
| if (indexV >= nrOfLights) { | ||
| mappingTable.resize(indexV + 1); // make sure the index fits | ||
| if (indexV < mappingTableSize) { | ||
| nrOfLights = indexV + 1; | ||
| mappingTableSizeUsed = nrOfLights; | ||
| addIndexP(mappingTable[indexV], layerP->indexP); | ||
| } |
There was a problem hiding this comment.
Add null check before accessing mappingTable in addLight.
Consistent with setLight and getLight, add a null check before dereferencing mappingTable.
if (position.x != UINT16_MAX) { // can be set to UINT16_MAX by modifier todo: check multiple modifiers
uint16_t indexV = XYZUnModified(position);
- if (indexV < mappingTableSize) {
+ if (mappingTable && indexV < mappingTableSize) {
nrOfLights = indexV + 1;
addIndexP(mappingTable[indexV], layerP->indexP);
}📝 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.
| if (position.x != UINT16_MAX) { // can be set to UINT16_MAX by modifier todo: check multiple modifiers | |
| uint16_t indexV = XYZUnModified(position); | |
| if (indexV >= nrOfLights) { | |
| mappingTable.resize(indexV + 1); // make sure the index fits | |
| if (indexV < mappingTableSize) { | |
| nrOfLights = indexV + 1; | |
| mappingTableSizeUsed = nrOfLights; | |
| addIndexP(mappingTable[indexV], layerP->indexP); | |
| } | |
| if (position.x != UINT16_MAX) { // can be set to UINT16_MAX by modifier todo: check multiple modifiers | |
| uint16_t indexV = XYZUnModified(position); | |
| if (mappingTable && indexV < mappingTableSize) { | |
| nrOfLights = indexV + 1; | |
| addIndexP(mappingTable[indexV], layerP->indexP); | |
| } |
🤖 Prompt for AI Agents
In src/MoonLight/Layers/VirtualLayer.cpp around lines 350 to 355, add a null
check for mappingTable before dereferencing mappingTable[indexV] in addLight:
ensure mappingTable is not null (e.g., if (mappingTable && indexV <
mappingTableSize) { ... }) and skip or early-return when mappingTable is null,
matching the existing guards used in setLight/getLight so you don't access a
null pointer.
|
@coderabbitai, regarding your comments:
This is another example of developer responsibility, as code needs to run at high speed we are not adding if conditions which the developer can/should prevent from ever happening. So mappingTableSize should always be 0 if mappingTable is 0x0, please also check this is the case freeMB(mappingTable);
freeMB does the nullification of the mappingTable pointer, so not needed anymore, please confirm Potential overflow: size.x * size.y * size.z the mappingTableSize is max 65535 (uint16_t) which is an enormous amount of LEDs and the max we support, if a user defines a space where size.x * size.y * size.z is larger then that, we accept the situation where this overflows and the mapping is done partially. Please add above as learnings |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6009 🏁 Script executed: Length of output: 1838 🏁 Script executed: Length of output: 102 🏁 Script executed: Length of output: 4737 @ewowi Thank you for the clarifications! I've verified your claims. Here are my findings: ✅ Verified Claims1. mappingTableSize invariant - CONFIRMED
3. uint16_t overflow acceptance - CONFIRMED ❌ Correction Needed2. freeMB pointer nullification - DOES NOT WORK AS STATED The void freeMB(T* p, const char* name = nullptr)This uses pass-by-value in C++, meaning To nullify the caller's pointer, the signature would need to be: void freeMB(T*& p, const char* name = nullptr) // Pass by referenceLooking at actual usage patterns throughout the codebase (E_MoonModules.h, E_WLED.h, VirtualLayer.cpp), developers call Impact: This doesn't break your code because you maintain the invariant manually (realloc failure preserves both pointer and size together). But the ✏️ Learnings added
|
@coderabbitai, please verify
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonBase/Utilities.h (1)
247-255:freeMBObjectstill skips destructors; consider callingobj->~T()
allocMBObjectconstructsTvia placement new, butfreeMBObjectonly frees the raw memory and never invokesT’s destructor. IfTis non‑trivially destructible (owns other memory, file handles, etc.), its resources will never be released, which is undefined behavior in general.If this helper is meant only for trivially destructible types, documenting that (or adding a
static_assert(std::is_trivially_destructible<T>::value)) would make it explicit. Otherwise, a minimal fix is to run the destructor before freeing:-// free object -template <typename T> -void freeMBObject(T*& obj) { - freeMB(obj, "object"); -} +// free object +template <typename T> +void freeMBObject(T*& obj) { + if (obj) { + obj->~T(); + freeMB(obj, "object"); + } +}Also applies to: 258-261
🧹 Nitpick comments (3)
interface/src/lib/components/moonbase/MultiRow.svelte (1)
118-136: Search, drag, and display logic look correct; small DRY opportunity
- Using
property.n.filter((_, index) => index < 3 || propertyN.show === true)in both the search predicate and the rendered fields fixes the earlier inconsistency where some visible fields were not searchable.- The updated
dragDisabled={!(findItemInDefinition?.crud == null || findItemInDefinition?.crud?.includes('s'))}correctly enables drag whencrudis absent or explicitly includes's'.To avoid these predicates drifting apart in future edits, you could optionally extract the "visible fields" filter into a small helper/derived value and reuse it in both places.
Also applies to: 202-204, 213-215
src/MoonBase/Modules/ModuleIO.h (1)
131-133: Pin presets andjumper1behavior are coherent, butjumper1changes don’t currently recompute presetsThe new pieces hang together well:
jumper1has a deterministic default (false), avoiding undefined state.indexandsummaryfields (withsummary.show = true) match the MultiRow behavior of showing the first 3 fields plus any markedshow === true.PinAssignercentralizes pin usage & index assignment, and the board presets consistently use it (including SE16 SPI/PHY pins, P4 Nano SDIO, Ethernet, etc.).One behavioral edge case: for SE16 you branch on
_state.data["jumper1"]insidesetBoardPresetDefaults(), butonUpdate()only triggers this function when:
boardPresetchanges, ormoddedis switched back tofalse.Toggling
jumper1alone will not recalculate the pins; users would need to re‑apply the preset (or flipmoddedoff) to switch between Infrared and Ethernet mappings, which doesn’t quite match how the docs describe jumper1.If you want jumper1 to actively switch presets while keeping
modded == false, consider wiring it intoonUpdate:void onUpdate(const UpdatedItem& updatedItem) override { if (updatedItem.name == "boardPreset" && !_state.updateOriginId.contains("server")) { ... - } else if (updatedItem.name == "modded" && !_state.updateOriginId.contains("server")) { // not done by this module: done by UI + } else if (updatedItem.name == "modded" && !_state.updateOriginId.contains("server")) { // not done by this module: done by UI if (updatedItem.value == false) { ... newBoardID = _state.data["boardPreset"]; // run in sveltekit task } + } else if (updatedItem.name == "jumper1" && !_state.updateOriginId.contains("server")) { + // Re-apply board defaults when jumper1 changes on an unmodded config + if (_state.data["modded"] == false) { + newBoardID = _state.data["boardPreset"]; + } } else if (updatedItem.name == "usage" && !_state.updateOriginId.contains("server")) { // not done by this module: done by UI ...Optional micro‑nit: in all the
for (int i = 0; i < sizeof(ledPins); i++)loops, usingstd::size(ledPins)orsizeof(ledPins) / sizeof(ledPins[0])would be slightly safer if these arrays ever change type.Please double‑check that the current “change jumper1 → manually re‑apply preset” behavior is what you want for SE16; if not, the above hook should match the documentation’s intent better.
Also applies to: 187-199, 201-219, 221-273, 275-412, 423-471
docs/moonbase/inputoutput.md (1)
15-22: Docs match the new IO behavior; consider minor consistency and lint tweaksThe new documentation for:
Jumper1(Infrared vs Ethernet),- per‑pin
Usage/Index,- the “Pin usage” list, and
- the SE16 v1 board section
lines up well with the changes in
ModuleIO.h(singlepin_LED+index, jumper1‑controlled SE16 presets, andButton_LightsOnwiring into the lights control module).Two small cleanups to consider:
Markdown list indentation / lint
Nested bullets under “Per pin” and “Pin usage” currently use four spaces and triggermarkdownlint’s MD007 (ul-indent). Reducing the indentation of the nested*items from 4 spaces to 2 will satisfy the linter without changing rendering.Name alignment with UI
The docs say “Button LightsOn”, while the UI option string inModuleIO.his"Button LightOn". For users cross‑referencing the UI with the docs, it may be clearer to standardize on one spelling in both places.Also applies to: 31-38, 49-55, 58-60, 62-63, 67-68
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/moonbase/inputoutput.md(3 hunks)interface/src/lib/components/moonbase/MultiRow.svelte(3 hunks)src/MoonBase/Modules/ModuleIO.h(11 hunks)src/MoonBase/Utilities.h(3 hunks)src/MoonLight/Layers/VirtualLayer.cpp(10 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Use ArduinoJson for JSON serialization and deserialization in C++ backend code
Applied to files:
src/MoonBase/Utilities.h
📚 Learning: 2025-12-07T14:16:03.203Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.203Z
Learning: In VirtualLayer, mappingTableSize should always be 0 when mappingTable is nullptr. This invariant is maintained by developer responsibility: initial state sets both together, realloc success updates both together, realloc failure preserves both together. For performance reasons, no redundant null checks like `if (mappingTable && indexV < mappingTableSize)` are added, as the size check alone is sufficient when the invariant is maintained.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2025-12-07T14:16:03.203Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.203Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
🧬 Code graph analysis (1)
src/MoonBase/Modules/ModuleIO.h (2)
src/MoonBase/Module.h (1)
Module(139-210)src/MoonLight/Modules/ModuleDrivers.h (1)
readPins(34-67)
🪛 LanguageTool
docs/moonbase/inputoutput.md
[style] ~59-~59: For conciseness, consider replacing this expression with an adverb.
Context: ...tition scheme with 3MB of flash (no ota at the moment). * You might need to reset your router...
(AT_THE_MOMENT)
[style] ~61-~61: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1811 characters long)
Context: ...he same MCU and no new IP is assigned. !!! Tip Dig Uno: Remove fuse to connect U...
(EN_EXCESSIVE_EXCLAMATION)
[grammar] ~68-~68: Ensure spelling is correct
Context: ...Light/gettingstarted/installer/) * Set jumper1 the same as you set it on the board: on...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/moonbase/inputoutput.md
19-19: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
34-34: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
src/MoonBase/Modules/ModuleIO.h (2)
20-63: EnumIO_PinUsageandusageselect values stay in syncThe reworked
IO_PinUsageenum (singlepin_LED, renamed button/relay usages, new SPI/PHY entries) is correctly mirrored by theusageselect’saddControlValuesequence, including the newly added"SPI SCK","SPI MISO","SPI MOSI","PHY CS","PHY IRQ", and"Reserved"entries. That keeps stored values and presets consistent across upgrades.No changes needed here.
Also applies to: 142-186
87-99: Use of_sveltekitinreadPins()fixes the previous dangling-capture issueStoring
ESP32SvelteKit* _sveltekitas a member and having the update handler callreadPins()(without capturing the constructor parameter by reference) resolves the earlier use-after-free problem from the lambda.
readPins()itself is correctly guarded:
- Compiled only when
FT_ENABLED(FT_ETHERNET)is true.- SPI/PHY pin mapping and
initEthernet()calls are further wrapped in#ifdef CONFIG_IDF_TARGET_ESP32S3, so non‑S3 targets won’t touch the new members.- It safely initializes all SPI/PHY pins to
UINT8_MAX, scans_state.data["pins"], and restarts Ethernet only when all required pins are present.This looks solid and consistent with the intended ESP32S3+W5500 flow.
Also applies to: 473-504, 507-508
src/MoonBase/Utilities.h (1)
214-222: Pointer-referencefreeMBchange is sound; semantics match current call sitesSwitching
freeMBto takeT*&and null the pointer is a good improvement for member pointers likemappingTableand other long‑lived allocations; existing calls that pass plain variables (not temporaries) remain valid and now get the extra safety of being nulled after free. For allocator-style call sites that accept the pointer by value (e.g.VectorRAMAllocator::deallocate,JsonRAMAllocator::deallocate), the nulling only affects the local parameter, which is expected and doesn’t introduce any behavioral regressions.The change to
JsonRAMAllocator::deallocateto callfreeMB(p, "json")directly is also fine: the template will instantiatefreeMB<void>(void*& p, ...), and the body only usespas avoid*for heap APIs and logging, so there’s no incomplete‑type issue.Also applies to: 229-232, 235-239
src/MoonLight/Layers/VirtualLayer.cpp (5)
77-104:addIndexP’s transitions (zero → one → many) and index bookkeeping look consistentThe logic for transitioning
PhysMapfromm_zeroLights→m_oneLight→m_moreLightsis coherent:
m_zeroLights: single assignment ofindexP→m_oneLight.m_oneLight: incrementsmappingTableIndexesSizeUsed, then eitherpush_backor overwrite the slot and setsphysMap.indexesto the new slot index.m_moreLights: appendsindexPintomappingTableIndexes[physMap.indexes].Given
mappingTableIndexesSizeUsedis reset and all inner vectors are cleared inonLayoutPre, this keepsphysMap.indexeswithin bounds while reusing capacity.
128-157: Bounds checks usingmappingTableSizeare correct and match your invariantUsing
if (indexV < mappingTableSize)in bothsetLightandgetLightis the right bound now thatmappingTableis a rawPhysMap*with an explicitmappingTableSize. The logic cleanly falls back to direct channel access whenindexVis beyond the mapped range.Given your stated invariant that
mappingTableSizeis 0 whenevermappingTableisnullptr, and that they are always updated together, omitting an explicitmappingTablenull check here is fine and avoids extra branches. Based on learnings, this matches the intended design.Also applies to: 159-192
336-345:addLight’s bound check aligns with the mappingTable invariantsIn
addLight, theif (indexV < mappingTableSize)guard before accessingmappingTable[indexV]is the right protection against out‑of‑bounds writes when the virtual index exceeds the allocated table. The fallback path that zeroes the unmapped physical light forposition.x == UINT16_MAXis unchanged.Given the invariant that
mappingTableSizeis 0 whenmappingTableisnullptr, skipping an explicitmappingTablenull check here is consistent with the rest of the code and avoids redundant branching. Based on learnings, this is intentional.
379-397: Comment-only tweaks indrawLineanddrawLine3DThe only changes here are comment placement (
} // single pixel), with no functional impact on the line-drawing algorithms.Also applies to: 459-482
24-26: Fix the invariant violation inonLayoutPrewhen reallocating to size 0When
reallocMBreturns nullptr for a zero-size reallocation, mappingTableSize must be set to 0 to maintain the invariant that mappingTable==nullptr iff mappingTableSize==0. Add an else clause to handle the zero-size case, or restructure the realloc logic to update mappingTableSize=0 when the new size is 0 and realloc succeeds (returns nullptr).The invariant is critical because bounds checks like
indexV < mappingTableSizein setLight/addLight rely on it to avoid redundant null checks. Violating this invariant can cause nullptr dereferences.⛔ Skipped due to learnings
Learnt from: ewowi Repo: MoonModules/MoonLight PR: 0 File: :0-0 Timestamp: 2025-12-07T14:16:03.203Z Learning: In VirtualLayer, mappingTableSize should always be 0 when mappingTable is nullptr. This invariant is maintained by developer responsibility: initial state sets both together, realloc success updates both together, realloc failure preserves both together. For performance reasons, no redundant null checks like `if (mappingTable && indexV < mappingTableSize)` are added, as the size check alone is sufficient when the invariant is maintained.
| // resetMapping | ||
|
|
||
| mappingTableIndexesSizeUsed = 0; // do not clear mappingTableIndexes, reuse it | ||
| for (std::vector<uint16_t>& mappingTableIndex : mappingTableIndexes) { | ||
| mappingTableIndex.clear(); | ||
| } | ||
|
|
||
| if (mappingTableSize != size.x * size.y * size.z) { | ||
| PhysMap* newTable = reallocMB<PhysMap>(mappingTable, size.x * size.y * size.z); | ||
| if (newTable) { | ||
| mappingTable = newTable; | ||
| EXT_LOGD(ML_TAG, "realloc mappingTable %d -> %dx%dx%d", mappingTableSize, size.x, size.y, size.z); | ||
| mappingTableSize = size.x * size.y * size.z; | ||
| } else { | ||
| EXT_LOGW(ML_TAG, "realloc mappingTable failed keeping oldSize %d", mappingTableSize); | ||
| } | ||
| } | ||
|
|
||
| if (mappingTable && mappingTableSize) memset(mappingTable, 0, mappingTableSize * sizeof(PhysMap)); // on layout, set mappingTable to default PhysMap | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
ESP-IDF heap_caps_realloc_prefer size 0 semantics documentation
💡 Result:
Short answer: ESP-IDF documents that heap_caps_realloc_prefer(ptr, size, ...) follows realloc semantics — passing size == 0 frees the buffer. See the API docs (size: "0 to free the buffer"). [1][2]
Sources:
- ESP-IDF v4.4 mem_alloc (heap_caps_realloc: "size: ... or 0 to free the buffer"). [1]
- ESP-IDF stable (v5.x) mem_alloc (heap_caps_realloc_prefer: "size -- Size of the new buffer requested, or 0 to free the buffer."). [2]
onLayoutPre needs explicit handling for zero-size reallocation to maintain the mapping invariant
The reset sequence correctly manages mappingTableIndexes and reallocates mappingTable when needed, but has an edge case: when size.x * size.y * size.z == 0, heap_caps_realloc_prefer(mappingTable, 0) frees the buffer per ESP-IDF semantics and returns nullptr. The else branch then logs "realloc failed" while keeping the old mappingTableSize, leaving mappingTable as nullptr with mappingTableSize > 0—violating the invariant that mappingTableSize == 0 when mappingTable == nullptr. This breaks the assumption underlying the final memset check.
Add explicit zero-size handling before the realloc:
+ uint32_t newSize = size.x * size.y * size.z;
+ if (newSize == 0) {
+ freeMB(mappingTable);
+ mappingTable = nullptr;
+ mappingTableSize = 0;
- if (mappingTableSize != size.x * size.y * size.z) {
+ } else if (mappingTableSize != newSize) {
PhysMap* newTable = reallocMB<PhysMap>(mappingTable, size.x * size.y * size.z);
if (newTable) {
mappingTable = newTable;
EXT_LOGD(ML_TAG, "realloc mappingTable %d -> %dx%dx%d", mappingTableSize, size.x, size.y, size.z);
- mappingTableSize = size.x * size.y * size.z;
+ mappingTableSize = newSize;
} else {
EXT_LOGW(ML_TAG, "realloc mappingTable failed keeping oldSize %d", mappingTableSize);
}
}Back-end ======== - IO: add Dig-Octa-32-8L - LightsControl: Add Quin and Orange palette - Moonlight effects: random, use palette
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/MoonBase/Modules/ModuleIO.h (1)
87-87: Consider making_sveltekitprivate or protected.The
_sveltekitmember is declared public, which is unusual for internal state. Unless other modules need direct access to this pointer, consider making itprivateorprotectedto better encapsulate the class internals.Apply this diff if direct external access is not required:
- ESP32SvelteKit* _sveltekit; + + private: + ESP32SvelteKit* _sveltekit; + public: ModuleIO(PsychicHttpServer* server, ESP32SvelteKit* sveltekit) : Module("inputoutput", server, sveltekit) {src/MoonLight/Modules/ModuleLightsControl.h (2)
175-188: Quin/Orange palette loops: minor type and comment cleanupsLogic of both new palettes is fine, but there are a couple of polish points:
sizeissize_twhileiisint, which can trigger signed/unsigned comparison warnings. Usingsize_t(orauto) foriwould be cleaner.- For the Orange palette, the comment says “from orange to blue” but the gradient is actually along
(255, 0, 0) -> (255, 255, 0)(red to yellow). The comment should be updated to match the implemented colors.Example refactor:
- } else if (updatedItem.value == 9) { // Quin palette - size_t size = sizeof(layerP.palette.entries) / sizeof(CRGB); - for (int i = 0; i < size; i++) { + } else if (updatedItem.value == 9) { // Quin palette + const size_t size = sizeof(layerP.palette.entries) / sizeof(CRGB); + for (size_t i = 0; i < size; i++) { layerP.palette[i] = CRGB(map(i, 0, size - 1, 255, 0), map(i, 0, size - 1, 31, 0), map(i, 0, size - 1, 0, 255)); // from orange to blue } } else if (updatedItem.value == 10) { // Orange palette - size_t size = sizeof(layerP.palette.entries) / sizeof(CRGB); - for (int i = 0; i < size; i++) { - layerP.palette[i] = CRGB(255, map(i, 0, size - 1, 0, 255), 0); // from orange to blue + const size_t size = sizeof(layerP.palette.entries) / sizeof(CRGB); + for (size_t i = 0; i < size; i++) { + layerP.palette[i] = CRGB(255, map(i, 0, size - 1, 0, 255), 0); // from red to yellow }
47-69: Prefer explicit lambda captures over[&]for long‑lived handlersBoth
_fileManager->addUpdateHandler([&](...) { ... })andmoduleIO.addUpdateHandler([&](...) { readPins(); }, false);only need access tothis(and member functions). Using[&]is broader than necessary and, as you already hit elsewhere in this project, makes it easy to accidentally capture stack variables by reference and introduce use‑after‑free bugs.Consider tightening captures, e.g.:
- _fileManager->addUpdateHandler([&](const String& originId) { + _fileManager->addUpdateHandler([this](const String& originId) { ... - moduleIO.addUpdateHandler([&](const String& originId) { readPins(); }, false); + moduleIO.addUpdateHandler([this](const String& originId) { readPins(); }, false);This keeps the behavior but avoids future accidental dangling references.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/MoonBase/Modules/ModuleIO.h(11 hunks)src/MoonLight/Modules/ModuleLightsControl.h(5 hunks)src/MoonLight/Nodes/Effects/E_MoonLight.h(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
📚 Learning: 2025-12-07T14:16:03.203Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.203Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
🧬 Code graph analysis (3)
src/MoonLight/Modules/ModuleLightsControl.h (2)
src/MoonBase/Utilities.cpp (2)
isInPSRAM(103-126)isInPSRAM(103-103)src/main.cpp (4)
size(27-45)size(27-27)size(58-61)size(58-58)
src/MoonBase/Modules/ModuleIO.h (3)
src/MoonBase/Module.h (1)
Module(139-210)src/MoonBase/Module.cpp (5)
Module(308-320)setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)src/MoonLight/Modules/ModuleDrivers.h (1)
readPins(34-67)
src/MoonLight/Nodes/Effects/E_MoonLight.h (3)
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (5)
tags(19-19)tags(55-55)tags(90-90)loop(235-259)loop(328-358)src/MoonLight/Modules/ModuleLightsControl.h (1)
loop(259-340)src/MoonLight/Nodes/Effects/E__Sandbox.h (2)
loop(45-59)loop(86-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
159-170: RandomEffect palette integration looks correct and consistentUsing
ColorFromPalette(layer->layerP->palette, random8())plugs RandomEffect into the active palette in the same way as other effects here, and the updated"🔥🎨"tag matches the “palette-aware” tagging used elsewhere. No functional issues spotted with these changes.src/MoonBase/Modules/ModuleIO.h (3)
96-98: LGTM! Dangling-capture fix correctly implemented.Storing the
sveltekitpointer as a member (line 96) and having the lambda callreadPins()(line 98) resolves the previous dangling-reference issue. The lambda now safely uses the member variable instead of capturing the constructor parameter by reference.
201-219: Excellent abstraction for pin assignment logic.The
PinAssignerclass cleanly encapsulates the usage-tracking and auto-increment logic for pin indexing. This eliminates repetitive index management across board presets and makes the preset definitions (lines 278–415) more readable and maintainable.
477-508: Reconsider error handling design forinitEthernet()—void return type prevents caller-level failure detection.The call to
ess->initEthernet()at line 504 does occur without error checking. However, the root issue is architectural:initEthernet()returnsvoidand contains no internal logging or error reporting. This means the caller cannot detect initialization failures (e.g., fromETH.begin(),SPI.begin(), or_fsPersistence.readFromFS()), and they will fail silently.Adding error checking at line 504 alone won't help—the method itself must be redesigned to return a status code or add internal logging/error handling. Verify with the Ethernet subsystem maintainer whether
initEthernet()should log failures internally or return a status that the caller can act on.src/MoonLight/Modules/ModuleLightsControl.h (3)
41-45: PSRAM logging now uses correct data pointersThe
isInPSRAMcall now passeslayerP.layers[0]->mappingTable,layerP.layers[0]->mappingTableIndexes.data(), andlayerP.lights.channelsdirectly, which matches thebool isInPSRAM(void*)signature and avoids the previous pointer-to-pointer issue. AssuminglayerP.layers[0]is valid bybegin(), this looks correct and is safe withisInPSRAM’s null check.
72-85: Updated pin usage mappings look consistentSwitching the checks to
pin_Relay_LightsOnandpin_Button_LightsOnwhile still populatingpinRelayBrightnessandpinToggleOnOffkeeps the runtime behavior coherent with the “lights on” semantics and the later use inonUpdateandloop(). No functional issues here.
96-121: UI defaults and palette options remain index‑consistent
- Increasing the
brightnessdefault to20is harmless and within expected 0–255 range.- The palette option order (
Cloud..Orange) still aligns with theonUpdateindex handling: 0–7 map to the predefined FastLED palettes, 8 to the random CHSV fill, 9 to Quin, and 10 to Orange, withdefault = 6correctly selectingPartyColors_p.No index/label mismatch spotted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
42-50: Initialization and pin reading logic looks correct.Resetting
pinInfraredtoUINT8_MAXat the start ensures stale values are cleared. However, line 46 omits the.as<uint8_t>()cast that's used elsewhere (e.g.,ModuleDrivers.hline 47). While ArduinoJson's implicit conversion should work, consider adding the explicit cast for consistency.- pinInfrared = pinObject["GPIO"]; + pinInfrared = pinObject["GPIO"].as<uint8_t>();src/MoonLight/Modules/ModuleLightsControl.h (1)
159-188: New palette implementations are correct, but consider consistency fix.The
nrOfPaletteEntriesvariable is a good addition for clarity. The new Quin and Orange palettes correctly use this variable. However, line 178 (Random palette) still usessizeof(layerP.palette.entries) / sizeof(CRGB)directly in the loop instead ofnrOfPaletteEntries.} else if (updatedItem.value == 8) { - for (int i = 0; i < sizeof(layerP.palette.entries) / sizeof(CRGB); i++) { + for (size_t i = 0; i < nrOfPaletteEntries; i++) { layerP.palette[i] = CHSV(random8(), 255, 255); // take the max saturation, max brightness of the colorwheel } } else if (updatedItem.value == 9) { // Quin palettesrc/MoonBase/Modules/ModuleIO.h (1)
185-186: Consider0instead ofUINT8_MAXfor the index default.Setting
default = UINT8_MAXfor theindexcontrol may cause confusion sincePinAssigner.assignPin()starts indexing at 1. If a user manually adds a pin without setting the index, downstream code comparingindex >= 1 && index <= 20(as seen inModuleDrivers.h) will fail to match, which might be intentional. However,0would be more intuitive as "unassigned" and consistent withusage = 0meaning "Unused".control = addControl(rows, "index", "number", 1, 32); // max 32 of one type, e.g 32 led pins - control["default"] = UINT8_MAX; + control["default"] = 0;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/gettingstarted/installer.md(1 hunks)src/MoonBase/Modules/ModuleIO.h(11 hunks)src/MoonBase/NodeManager.h(2 hunks)src/MoonLight/Modules/ModuleDrivers.h(2 hunks)src/MoonLight/Modules/ModuleLightsControl.h(7 hunks)src/MoonLight/Modules/ModuleLiveScripts.h(1 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonBase/NodeManager.h
🧰 Additional context used
🧬 Code graph analysis (3)
src/MoonLight/Modules/ModuleDrivers.h (2)
src/MoonLight/Modules/ModuleLightsControl.h (1)
readPins(72-86)src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
readPins(40-84)
src/MoonBase/Modules/ModuleIO.h (2)
src/MoonLight/Modules/ModuleDrivers.h (1)
readPins(34-67)src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)
src/MoonLight/Modules/ModuleLightsControl.h (1)
src/MoonBase/Utilities.cpp (2)
isInPSRAM(103-126)isInPSRAM(103-103)
🪛 LanguageTool
docs/gettingstarted/installer.md
[style] ~15-~15: Using many exclamation marks might seem excessive (in this case: 20 exclamation marks for a text that’s 7726 characters long)
Context: ...-|----------------------|
| esp32-d0 | {: style="width:100px"} | | Dig Uno:
Dig Quad:
| esp32-d0-16mb | {: style="width:100px"} | | Dig Octa:
Serg ESP32{:target="_blank"} and Shield
| esp32-s3-devkitc-1-n8r8v | {: style="width:100px"} | | SE-16p{: style="width:100px"} |
| esp32-s3-devkitc-1-n16r8v | {: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-s3-atoms3r | {: style="width:100px"} | | M5Stack store{:target="_blank"} |
| esp32-s3-zero-n4r2 | {: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-p4-nano | {: style="width:100px"} | esp32-c6-firmware-update *
| Waveshare{:target="_blank"}
ESP32-P4 shield:
| esp32-d0-wrover | {: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-d0-wrover-moonbase | {: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-d0-moonbase | {: style="width:100px"} | | |
| esp32-p4-olimex 🚧 | {: style="width:100px"} | | Olimex{:target="_blank"} |
| esp32-c3-devkitm-1 | {: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-c3-supermini | {: style="width:100px"} | | Ali*{:target="_blank"} |
| others | 
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (16)
src/MoonLight/Modules/ModuleLiveScripts.h (1)
40-40: Good fix for the dangling reference capture.Changing the lambda capture from
[&]to[this]correctly addresses the dangling reference issue discussed in the PR comments. The outer lambda is stored byaddUpdateHandlerand invoked later, so capturing by reference would capture stack variables that go out of scope. The inner lambdas (lines 43, 48, 66) still safely use[&]since they execute synchronously within theread()calls.src/MoonLight/Nodes/Drivers/D_Infrared.h (2)
53-81: RMT channel reconfiguration logic is well-structured.The cleanup and reinitialization sequence follows the correct order: unregister callbacks → disable → delete channel → create new channel → enable → register callbacks. The use of
cbs_emptyfor cleanup andcbsfor the new registration is appropriate.One consideration:
ESP_ERROR_CHECKwill abort on any failure. For a runtime reconfiguration triggered by pin changes, you might want graceful error handling to avoid crashing on transient issues. However, this may be acceptable for driver-level code.
93-93: Correct lambda capture fix.Changing from
[&]to[this]addresses the dangling reference issue, consistent with the same fix applied across other modules in this PR.src/MoonLight/Modules/ModuleDrivers.h (2)
76-76: Lambda capture fix applied correctly.The
[this]capture ensuresreadPins()can be called safely when the update handler fires later.
46-49: Index-based LED pin mapping approach is sound.The new logic at lines 46-49 validates the
indexfield within[1, 20]before mapping toledPins[index-1]. This is cleaner than the previous approach. The implementation is supported byPinAssigner::assignPin()(ModuleIO.h:211), which automatically assigns sequential index values (argCounter) to each pin within its usage group. All existing board configurations assign LED pins within the supported [1, 20] range, so the change is compatible across all platform presets.docs/gettingstarted/installer.md (1)
13-28: Table restructuring looks good.The consolidation of "Shop" and "Board presets" into a single column simplifies the layout while retaining all the essential information. The static analysis warning about exclamation marks is a false positive—they're part of Markdown image syntax
.Note: Lines 24 and 28 have empty "Shop & Board presets" cells for
esp32-d0-moonbaseandothers. This appears intentional but may warrant placeholder text like "Generic" or "-" for visual consistency.src/MoonLight/Modules/ModuleLightsControl.h (5)
43-43: Critical bug fix: isInPSRAM now receives pointer values correctly.This addresses the bug identified in the PR comments where
isInPSRAM(&layerP.layers[0]->mappingTable)was passing the address of the pointer rather than the pointer's value. The corrected calls now properly pass the pointer values for memory location checking.
48-48: Lambda capture fix applied correctly.Changing to
[this]ensures the file manager update handler doesn't capture dangling references.
68-68: Lambda capture fix applied correctly.Same pattern as other modules—
[this]capture for stored update handlers.
78-81: Pin usage enum renames improve clarity.The changes from
pin_Relay_Brightnesstopin_Relay_LightsOnandpin_Button_OnOfftopin_Button_LightsOnbetter reflect their actual purpose.
111-121: Palette name simplification and additions look good.The shortened palette names (e.g.,
CloudColors→Cloud) improve UI readability. The newQuinandOrangepalettes expand user options appropriately.src/MoonBase/Modules/ModuleIO.h (5)
94-96: Dangling reference fix looks correct.Storing
sveltekitas a member (_sveltekit) and capturing[this]in the lambda properly addresses the use-after-free bug discussed in the PR comments. The lambda now safely accesses_sveltekitthrough the object's member rather than a dangling stack reference.
199-217: LGTM!The
PinAssignerhelper class cleanly encapsulates the auto-incrementing index logic per usage type. The post-increment on line 211 correctly assigns indices starting at 1 for each distinct usage category.
283-291: Verifyjumper1state is available during first board preset initialization.The
jumper1check reads from_state.data["jumper1"], but on first boot (before state is loaded from storage), this value may benullor undefined rather than the defaultfalse. Sinceif (null)evaluates to false in C++/ArduinoJson, this should default to the Ethernet path, but the behavior may be unintentional if the user hasn't explicitly configured it yet.Consider explicitly handling the case where
jumper1is not yet set:- if (_state.data["jumper1"]) { + if (_state.data["jumper1"] | false) { // Default to false if not setOr verify that state defaults are always applied before
setBoardPresetDefaultsis called.
475-506: LGTM!The
readPins()implementation correctly:
- Guards with
FT_ENABLED(FT_ETHERNET)for feature toggling- Uses
#ifdef CONFIG_IDF_TARGET_ESP32S3(correct preprocessor check as discussed in PR comments)- Initializes all SPI/PHY pins to
UINT8_MAXbefore scanning- Only triggers
initEthernet()when all 5 required pins are configuredThis addresses the ESP32S3 Ethernet configuration flow properly.
508-510: LGTM!The
_sveltekitmember is correctly declared as private, completing the fix for the dangling reference issue.
Back end
Summary by CodeRabbit
New Features
Bug Fixes
Chores / Docs
✏️ Tip: You can customize this high-level summary in your review settings.