Skip to content
254 changes: 254 additions & 0 deletions usermods/user_fx/user_fx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@

// for information how FX metadata strings work see https://kno.wled.ge/interfaces/json-api/#effect-metadata

// paletteBlend: 0 - wrap when moving, 1 - always wrap, 2 - never wrap, 3 - none (undefined)
#define PALETTE_SOLID_WRAP (paletteBlend == 1 || paletteBlend == 3)

#define indexToVStrip(index, stripNr) ((index) | (int((stripNr)+1)<<16))

// static effect, used if an effect fails to initialize
static uint16_t mode_static(void) {
SEGMENT.fill(SEGCOLOR(0));
return strip.isOffRefreshRequired() ? FRAMETIME : 350;
}


/////////////////////////
// User FX functions //
/////////////////////////
Expand Down Expand Up @@ -89,6 +95,253 @@ unsigned dataSize = cols * rows; // SEGLEN (virtual length) is equivalent to vW
static const char _data_FX_MODE_DIFFUSIONFIRE[] PROGMEM = "Diffusion Fire@!,Spark rate,Diffusion Speed,Turbulence,,Use palette;;Color;;2;pal=35";


/*
* Spinning Wheel effect - LED animates around 1D strip (or each column in a 2D matrix), slows down and stops at random position
* Created by Bob Loeffler and claude.ai
* First slider (Spin speed) is for the speed of the moving/spinning LED (random number within a narrow speed range).
* If value is 0, a random speed will be selected from the full range of values.
* Second slider (Spin slowdown start time) is for how long before the slowdown phase starts (random number within a narrow time range).
* If value is 0, a random time will be selected from the full range of values.
* Third slider (Spinner size) is for the number of pixels that make up the spinner.
* Fourth slider (Spin delay) is for how long it takes for the LED to start spinning again after the previous spin.
* The first checkbox sets the color mode (color wheel or palette).
* The second checkbox sets "color per block" mode. Enabled means that each spinner block will be the same color no matter what its LED position is.
* The third checkbox enables synchronized restart (all spinners restart together instead of individually).
* aux0 stores the settings checksum to detect changes
* aux1 stores the color scale for performance
*/

