Draft
Conversation
Extend the Alexa/Hue emulation to optionally expose each active segment as a separate Alexa device. When enabled via the new "Also emulate a device for each active segment" checkbox in Sync settings, users can independently control brightness, color, and on/off state per segment through Alexa voice commands. Device index layout: 0=main, 1..N=presets, (N+1)..=segments. Backwards compatible: disabled by default, existing preset behavior is unchanged. ESPALEXA_MAXDEVICES raised from 10 to 20 to accommodate segment devices alongside presets. Agent-Logs-Url: https://github.com/wled/WLED/sessions/2bb549aa-be26-40ef-870e-20a7379d69e0 Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
…ve clarity - Extract mapSegDevToSegIndex() helper function to eliminate duplicated segment-to-device mapping logic - Add bounds validation (return -1) when segment device offset exceeds active segment count (handles segments deactivated after discovery) - Simplify opacity logic: only set to 255 if currently 0 - Extract presetEnd variable to avoid repeated ternary expression - Reduce String heap fragmentation in segment name construction - Add comment documenting ESPALEXA_MAXDEVICES rationale Agent-Logs-Url: https://github.com/wled/WLED/sessions/2bb549aa-be26-40ef-870e-20a7379d69e0 Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add option to emulate Alexa devices per segment
Add per-segment Alexa device emulation
Apr 9, 2026
softhack007
reviewed
Apr 10, 2026
| } | ||
| } else if (seg.hasWhite()) { | ||
| switch (ct) { | ||
| case 199: rgbw[0]=255; rgbw[1]=255; rgbw[2]=255; rgbw[3]=255; break; |
Member
There was a problem hiding this comment.
@coderabbitai 🤔 why are some values treated differently? optimization for common values?
Contributor
There was a problem hiding this comment.
@softhack007 These specific ct values (199, 234, 284, 350, 383 mireds) are not an optimization for speed — they are empirically tuned RGBW channel mappings for Alexa's fixed color temperature presets. Alexa (emulating a Philips Hue bridge) uses a small, fixed set of named color temperature steps (e.g., "warm white", "soft white", "white", "daylight", "cool white") that always map to exactly these mired values. The generic colorKtoRGB() conversion is used as the fallback for arbitrary temperatures, but for these well-known Alexa presets the RGBW values were hand-tuned to look better on RGBW strips.
The existing main-device handler already has this exact same switch (ct) block with the comment "these values empirically look good on RGBW" — this new segment-device code is simply a copy of that established pattern. Converting mireds to kelvin:
| ct (mireds) | ≈ Kelvin | Alexa preset |
|---|---|---|
| 199 | ~5025 K | Cool white / Daylight |
| 234 | ~4274 K | White |
| 284 | ~3521 K | Soft white |
| 350 | ~2857 K | Warm white |
| 383 | ~2610 K | Incandescent |
So in short: empirical calibration for Alexa's known fixed presets, not a performance optimization.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4893
File: wled00/set.cpp:95-95
Timestamp: 2026-03-14T20:56:46.543Z
Learning: Guideline: Ensure WiFi hostname is set after WiFi.mode() but before WiFi.begin() to avoid default esp-XXXXXX hostname being used in DHCP. This ordering only takes effect after the STA interface exists (so avoid placing hostname setting before initConnection). In WLED, place the hostname configuration inside initConnection() (after WiFi.disconnect(true) and before WiFi.begin()) rather than in earlier boot code like deserializeConfig(). This rule should be applied in code reviews for WLED’s network initialization paths in wled00/*.cpp, and note that on ESP8266 the ordering is less strict but still desirable for consistency.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When calling raw lwIP APIs (e.g., around `ntpUdp.begin()` or any `lwIP`/`tcpip`-layer call) in this codebase on ESP32 Arduino-ESP32 platforms where core locking/checking is enabled, wrap the lwIP call with `LOCK_TCPIP_CORE()` / `UNLOCK_TCPIP_CORE()` from `lwip/tcpip.h`. This prevents thread-safety/core-violation crashes without requiring `sdkconfig` changes.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When using lwIP “raw” APIs in WLED on ESP32 (Arduino-ESP32 / IDF 5.5+), don’t rely on LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() unless CONFIG_LWIP_TCPIP_CORE_LOCKING=y is guaranteed. CONFIG_LWIP_CHECK_THREAD_SAFETY=y can trigger the assertion “Required to lock TCPIP core functionality!” when raw lwIP calls occur off the TCPIP thread. If the locking mode isn’t enabled (or can’t be changed via sdkconfig), schedule lwIP work (e.g., ntpUdp.begin() and similar raw lwIP calls) via tcpip_callback() so it runs on the TCPIP thread; this should work regardless of the locking-mode setting. Review any similar raw lwIP usage for correct thread/context handling.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:698-700
Timestamp: 2026-03-28T01:37:15.541Z
Learning: In this WLED codebase, when using `DEBUG_PRINTLN(F("..."))`, an explicit trailing `\n` inside the `F("...")` string (e.g., `DEBUG_PRINTLN(F("Warning!\n"))`) may be intentional to create a blank line in debug output as a visual separator. During code review, do not automatically flag these as “double newline” or recommend removing the `\n`—first verify with the author/context that the extra newline is deliberate.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:02.808Z
Learning: When working with WLED strings stored in PROGMEM on ESP8266/ESP32, assume you can’t use `strchr_P` for character scanning. If you need to scan a PROGMEM string for a character (e.g., searching for '@' within effect data), review changes to ensure they perform an explicit RAM copy first (e.g., via `strncpy_P` into a buffer) and then scan in RAM. Avoid proposing incremental/streaming PROGMEM character-by-character scans without a RAM buffer on these targets, since it’s impractical with the available APIs.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T07:26:09.816Z
Learning: In WLED’s WLED code, if a pixel/buffer allocation uses `BFRALLOC_NOBYTEACCESS` (and especially on classic ESP32 where byte-level access to IRAM-resident buffers is unsafe), avoid using byte-wise operations like `memset`/`memcpy` on that buffer. Specifically, do not combine `BFRALLOC_CLEAR | BFRALLOC_NOBYTEACCESS` and do not perform `memcpy`/`memset` over `Segment::pixels` (e.g., in `setGeometry()`/`finalizeInit()` or copy ctor/assignment). Instead, use element-wise 32-bit access (loop over `uint32_t*` and assign/copy per element) to ensure 32-bit access only.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T09:14:29.218Z
Learning: In WLED, when reviewing ESP32 code that uses pixel buffers allocated with `BFRALLOC_NOBYTEACCESS` (e.g., `WS2812FX::_pixels` and `Segment::pixels`), treat `memset`/`memcpy` on those buffers as an intentionally low-risk, accepted pattern. These buffers are designed to be IRAM-safe for 32-bit-only access, and the Xtensa newlib assembly implementations perform 32-bit word stores for word-aligned, word-multiple-sized buffers (so the IRAM constraint is satisfied in practice). Do not flag `memset`/`memcpy` on `BFRALLOC_NOBYTEACCESS` pixel buffers as a critical bug; if any concern arises, downgrade severity to low and focus on potential future regressions rather than an immediate correctness violation.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
WalkthroughImplemented optional Alexa segment device support by adding the ability to expose each active WLED segment as a separate Alexa device. When enabled via the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/alexa.cpp (1)
39-65:⚠️ Potential issue | 🟠 MajorHandle
Espalexa::addDevice()failure or the segment-device mapping will drift.The vendored Espalexa implementation stops accepting devices once
ESPALEXA_MAXDEVICES(default 10) is reached and returns 0 without storing the device. Lines 39–65 ignore that outcome, causing rejectedEspalexaDeviceallocations to leak andmapSegDevToSegIndex()to count active segments that were never registered. After the first overflow, Alexa device IDs no longer point at the intended segments. Line 57 also snapshotsalexaSegmentDeviceStartbefore any segment device acceptance is confirmed.🛠️ Suggested guard for rejected devices
void alexaInit() { if (!alexaEnabled || !WLED_CONNECTED) return; espalexa.removeAllDevices(); alexaSegmentDeviceStart = 0; + + auto addAlexaDevice = [](EspalexaDevice* device) -> bool { + if (!device) return false; + unsigned before = espalexa.getDeviceCount(); + espalexa.addDevice(device); + if (espalexa.getDeviceCount() == before) { + delete device; + return false; + } + return true; + }; // the original configured device for on/off or macros (added first, i.e. index 0) espalexaDevice = new EspalexaDevice(alexaInvocationName, onAlexaChange, EspalexaDeviceType::extendedcolor); - espalexa.addDevice(espalexaDevice); + if (!addAlexaDevice(espalexaDevice)) { + espalexaDevice = nullptr; + return; + } // up to 9 devices ... if (alexaNumPresets) { String name = ""; for (unsigned presetIndex = 1; presetIndex <= alexaNumPresets; presetIndex++) { if (!getPresetName(presetIndex, name)) break; // no more presets EspalexaDevice* dev = new EspalexaDevice(name.c_str(), onAlexaChange, EspalexaDeviceType::extendedcolor); - espalexa.addDevice(dev); + if (!addAlexaDevice(dev)) break; } } if (alexaExposeSegments) { - alexaSegmentDeviceStart = espalexa.getDeviceCount(); // first segment device index + unsigned segmentStart = espalexa.getDeviceCount(); for (unsigned i = 0; i < strip.getSegmentsNum(); i++) { Segment& seg = strip.getSegment(i); if (!seg.isActive()) continue; String segName(seg.name ? seg.name : ""); if (!segName.length()) segName = String(F("Segment ")) + String(i); EspalexaDevice* dev = new EspalexaDevice(segName.c_str(), onAlexaChange, EspalexaDeviceType::extendedcolor); - espalexa.addDevice(dev); + if (!addAlexaDevice(dev)) break; } + if (espalexa.getDeviceCount() > segmentStart) alexaSegmentDeviceStart = segmentStart; } espalexa.begin(&server); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/alexa.cpp` around lines 39 - 65, The code ignores Espalexa::addDevice() return values causing leaks and misaligned segment-to-device mapping; update the main-device creation (espalexaDevice) and all places that call espalexa.addDevice(...) (presets and segment loop) to check the return value, and if addDevice indicates failure (0/false) delete the just-allocated EspalexaDevice and skip/break appropriately to avoid leaks and wrong counts; for segments, do not set alexaSegmentDeviceStart before verifying a device was actually accepted — instead initialize alexaSegmentDeviceStart to an invalid sentinel and on the first successful addDevice() call record the actual start index (use espalexa.getDeviceCount()-1 or the addDevice return) so mapSegDevToSegIndex() and segment counting remain correct.
🧹 Nitpick comments (4)
wled00/alexa.cpp (2)
109-143: Extract the Alexa CT/RGB translation into one helper.These two branches now carry the same color-temperature and RGBW translation logic twice. Any future tweak to the Alexa color mapping now has to be made in both places, which makes drift between main-device and segment-device behavior very likely.
Also applies to: 204-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/alexa.cpp` around lines 109 - 143, Extract the duplicated Alexa CT/RGBW translation logic into a single helper function (e.g., translateAlexaColorToRGBW) that takes the device (EspalexaDevice* or an interface exposing getColorMode(), getCt(), getRGB(), setValue()) and the target segment (or returns RGBW/uint32_t) and performs the k = 1000000/ct computation, the seg.isCCT()/seg.setCCT handling, the seg.hasWhite() branching with the ct switch and colorKtoRGB fallback, and returns the final RGBW (or color value) so both callers (the block using dev->getColorMode() == EspalexaColorMode::ct and the other similar branch around lines 204-238) can call translateAlexaColorToRGBW and then call seg.setColor(...) and stateUpdated(CALL_MODE_ALEXA); this removes the duplicated logic in both places and centralizes ct/RGBW mapping.
91-107: Preserve the segment’s last level when Alexa sendsbri=0.The brightness path stores
0directly inseg.opacity, and the nextoncommand then snaps to full scale at Line 94. That means per-segment Alexa control forgets the previous level instead of restoring it the way the main-device path restoresbriLast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/alexa.cpp` around lines 91 - 107, When handling Alexa bri, avoid overwriting the segment's saved last non-zero level so that a later ON restores it: in the EspalexaDeviceProperty::bri branch use dev->getValue() to update seg.setOpacity(...) only when the value is >0 and when value==0 store the zero state but do NOT clobber a seg.briLast (create seg.briLast if missing) — update seg.briLast only when dev->getValue() > 0; also adjust the EspalexaDeviceProperty::on branch (where seg.setOpacity(255) is applied when !seg.opacity) to restore from seg.briLast if present (seg.setOpacity(seg.briLast)) instead of forcing 255. Ensure you reference seg.setOpacity, dev->getValue(), seg.setOption(SEG_OPTION_ON, ...), and stateUpdated(CALL_MODE_ALEXA) when applying the changes.wled00/data/settings_sync.htm (1)
198-199: Add an Alexa rediscovery hint next to these new topology options.Preset count and per-segment exposure only affect what gets announced during discovery, so saving this form alone will not update Alexa’s device list. A short note here will make the limitation discoverable instead of looking like a broken setting.
💡 Suggested UI copy
Also emulate devices to call the first <input name="AP" type="number" class="s" min="0" max="9"> presets<br> -Also emulate a device for each active segment: <input type="checkbox" name="AS"><br><br> +Also emulate a device for each active segment: <input type="checkbox" name="AS"><br> +<i>Changing preset count or per-segment exposure requires Alexa re-discovery.</i><br><br>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_sync.htm` around lines 198 - 199, Add a short Alexa rediscovery hint adjacent to the "AP" (preset count) and "AS" (per-segment exposure) controls explaining that these settings only affect what is announced during discovery and that saving the form will not update Alexa’s device list; instruct users to run Alexa discovery (e.g., "Discover Devices" in the Alexa app or use the voice command) or re-enable integration to refresh the device list. Place this one-line note near the <input name="AP"> and <input name="AS"> elements so it’s visible when those options are toggled.wled00/wled.h (1)
138-139: Keep the new default-off flag, but make the device-cap comment budget-based.
alexaExposeSegmentsdefaulting tofalseis the right shape here. I’d just reword theESPALEXA_MAXDEVICEScomment as a shared 20-device budget across main, preset, and segment devices so later code does not infer a hard “10 segment devices” rule from the comment.Also applies to: 445-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled.h` around lines 138 - 139, Keep the new default-off flag alexaExposeSegments as-is, but update the comment for ESPALEXA_MAXDEVICES to clarify it represents a shared 20-device budget across the main device, presets and segment devices (not a hard per-category limit such as "up to 10 segment devices"); change the similar comment near the other occurrence (the block referencing ESPALEXA_MAXDEVICES around the preset/segment handling) to the same budget-based wording and do not alter the numeric value of ESPALEXA_MAXDEVICES.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@wled00/alexa.cpp`:
- Around line 39-65: The code ignores Espalexa::addDevice() return values
causing leaks and misaligned segment-to-device mapping; update the main-device
creation (espalexaDevice) and all places that call espalexa.addDevice(...)
(presets and segment loop) to check the return value, and if addDevice indicates
failure (0/false) delete the just-allocated EspalexaDevice and skip/break
appropriately to avoid leaks and wrong counts; for segments, do not set
alexaSegmentDeviceStart before verifying a device was actually accepted —
instead initialize alexaSegmentDeviceStart to an invalid sentinel and on the
first successful addDevice() call record the actual start index (use
espalexa.getDeviceCount()-1 or the addDevice return) so mapSegDevToSegIndex()
and segment counting remain correct.
---
Nitpick comments:
In `@wled00/alexa.cpp`:
- Around line 109-143: Extract the duplicated Alexa CT/RGBW translation logic
into a single helper function (e.g., translateAlexaColorToRGBW) that takes the
device (EspalexaDevice* or an interface exposing getColorMode(), getCt(),
getRGB(), setValue()) and the target segment (or returns RGBW/uint32_t) and
performs the k = 1000000/ct computation, the seg.isCCT()/seg.setCCT handling,
the seg.hasWhite() branching with the ct switch and colorKtoRGB fallback, and
returns the final RGBW (or color value) so both callers (the block using
dev->getColorMode() == EspalexaColorMode::ct and the other similar branch around
lines 204-238) can call translateAlexaColorToRGBW and then call
seg.setColor(...) and stateUpdated(CALL_MODE_ALEXA); this removes the duplicated
logic in both places and centralizes ct/RGBW mapping.
- Around line 91-107: When handling Alexa bri, avoid overwriting the segment's
saved last non-zero level so that a later ON restores it: in the
EspalexaDeviceProperty::bri branch use dev->getValue() to update
seg.setOpacity(...) only when the value is >0 and when value==0 store the zero
state but do NOT clobber a seg.briLast (create seg.briLast if missing) — update
seg.briLast only when dev->getValue() > 0; also adjust the
EspalexaDeviceProperty::on branch (where seg.setOpacity(255) is applied when
!seg.opacity) to restore from seg.briLast if present
(seg.setOpacity(seg.briLast)) instead of forcing 255. Ensure you reference
seg.setOpacity, dev->getValue(), seg.setOption(SEG_OPTION_ON, ...), and
stateUpdated(CALL_MODE_ALEXA) when applying the changes.
In `@wled00/data/settings_sync.htm`:
- Around line 198-199: Add a short Alexa rediscovery hint adjacent to the "AP"
(preset count) and "AS" (per-segment exposure) controls explaining that these
settings only affect what is announced during discovery and that saving the form
will not update Alexa’s device list; instruct users to run Alexa discovery
(e.g., "Discover Devices" in the Alexa app or use the voice command) or
re-enable integration to refresh the device list. Place this one-line note near
the <input name="AP"> and <input name="AS"> elements so it’s visible when those
options are toggled.
In `@wled00/wled.h`:
- Around line 138-139: Keep the new default-off flag alexaExposeSegments as-is,
but update the comment for ESPALEXA_MAXDEVICES to clarify it represents a shared
20-device budget across the main device, presets and segment devices (not a hard
per-category limit such as "up to 10 segment devices"); change the similar
comment near the other occurrence (the block referencing ESPALEXA_MAXDEVICES
around the preset/segment handling) to the same budget-based wording and do not
alter the numeric value of ESPALEXA_MAXDEVICES.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01eff2b0-ad31-4f86-900f-31c241dd70f1
📒 Files selected for processing (6)
wled00/alexa.cppwled00/cfg.cppwled00/data/settings_sync.htmwled00/set.cppwled00/wled.hwled00/xml.cpp
Member
|
@coderabbitai the list of segments is a vector that is changed very frequently, for example when loading a preset, activating an effect or changing effect options (see json.cpp). segment vector items are often re-used, but in principle a segment "left bedside" may not be the same object any more even when the name stays the same - technically we consider them as volatile items, even when they might appear "the same" for users. Is the code in this PR able to handle this? |
softhack007
added
the
AI
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the actual 🏁 Script executed: Length of output: 9741
Here is the specific failure path: How the mapping works
static int mapSegDevToSegIndex(unsigned segDevIdx) {
unsigned activeCount = 0;
for (unsigned i = 0; i < strip.getSegmentsNum(); i++) {
if (!strip.getSegment(i).isActive()) continue; // ← live state, not init-time state
if (activeCount == segDevIdx) return i;
activeCount++;
}
return -1;
}But the Espalexa device for offset Silent wrong-segment scenario
Alexa "Bedside" now silently controls "Ceiling" — no error, no warning, wrong segment. The
|
Member
|
@coderabbitai please create a prioritized list of recommended changes. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🔴 P1 — Architectural Bug: Segment Mapping Breaks on Any Preset Load or Effect Change
The // at alexaInit()
static std::vector<unsigned> alexaSegmentIndices;
alexaSegmentIndices.clear();
for (unsigned i = 0; i < strip.getSegmentsNum(); i++) {
Segment& seg = strip.getSegment(i);
if (!seg.isActive()) continue;
// ... create & add device ...
alexaSegmentIndices.push_back(i); // ← store physical index
}
// at onAlexaChange()
unsigned offset = devId - alexaSegmentDeviceStart;
if (offset >= alexaSegmentIndices.size()) return;
unsigned segIdx = alexaSegmentIndices[offset]; // stable physical index
Segment& seg = strip.getSegment(segIdx);
if (!seg.isActive()) return; // guard for post-discovery deactivation
🔴 P2 — Bug:
|
| # | Severity | File | Issue |
|---|---|---|---|
| P1 | 🔴 Architectural | alexa.cpp |
Live re-count of active segments drifts from discovery-time binding after any preset load |
| P2 | 🔴 Bug | alexa.cpp |
addDevice() return unchecked → memory leak + wrong offset on overflow |
| P3 | 🟡 Logic | alexa.cpp |
alexaSegmentDeviceStart > 0 always true; doesn't detect zero registered segments |
| P4 | 🟡 Quality | alexa.cpp |
CT/color conversion block duplicated between main and segment handlers |
| P5 | 🟡 UX | settings_sync.htm |
No UI warning about re-discovery requirement after segment changes |
| P6 | 🔵 Minor | alexa.cpp |
Segment on always restores to opacity 255 rather than saved level |
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T07:26:09.816Z
Learning: In WLED (wled00/util.cpp), `allocate_buffer()` processes `BFRALLOC_NOBYTEACCESS` in an `if` with an `else` that covers all other flags. On classic ESP32, this means `BFRALLOC_ENFORCE_PSRAM` and `BFRALLOC_PREFER_PSRAM` are silently ignored when `BFRALLOC_NOBYTEACCESS` is also set — IRAM (MALLOC_CAP_32BIT) is always tried first. Combining `BFRALLOC_CLEAR | BFRALLOC_NOBYTEACCESS` is unsafe: the allocator's internal `memset` at util.cpp:901 will perform byte-level access on a potentially IRAM-resident buffer. This affects `Segment::pixels` in `setGeometry()` (FX.h) and `_pixels` in `finalizeInit()` (FX_fcn.cpp). Additionally, `memcpy` on `Segment::pixels` in the copy constructor and copy assignment (FX_fcn.cpp lines ~73 and ~116) is also a byte-level violation when the buffer is in IRAM. The safe pattern is element-wise 32-bit access (for loops over `uint32_t*`) rather than `memset`/`memcpy` for all `BFRALLOC_NOBYTEACCESS` buffers.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T09:14:29.218Z
Learning: In WLED, pixel buffers allocated with `BFRALLOC_NOBYTEACCESS` (both `WS2812FX::_pixels` and `Segment::pixels`) are intentionally placed in 32-bit-only accessible IRAM by design. Buffers up to ~1000 LEDs (~4 kB) easily fit within IRAM's ~50 kB pool. `memset` and `memcpy` on these buffers work correctly in practice because the Xtensa newlib assembly implementations use 32-bit stores for word-aligned, word-multiple-size buffers — satisfying the IRAM hardware constraint as an implementation property of the assembly code, which is unlikely to change across IDF versions. The risk of a future violation exists but is accepted as very low. Do not flag `memset`/`memcpy` usage on `BFRALLOC_NOBYTEACCESS` pixel buffers as a critical bug in WLED reviews; note it as a low-risk accepted pattern.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.
Learnt from: 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: 2026-04-10T16:10:42.419Z
Learning: In WLED (wled/WLED), the segments vector (`strip.segments`) is highly volatile: preset loads, effect changes, and JSON state updates in json.cpp can add/remove/reorder segments and change their active status at any time. Segments have no stable persistent identifier — only their ordinal index in the vector, which also changes. Code that tries to maintain a discovery-time binding from an external protocol (e.g., Espalexa/Alexa) to a specific segment using ordinal-among-active-segments as a key will silently map to wrong segments after any preset load. A correct implementation must store the init-time segment-index-to-device mapping explicitly and guard against the segment at that index changing identity after a vector rebuild.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:13:21.670Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), `seg.freeze` means "do not run the effect function (_mode[seg.mode]())" — it does NOT mean "skip show()". A frozen segment's pixel buffer can still be updated externally (e.g., realtime control or single-pixel JSON API). `strip.trigger()` is the primary mechanism to flush those externally written pixels to the LEDs on the next service tick. Therefore, frozen segments must remain part of the `doShow`/`show()` path, and it is architecturally wrong to exclude frozen segments from `doShow`. Do not suggest skipping frozen segments from the show path in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/json.cpp:310-310
Timestamp: 2026-03-21T18:12:09.437Z
Learning: In WLED's `deserializeSegment()` (wled00/json.cpp), the blend mode field `seg.blendMode` is intentionally written without a post-read clamp (`getVal(elem["bm"], seg.blendMode)`). Out-of-range or unsupported blend mode values are handled safely in `WS2812FX::blendSegment()` (wled00/FX_fcn.cpp), which defaults to mode 0 for any unsupported value via a bounds check against the `BLENDMODES` constant. Do not flag the missing clamp in deserializeSegment as a bug in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-25T12:25:11.610Z
Learning: In WLED's `Segment::startTransition()` (wled00/FX_fcn.cpp:284), when `isInTransition()` is true and `_t->_oldSegment` already exists, the function silently returns without updating `_t->_start` or `_t->_bri`. This causes a bug where rapid successive on/off toggles during a non-FADE blending transition (e.g., fairy dust) leave the transition clock stale: by the time of the second re-trigger, elapsed time may already exceed `_dur`, so `updateTransitionProgress()` sets `_progress = 0xFFFF` immediately on the next service tick and `stopTransition()` fires — the blending effect never plays. The fix is to always reset `_t->_start = millis()`, `_t->_dur = dur`, and `_t->_bri = currentBri()` (current visible brightness) in the `isInTransition()` branch, regardless of whether `_oldSegment` exists.
Learnt from: 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: 2026-04-10T11:41:46.726Z
Learning: In the WLED repository (wled/WLED), trailing-whitespace removals or other pure-whitespace "optimizations" must NOT be intermixed with real code/logic changes in the same PR. Reviewers prefer whitespace cleanup to be submitted in a dedicated, separate PR. Reason: intermixing makes diffs harder to read (obscures actual logic changes) and causes trouble when cherry-picking commits. Always flag PRs that mix whitespace-only edits with functional changes and ask the contributor to separate them.
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: 5048
File: wled00/set.cpp:551-555
Timestamp: 2026-03-27T21:00:25.902Z
Learning: In WLED PR `#5048` (wled00/set.cpp lines 551-555), the CONFIG_IDF_TARGET_ESP32C5 block that unconditionally forces ntpEnabled = false is an intentional **temporary hotfix** by softhack007 for a known ESP32-C5 crash: `assert failed: udp_new_ip_type udp.c:1278 (Required to lock TCPIP core functionality!)`. Do not flag this as a permanent design issue; the TODO comment in the code already notes it should be resolved properly once the underlying IDF/TCPIP bug on C5 is fixed. A future permanent solution should use a target capability flag rather than overwriting the user's setting.
Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:31:01.023Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.
Learnt from: softhack007
Repo: wled/WLED PR: 5480
File: docs/cpp.instructions.md:511-511
Timestamp: 2026-04-08T19:10:48.754Z
Learning: In WLED documentation and review guidance (`docs/cpp.instructions.md`), using `WLED_MAX_SEGNAME_LEN = 64` as the example value in LittleFS filename-length checks is intentional and correct. The reasoning: ESP32 uses 64-char segment names (the larger default), and if a constructed path (e.g. `/ledmap_` + segment name + `.json`) stays within 255 bytes when the name is 64 chars, it is trivially safe on ESP8266 where segment names are limited to 32 chars. WLED core has no ESP8266-only features, so validating against the larger ESP32 default is the right cross-platform conservative choice. Do NOT flag `WLED_MAX_SEGNAME_LEN = 64` in this guidance as misleading or platform-specific.
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: softhack007
Repo: wled/WLED PR: 4838
File: platformio.ini:149-150
Timestamp: 2026-03-29T16:47:56.452Z
Learning: In WLED PR `#4838` (ESP-IDF V5 branch, platformio.ini): The original FastLED library has been replaced with a custom `fastled-slim` fork. As a result, the `[v5_pioarduino_workaround]` section (containing `lib_archive = yes`) and the commented-out `post:pio-scripts/fastled_cxx_workaround.py` extra script are intentional dead code pending removal after back-to-back testing. Do NOT flag `lib_archive = yes` in `[v5_pioarduino_workaround]` as unreferenced or suggest moving it to active env blocks — the entire section is slated for removal.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:08.847Z
Learning: In WLED's `respondModeData()` (wled00/json.cpp), the 256-character `lineBuffer` limit for effect descriptor strings (getModeData) is an intentional constraint that matches the same 256-char limit used ~6 other places in the codebase. It is not new to this PR and should not be flagged as a regression. Lifting it requires a future refactor (better type system or dynamic buffer).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
WLED's Alexa/Hue emulation only exposes a single global device, so multi-segment setups (e.g. two strips on one controller) can't be controlled independently via Alexa without preset workarounds.
This adds an opt-in setting to expose each active segment as a separate Alexa-discoverable device with independent on/off, brightness (via segment opacity), and color control.
Changes
wled00/alexa.cpp— ExtendedalexaInit()to register segment devices after main+preset devices. ExtendedonAlexaChange()with per-segment dispatch for on/off (SEG_OPTION_ON), brightness (setOpacity), and color/CT. AddedmapSegDevToSegIndex()helper with bounds checking for segments deactivated after discovery.wled00/wled.h— AddedalexaExposeSegmentsglobal (defaultfalse). RaisedESPALEXA_MAXDEVICESfrom 10 to 20.wled00/data/settings_sync.htm— Checkbox in Alexa settings section.wled00/set.cpp/wled00/xml.cpp/wled00/cfg.cpp— Plumb the new setting through form handling, JS serialization, and JSON config (interfaces.va.seg).Device index layout
Segments use their configured name if set, otherwise
"Segment <index>". Presets and segments can coexist. Segment changes after discovery require Alexa re-discovery (same limitation as presets).Summary by CodeRabbit