Zoom and rotate segment (alternative to #5202) #5226
Closed
+188
−26
Conversation
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
- see #5202 for original implementation - does not include mirroring and always wraps - implemented as Segment property - reduced index.js footprint by coalescing several similar functions - slightly optimised push transition logic
Contributor
WalkthroughAdds per-segment zoom and rotation state and controls; extends mode defaults and JSON I/O; implements rotation/zoom sampling, intermediate buffers, and angle progression in blending/push logic; updates frontend segment UI to expose zoom/rotate controls and consolidates segment property setters. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX_fcn.cpp (1)
1479-1598: Fix segO rotation condition and align allocation with WLED conventionsTwo issues in the 2D rotate/zoom block:
- Old segment rotation uses wrong speed flag at line 1568:
if (segO->rotateSpeed || segO->zoomAmount != 8) { _pixelsO = new uint32_t[oCols * oRows]; ... - if (topSegment.rotateSpeed != 0) { + if (segO->rotateSpeed != 0) { segO->rotatedAngle += segO->rotateSpeed;Currently, the condition checks
topSegment.rotateSpeed, preventing the old segment from advancing its angle whenever the new segment has zero rotation—breaking transitions between rotating and stationary states.
- Allocation pattern deviates from WLED conventions. Both
_pixelsNand_pixelsOuse plainnewwithout error handling. Throughout WLED (FX.cpp, segments), large temporary buffers useallocate_buffer()with null checks:_pixelsN = static_cast<uint32_t*>(allocate_buffer(nCols * nRows * sizeof(uint32_t), BFRALLOC_...)); if (!_pixelsN) { /* graceful fallback */ }This approach is safer on devices where exceptions may be disabled, and integrates with WLED's memory management strategy.
🧹 Nitpick comments (3)
wled00/data/index.js (2)
741-783: GenericsetSegPropwiring for segment UI looks good; consider gating zoom/rotation to true 2D segmentsThe refactor to a generic
setSegProp(s, p)is consistent and readable:
- All per-segment toggles (
rev,mi,rY,mY,tp), selectors (m12,bm,si) and sliders (bri,zA,rS) now post{ "seg": { "id": s, [p]: value } }, with booleans taken from.checkedand others parsed as integers.- IDs (
seg${i}bri,seg${i}zA,seg${i}rS,seg${i}rev, etc.) all line up with the constructed DOM, andpopulateSegments()correctly callsupdateTrail()for the new sliders when present.One minor UX detail:
zoomandrotationsliders are shown for all matrix segments (isMSeg), including 1D segments that live on a 2D matrix edge. Since the backend rotate/zoom logic only affects 2D segments, these sliders will effectively be no‑ops for 1D‑in‑matrix segments.If you want to avoid confusion, you could gate the zoom/rotation controls similarly to the 2D‑only options (e.g. require both
stoX - staX > 1andstoY - staY > 1).You may want to quickly confirm in the UI that changing
zA/rSon a vertical/horizontal 1D segment in a matrix truly has no effect, and decide whether hiding the controls in that case is preferable.Also applies to: 804-812, 861-869, 884-891, 2286-2292
2293-2369: Large block of commented-out legacy per-segment settersThe old per-field setters (
setRev,setMi,setM12,setSi,setBm,setTp,setZoom,setRotation,setSegBri) are now fully superseded bysetSegPropand only kept as commented reference.Keeping this much commented-out code in
index.jsadds noise and makes future maintenance harder. Once you’re confident in the new generic handler, it would be cleaner to remove these functions (or move concise documentation of the old API into a separate doc comment or markdown file if you want to preserve history).If you rely on these snippets as upgrade documentation, consider shrinking them to a short explanatory comment instead of full commented functions before merging.
Also applies to: 2377-2383
wled00/json.cpp (1)
283-284: Consider defining constants for the magic number 15.The constraints are correct for 4-bit fields (0-15). Based on learnings about avoiding magic numbers, consider defining constants like
MAX_ROTATE_SPEEDandMAX_ZOOM_AMOUNTfor improved maintainability, especially if these limits are referenced elsewhere in the codebase.That said, the existing code uses similar inline literals (e.g.,
constrain(set, 0, 3)), so this is a minor stylistic suggestion.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/FX.hwled00/FX_fcn.cppwled00/data/index.jswled00/json.cpp
🧰 Additional context used
📓 Path-based instructions (4)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX_fcn.cppwled00/json.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/index.js
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/index.js
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/FX.h
🧠 Learnings (23)
📓 Common learnings
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: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
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.
📚 Learning: 2025-08-26T11:51:21.817Z
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.
Applied to files:
wled00/FX_fcn.cppwled00/data/index.jswled00/json.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-11-14T05:48:44.673Z
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.
Applied to files:
wled00/FX_fcn.cppwled00/data/index.jswled00/FX.h
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-12-13T22:12:48.838Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5188
File: wled00/FX.cpp:9449-9451
Timestamp: 2025-12-13T22:12:48.838Z
Learning: wled00/FX.cpp mode_particlePinball: In the 1D ParticleSystem, PartSys->setBounce(...) toggles end-wall/ground bouncing and is independent of gravity. Keeping setBounce(SEGMENT.custom3) is intended; do not couple bounce enablement to the mapped gravity value.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
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.
Applied to files:
wled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-10-20T09:41:50.374Z
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.
Applied to files:
wled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-04-24T09:31:06.879Z
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.
Applied to files:
wled00/FX_fcn.cppwled00/data/index.jswled00/json.cpp
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
wled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-04-24T09:31:06.879Z
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.
Applied to files:
wled00/FX_fcn.cppwled00/data/index.jswled00/json.cpp
📚 Learning: 2025-09-11T04:36:42.869Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4754
File: usermods/user_fx/README.md:169-179
Timestamp: 2025-09-11T04:36:42.869Z
Learning: In WLED, SEGENV.data is already typed as uint8_t* and can be accessed directly without casting when storing per-pixel state data.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/index.jswled00/json.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
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.
Applied to files:
wled00/data/index.jswled00/json.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/data/index.js
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/data/index.js
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/data/index.js
📚 Learning: 2025-02-19T12:43:34.200Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
Applied to files:
wled00/FX.h
📚 Learning: 2025-10-20T09:38:51.997Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-20T09:38:51.997Z
Learning: WLED matrices: each dimension (SEG_W, SEG_H) is limited to ≤255; 256 or larger per side is not supported/feasible on ESP32, so effects should assume per-dimension max 255.
Applied to files:
wled00/FX.h
📚 Learning: 2025-05-09T18:43:15.355Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Applied to files:
wled00/FX.hwled00/json.cpp
🧬 Code graph analysis (1)
wled00/FX_fcn.cpp (2)
wled00/util.cpp (2)
extractModeDefaults(349-367)extractModeDefaults(349-349)wled00/colors.h (1)
color_blend16(118-118)
⏰ 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: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (6)
wled00/FX_fcn.cpp (1)
551-587: Mode defaults:custom3clamp and newrS/zAwiring look correctThe changes in
Segment::setModedo what they should:
custom3is now clamped viaconstrain(sOpt, 0, 31), which matches the 5‑bitcustom3bitfield and the documented 0–31 range.- New defaults for
"rS"and"zA"are read from effect metadata and clamped to 0–15, aligning with the 4‑bitrotateSpeed/zoomAmountfields and the UI slider ranges.- Existing behavior for other defaults is preserved.
No issues from a correctness perspective here.
Please double‑check that your FX data strings actually use the
"rS"/"zA"keys and that their numeric ranges match the intended 0–15 slider ranges.wled00/json.cpp (5)
38-40: LGTM!The new fields for
blendMode,zoomAmount, androtateSpeedare correctly added to theSegmentCopystruct for change tracking.
63-65: LGTM!The difference checks correctly flag changes to
blendMode,zoomAmount, androtateSpeedasSEG_DIFFERS_OPT, consistent with other segment options.
117-120: LGTM!The initialization order correctly matches the struct field order, and the new segment properties are properly captured for change detection.
164-165: LGTM!The deserialization follows the established pattern for segment properties, reading
rSandzAwith appropriate fallback to current segment values.
647-648: LGTM!The serialization uses the same JSON keys (
rS,zA) as deserialization, ensuring round-trip consistency. The placement alongsideblendModeis appropriate.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wled00/FX_fcn.cpp (1)
1479-1545: Dead code and magic numbers in RotateAndZoom lambda.
Dead code at line 1539: The bounds check
if ((unsigned)srcX >= (unsigned)cols || (unsigned)srcY >= (unsigned)rows) continue;is unreachable after the unconditional wrap logic at lines 1523-1524, which always brings coordinates within bounds. This check would only apply if wrap was disabled (the commented-out code path).Magic numbers: Consider defining constants for clarity:
3600(lines 1554, 1569) represents 360° × 10 (tenths of degrees)zoomRange = 768and the zoom formula could use inline commentsBased on learnings, magic numbers should be replaced with defined constants when meaningful in the context.
🔎 Suggested improvements
+ constexpr int ANGLE_FULL_ROTATION = 3600; // 360 degrees in tenths + auto RotateAndZoom = [](uint32_t *srcPixels, uint32_t *destPixels, int midX, int midY, int cols, int rows, int shearAngle, int zoomOffset) { // ... existing code ... - //else - if ((unsigned)srcX >= (unsigned)cols || (unsigned)srcY >= (unsigned)rows) continue; + // Note: bounds check removed as wrap logic above guarantees valid coordinates // Sample from source & write to destinationAnd for angle wrapping:
- while (topSegment._rotatedAngle > 3600) topSegment._rotatedAngle -= 3600; + while (topSegment._rotatedAngle > ANGLE_FULL_ROTATION) topSegment._rotatedAngle -= ANGLE_FULL_ROTATION;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX.hwled00/FX_fcn.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/FX.h
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX_fcn.cpp
🧠 Learnings (15)
📓 Common learnings
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: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
📚 Learning: 2025-08-26T11:51:21.817Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T05:48:44.673Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-10-20T09:41:50.374Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-09-11T04:36:42.869Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4754
File: usermods/user_fx/README.md:169-179
Timestamp: 2025-09-11T04:36:42.869Z
Learning: In WLED, SEGENV.data is already typed as uint8_t* and can be accessed directly without casting when storing per-pixel state data.
Applied to files:
wled00/FX_fcn.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). (19)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (3)
wled00/FX_fcn.cpp (3)
566-577: LGTM!The constraints for
c3(0..31),rS(0..15), andzA(0..15) are consistent with the 4-bit/5-bit property design mentioned in the PR objectives. The inline comments clarifying default behaviors (0 = no rotation, 8 = no zoom) are helpful.
1633-1635: Cleanup logic is correct but depends on allocation success.The cleanup conditions at lines 1634-1635 correctly match the allocation conditions. However, if null checks are added (per previous comment), the cleanup conditions must also track whether allocation actually succeeded to avoid deleting the fallback pointer.
Consider using a flag or comparing against the original pointer:
bool allocatedN = (topSegment.rotateSpeed || topSegment.zoomAmount != 8) && _pixelsN != topSegment.getPixels(); if (allocatedN) delete[] _pixelsN;
1580-1602: Push transition logic improvements look good.The replacement of modulo with while-loop wrap (lines 1594-1595) is a valid optimization for embedded platforms where integer division is expensive. The buffer pointer integration (
_pixelsR,_pixelsN,_pixelsO) correctly routes rotated/zoomed pixel data through the blending path.The while loops are safe:
nColsandnRowsare derived fromvirtualWidth()andvirtualHeight(), which are designed to always preserve at least 1 LED dimension (documented in their implementations: "leave at least a single LED"). This invariant prevents infinite loops.
Comment on lines
1547
to
1575
| uint32_t *_pixelsN = topSegment.getPixels(); // we will use this pointer as a source later insetad of getPixelColorRaw() | ||
| if (topSegment.rotateSpeed || topSegment.zoomAmount != 8) { | ||
| _pixelsN = new uint32_t[nCols * nRows]; // may use allocateBuffer() if needed | ||
| const int midX = nCols / 2; | ||
| const int midY = nRows / 2; | ||
| if (topSegment.rotateSpeed != 0) { | ||
| topSegment._rotatedAngle += topSegment.rotateSpeed; | ||
| while (topSegment._rotatedAngle > 3600) topSegment._rotatedAngle -= 3600; | ||
| } else { | ||
| topSegment._rotatedAngle = 0; // reset angle if no rotation | ||
| } | ||
| RotateAndZoom(topSegment.getPixels(), _pixelsN, midX, midY, nCols, nRows, topSegment._rotatedAngle/10, topSegment.zoomAmount - 8); | ||
| } | ||
| uint32_t *_pixelsO = topSegment.getPixels(); // we will use this pointer as a source (old segment during transition) later insetad of getPixelColorRaw() | ||
| if (segO) { | ||
| _pixelsO = segO->getPixels(); // default to unmodified old segment pixels | ||
| if (segO->rotateSpeed || segO->zoomAmount != 8) { | ||
| _pixelsO = new uint32_t[oCols * oRows]; // may use allocateBuffer() if needed | ||
| const int midXo = oCols / 2; | ||
| const int midYo = oRows / 2; | ||
| if (topSegment.rotateSpeed != 0) { | ||
| segO->_rotatedAngle += segO->rotateSpeed; | ||
| while (segO->_rotatedAngle > 3600) segO->_rotatedAngle -= 3600; | ||
| } else { | ||
| segO->_rotatedAngle = 0; | ||
| } | ||
| RotateAndZoom(segO->getPixels(), _pixelsO, midXo, midYo, oCols, oRows, segO->_rotatedAngle/10, segO->zoomAmount - 8); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing null checks for dynamic allocations in hot path.
Lines 1549 and 1564 use new uint32_t[...] without checking for allocation failure. If memory is exhausted, these will throw std::bad_alloc (or return nullptr with new(std::nothrow)), potentially causing a crash or null pointer dereference in RotateAndZoom.
Additionally, these allocations occur per-frame in blendSegment() (called from show()), which could lead to heap fragmentation on constrained embedded systems.
🔎 Proposed fix with null checks
uint32_t *_pixelsN = topSegment.getPixels();
if (topSegment.rotateSpeed || topSegment.zoomAmount != 8) {
- _pixelsN = new uint32_t[nCols * nRows]; // may use allocateBuffer() if needed
+ _pixelsN = new(std::nothrow) uint32_t[nCols * nRows];
+ if (!_pixelsN) {
+ _pixelsN = topSegment.getPixels(); // fallback to unrotated
+ } else {
const int midX = nCols / 2;
const int midY = nRows / 2;
// ... rotation logic ...
RotateAndZoom(topSegment.getPixels(), _pixelsN, midX, midY, nCols, nRows, topSegment._rotatedAngle/10, topSegment.zoomAmount - 8);
+ }
}Similar change needed for _pixelsO allocation at line 1564.
Committable suggestion skipped: line range outside the PR's diff.
Contributor
Author
There was a problem hiding this comment.
Operator new will not return nullptr unless using (std::nothrow) it will cause an exception instead. Exception will crash ESP.
Contributor
There was a problem hiding this comment.
@blazoncek, thank you for the clarification about exception behavior on ESP platforms—that's an important distinction.
You're right that new will throw std::bad_alloc and crash the device on allocation failure. Given that, I noticed your inline comment mentions "may use allocateBuffer() if needed." Since allocate_buffer() is used extensively elsewhere in the codebase (e.g., for segment pixels, data buffers, etc.) and presumably has safer failure handling for embedded systems, would it make sense to use it here instead of new?
These allocations happen per-frame in the hot path when rotation/zoom is active, so an allocation failure during runtime could be problematic. Using allocate_buffer() might provide more graceful degradation (falling back to unrotated rendering) rather than crashing.
Was there a specific reason you opted for new here, or is this something you're planning to refine?
🧠 Learnings used
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: 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: 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: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Learnt from: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:21:06.181Z
Learning: In WLED's image_loader.cpp, decoder.alloc() uses `new AnimatedGIF()`. When `new` throws an exception (e.g., std::bad_alloc), no pointer is assigned and C++ semantics ensure automatic cleanup of any partially constructed storage. Therefore, calling decoder.dealloc() in the catch block after a failed alloc() is unnecessary. More generally, "safety" cleanup or de-allocation code should only be suggested when the library or sub-function involved is not already taking care of cleanup after failure per its documented semantics.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:20:21.639Z
Learning: In WLED's image_loader.cpp, decoder.alloc() uses `new AnimatedGIF()`. When `new` throws an exception (e.g., std::bad_alloc), no pointer is assigned and C++ semantics ensure automatic cleanup of any partially constructed storage. Therefore, calling decoder.dealloc() in the catch block after a failed alloc() is unnecessary. More generally, "safety" cleanup or de-allocation code should only be suggested when the library or sub-function involved is not already taking care of cleanup after failure per its documented semantics.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:22:20.525Z
Learning: In WLED's image_loader.cpp and similar contexts, when suggesting cleanup or de-allocation code in error handlers, first verify whether the underlying library or function already handles cleanup after failure. For example, `new` expressions that throw exceptions do not assign partial pointers and automatically clean up storage, making explicit dealloc() calls in catch blocks unnecessary. Only suggest "safety" cleanup when the library/function's documented semantics do not already provide graceful failure handling.
Learnt from: willmmiles
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-08-29T01:28:10.512Z
Learning: In ESP8266 code, the system_get_rst_info() function cannot return null as it returns a pointer to static memory. This is a common C API pattern that hides internal structure details, so null checks are unnecessary for this function.
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-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: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/FX_fcn.cpp (1)
1567-1572: Bug still present: Wrong segment'srotateSpeedchecked.This bug was flagged in a previous review and acknowledged by the author, but the fix has not been applied yet. Line 1567 still checks
topSegment.rotateSpeedwhen it should checksegO->rotateSpeed.🔎 Proposed fix
- if (topSegment.rotateSpeed != 0) { + if (segO->rotateSpeed != 0) {
🧹 Nitpick comments (1)
wled00/FX_fcn.cpp (1)
1479-1545: Consider defining constants for magic numbers and verify wrap loop termination.The
RotateAndZoomlambda uses several magic numbers that could benefit from named constants for maintainability:
8as the neutral zoom value (appears in multiple places: line 1487, 1548, 1558, etc.)3600for the angle representation (360.0° × 10)- The zoom range calculation
(Fixed_Scale * 3) / 4Additionally, the
whileloops at lines 1523-1524 assumeWRAP_PADis sufficient to make coordinates positive before wrapping. With extreme zoom-out scenarios, very large negative source coordinates could require many iterations. Consider using a single modulo operation after ensuring positivity, or adding a safety bound:srcX = ((srcX % cols) + cols) % cols; // handles negatives correctlyBased on learnings, suggest replacing magic numbers with defined constants for improved maintainability.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX_fcn.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX_fcn.cpp
🧠 Learnings (27)
📓 Common learnings
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.
📚 Learning: 2025-08-26T11:51:21.817Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T05:48:44.673Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-10-20T09:41:50.374Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-12-13T22:12:48.838Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5188
File: wled00/FX.cpp:9449-9451
Timestamp: 2025-12-13T22:12:48.838Z
Learning: wled00/FX.cpp mode_particlePinball: In the 1D ParticleSystem, PartSys->setBounce(...) toggles end-wall/ground bouncing and is independent of gravity. Keeping setBounce(SEGMENT.custom3) is intended; do not couple bounce enablement to the mapped gravity value.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T13:22:20.525Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:22:20.525Z
Learning: In WLED's image_loader.cpp and similar contexts, when suggesting cleanup or de-allocation code in error handlers, first verify whether the underlying library or function already handles cleanup after failure. For example, `new` expressions that throw exceptions do not assign partial pointers and automatically clean up storage, making explicit dealloc() calls in catch blocks unnecessary. Only suggest "safety" cleanup when the library/function's documented semantics do not already provide graceful failure handling.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T13:21:06.181Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:21:06.181Z
Learning: In WLED's image_loader.cpp, decoder.alloc() uses `new AnimatedGIF()`. When `new` throws an exception (e.g., std::bad_alloc), no pointer is assigned and C++ semantics ensure automatic cleanup of any partially constructed storage. Therefore, calling decoder.dealloc() in the catch block after a failed alloc() is unnecessary. More generally, "safety" cleanup or de-allocation code should only be suggested when the library or sub-function involved is not already taking care of cleanup after failure per its documented semantics.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-12-01T07:01:16.949Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-08-29T01:28:10.512Z
Learnt from: willmmiles
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-08-29T01:28:10.512Z
Learning: In ESP8266 code, the system_get_rst_info() function cannot return null as it returns a pointer to static memory. This is a common C API pattern that hides internal structure details, so null checks are unnecessary for this function.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-09-11T04:36:42.869Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4754
File: usermods/user_fx/README.md:169-179
Timestamp: 2025-09-11T04:36:42.869Z
Learning: In WLED, SEGENV.data is already typed as uint8_t* and can be accessed directly without casting when storing per-pixel state data.
Applied to files:
wled00/FX_fcn.cpp
🧬 Code graph analysis (1)
wled00/FX_fcn.cpp (2)
wled00/util.cpp (2)
extractModeDefaults(349-367)extractModeDefaults(349-349)wled00/colors.h (1)
color_blend16(118-118)
⏰ 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: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (4)
wled00/FX_fcn.cpp (4)
566-577: LGTM!The default handling for
custom3,rotateSpeed, andzoomAmountfollows the established pattern and correctly applies the 4-bit constraints (0-15) as described in the PR objectives.
1547-1559: Buffer management looks intentional; comment clarifies future consideration.The inline comment "may use allocateBuffer() if needed" indicates awareness of the allocation strategy. Given the discussion that
newthrows on ESP (crashing the device), and this is a hot path, consider whether the feature should degrade gracefully if memory is tight rather than crash.
1577-1602: Push-style blending updates integrate rotated buffers correctly.The changes properly:
- Update offset calculations for PUSH_RIGHT/PUSH_UP directions
- Use
whileloops for wrapping (consistent with RotateAndZoom pattern)- Sample from the appropriate rotated/zoomed buffer (
_pixelsRor_pixelsO)The while loops here are safe since
offsetX <= nColsandoffsetY <= nRowsby construction, limiting iterations to at most one.
1633-1635: LGTM!The cleanup conditions correctly mirror the allocation conditions, ensuring no memory leaks. The
delete[]usage matches thenew[]allocations.
Contributor
|
Tested this out for a bit. Everything seems to work fine. I think I would change the zoom slider to be an odd range so that no zoom is centered on the slider instead of off to the right. Regarding wrap options I think the wrap implemented is best for 1D bar effects and 2D effects like akemi that you wouldn't want to mirror. Otherwise wrap with mirror I think looks best by hiding all the edges better. |
Contributor
Author
I'm not sure I understand. Can you rephrase? So, for 1D you would recommend zoom and mirror? On a second thought it could do that for everything. |
Contributor
Author
|
I have no intention of pursuing this any further (and I no longer have write permissions to this branch) so I'm closing. If any of WLED team members will pick up they can reopen or create a new PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Original zoom and rotate code by @Brandon502 preserved entirely (but some parts commented out since they are not implemented). Not implemented parts (optional wrapping and mirroring) would require two additional Segment properties and UI elements. I've decided that, although nice, do not justify cluttering (already cluttered) UI.
I opted to use 4 bit properties (0-15) to reduce RAM footprint but it may affect speed. If this becomes an issue both can be expanded to 8 bit with little effort. Rotation speed has been tested at 80 FPS and may require additional tweaking for higher FPS.
Summary by CodeRabbit
New Features
Bug Fixes / Stability
UI
✏️ Tip: You can customize this high-level summary in your review settings.