static uint16_t mode_spinning_wheel(void) {
if (SEGLEN < 1) return mode_static();

unsigned strips = SEGMENT.nrOfVStrips();
if (strips == 0) return mode_static();

constexpr unsigned stateVarsPerStrip = 8;
unsigned dataSize = sizeof(uint32_t) * stateVarsPerStrip;
if (!SEGENV.allocateData(dataSize * strips)) return mode_static();
uint32_t* state = reinterpret_cast<uint32_t*>(SEGENV.data);
// state[0] = current position (fixed point: upper 16 bits = position, lower 16 bits = fraction)
// state[1] = velocity (fixed point: pixels per frame * 65536)
// state[2] = phase (0=fast spin, 1=slowing, 2=wobble, 3=stopped)
// state[3] = stop time (when phase 3 was entered)
// state[4] = wobble step (0=at stop pos, 1=moved back, 2=returned to stop)
// state[5] = slowdown start time (when to transition from phase 0 to phase 1)
// state[6] = wobble timing (for 200ms / 400ms / 300ms delays)
// state[7] = store the stop position per strip

// state[] index values for easier readability
constexpr unsigned CUR_POS_IDX = 0; // state[0]
constexpr unsigned VELOCITY_IDX = 1;
constexpr unsigned PHASE_IDX = 2;
constexpr unsigned STOP_TIME_IDX = 3;
constexpr unsigned WOBBLE_STEP_IDX = 4;
constexpr unsigned SLOWDOWN_TIME_IDX = 5;
constexpr unsigned WOBBLE_TIME_IDX = 6;
constexpr unsigned STOP_POS_IDX = 7;

SEGMENT.fill(SEGCOLOR(1));

// Handle random seeding globally (outside the virtual strip)
if (SEGENV.call == 0) {
random16_set_seed(hw_random16());
SEGENV.aux1 = (255 << 8) / SEGLEN; // Cache the color scaling
}
Comment on lines +147 to +149
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's read the specific lines mentioned in the review
cat -n usermods/user_fx/user_fx.cpp | sed -n '140,150p; 143,150p; 298,325p'

Repository: wled/WLED

Length of output: 2394


🏁 Script executed:

# Find the definition and usage of indexToVStrip
rg -n "indexToVStrip" -B 2 -A 5

Repository: wled/WLED

Length of output: 6486


🏁 Script executed:

# Search for virtualHeight and related functions
rg -n "virtualHeight|vHeight\(\)|nrOfVStrips" -B 2 -A 2

Repository: wled/WLED

Length of output: 19142


🏁 Script executed:

# Look for how virtual strips are used in effects
rg -n "indexToVStrip" usermods/user_fx/user_fx.cpp -B 3 -A 8

Repository: wled/WLED

Length of output: 974


🏁 Script executed:

# Check SEGLEN definition and usage
rg -n "define SEGLEN|#define SEGLEN" -A 2

Repository: wled/WLED

Length of output: 231


🏁 Script executed:

# Find setPixelColor implementation and how it handles indexToVStrip encoding
rg -n "void.*setPixelColor\(" wled00/FX_fcn.cpp -A 15 | head -60

Repository: wled/WLED

Length of output: 1502


🏁 Script executed:

# Check if there's any mapping logic that interprets the high 16 bits (stripNr encoding)
rg -n "stripNr|high.*16|>>.*16" wled00/FX_fcn.cpp | head -30

Repository: wled/WLED

Length of output: 515


🏁 Script executed:

# Verify how the Fire 2012 effect handles virtual strips - does it always use full SEGLEN for each strip?
rg -n "mode_fire_2012" wled00/FX.cpp -A 40 | head -50

Repository: wled/WLED

Length of output: 2593


🏁 Script executed:

# Check if there's explicit bounds checking when using setPixelColor with virtual strips
rg -n "setPixelColor.*indexToVStrip|indexToVStrip.*setPixelColor" wled00/FX.cpp -B 2 -A 2 | head -40

Repository: wled/WLED

Length of output: 2327


🏁 Script executed:

# Look for documentation or comments about per-strip indexing vs full SEGLEN
rg -n "virtual.*strip|vstrip" wled00/FX.h -B 2 -A 2 | head -40

Repository: wled/WLED

Length of output: 533


Use per-strip length instead of SEGLEN when rendering virtual strips in M12_pBar mode.

For M12_pBar (expanding 1D effects to 2D bars), each virtual strip has length virtualHeight(), not SEGLEN (which is virtualWidth() * virtualHeight()). Moduloing and scaling by SEGLEN produces indices beyond the per-strip bounds, causing pixel wrapping or clipping errors.

🛠️ Proposed fix
+  const uint16_t vstripLen = SEGMENT.is2D() ? SEGMENT.virtualHeight() : SEGLEN;
   if (SEGENV.call == 0) {
     random16_set_seed(hw_random16());
-    SEGENV.aux1 = (255 << 8) / SEGLEN; // Cache the color scaling
+    SEGENV.aux1 = (255 << 8) / vstripLen; // Cache the color scaling
   }
...
-  uint16_t pos = (pos_fixed >> 16) % SEGLEN;
+  uint16_t pos = (pos_fixed >> 16) % vstripLen;
...
-          uint16_t drawPos = (pos + y) % SEGLEN;
+          uint16_t drawPos = (pos + y) % vstripLen;

Also applies to: 298–310, 318–323

🤖 Prompt for AI Agents
In `@usermods/user_fx/user_fx.cpp` around lines 145 - 147, SEGENV.aux1 and any
modulo/index math used when rendering virtual strips in M12_pBar currently use
SEGLEN (full strip area) but must use the per-virtual-strip length
virtualHeight(); replace uses of SEGLEN in the M12_pBar branch (including the
SEGENV.aux1 computation near random16_set_seed(hw_random16()) and the
index/modulo/scaling logic around the blocks you noted at ~298–310 and ~318–323)
with virtualHeight() so indices and scaling are computed per-strip, not
per-area, preventing out-of-bounds wrapping/clipping.

✅ Addressed in commit 0f5469a

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai The vstripLen suggestion is good when using the M12_pBar, but when I use the Pinwheel option, part of the pinwheel is missing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for M12 mapping mode definitions and usage
rg -n "M12_|mapping.*1D2D|pBar|Pinwheel" wled00/const.h wled00/FX.h -B 2 -A 2

Length of output: 668


🏁 Script executed:

#!/bin/bash
# Look for how other effects handle different mapping modes with virtual strips
rg -n "nrOfVStrips|virtualHeight|mapping" wled00/FX.cpp -B 3 -A 3 | grep -A 5 -B 5 "M12\|pBar\|mapping"

Length of output: 3257


🏁 Script executed:

#!/bin/bash
# Search for the mapping enum or constants
rg -n "enum.*map|MAP.*=|mapping.*mode" wled00/const.h wled00/FX.h -B 1 -A 3

Length of output: 157


🏁 Script executed:

#!/bin/bash
# Look at how fire effects or other 1D-to-2D effects determine strip length
rg -n "mode_fire_2012|Fire 2012" wled00/FX.cpp -A 50 | head -80

Length of output: 3682


@BobLoeffler68 Thank you for catching that! You're absolutely right. My initial suggestion was too narrow—it only considered M12_pBar mode.

The issue is that nrOfVStrips() returns different values depending on the mapping mode:

  • M12_pBar (bars): returns virtualWidth() - creates multiple vertical strips
  • M12_sPinwheel (pinwheel) and other modes: returns 1 - uses a single virtual strip

Here's the corrected fix that handles all mapping modes:

Corrected fix that works for all M12 mapping modes:

  unsigned strips = SEGMENT.nrOfVStrips();
  constexpr unsigned stateVarsPerStrip = 8;
  unsigned dataSize = sizeof(uint32_t) * stateVarsPerStrip;
  if (!SEGENV.allocateData(dataSize * strips)) return mode_static();
  uint32_t* state = reinterpret_cast<uint32_t*>(SEGENV.data);
  
+ // Calculate per-strip length based on mapping mode
+ const uint16_t vstripLen = SEGLEN / strips;
+
  // state[0] = current position (fixed point: upper 16 bits = position, lower 16 bits = fraction)
  ...
  
  if (SEGENV.call == 0) {
    random16_set_seed(hw_random16());
-   SEGENV.aux1 = (255 << 8) / SEGLEN; // Cache the color scaling
+   SEGENV.aux1 = (255 << 8) / vstripLen; // Cache the color scaling
  }
  
  ...
  
- uint16_t pos = (pos_fixed >> 16) % SEGLEN;
+ uint16_t pos = (pos_fixed >> 16) % vstripLen;
  
  ...
  
  for (int8_t x = 0; x < spinnerSize; x++) {
    for (uint8_t y = 0; y < spinnerSize; y++) {
-     uint16_t drawPos = (pos + y) % SEGLEN;
+     uint16_t drawPos = (pos + y) % vstripLen;
      int16_t drawStrip = stripNr + x;

Why this works:

  • vstripLen = SEGLEN / strips automatically calculates the correct length per virtual strip
  • M12_pBar mode: strips = virtualWidth(), so vstripLen = (virtualWidth() × virtualHeight()) / virtualWidth() = virtualHeight()
  • Pinwheel mode: strips = 1, so vstripLen = SEGLEN / 1 = SEGLEN

This matches how Fire 2012 and other effects handle virtual strips—they always use SEGLEN in their loops, which naturally adapts based on how many virtual strips are active.

Sorry for the incomplete initial suggestion! This should now work correctly for both M12_pBar and Pinwheel modes.


🧬 Code Graph Analysis Results

[usermods/user_fx/user_fx.cpp: Macro and usage related to virtual strips mapping]

  • Summary: This section defines a macro indexToVStrip(index, stripNr) used to map a LED index within a virtual strip to a specific physical strip. It is then used in the Spinning Wheel drawing logic to set a pixel color on the appropriate virtual/physical combination. Key details:
    • Macro parameters:
      • index: LED index within the 1D representation of a strip.
      • stripNr: The virtual strip number being addressed.
    • Return value: A 32-bit value encoding both the LED index and the target virtual strip number, used by SEGMENT.setPixelColor to address the correct LED in a potentially multi-strip (1D/2D) arrangement.
    • Usage context: In mode_spinning_wheel, when rendering the spinner, the code computes drawPos (LED position within a strip) and drawStrip (target strip), then calls SEGMENT.setPixelColor(indexToVStrip(drawPos, drawStrip), color) to apply the color to the appropriate LED on the correct strip.
    • Important implications: This mapping underpins correct rendering across multi-strip or 2D configurations; mismatches can cause parts of the pinwheel to be missing or misaligned if vstripLen or strip counting is inconsistent.

[usermods/user_fx/user_fx.cpp: mode_spinning_wheel drawing logic using virtual-strip mapping]

  • Summary: This portion of the Spinning Wheel implementation handles rendering across virtual strips by calculating positions (pos, spinnerSize), determining color per block, and applying color to the correct LEDs using the virtual-to-physical mapping described above.
    • Core behavior:
      • Determines per-strip state and spinner parameters (spinnerSize, etc.).
      • Computes the current LED position (pos) and color hue for each spinner block.
      • Iterates over a spinnerSize x spinnerSize block, computes drawPos and drawStrip for each LED in the block, and uses indexToVStrip(drawPos, drawStrip) to set the LED color.
    • Important details:
      • The rendering relies on a correct virtual strip layout (nrOfVStrips) and a consistent vstrip length (vstripLen). If vstripLen is misaligned with the actual hardware layout (e.g., when using Pinwheel mode), portions of the spinner can appear missing.
      • The logic uses SEGMENT.setPixelColor(indexToVStrip(drawPos, drawStrip), color) to address LEDs across the virtual/physical mapping.
    • Potential impact on issues described: A mismatch between vstripLen expectations and the actual hardware arrangement (especially when switching between M12_pBar and Pinwheel options) can produce incomplete rendering of the pinwheel, as observed in the user comment.

[Note: If no additional relevant code snippets are found, respond with the above two contexts. If you need exact line-number references, please provide a version with line numbers or request a line-numbered snippet extraction.]


🧠 Learnings used
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: 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: 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: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).

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: 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: 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: 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: 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: 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: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:20.560Z
Learning: In WLED effects (usermods/user_fx/user_fx.cpp and similar), when storing future timestamps, assignments like `stored_time = now + delay` are correct and handle uint32_t wraparound properly. Only comparison statements need to use subtraction-based elapsed-time checks (e.g., `(now - stored_time) >= delay`). Do not flag assignment statements as needing fixes for wraparound handling.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai I tried your fix, but then I get 2 errors when compiling. Here is one error that pertains to the uint16_t drawPos = (pos + y) % vstripLen; line: usermods/user_fx/user_fx.cpp:321:42: error: use of local variable with automatic storage from containing function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BobLoeffler68 You're absolutely right! I apologize for the error. The problem is that vstripLen is a local variable in mode_spinning_wheel(), but virtualStrip::runStrip() is a static method that cannot access variables from the containing function's scope.

The fix is to pass vstripLen as a parameter to runStrip():

static uint16_t mode_spinning_wheel(void) {
  if (SEGLEN < 1) return mode_static();
  
  unsigned strips = SEGMENT.nrOfVStrips();
  constexpr unsigned stateVarsPerStrip = 8;
  unsigned dataSize = sizeof(uint32_t) * stateVarsPerStrip;
  if (!SEGENV.allocateData(dataSize * strips)) return mode_static();
  uint32_t* state = reinterpret_cast<uint32_t*>(SEGENV.data);
  
  // Calculate per-strip length based on mapping mode
  const uint16_t vstripLen = SEGLEN / strips;
  
  // state[] index values for easier readability
  constexpr unsigned CUR_POS_IDX       = 0;
  constexpr unsigned VELOCITY_IDX      = 1;
  constexpr unsigned PHASE_IDX         = 2;
  constexpr unsigned STOP_TIME_IDX     = 3;
  constexpr unsigned WOBBLE_STEP_IDX   = 4;
  constexpr unsigned SLOWDOWN_TIME_IDX = 5;
  constexpr unsigned WOBBLE_TIME_IDX   = 6;
  constexpr unsigned STOP_POS_IDX      = 7;

  SEGMENT.fill(SEGCOLOR(1));

  // Handle random seeding globally (outside the virtual strip)
  if (SEGENV.call == 0) {
    random16_set_seed(hw_random16());
    SEGENV.aux1 = (255 << 8) / vstripLen; // Cache the color scaling
  }
  
  // ... rest of setup code ...

  struct virtualStrip {
    
    // Add vstripLen as a parameter here
    static void runStrip(uint16_t stripNr, uint32_t* state, bool settingsChanged, bool allReadyToRestart, uint16_t vstripLen) {

      uint8_t phase = state[PHASE_IDX];
      uint32_t now = strip.now;

      // ... rest of the function stays the same until we get to the drawing part ...
      
      // Update position (phases 0 and 1 only)
      if (phase == 0 || phase == 1) {
        pos_fixed += velocity;
        state[CUR_POS_IDX] = pos_fixed;
      }
      
      // Draw LED for all phases
      uint16_t pos = (pos_fixed >> 16) % vstripLen;  // Now uses the parameter

      uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);

      // ... color calculation ...

      // Draw the spinner with configurable size (1-10 LEDs)
      for (int8_t x = 0; x < spinnerSize; x++) {
        for (uint8_t y = 0; y < spinnerSize; y++) {
          uint16_t drawPos = (pos + y) % vstripLen;  // Now uses the parameter
          int16_t drawStrip = stripNr + x;
          
          // Wrap horizontally if needed, or skip if out of bounds
          if (drawStrip >= 0 && drawStrip < (int16_t)SEGMENT.nrOfVStrips()) {
            SEGMENT.setPixelColor(indexToVStrip(drawPos, drawStrip), color);
          }
        }
      }
    }
  };

  // Pass vstripLen as a parameter when calling runStrip()
  for (unsigned stripNr=0; stripNr<strips; stripNr++) {
    uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);
    if (stripNr % spinnerSize == 0) {
      virtualStrip::runStrip(stripNr, &state[stripNr * stateVarsPerStrip], settingsChanged, allReadyToRestart, vstripLen);
    }
  }

  return FRAMETIME;
}

