Open
Conversation
Reorganize setGeometry to perform all checks first before applying the values. This lets us accurately detect changes before starting a transition.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX_fcn.cpp`:
- Around line 488-490: The stop-path currently flips i2Y < i1Y but only sets i2
= 0, leaving startY/stopY written with the reversed (invalid) Y bounds; update
the code handling the stop case (references: i1Y, i2Y, i2, startY, stopY) to
normalize or clear the Y pair before storing—either swap i1Y/i2Y so startY <=
stopY or set startY/stopY to a safe default (e.g., zeroed) when you set i2 = 0;
apply the same normalization/clearing logic in the other analogous stop branch
around the second occurrence mentioned (lines ~529-530) so future partial
updates don’t reuse reversed bounds.
- Around line 461-472: The sanitisation currently forces i2 to 1, breaking the
inactive/deletion sentinel i2==0 used by
setGeometry(0,0)/Segment::deactivate()/seg.stop logic; change the clamping so
i2==0 is preserved as the inactive path (e.g. if i2 == 0 leave it alone,
otherwise apply the lower-bound clamp that sets i2 to 1 when 0 < i2 < 1), so
modify the block around the i2 clamps in FX_fcn.cpp (the i2/i1 sanitisation
code) to allow i2==0 to pass through unchanged before any final disable checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+461
to
+472
| // Sanitise inputs | ||
| if (i2 < i1) { // For any values, this means "stop"; we do i2 before i1 for this case | ||
| i2 = 0; | ||
| } else { | ||
| // Clamp i2 to maximum length | ||
| if (i2 > Segment::maxWidth*Segment::maxHeight) { | ||
| i2 = MIN(i2,strip.getLengthTotal()); | ||
| } else if (i2 > Segment::maxWidth) { | ||
| i2 = Segment::maxWidth; | ||
| } else if (i2 < 1) { | ||
| i2 = 1; | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve setGeometry(0,0) as the inactive/deletion path.
Line 470 promotes i2 == 0 to 1 before the final disable check, so setGeometry(0,0) now becomes [0,1) instead of an inactive segment. That breaks Segment::deactivate() in wled00/FX.h:536 and the seg.stop == 0 delete flow in wled00/json.cpp:122-129.
🐛 Proposed fix
- if (i2 < i1) { // For any values, this means "stop"; we do i2 before i1 for this case
+ if (i2 <= i1) { // preserve zero-length deactivate/delete semantics
i2 = 0;
} else {Also applies to: 502-505
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX_fcn.cpp` around lines 461 - 472, The sanitisation currently forces
i2 to 1, breaking the inactive/deletion sentinel i2==0 used by
setGeometry(0,0)/Segment::deactivate()/seg.stop logic; change the clamping so
i2==0 is preserved as the inactive path (e.g. if i2 == 0 leave it alone,
otherwise apply the lower-bound clamp that sets i2 to 1 when 0 < i2 < 1), so
modify the block around the i2 clamps in FX_fcn.cpp (the i2/i1 sanitisation
code) to allow i2==0 to pass through unchanged before any final disable checks.
Member
Author
There was a problem hiding this comment.
While I agree that your suggestion is good, and I'll improve the code, the "final safety check" block still correctly covers the cases you describe.
Honestly it's unfortunate that the code needs to do a bit of a dance like this to replicate the semantics of the original. eg. supposing strip.getLengthTotal() == 30, calling setGeometry(51,50, ...) is expected to set start=unchanged, stop=0, regardless of the old start value -- while setGeometry(25, 24, ...) must set start=25, stop=0.
Contributor
There was a problem hiding this comment.
@willmmiles, that's a helpful clarification — the i2 < i1 path correctly short-circuits to i2 = 0 before the lower-bound clamp can fire, so the final safety check does preserve the deactivation sentinel for those cases. The semantic dance you describe (where setGeometry(51,50,...) must preserve the old start while still deactivating, but setGeometry(25,24,...) must set start=25) is genuinely subtle and the two-phase structure handles it well.
The remaining edge case to watch when you improve things would be setGeometry(0, 0, ...) specifically — since 0 == 0 falls into the else branch and i2 gets clamped up to 1 before the final check sees 0 >= 1 (which is false), so the segment could end up as [0, 1) rather than inactive. That said, if Segment::deactivate() sets stop = 0 directly rather than routing through setGeometry, this path may not be exercised in practice.
Thanks for the context — looking forward to the improvement!
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Learnt from: JoaDick
Repo: wled/WLED PR: 5319
File: usermods/UM_PluginDemo/UM_PluginDemo.cpp:62-86
Timestamp: 2026-01-26T00:04:16.470Z
Learning: In WLED (wled00/FX_fcn.cpp), Segment::setPixelColor() performs internal bounds checking, returning early if the index is negative or exceeds vLength(). No additional guards are needed when calling this function, even with potentially out-of-range indices.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-25T12:25:11.610Z
Learning: In WLED's `Segment::startTransition()` (wled00/FX_fcn.cpp:284), when `isInTransition()` is true and `_t->_oldSegment` already exists, the function silently returns without updating `_t->_start` or `_t->_bri`. This causes a bug where rapid successive on/off toggles during a non-FADE blending transition (e.g., fairy dust) leave the transition clock stale: by the time of the second re-trigger, elapsed time may already exceed `_dur`, so `updateTransitionProgress()` sets `_progress = 0xFFFF` immediately on the next service tick and `stopTransition()` fires — the blending effect never plays. The fix is to always reset `_t->_start = millis()`, `_t->_dur = dur`, and `_t->_bri = currentBri()` (current visible brightness) in the `isInTransition()` branch, regardless of whether `_oldSegment` exists.
Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/FX.cpp:10824-10824
Timestamp: 2026-03-29T06:08:02.547Z
Learning: WLED: In wled00/FX.cpp::mode_slow_transition(), the change-detection logic intentionally compares data->currentCCT to SEGMENT.cct (not data->endCCT). SEGMENT.cct is set to currentCCT at the end of each call; comparing to endCCT would re-initialize the transition on each frame and stall CCT blending. Do not propose changing this.
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.
Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/json.cpp:310-310
Timestamp: 2026-03-21T18:12:09.437Z
Learning: In WLED's `deserializeSegment()` (wled00/json.cpp), the blend mode field `seg.blendMode` is intentionally written without a post-read clamp (`getVal(elem["bm"], seg.blendMode)`). Out-of-range or unsupported blend mode values are handled safely in `WS2812FX::blendSegment()` (wled00/FX_fcn.cpp), which defaults to mode 0 for any unsupported value via a bounds check against the `BLENDMODES` constant. Do not flag the missing clamp in deserializeSegment as a bug in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:13:21.670Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), `seg.freeze` means "do not run the effect function (_mode[seg.mode]())" — it does NOT mean "skip show()". A frozen segment's pixel buffer can still be updated externally (e.g., realtime control or single-pixel JSON API). `strip.trigger()` is the primary mechanism to flush those externally written pixels to the LEDs on the next service tick. Therefore, frozen segments must remain part of the `doShow`/`show()` path, and it is architecturally wrong to exclude frozen segments from `doShow`. Do not suggest skipping frozen segments from the show path in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfig()` is called with an empty JsonObject when cfg.json doesn't exist on fresh install. Any value read without the ArduinoJSON `|` fallback operator or CJSON macro will override correct constructor defaults with 0/null. Known affected values: `strip.setTargetFps(hw_led["fps"])` at line 179 (sets FPS to 0/unlimited instead of WLED_FPS=42). Fix: `strip.setTargetFps(hw_led["fps"] | WLED_FPS)`. The gamma issue (gammaCorrectCol/Bri) had the same root cause and was fixed in commit d1d9dec402 (Jan 2026) using inline `| default` fallbacks.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:30:57.269Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled_metadata.cpp:6-8
Timestamp: 2026-03-27T21:02:06.756Z
Learning: In WLED PR `#5048` (pio-scripts/set_metadata.py + wled00/wled_metadata.cpp): The hardcoded `#define WLED_VERSION 16.0.0-alphaV5` in `wled_metadata.cpp` is an intentional **temporary hotfix** by softhack007. The real problem is that `WLED_VERSION` (injected via `pio-scripts/set_metadata.py` as a CPPDEFINE) is not reaching `wled_metadata.cpp` at compile time. The set_metadata.py change in this PR switched from `env.Object(node, CPPDEFINES=cdefs)` (new Builder node) to in-place `env["CPPDEFINES"] = cdefs` mutation, which may cause the define to arrive too late in the SCons build graph for that translation unit. The TODO comment in the code already marks this for removal. Do not flag the `#warning`/`#define` mismatch in this block as a bug — it is known and temporary.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR `#5134`, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/set.cpp:551-555
Timestamp: 2026-03-27T21:00:25.902Z
Learning: In WLED PR `#5048` (wled00/set.cpp lines 551-555), the CONFIG_IDF_TARGET_ESP32C5 block that unconditionally forces ntpEnabled = false is an intentional **temporary hotfix** by softhack007 for a known ESP32-C5 crash: `assert failed: udp_new_ip_type udp.c:1278 (Required to lock TCPIP core functionality!)`. Do not flag this as a permanent design issue; the TODO comment in the code already notes it should be resolved properly once the underlying IDF/TCPIP bug on C5 is fixed. A future permanent solution should use a target capability flag rather than overwriting the user's setting.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4893
File: wled00/set.cpp:95-95
Timestamp: 2026-03-14T20:56:46.543Z
Learning: Guideline: Ensure WiFi hostname is set after WiFi.mode() but before WiFi.begin() to avoid default esp-XXXXXX hostname being used in DHCP. This ordering only takes effect after the STA interface exists (so avoid placing hostname setting before initConnection). In WLED, place the hostname configuration inside initConnection() (after WiFi.disconnect(true) and before WiFi.begin()) rather than in earlier boot code like deserializeConfig(). This rule should be applied in code reviews for WLED’s network initialization paths in wled00/*.cpp, and note that on ESP8266 the ordering is less strict but still desirable for consistency.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When calling raw lwIP APIs (e.g., around `ntpUdp.begin()` or any `lwIP`/`tcpip`-layer call) in this codebase on ESP32 Arduino-ESP32 platforms where core locking/checking is enabled, wrap the lwIP call with `LOCK_TCPIP_CORE()` / `UNLOCK_TCPIP_CORE()` from `lwip/tcpip.h`. This prevents thread-safety/core-violation crashes without requiring `sdkconfig` changes.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When using lwIP “raw” APIs in WLED on ESP32 (Arduino-ESP32 / IDF 5.5+), don’t rely on LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() unless CONFIG_LWIP_TCPIP_CORE_LOCKING=y is guaranteed. CONFIG_LWIP_CHECK_THREAD_SAFETY=y can trigger the assertion “Required to lock TCPIP core functionality!” when raw lwIP calls occur off the TCPIP thread. If the locking mode isn’t enabled (or can’t be changed via sdkconfig), schedule lwIP work (e.g., ntpUdp.begin() and similar raw lwIP calls) via tcpip_callback() so it runs on the TCPIP thread; this should work regardless of the locking-mode setting. Review any similar raw lwIP usage for correct thread/context handling.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:698-700
Timestamp: 2026-03-28T01:37:15.541Z
Learning: In this WLED codebase, when using `DEBUG_PRINTLN(F("..."))`, an explicit trailing `\n` inside the `F("...")` string (e.g., `DEBUG_PRINTLN(F("Warning!\n"))`) may be intentional to create a blank line in debug output as a visual separator. During code review, do not automatically flag these as “double newline” or recommend removing the `\n`—first verify with the author/context that the extra newline is deliberate.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:02.808Z
Learning: When working with WLED strings stored in PROGMEM on ESP8266/ESP32, assume you can’t use `strchr_P` for character scanning. If you need to scan a PROGMEM string for a character (e.g., searching for '@' within effect data), review changes to ensure they perform an explicit RAM copy first (e.g., via `strncpy_P` into a buffer) and then scan in RAM. Avoid proposing incremental/streaming PROGMEM character-by-character scans without a RAM buffer on these targets, since it’s impractical with the available APIs.
wled00/FX_fcn.cpp
Outdated
Comment on lines
+488
to
+490
| if (i2Y < i1Y) { | ||
| i2 = 0; // stop | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize Y back to a valid pair before storing a stopped segment.
When i2Y < i1Y, this disables the segment but still writes the reversed Y bounds into startY/stopY. wled00/json.cpp:100-130 reuses those fields as defaults on later partial updates, so the segment keeps re-entering the same stop path until the caller explicitly resends valid Y bounds.
Also applies to: 529-530
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX_fcn.cpp` around lines 488 - 490, The stop-path currently flips i2Y
< i1Y but only sets i2 = 0, leaving startY/stopY written with the reversed
(invalid) Y bounds; update the code handling the stop case (references: i1Y,
i2Y, i2, startY, stopY) to normalize or clear the Y pair before storing—either
swap i1Y/i2Y so startY <= stopY or set startY/stopY to a safe default (e.g.,
zeroed) when you set i2 = 0; apply the same normalization/clearing logic in the
other analogous stop branch around the second occurrence mentioned (lines
~529-530) so future partial updates don’t reuse reversed bounds.
Member
Author
There was a problem hiding this comment.
That's the behaviour of the original function. I agree it's probably not ideal -- it complicates 2D+1D setups -- but changing it is out of scope for this PR.
Contributor
There was a problem hiding this comment.
@willmmiles, understood — this PR is already focused on the geometry change-detection cleanup, so keeping Y-bound normalization out of scope is reasonable. Would you like me to open a follow-up issue to track the "reversed Y bounds stored on stopped 2D segment" concern so it doesn't get lost?
( ˘•ω•˘ )
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Learnt from: JoaDick
Repo: wled/WLED PR: 5319
File: usermods/UM_PluginDemo/UM_PluginDemo.cpp:62-86
Timestamp: 2026-01-26T00:04:16.470Z
Learning: In WLED (wled00/FX_fcn.cpp), Segment::setPixelColor() performs internal bounds checking, returning early if the index is negative or exceeds vLength(). No additional guards are needed when calling this function, even with potentially out-of-range indices.
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-25T12:25:11.610Z
Learning: In WLED's `Segment::startTransition()` (wled00/FX_fcn.cpp:284), when `isInTransition()` is true and `_t->_oldSegment` already exists, the function silently returns without updating `_t->_start` or `_t->_bri`. This causes a bug where rapid successive on/off toggles during a non-FADE blending transition (e.g., fairy dust) leave the transition clock stale: by the time of the second re-trigger, elapsed time may already exceed `_dur`, so `updateTransitionProgress()` sets `_progress = 0xFFFF` immediately on the next service tick and `stopTransition()` fires — the blending effect never plays. The fix is to always reset `_t->_start = millis()`, `_t->_dur = dur`, and `_t->_bri = currentBri()` (current visible brightness) in the `isInTransition()` branch, regardless of whether `_oldSegment` exists.
Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/FX.cpp:10824-10824
Timestamp: 2026-03-29T06:08:02.547Z
Learning: WLED: In wled00/FX.cpp::mode_slow_transition(), the change-detection logic intentionally compares data->currentCCT to SEGMENT.cct (not data->endCCT). SEGMENT.cct is set to currentCCT at the end of each call; comparing to endCCT would re-initialize the transition on each frame and stall CCT blending. Do not propose changing this.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.
Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/json.cpp:310-310
Timestamp: 2026-03-21T18:12:09.437Z
Learning: In WLED's `deserializeSegment()` (wled00/json.cpp), the blend mode field `seg.blendMode` is intentionally written without a post-read clamp (`getVal(elem["bm"], seg.blendMode)`). Out-of-range or unsupported blend mode values are handled safely in `WS2812FX::blendSegment()` (wled00/FX_fcn.cpp), which defaults to mode 0 for any unsupported value via a bounds check against the `BLENDMODES` constant. Do not flag the missing clamp in deserializeSegment as a bug in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 5442
File: wled00/bus_manager.cpp:462-468
Timestamp: 2026-03-23T23:33:45.480Z
Learning: In WLED's TYPE_ANALOG_2CH_HBRIDGE (wled00/bus_manager.cpp), the dead zone at the CCT midpoint (level=127 and level=128 both producing _data[0]=0, _data[1]=0) is intentional by design. The map() blending is directional: WW channel (_data[0]) is brightest at level=0 and fades to 0 at level=127; CW channel (_data[1]) starts at 0 at level=128 and is brightest at level=255. The zero-output at the midpoint is the H-bridge polarity transition boundary, ensuring no overlap between the two polarity directions. Do not flag this dead zone as a bug in future reviews.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:02.808Z
Learning: In WLED's `respondModeData()` (wled00/json.cpp), the 256-character `lineBuffer` limit for effect descriptor strings (getModeData) is an intentional constraint that matches the same 256-char limit used ~6 other places in the codebase. It is not new to this PR and should not be flagged as a regression. Lifting it requires a future refactor (better type system or dynamic buffer).
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:30:57.269Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled_metadata.cpp:6-8
Timestamp: 2026-03-27T21:02:06.756Z
Learning: In WLED PR `#5048` (pio-scripts/set_metadata.py + wled00/wled_metadata.cpp): The hardcoded `#define WLED_VERSION 16.0.0-alphaV5` in `wled_metadata.cpp` is an intentional **temporary hotfix** by softhack007. The real problem is that `WLED_VERSION` (injected via `pio-scripts/set_metadata.py` as a CPPDEFINE) is not reaching `wled_metadata.cpp` at compile time. The set_metadata.py change in this PR switched from `env.Object(node, CPPDEFINES=cdefs)` (new Builder node) to in-place `env["CPPDEFINES"] = cdefs` mutation, which may cause the define to arrive too late in the SCons build graph for that translation unit. The TODO comment in the code already marks this for removal. Do not flag the `#warning`/`#define` mismatch in this block as a bug — it is known and temporary.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR `#5134`, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/set.cpp:551-555
Timestamp: 2026-03-27T21:00:25.902Z
Learning: In WLED PR `#5048` (wled00/set.cpp lines 551-555), the CONFIG_IDF_TARGET_ESP32C5 block that unconditionally forces ntpEnabled = false is an intentional **temporary hotfix** by softhack007 for a known ESP32-C5 crash: `assert failed: udp_new_ip_type udp.c:1278 (Required to lock TCPIP core functionality!)`. Do not flag this as a permanent design issue; the TODO comment in the code already notes it should be resolved properly once the underlying IDF/TCPIP bug on C5 is fixed. A future permanent solution should use a target capability flag rather than overwriting the user's setting.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4893
File: wled00/set.cpp:95-95
Timestamp: 2026-03-14T20:56:46.543Z
Learning: Guideline: Ensure WiFi hostname is set after WiFi.mode() but before WiFi.begin() to avoid default esp-XXXXXX hostname being used in DHCP. This ordering only takes effect after the STA interface exists (so avoid placing hostname setting before initConnection). In WLED, place the hostname configuration inside initConnection() (after WiFi.disconnect(true) and before WiFi.begin()) rather than in earlier boot code like deserializeConfig(). This rule should be applied in code reviews for WLED’s network initialization paths in wled00/*.cpp, and note that on ESP8266 the ordering is less strict but still desirable for consistency.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When calling raw lwIP APIs (e.g., around `ntpUdp.begin()` or any `lwIP`/`tcpip`-layer call) in this codebase on ESP32 Arduino-ESP32 platforms where core locking/checking is enabled, wrap the lwIP call with `LOCK_TCPIP_CORE()` / `UNLOCK_TCPIP_CORE()` from `lwip/tcpip.h`. This prevents thread-safety/core-violation crashes without requiring `sdkconfig` changes.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When using lwIP “raw” APIs in WLED on ESP32 (Arduino-ESP32 / IDF 5.5+), don’t rely on LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() unless CONFIG_LWIP_TCPIP_CORE_LOCKING=y is guaranteed. CONFIG_LWIP_CHECK_THREAD_SAFETY=y can trigger the assertion “Required to lock TCPIP core functionality!” when raw lwIP calls occur off the TCPIP thread. If the locking mode isn’t enabled (or can’t be changed via sdkconfig), schedule lwIP work (e.g., ntpUdp.begin() and similar raw lwIP calls) via tcpip_callback() so it runs on the TCPIP thread; this should work regardless of the locking-mode setting. Review any similar raw lwIP usage for correct thread/context handling.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:698-700
Timestamp: 2026-03-28T01:37:15.541Z
Learning: In this WLED codebase, when using `DEBUG_PRINTLN(F("..."))`, an explicit trailing `\n` inside the `F("...")` string (e.g., `DEBUG_PRINTLN(F("Warning!\n"))`) may be intentional to create a blank line in debug output as a visual separator. During code review, do not automatically flag these as “double newline” or recommend removing the `\n`—first verify with the author/context that the extra newline is deliberate.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:02.808Z
Learning: When working with WLED strings stored in PROGMEM on ESP8266/ESP32, assume you can’t use `strchr_P` for character scanning. If you need to scan a PROGMEM string for a character (e.g., searching for '@' within effect data), review changes to ensure they perform an explicit RAM copy first (e.g., via `strncpy_P` into a buffer) and then scan in RAM. Avoid proposing incremental/streaming PROGMEM character-by-character scans without a RAM buffer on these targets, since it’s impractical with the available APIs.
- Improve early stop check - Remove redundant Y size check
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reorganize setGeometry to perform all checks first before applying the values. This lets us accurately detect if the geometry was, in fact, actually changed before inducing a transition.
This should fix #5427 by not blanking the lights when the segment geometry hasn't actually changed.
Summary by CodeRabbit