disable SPC optimizations when outputs overlap (solves #296) #315
Conversation
support for "custom bus indices" that lead to overlaping output pixel ranges
📝 WalkthroughWalkthroughAdds runtime overlap detection when registering buses: new bus ranges are compared against existing ranges, overlaps set Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (20)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @wled00/bus_manager.cpp:
- Around line 1295-1322: The code dereferences newBus (newBus->getStart(),
newBus->getLength()) before checking for null/isOk; move the null/isOk check to
happen immediately after obtaining newBus (Bus *newBus = busses[numBusses]) and
only compute newStart/newEnd if newBus != nullptr && newBus->isOk(); guard
getLength() so if length==0 you treat the range as empty (skip overlap checks)
or avoid unsigned underflow when computing newEnd (e.g., check length>0 before
doing length-1); also add a trailing newline to the USER_PRINTF call in the
overlap branch to avoid concatenated logs; symbols to update: newBus,
getStart(), getLength(), newStart/newEnd calculation, USER_PRINTF,
overlapingBusses and slowMode assignment.
🧹 Nitpick comments (2)
wled00/bus_manager.h (2)
376-376: Address or remove the TODO-style comment.This comment
// ??? not implemented???indicates uncertainty about whetherBusNetwork::setColorOrder()has an implementation. If the method is truly unimplemented, consider adding a stub implementation or documenting the intentional no-op. If it's implemented elsewhere, remove this comment to avoid confusion.
510-510: Typo: "overlapingBusses" should be "overlappingBusses".The word "overlapping" has two 'p's. Consider fixing for consistency and readability. Note that this typo is consistent throughout the PR (including the branch name), so a bulk rename would be needed.
✏️ Suggested fix
- bool overlapingBusses = false; // WLEDMM needed to enforce "slowMode" when busses overlap (=custom bus start indices) + bool overlappingBusses = false; // WLEDMM needed to enforce "slowMode" when busses overlap (=custom bus start indices)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/bus_manager.cppwled00/bus_manager.h
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/bus_manager.cppwled00/bus_manager.h
🧠 Learnings (1)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
🧬 Code graph analysis (1)
wled00/bus_manager.h (1)
wled00/bus_manager.cpp (2)
setColorOrder(259-263)setColorOrder(259-259)
⏰ 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). (20)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
🔇 Additional comments (3)
wled00/bus_manager.h (1)
467-468: LGTM - slowMode preservation logic is correct.The conditional
if (isRTMode || !overlapingBusses) slowMode = isRTMode;correctly preservesslowMode = truewhen buses overlap, while allowing reset when there's no overlap. This ensures the SPC optimization remains disabled for overlapping bus configurations.wled00/bus_manager.cpp (2)
1342-1342: LGTM - properly resets overlap state on cleanup.Resetting
overlapingBusses = falseinremoveAll()ensures the flag is cleared when all buses are removed, preventing stale state from affecting subsequent bus configurations.
1460-1465: LGTM - tightened canAllShow() gate logic.Adding the
isOk()check beforecanShow()is a sensible improvement. This prevents waiting for buses that aren't properly initialized, which could otherwise block indefinitely or cause unnecessary delays.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @wled00/bus_manager.cpp:
- Around line 1299-1300: The code computes newEnd using unsigned arithmetic that
underflows when newBus->getLength() == 0; modify the logic so you don't subtract
1 from an unsigned zero — either check newBus->isOk() or check
newBus->getLength() == 0 before computing newEnd and compute lengthMinusOne as
(len ? len - 1 : 0), e.g. obtain unsigned len = newBus->getLength(); and set
newEnd = newStart + (len ? len - 1 : 0); or move the isOk() check to run before
computing newEnd to avoid using invalid bus lengths.
🧹 Nitpick comments (2)
wled00/bus_manager.cpp (2)
1304-1304: Redundant nullptr check.The
newBus != nullptrcheck is unnecessary since line 1297 already returns early whennewBusis null.Suggested simplification
- if (newBus != nullptr && newBus->isOk()) { + if (newBus->isOk()) {
1321-1322: Typo in variable name:overlapingBusses.The variable name is missing a 'p' - should be
overlappingBusses. This appears in both this file andbus_manager.h, so fixing would require updating both locations.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/bus_manager.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/bus_manager.cpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/bus_manager.cpp
⏰ 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). (20)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32c3dev_4MB_M)
🔇 Additional comments (2)
wled00/bus_manager.cpp (2)
1342-1343: LGTM!Correctly resets both
slowModeandoverlapingBussesflags when all buses are removed.
1461-1466: LGTM!The additional
isOk()check ensures only valid buses are considered when evaluating show readiness. This aligns with similar guard patterns used insetPixelColor(),getPixelColor(), andsetStatusPixel().
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @wled00/bus_manager.h:
- Line 376: The header declares BusNetwork::setColorOrder but there is no
implementation, causing linker errors; either implement
BusNetwork::setColorOrder in bus_manager.cpp mirroring the behavior/signature of
BusDigital::setColorOrder (accepting uint8_t colorOrder and updating the
network's internal color order state and any dependent configuration) or remove
the declaration from bus_manager.h so it is not exposed; ensure the implemented
method name and signature exactly match the declaration
(BusNetwork::setColorOrder(uint8_t)) and update any callers accordingly.
🧹 Nitpick comments (1)
wled00/bus_manager.cpp (1)
1295-1324: Overlap detection logic is correct, with minor redundancy.The algorithm correctly identifies overlapping bus ranges using the standard overlap check. A few observations:
Line 1304: The
newBus != nullptrcheck is redundant after the early exit on line 1297.Line 1322: The condition
numBusses < 1resets flags, but whennumBusses == 0, the loop never executes andfoundOverlapcan never be true, making this effectively a no-op. Consider removing or clarifying intent.Line 1300: Safe in practice since valid busses have
getLength() >= 1, but the unsigned subtraction could theoretically underflow if an invalid bus with length 0 slipped through.These are minor clarity concerns; the core logic is sound.
♻️ Optional: Simplify redundant checks
Bus *newBus = busses[numBusses]; if (newBus == nullptr) return numBusses; // WLEDMM early exit if bus creation failed unsigned newStart = newBus->getStart(); unsigned newEnd = newStart + max(newBus->getLength() - 1, 0); // "max" needed for single-pixel busses // WLEDMM check if added bus overlaps with any existing bus bool foundOverlap = false; unsigned busCount = getNumBusses(); - if (newBus != nullptr && newBus->isOk()) { + if (newBus->isOk()) { // newBus != nullptr already verified above for (unsigned i=0; i<busCount; i++) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/bus_manager.cppwled00/bus_manager.h
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/bus_manager.hwled00/bus_manager.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/bus_manager.cpp
🧬 Code graph analysis (1)
wled00/bus_manager.h (1)
wled00/bus_manager.cpp (2)
setColorOrder(259-263)setColorOrder(259-259)
⏰ 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: Builds (esp32c3mini_dio_4MB_M)
🔇 Additional comments (4)
wled00/bus_manager.h (2)
462-468: LGTM!The conditional correctly preserves
slowMode = truewhen overlapping busses exist, while still allowing RT mode to control slowMode when there are no overlaps. This ensures the SPC optimization remains disabled for overlapping configurations.
509-510: LGTM!The new
overlappingBussesflag is well-placed alongside the existingslowModeflag and correctly default-initialized tofalse.wled00/bus_manager.cpp (2)
1342-1343: LGTM!Correctly resets
overlappingBussestofalsewhen all busses are removed, maintaining consistent state alongsideslowMode.
1461-1466: LGTM!Good defensive fix. Non-OK busses are now correctly excluded from the
canAllShow()check, preventing invalid busses from blocking show operations. This aligns with theisOk()checks used elsewhere in the codebase (e.g.,setStatusPixel,setPixelColor).
* moved newStart and newEnd calculations into lower scope (``if (newBus->isOk())``) * removed duplicate nullptr check
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/bus_manager.cpp (1)
1304-1312: Inconsistent zero-length edge case handling.Line 1304 guards against zero-length with
(newLen > 0) ? ... : newStart, but line 1312 does not apply the same guard fortheEnd. If an existing bus is "ok" but somehow has zero length, this would cause unsigned underflow.For consistency and defensive coding, consider applying the same pattern:
Suggested fix
// check for overlap unsigned theStart = theBus->getStart(); - unsigned theEnd = theStart + theBus->getLength() - 1; + unsigned theLen = theBus->getLength(); + unsigned theEnd = (theLen > 0) ? theStart + theLen - 1 : theStart;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/bus_manager.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/bus_manager.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/bus_manager.cpp
🔇 Additional comments (3)
wled00/bus_manager.cpp (3)
1295-1324: Overlap detection logic looks correct.The implementation correctly:
- Early exits on failed bus creation (line 1297)
- Uses the standard range overlap algorithm (line 1314)
- Sets both
overlappingBussesandslowModewhen overlaps are detectedThe defensive reset on line 1322 when
numBusses < 1provides good safety for edge cases.
1342-1343: LGTM!Correctly resets both
slowModeandoverlappingBussesflags when all buses are removed, ensuring a clean state for subsequent configurations.
1461-1466: LGTM!The tightened
canAllShow()check correctly ignores non-OK buses, preventing uninitialized or failed buses from blocking show operations. This aligns with theisOk()checks used in other methods likesetPixelColor()andgetPixelColor().
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
don't repeat slowMode warning when several busses overlap
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @wled00/bus_manager.cpp:
- Around line 1296-1327: There is an unmatched `#endif` in the Bus addition
logic that causes a compile error; remove the stray `#endif` after the
overlappingBusses/slowMode handling so the function returns `numBusses++` inside
the correct preprocessor context, ensuring the block that manipulates
overlappingBusses, slowMode and prints the USER_PRINT message (symbols:
overlappingBusses, slowMode, USER_PRINT, getNumBusses, numBusses) is not
prematurely closed by a leftover preprocessor terminator.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/bus_manager.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/bus_manager.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2026-01-12T18:44:54.503Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 315
File: wled00/bus_manager.h:376-376
Timestamp: 2026-01-12T18:44:54.503Z
Learning: In MoonModules/WLED-MM, BusNetwork::setColorOrder is intentionally declared but not implemented in bus_manager.cpp. The linker error is acceptable as a safeguard if private usermods attempt to call this method, which should not be called.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/bus_manager.cpp
🪛 Cppcheck (2.19.0)
wled00/bus_manager.cpp
[error] 1326-1326: #endif without #if
(syntaxError)
⏰ 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). (19)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
🔇 Additional comments (4)
wled00/bus_manager.cpp (4)
1275-1275: LGTM!Capturing the previous
slowModestate before modification is the correct approach to implement the "show warning once" behavior.
1302-1321: Overlap detection logic is correct.The range overlap formula
(newStart <= theEnd) && (theStart <= newEnd)correctly handles all overlap scenarios. The edge case handling for zero-length buses is appropriate defensive coding.Minor note: The self-skip check at line 1307 is unreachable since
iiterates from0tobusCount-1(wherebusCount == numBusses), but it's harmless as documented by the "should not happen" comment.
1346-1346: LGTM!Correctly resets
overlappingBusseswhen all buses are removed, consistent with resettingslowMode.
1466-1466: LGTM!Adding the
isOk()check ensures that invalid or uninitialized buses don't blockcanAllShow()from returningtrue. This prevents non-functional buses from stalling the entire output pipeline.
Support for "custom bus indices" that lead to overlaping output pixel ranges.
With overlapping outputs, MM specific optimizations in busses.setPixelColor() need to be disabled - a single sPC must be forwarded to all busses. gPC can stay in optimized mode, because each possible bus pixel will have the same value.
Solves #296
Important: "slowmode" will cost performance, especially when you have many output pins with a large number of LEDs per pin!
In a test with esp32, 8 outputs with 512 LEDs each, performance dropped from 40fps to 30fps in "slowmode".
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.