Summary of changes:

  1. Added uint16_t vstripLen as the last parameter to virtualStrip::runStrip()
  2. Passed vstripLen when calling runStrip() in the for loop at the bottom
  3. Now vstripLen is accessible inside the static method as a parameter

This should compile correctly and work for both M12_pBar and Pinwheel modes!


🧠 Learnings used
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: 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: 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: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).

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: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:20.560Z
Learning: In WLED effects (usermods/user_fx/user_fx.cpp and similar), when storing future timestamps, assignments like `stored_time = now + delay` are correct and handle uint32_t wraparound properly. Only comparison statements need to use subtraction-based elapsed-time checks (e.g., `(now - stored_time) >= delay`). Do not flag assignment statements as needing fixes for wraparound handling.

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: 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.

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: 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: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: 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai I had not pushed my latest code yet, so you were using old code. I just pushed the latest code with your suggestions, but it still does not work. Please check the code to see why. Thx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted back to using SEGLEN instead of vstripLen, except for this line:
SEGENV.aux1 = (255 << 8) / vstripLen;
and now it's working correctly again for M12_pBar, M12_pPixel and M12_pPinwheel
But, the color/hue is off when using palettes. The M12_pBar only shows the first 66% of the colors in the palette. If I change that line back to:
SEGENV.aux1 = (255 << 8) / SEGLEN;
then it uses all of the colors of the palette.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BobLoeffler68 You've found the correct solution! The key insight is that the color scaling must match the position range.

Here's what's happening:

Why the color issue occurs with vstripLen:

  1. Your position calculation uses: pos = (pos_fixed >> 16) % SEGLEN

    • For a 32×32 matrix: pos ranges from 0 to 1023
  2. Color calculation uses: hue = (SEGENV.aux1 * pos) >> 8

    • With aux1 = (255 << 8) / vstripLen = (255 << 8) / 32 = 2040
    • When pos = 32: hue = (2040 × 32) >> 8 = 255 (full palette cycle complete!)
    • But pos continues to 1023, causing palette to repeat ~32 times
  3. The result: You only see the first 1/32 of the animation span using the full palette range, which appears as "only 66% of colors" in M12_pBar.

Why SEGLEN works correctly:

  • With aux1 = (255 << 8) / SEGLEN = (255 << 8) / 1024 = 63.75
  • As pos goes from 0 to 1023, hue smoothly transitions from 0 to 255
  • The palette is distributed across the entire position range

The correct solution is what you already have:

SEGENV.aux1 = (255 << 8) / SEGLEN;  // Keep this!

My earlier suggestion to use vstripLen was based on the assumption that we'd also change all position calculations to use vstripLen, making each virtual strip independent with its own position wrapping. But since the effect works correctly with SEGLEN-based position wrapping (and handles M12_pBar, pPixel, and pPinwheel correctly), that means the indexToVStrip macro and virtual strip addressing is handling the mapping correctly at the setPixelColor level.

Keep your current implementation - it's working correctly! The color scale should match the position range, and both should use SEGLEN.


✏️ Learnings added
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:149-151
Timestamp: 2026-01-24T18:51:02.052Z
Learning: In WLED's Spinning Wheel effect (usermods/user_fx/user_fx.cpp), the color scale calculation (SEGENV.aux1) must match the position modulo range. When position uses `pos = (pos_fixed >> 16) % SEGLEN`, the color scale must be `aux1 = (255 << 8) / SEGLEN` to distribute the palette evenly across the full position range. Using vstripLen for color scaling while position uses SEGLEN causes palette colors to cycle too quickly and appear incomplete.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
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: 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: 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: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).

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: 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: 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: 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: 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: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:20.560Z
Learning: In WLED effects (usermods/user_fx/user_fx.cpp and similar), when storing future timestamps, assignments like `stored_time = now + delay` are correct and handle uint32_t wraparound properly. Only comparison statements need to use subtraction-based elapsed-time checks (e.g., `(now - stored_time) >= delay`). Do not flag assignment statements as needing fixes for wraparound handling.

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: 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: 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: 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.

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.

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.

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.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-15T20:25:25.815Z
Learning: The CSS palette preview in wled00/data/index.js genPalPrevCss() function uses raw RGB values in CSS linear-gradient() without applying WLED's gamma correction, while actual LED output goes through NeoGammaWLEDMethod gamma correction. This causes inherent discrepancies between the web UI palette preview and actual LED colors, especially noticeable with different gamma settings.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thx. I'll keep it the way I have it now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BobLoeffler68 Perfect! Your solution is correct—keeping SEGLEN for both position calculations and color scaling ensures the palette is distributed evenly across the entire animation range and works properly with all M12 mapping modes. 👍


🧠 Learnings used
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:149-151
Timestamp: 2026-01-24T18:51:02.052Z
Learning: In WLED's Spinning Wheel effect (usermods/user_fx/user_fx.cpp), the color scale calculation (SEGENV.aux1) must match the position modulo range. When position uses `pos = (pos_fixed >> 16) % SEGLEN`, the color scale must be `aux1 = (255 << 8) / SEGLEN` to distribute the palette evenly across the full position range. Using vstripLen for color scaling while position uses SEGLEN causes palette colors to cycle too quickly and appear incomplete.

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: 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: 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: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).

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: 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: 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: 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: 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: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:20.560Z
Learning: In WLED effects (usermods/user_fx/user_fx.cpp and similar), when storing future timestamps, assignments like `stored_time = now + delay` are correct and handle uint32_t wraparound properly. Only comparison statements need to use subtraction-based elapsed-time checks (e.g., `(now - stored_time) >= delay`). Do not flag assignment statements as needing fixes for wraparound handling.

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: 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: 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: 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.

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.

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]

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.


// Check if settings changed (do this once, not per virtual strip)
uint32_t settingssum = SEGMENT.speed + SEGMENT.intensity + SEGMENT.custom1 + SEGMENT.custom3 + SEGMENT.check3;
bool settingsChanged = (SEGENV.aux0 != settingssum);
if (settingsChanged) {
random16_add_entropy(hw_random16());
SEGENV.aux0 = settingssum;
}

// Check if all spinners are stopped and ready to restart (for synchronized restart)
bool allReadyToRestart = true;
if (SEGMENT.check3) {
uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);
uint16_t spin_delay = map(SEGMENT.custom3, 0, 31, 2000, 15000);
uint32_t now = strip.now;

for (unsigned stripNr = 0; stripNr < strips; stripNr += spinnerSize) {
uint32_t* stripState = &state[stripNr * stateVarsPerStrip];
// Check if this spinner is stopped AND has waited its delay
if (stripState[PHASE_IDX] != 3 || stripState[STOP_TIME_IDX] == 0) {
allReadyToRestart = false;
break;
}
// Check if delay has elapsed
if ((now - stripState[STOP_TIME_IDX]) < spin_delay) {
allReadyToRestart = false;
break;
}
}
}

struct virtualStrip {
static void runStrip(uint16_t stripNr, uint32_t* state, bool settingsChanged, bool allReadyToRestart) {

uint8_t phase = state[PHASE_IDX];
uint32_t now = strip.now;

// Check for restart conditions
bool needsReset = false;
if (SEGENV.call == 0) {
needsReset = true;
} else if (settingsChanged) {
needsReset = true;
} else if (phase == 3 && state[STOP_TIME_IDX] != 0) {
// If synchronized restart is enabled, only restart when all strips are ready
if (SEGMENT.check3) {
if (allReadyToRestart) {
needsReset = true;
}
} else {
// Normal mode: restart after individual strip delay
uint16_t spin_delay = map(SEGMENT.custom3, 0, 31, 2000, 15000);
if ((now - state[STOP_TIME_IDX]) >= spin_delay) {
needsReset = true;
}
}
}

// Initialize or restart
if (needsReset) {
state[CUR_POS_IDX] = 0;

// Set velocity
uint16_t speed = map(SEGMENT.speed, 0, 255, 300, 800);
if (speed == 300) { // random speed (user selected 0 on speed slider)
state[VELOCITY_IDX] = random16(200, 900) * 655; // fixed-point velocity scaling (approx. 65536/100)
} else {
state[VELOCITY_IDX] = random16(speed - 100, speed + 100) * 655;
}

// Set slowdown start time
uint16_t slowdown = map(SEGMENT.intensity, 0, 255, 3000, 5000);
if (slowdown == 3000) { // random slowdown start time (user selected 0 on intensity slider)
state[SLOWDOWN_TIME_IDX] = now + random16(2000, 6000);
} else {
state[SLOWDOWN_TIME_IDX] = now + random16(slowdown - 1000, slowdown + 1000);
}

state[PHASE_IDX] = 0;
state[STOP_TIME_IDX] = 0;
state[WOBBLE_STEP_IDX] = 0;
state[WOBBLE_TIME_IDX] = 0;
state[STOP_POS_IDX] = 0; // Initialize stop position
phase = 0;
}

uint32_t pos_fixed = state[CUR_POS_IDX];
uint32_t velocity = state[VELOCITY_IDX];

// Phase management
if (phase == 0) {
// Fast spinning phase
if ((int32_t)(now - state[SLOWDOWN_TIME_IDX]) >= 0) {
phase = 1;
state[PHASE_IDX] = 1;
}
} else if (phase == 1) {
// Slowing phase - apply deceleration
uint32_t decel = velocity / 80;
if (decel < 100) decel = 100;

velocity = (velocity > decel) ? velocity - decel : 0;
state[VELOCITY_IDX] = velocity;

// Check if stopped
if (velocity < 2000) {
velocity = 0;
state[VELOCITY_IDX] = 0;
phase = 2;
state[PHASE_IDX] = 2;
state[WOBBLE_STEP_IDX] = 0;
uint16_t stop_pos = (pos_fixed >> 16) % SEGLEN;
state[STOP_POS_IDX] = stop_pos;
state[WOBBLE_TIME_IDX] = now;
}
} else if (phase == 2) {
// Wobble phase (moves the LED back one and then forward one)
uint32_t wobble_step = state[WOBBLE_STEP_IDX];
uint16_t stop_pos = state[STOP_POS_IDX];
uint32_t elapsed = now - state[WOBBLE_TIME_IDX];

if (wobble_step == 0 && elapsed >= 200) {
// Move back one LED from stop position
uint16_t back_pos = (stop_pos == 0) ? SEGLEN - 1 : stop_pos - 1;
pos_fixed = ((uint32_t)back_pos) << 16;
state[CUR_POS_IDX] = pos_fixed;
state[WOBBLE_STEP_IDX] = 1;
state[WOBBLE_TIME_IDX] = now;
} else if (wobble_step == 1 && elapsed >= 400) {
// Move forward to the stop position
pos_fixed = ((uint32_t)stop_pos) << 16;
state[CUR_POS_IDX] = pos_fixed;
state[WOBBLE_STEP_IDX] = 2;
state[WOBBLE_TIME_IDX] = now;
} else if (wobble_step == 2 && elapsed >= 300) {
// Wobble complete, enter stopped phase
phase = 3;
state[PHASE_IDX] = 3;
state[STOP_TIME_IDX] = now;
}
}

// Update position (phases 0 and 1 only)
if (phase == 0 || phase == 1) {
pos_fixed += velocity;
state[CUR_POS_IDX] = pos_fixed;
}

// Draw LED for all phases
uint16_t pos = (pos_fixed >> 16) % SEGLEN;

uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);

// Calculate color once per spinner block (based on strip number, not position)
uint8_t hue;
if (SEGMENT.check2) {
// Each spinner block gets its own color based on strip number
uint16_t numSpinners = max(1U, (SEGMENT.nrOfVStrips() + spinnerSize - 1) / spinnerSize);
hue = (255 * (stripNr / spinnerSize)) / numSpinners;
} else {
// Color changes with position
hue = (SEGENV.aux1 * pos) >> 8;
}

uint32_t color = SEGMENT.check1 ? SEGMENT.color_wheel(hue) : SEGMENT.color_from_palette(hue, true, PALETTE_SOLID_WRAP, 0);

// Draw the spinner with configurable size (1-10 LEDs)
for (int8_t x = 0; x < spinnerSize; x++) {
for (uint8_t y = 0; y < spinnerSize; y++) {
uint16_t drawPos = (pos + y) % SEGLEN;
int16_t drawStrip = stripNr + x;

// Wrap horizontally if needed, or skip if out of bounds
if (drawStrip >= 0 && drawStrip < (int16_t)SEGMENT.nrOfVStrips()) {
SEGMENT.setPixelColor(indexToVStrip(drawPos, drawStrip), color);
}
}
}
}
};

for (unsigned stripNr=0; stripNr<strips; stripNr++) {
// Only run on strips that are multiples of spinnerSize to avoid overlap
uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);
if (stripNr % spinnerSize == 0) {
virtualStrip::runStrip(stripNr, &state[stripNr * stateVarsPerStrip], settingsChanged, allReadyToRestart);
}
}

return FRAMETIME;
}
static const char _data_FX_MODE_SPINNINGWHEEL[] PROGMEM = "Spinning Wheel@Speed (0=random),Slowdown (0=random),Spinner size,,Spin delay,Color mode,Color per block,Sync restart;!,!;!;;m12=1,c1=1,c3=8";



/////////////////////
// UserMod Class //
/////////////////////
Expand All @@ -98,6 +351,7 @@ class UserFxUsermod : public Usermod {
public:
void setup() override {
strip.addEffect(255, &mode_diffusionfire, _data_FX_MODE_DIFFUSIONFIRE);
strip.addEffect(255, &mode_spinning_wheel, _data_FX_MODE_SPINNINGWHEEL);

////////////////////////////////////////
// add your effect function(s) here //
Expand Down