V0.15.3 no v #5284
V0.15.3 no v #5284
Conversation
WalkthroughAdds a new ESP32 high‑priority RMT driver and ISR shim, embedded build metadata and OTA validation, large bus/segment/buffer refactors (vectorized buses, parallel I2S), renames primary color buffer to colPri, file backup/bootloop utilities, PlatformIO/CI and web UI version/reporting changes. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
usermods/usermod_rotary_brightness_color/usermod_rotary_brightness_color.h (1)
98-111: Fix hue wraparound logic: use 256 instead of 255 for proper modulo arithmetic.The variable renaming to
colPriis correct, but the hue wraparound calculations contain a logic error. In FastLED, hue values range from 0–255 (8-bit representation). When the hue exceeds or falls below this range, the code must use modulo 256 arithmetic, not 255.Incorrect:
if (new_val > 255) new_val -= 255; if (new_val < 0) new_val += 255;Correct:
if (new_val > 255) new_val -= 256; if (new_val < 0) new_val += 256;For example, hue 255 + fadeAmount 1 = 256, which should wrap to hue 0, not hue 1. This applies to both clockwise (lines 103–104) and counter-clockwise (lines 127–128) rotation branches.
wled00/FX.h (1)
1-21: Redundant include guards and potential circular dependency.The file now has both
#pragma once(line 1) and the traditional#ifndef WS2812FX_hguard (lines 16-17). While harmless, one should be removed for consistency. More importantly, adding#include "wled.h"at line 20 could create circular include issues since wled.h includes FX.h at line 189.🐛 Suggested fix
Remove the duplicate include guard and verify the wled.h include is necessary:
#pragma once ... -#ifndef WS2812FX_h -#define WS2812FX_h - #include <vector> -#include "wled.h" +// Forward declarations or minimal includes instead of full wled.h #include "const.h" ... -#endifIf wled.h is needed, ensure proper include guards prevent recursion.
wled00/FX.cpp (1)
7287-7333:mode_waterfall: new buffer shift order duplicates the newest pixel (logic bug)The updated code sets
pixels[k]to the new color, then shifts:unsigned k = SEGLEN - 1; pixels[k] = newColor; SEGMENT.setPixelColor(k, pixels[k]); for (unsigned i = 0; i < k; i++) { pixels[i] = pixels[i+1]; SEGMENT.setPixelColor(i, pixels[i]); }Because the shift reads from
pixels[i+1]afterpixels[k]was written, the new color ends up at both indiceskandk-1in the buffer, which is not a pure scroll and differs from the typical “shift then insert” pattern used elsewhere (e.g.,mode_matripix). This will subtly distort the waterfall trail.A safer order is to shift first, then write the new sample into the tail:
🔧 Proposed fix for `mode_waterfall` shift logic
- unsigned k = SEGLEN-1; - if (samplePeak) { - pixels[k] = (uint32_t)CRGB(CHSV(92,92,92)); - } else { - pixels[k] = color_blend(SEGCOLOR(1), SEGMENT.color_from_palette(pixCol+SEGMENT.intensity, false, PALETTE_SOLID_WRAP, 0), (uint8_t)my_magnitude); - } - SEGMENT.setPixelColor(k, pixels[k]); - // loop will not execute if SEGLEN equals 1 - for (unsigned i = 0; i < k; i++) { - pixels[i] = pixels[i+1]; // shift left - SEGMENT.setPixelColor(i, pixels[i]); - } + unsigned k = SEGLEN - 1; + // loop will not execute if SEGLEN equals 1 + for (unsigned i = 0; i < k; i++) { + pixels[i] = pixels[i + 1]; // shift left + SEGMENT.setPixelColor(i, pixels[i]); + } + if (samplePeak) { + pixels[k] = (uint32_t)CRGB(CHSV(92, 92, 92)); + } else { + pixels[k] = color_blend( + SEGCOLOR(1), + SEGMENT.color_from_palette(pixCol + SEGMENT.intensity, false, PALETTE_SOLID_WRAP, 0), + (uint8_t)my_magnitude + ); + } + SEGMENT.setPixelColor(k, pixels[k]);This keeps the buffer and the visible LEDs in sync and preserves the intended scrolling behavior.
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Line 29: The workflow references the deprecated runner action "uses:
softprops/action-gh-release@v1" which relies on an old Node runtime; update that
step to a maintained release action (for example
"softprops/action-gh-release@v2" or another maintained alternative like
"ncipollo/release-action") and adjust any inputs per the new action's docs;
after swapping the "uses: softprops/action-gh-release@v1" line to the chosen
supported tag, run a workflow test to confirm there are no breaking input/name
changes.
In @lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:
- Around line 173-180: The construct() method currently calls malloc for
_dataEditing and _dataSending without checking for failure; add null checks
after each allocation (for _dataEditing and _dataSending using _sizeData) and
handle allocation failures by either freeing the other buffer if allocated and
setting a clear error state/return value or asserting/logging and aborting
initialization so no further code dereferences nullptr; ensure the chosen error
handling is consistent with the class initialization pattern and prevents use of
these buffers if allocation failed.
In @pio-scripts/build_ui.py:
- Around line 6-9: The current failure path uses env.Execute("null") which is
invalid on Windows; update the block that checks node_ex (the Node.js detection
branch) to perform a proper Python exit: import sys if not already present, then
call sys.exit(1) (or exit(1)) instead of env.Execute("null"); remove the
env.Execute("null") call and ensure the printed error message for node_ex is
preserved before exiting so the script fails gracefully cross-platform.
In @wled00/bus_manager.cpp:
- Around line 925-926: There is a missing semicolon after the increment
expression "u++" which causes a syntax error; locate the increment statement
using the symbol "u" (the line containing "u++") inside the relevant loop or
function in bus_manager.cpp and terminate the statement by adding a semicolon so
the increment becomes "u++;". Ensure the surrounding statement/line ends with
the semicolon and that no other tokens are accidentally merged.
- Line 23: Remove the non-portable internal header inclusion "#include
<bits/unique_ptr.h>" from bus_manager.cpp; since the code currently comments out
std::unique_ptr usage, simply delete that include and do not replace it with an
internal header—if you later need std::unique_ptr or other smart pointers,
include the standard header "<memory>" and reference std::unique_ptr explicitly
(check the commented-out unique_ptr usage to decide if adding <memory> is
required).
In @wled00/bus_manager.h:
- Around line 10-11: The header wled00/bus_manager.h currently includes <memory>
but none of its facilities are used (smart pointer code is commented out);
remove the unused include directive "#include <memory>" from
wled00/bus_manager.h so only necessary headers (like <vector>) remain to reduce
unnecessary dependencies.
In @wled00/cfg.cpp:
- Around line 198-201: The busConfigs vector is not cleared before repopulating
in deserializeConfig, causing duplicate entries on repeated calls; update the
parsing start (before the bus loop where int s = 0 is set) to call
busConfigs.clear() so the vector is reset, and keep the existing safeguard that
if fromFS is true you call BusManager::removeAll() rather than manipulating
busses directly in the network callback; ensure this change aligns with
finalizeInit() / beginStrip() semantics so initialization still finalizes once
later.
In @wled00/data/settings_leds.htm:
- Line 616: The code uses an undefined variable oMaxB in the loop calling
addLEDs inside loadCfg; replace oMaxB with the correct global maxB so the loop
reads for (var i=0; i<maxB+maxV; i++) addLEDs(-1) (i.e., update the reference to
maxB wherever oMaxB appears in the loadCfg block) to prevent the ReferenceError.
In @wled00/fcn_declare.h:
- Around line 121-124: The inline wrappers readObjectFromFileUsingId and
readObjectFromFile declare a filter parameter but don't forward it to the
underlying C-string overloads; update these wrappers to pass the filter through
(i.e., call readObjectFromFileUsingId(file.c_str(), id, dest, filter) and
readObjectFromFile(file.c_str(), key, dest, filter)) so the optional filtering
actually takes effect, or if filtering is not supported remove the filter
parameter from the wrapper signatures to avoid an unused parameter.
- Around line 413-421: Remove the duplicate function declarations: keep a single
declaration for handleBootLoop() and a single conditional declaration for
bootloopCheckOTA() (inside the #ifndef ESP8266 block) and delete the repeated
block; ensure only one occurrence of each symbol (handleBootLoop and
bootloopCheckOTA) remains in fcn_declare.h to avoid redundancy.
In @wled00/FX_fcn.cpp:
- Around line 608-620: Segment::setName currently mixes malloc/realloc with
other paths that use new[]/delete[] and writes to name without checking
allocation success; change allocation to use new char[newLen+1] (allocate a new
buffer), copy the string (respecting newLen), delete[] the old name pointer only
after successful allocation (or delete[] before creating the new buffer if you
prefer RAII order), and ensure you check the new pointer for nullptr before
writing name[newLen] = 0; if allocation fails or newLen is zero call clearName()
and return. Ensure references: Segment::setName, clearName, and the member name
are updated consistently.
- Around line 1202-1234: The loop currently aborts creating further buses when
the running mem + bus.memUsage(...) exceeds MAX_LED_MEMORY; instead, clamp each
bus's LED count down so the bus can still be created in a degraded state: when
mem + bus.memUsage(fullCount) > MAX_LED_MEMORY, compute the largest reducedCount
such that mem + bus.memUsage(reducedCount) <= MAX_LED_MEMORY (binary search or
decrement loop), construct a temporary BusConfig with that reduced count and
call BusManager::add() with it, update mem with the actual memUsage used, and
continue to the next bus rather than skipping all remaining ones; apply the same
clamping logic to the other default bus-creation loop that also uses mem and
MAX_LED_MEMORY.
In @wled00/ota_update.cpp:
- Around line 5-7: The include of esp_spi_flash.h is unused and breaks builds on
ESP-IDF 5.x; remove it or guard it with an IDF-version macro. Replace the bare
#include <esp_spi_flash.h> with either no include at all, or wrap it in a
preprocessor check such as only including esp_spi_flash.h when ESP-IDF major
version is less than 5 (e.g. use ESP_IDF_VERSION_MAJOR), keeping the existing
includes of esp_ota_ops.h and mbedtls/sha256.h intact so version-guarded calls
to esp_flash_read()/spi_flash_read() (and any references to esp_flash_read or
spi_flash_read) continue to compile on both older and newer IDF versions.
In @wled00/pin_manager.cpp:
- Around line 217-231: The issue is inconsistent indentation in the
chip-specific pin handling block around the checks using ESP.getChipModel(),
strncmp_P, and the psramFound() check; reformat the entire if/else block (the
two strncmp_P branches, the else, and the subsequent gpio==16/17 checks that
reference ESP32-U4WDH, ESP32-PICO(-D2/-D4), and psramFound()) to use 2-space
indentation per project guidelines, preserving existing logic, comments, and
line breaks while adjusting leading spaces for all lines in that block.
In @wled00/wled_metadata.cpp:
- Around line 56-63: The struct initializer for WLED_BUILD_DESCRIPTION (type
wled_metadata_t, placed in BUILD_METADATA_SECTION) uses 4-space indentation;
reformat the initializer lines so each nested entry is indented with 2 spaces
per coding guidelines (affect the lines containing WLED_CUSTOM_DESC_MAGIC,
WLED_CUSTOM_DESC_VERSION, TOSTRING(WLED_VERSION), WLED_RELEASE_NAME, the
std::integral_constant<uint32_t, djb2_hash_constexpr(WLED_RELEASE_NAME)>::value
entry, and the { 0, 0, 0 } array) so the block aligns with the rest of the C++
files in the wled00 directory.
🧹 Nitpick comments (24)
usermods/BME280_v2/usermod_bme280.h (1)
447-447: Good defensive initialization; consider F() macro for consistency.This initialization prevents
tempScalefrom being uninitialized ifaddToJsonInfo()or_mqttInitialize()is called before the first sensor reading, which is a solid improvement.♻️ Optional: Use F() macro for consistency
For consistency with lines 84 and 100 in
UpdateBME280Data, and to store the string literals in flash memory rather than RAM:- tempScale = UseCelsius ? "°C" : "°F"; + tempScale = UseCelsius ? F("°C") : F("°F");wled00/data/settings_leds.htm (1)
753-780: Logic is correct; consider adding a clarifying comment for the PWM disable selector.The
updateTypeDropdownsfunction correctly enforces bus limits. ThemaxDBcalculation on line 773 correctly accounts for parallel vs mono I2S modes across ESP32 variants.The PWM type disable logic on line 778 is clever but may be hard to follow. It constructs a selector like
option[data-type^="AAAA"]to disable any PWM type that needs more pins than are available.💡 Consider adding a brief inline comment
// Disable PWM types that need more pins than available (accounting for current type's pins if PWM) - disable(`option[data-type^="${'A'.repeat(maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1)}"]`); + // PWM types have data-type="A", "AA", "AAA", etc. matching pin count; disable types exceeding available pins + let availPins = maxA - analogB + (isPWM(curType) ? numPins(curType) : 0); + disable(`option[data-type^="${'A'.repeat(availPins + 1)}"]`);.github/workflows/release.yml (1)
21-27: Consider parameterizing hardcoded release tags.The
sinceTag(v0.15.0) andreleaseBranch(0_15_x) values are hardcoded, requiring manual updates for each release. Consider using workflow inputs, environment variables, or deriving these values dynamically from the tag being released.♻️ Suggested approach using dynamic tag derivation
- sinceTag: v0.15.0 - releaseBranch: 0_15_x + sinceTag: ${{ github.event.repository.default_branch }} + releaseBranch: ${{ github.ref_name }}Alternatively, use workflow_dispatch with input parameters for more control.
pio-scripts/output_bins.py (1)
26-27: Add error handling for package.json access.The script reads from
package.jsonwithout error handling. If the file is missing, malformed, or lacks aversionkey, the build will fail with an unclear error message.♻️ Proposed fix with error handling
- with open("package.json", "r") as package: - version = json.load(package)["version"] + try: + with open("package.json", "r") as package: + package_data = json.load(package) + version = package_data["version"] + except FileNotFoundError: + print("Error: package.json not found") + env.Exit(1) + except KeyError: + print("Error: 'version' key not found in package.json") + env.Exit(1) + except json.JSONDecodeError as e: + print(f"Error: Invalid JSON in package.json: {e}") + env.Exit(1)wled00/button.cpp (1)
378-378: Use non-blocking approach for relay control to improve loop responsiveness.The 50ms
delay()blocks the event loop during relay switching. While this occurs only during LED power-on (infrequent), consider implementing a non-blocking state machine to avoid unnecessary blocking even in this infrequent scenario.wled00/data/settings_sec.htm (1)
71-76: Redundant version display.The version is displayed twice in the About section: once on line 71 within the GitHub link text and again on line 76 as "Installed version". Consider whether both are necessary or if one should be removed for cleaner UI.
pio-scripts/build_ui.py (1)
19-21: Inconsistent indentation.Lines 20-21 use 6 spaces instead of the 4-space indentation pattern used elsewhere in this file.
♻️ Proposed fix
# If it failed, abort the build if (exitCode): - print('\x1b[0;31;43m' + 'npm run build fails check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m') - exit(exitCode) + print('\x1b[0;31;43m' + 'npm run build fails check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m') + exit(exitCode)wled00/remote.cpp (1)
217-235: Missing braces around if-block body.The
if(ESPNowButton >= 0)block contains multiple statements but lacks enclosing braces after the innerif (!remoteJson(ESPNowButton)). While syntactically valid, this reduces readability and increases risk of future maintenance errors.♻️ Proposed fix for clarity
void handleRemote() { - if(ESPNowButton >= 0) { - if (!remoteJson(ESPNowButton)) + if (ESPNowButton >= 0) { + if (!remoteJson(ESPNowButton)) { switch (ESPNowButton) { case WIZMOTE_BUTTON_ON : setOn(); break; // ... other cases ... default: break; } + } } ESPNowButton = -1; }wled00/fcn_declare.h (1)
1-3: Redundant include guard.The
#pragma onceon line 1 makes the#ifndef WLED_FCN_DECLARE_H/#define/#endifguards redundant. While harmless, consider keeping just one style for consistency. Most modern compilers support#pragma once, making it the cleaner choice.platformio_override.sample.ini (1)
529-531: Consider updating TFT_eSPI to version 2.5.43 if the ESP32 default platform is upgraded to espressif32 @ ~6.3.2.TFT_eSPI 2.5.33 is pinned for compatibility with espressif32@3.5.0. However, the latest version 2.5.43 (released March 6, 2025) is compatible with espressif32 @ ~6.3.2. Other ESP32 environments in this file (
esp32_eth,lolin_s2_mini,esp32c3dev,esp32s3dev_16MB_opi) already use the newer platform version (6.3.2), so updating the default[esp32]platform would allow this environment to also use the latest TFT_eSPI release.wled00/wled_metadata.h (1)
1-62: Well-structured metadata header for OTA validation.The packed struct design is appropriate for binary embedding and the API is clean. Minor typo on line 36: "metdata" should be "metadata".
📝 Fix typo
-// Convenient metdata pointers +// Convenient metadata pointerslib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S (2)
8-15: Redundant guard check.Line 15 checks
#ifndef CONFIG_BTDM_CTRL_HLIagain after line 8 already includes!defined(CONFIG_BTDM_CTRL_HLI)in the outer guard. This is harmless but redundant.
103-107: FIXME note for 32-register CPU configuration.The FIXME on line 105 notes this code assumes 64 physical registers. If ESP32 variants with 32 registers are ever targeted, this would need adjustment. Consider creating an issue to track this if it's a real concern.
Would you like me to open an issue to track the 32-register CPU configuration concern?
wled00/wled_metadata.cpp (1)
88-92: Byte-level magic number check optimization.Line 89 checks only the first byte of the magic number before doing the full
memcpy. This is a valid optimization -static_cast<char>(WLED_CUSTOM_DESC_MAGIC)yields0x53('S'), the low byte of0x57535453. Consider adding a brief comment explaining this optimization.📝 Suggested comment
for (size_t offset = 0; offset <= dataSize - sizeof(wled_metadata_t); offset++) { + // Quick check of first byte before expensive memcpy if ((binaryData[offset]) == static_cast<char>(WLED_CUSTOM_DESC_MAGIC)) {wled00/const.h (1)
487-493: Inconsistent macro usage for ESP32S2 detection.Line 487 uses
ARDUINO_ARCH_ESP32S2while line 476 usesCONFIG_IDF_TARGET_ESP32S2. Both work but for consistency within this file, consider using the same macro throughout.📝 Suggested fix for consistency
- #if defined(ARDUINO_ARCH_ESP32S2) + #if defined(CONFIG_IDF_TARGET_ESP32S2) #define MAX_LED_MEMORY 16000 - #elif defined(ARDUINO_ARCH_ESP32C3) + #elif defined(CONFIG_IDF_TARGET_ESP32C3) #define MAX_LED_MEMORY 32000wled00/ota_update.h (1)
3-10:#pragma onceshould appear before includes.The
#pragma oncedirective at line 10 is placed after the#includestatements, which is non-standard. The include guard should be at the very beginning of the header to prevent redundant parsing. Additionally, line 7 uses 3-space indentation instead of the 2-space convention per coding guidelines.♻️ Proposed fix
-// WLED OTA update interface +#pragma once +// WLED OTA update interface -#include <Arduino.h> #ifdef ESP8266 #include <Updater.h> #else - #include <Update.h> + #include <Update.h> #endif - -#pragma oncelib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp (1)
388-396: Remove commented-out debug code.Line 390 contains commented-out debug code that should be cleaned up before merge.
♻️ Proposed fix
// Use the platform code to allocate the interrupt // If we need the additional assembly bridge, we pass it as the "arg" to the IDF so it gets linked in err = esp_intr_alloc(ETS_RMT_INTR_SOURCE, INT_LEVEL_FLAG | ESP_INTR_FLAG_IRAM, HI_IRQ_HANDLER, (void*) HI_IRQ_HANDLER_ARG, &isrHandle); - //err = ESP_ERR_NOT_FINISHED; #elsewled00/util.cpp (1)
630-636: ESP8266 RTC memory offset should be documented.The comment explains the offset of 32, but the magic number could benefit from a named constant. This is a minor readability suggestion.
♻️ Proposed improvement
+// Offset into RTC user memory - first 128 bytes may be used by OTA system +// Ref: https://github.com/esp8266/Arduino/blob/... +constexpr uint32_t RTC_BOOTLOOP_OFFSET = 32; + // Place variables in RTC memory via references... -static volatile uint32_t& bl_last_boottime = *(RTC_USER_MEM + 32); -static volatile uint32_t& bl_crashcounter = *(RTC_USER_MEM + 33); -static volatile uint32_t& bl_actiontracker = *(RTC_USER_MEM + 34); +static volatile uint32_t& bl_last_boottime = *(RTC_USER_MEM + RTC_BOOTLOOP_OFFSET); +static volatile uint32_t& bl_crashcounter = *(RTC_USER_MEM + RTC_BOOTLOOP_OFFSET + 1); +static volatile uint32_t& bl_actiontracker = *(RTC_USER_MEM + RTC_BOOTLOOP_OFFSET + 2);wled00/ota_update.cpp (1)
209-248: Consider edge case: chunk spans metadata but is smaller than METADATA_SEARCH_RANGE.The metadata validation logic buffers data when
availableDataAfterOffset < METADATA_SEARCH_RANGE. If the firmware is smaller than expected or transmission is interrupted, the buffer could grow unbounded untilisFinal. Consider adding a maximum buffer size check.♻️ Suggested guard
} else { // Store the data we just got for next pass + if (context->releaseMetadataBuffer.size() > 4096) { + context->errorMessage = F("Metadata buffer overflow"); + return; + } context->releaseMetadataBuffer.insert(context->releaseMetadataBuffer.end(), data, data+len); }wled00/data/index.js (2)
3302-3358: Consider sanitizing version strings before injecting into HTML.The
oldVersionvalue originates fromversion-info.jsonon the device filesystem. While the attack surface is limited (requires authenticated upload access), usinginnerHTMLwith these values is a minor XSS risk. Consider usingtextContentfor the version values or a simple sanitization.♻️ Suggested safer approach
- const description = isInstall - ? `You are now running WLED <strong style="text-wrap: nowrap">${newVersion}</strong>.` - : `Your WLED has been upgraded from <strong style="text-wrap: nowrap">${oldVersion}</strong> to <strong style="text-wrap: nowrap">${newVersion}</strong>.`; + // Sanitize version strings to prevent potential XSS + const escapeHtml = (str) => str.replace(/[&<>"']/g, c => ({'&':'&','<':'<','>':'>','"':'"',"'":'''}[c])); + const safeOld = oldVersion ? escapeHtml(oldVersion) : ''; + const safeNew = escapeHtml(newVersion); + + const description = isInstall + ? `You are now running WLED <strong style="text-wrap: nowrap">${safeNew}</strong>.` + : `Your WLED has been upgraded from <strong style="text-wrap: nowrap">${safeOld}</strong> to <strong style="text-wrap: nowrap">${safeNew}</strong>.`;
3389-3396: Consider adding a timeout for the external API call.The POST to
usage.wled.mehas no timeout. If the server is slow or unreachable, the user sees "Reporting upgrade..." with no feedback until the browser's default timeout (which could be 30+ seconds).♻️ Suggested timeout implementation
+ // Abort if request takes too long + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); + // Make AJAX call to postUpgradeEvent API return fetch('https://usage.wled.me/api/usage/upgrade', { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(upgradeData) + body: JSON.stringify(upgradeData), + signal: controller.signal + }).finally(() => { + clearTimeout(timeoutId); });lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (1)
389-402: Inconsistent typedef ordering for Channel 3 inverted methods.The Ws2805 and Ws2816 typedefs are declared in a different order compared to other channels. For channels 0, 1, 2, 4, 5, 6, 7, the order is Ws2812x → Ws2816 → Ws2805. However, for channel 3, the order is Ws2812x → Ws2805 → Ws2816.
This inconsistency doesn't affect functionality but could cause confusion during maintenance.
Suggested reordering
typedef NeoEsp32RmtHIMethodBase<NeoEsp32RmtInvertedSpeedWs2811, NeoEsp32RmtChannel3> NeoEsp32RmtHI3Ws2811InvertedMethod; typedef NeoEsp32RmtHIMethodBase<NeoEsp32RmtInvertedSpeedWs2812x, NeoEsp32RmtChannel3> NeoEsp32RmtHI3Ws2812xInvertedMethod; -typedef NeoEsp32RmtHIMethodBase<NeoEsp32RmtInvertedSpeedWs2805, NeoEsp32RmtChannel3> NeoEsp32RmtHI3Ws2805InvertedMethod; typedef NeoEsp32RmtHIMethodBase<NeoEsp32RmtInvertedSpeedWs2812x, NeoEsp32RmtChannel3> NeoEsp32RmtHI3Ws2816InvertedMethod; +typedef NeoEsp32RmtHIMethodBase<NeoEsp32RmtInvertedSpeedWs2805, NeoEsp32RmtChannel3> NeoEsp32RmtHI3Ws2805InvertedMethod; typedef NeoEsp32RmtHIMethodBase<NeoEsp32RmtInvertedSpeedSk6812, NeoEsp32RmtChannel3> NeoEsp32RmtHI3Sk6812InvertedMethod;wled00/FX.cpp (1)
601-631:dissolve()SEGENV pixel buffer + write‑back loop looks correctThe new
uint32_tbuffer inSEGENV.datasized assizeof(uint32_t) * SEGLENand indexed only from0to< SEGLENavoids repeatedgetPixelColorcalls and makes the finalforloop that pushespixels[i]intoSEGMENTstraightforward and safe. Allocation failure falls back tomode_static(), which matches other effects. This aligns with the per‑effect buffer pattern used elsewhere in the file.wled00/bus_wrapper.h (1)
1215-1311: getDataSize/memUsage heuristics need confirmation against NeoPixelBus internals
getDataSize()andmemUsage()introduce detailed per-bus memory estimates:
- For ESP8266 DMA drivers, using
PixelsSize() * 5(front + DMA overhead) and scaling for 4/5-channel and 16-bit formats.- For ESP32 RMT drivers, using
PixelsSize() * 2to reflect front/back buffering.- For ESP32 I2S/parallel I2S drivers, using
PixelsSize() * 4to account for front + DMA with 4× cadence, adjusted for channel count and bit depth.- For SPI-like methods, doubling
PixelsSize()to include front/back buffers.These formulas look directionally correct and match the comments, but they depend heavily on NeoPixelBus’s current buffer layout and could drift if the library changes.
I recommend double-checking these multipliers against the actual
PixelsSize()and driver implementations (especially for DMA and I2S/parallel I2S) so thatbus.memUsage()andBusManager::memUsage()remain accurate and we don’t under- or over-enforceMAX_LED_MEMORY.Also applies to: 1313-1379
| sinceTag: v0.15.0 | ||
| releaseBranch: 0_15_x | ||
| - name: Create draft release | ||
| uses: softprops/action-gh-release@v1 |
There was a problem hiding this comment.
Update GitHub Action to address runner deprecation.
The softprops/action-gh-release@v1 action uses a deprecated Node.js runtime.
🔧 Proposed fix
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@v2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: softprops/action-gh-release@v1 | |
| uses: softprops/action-gh-release@v2 |
🧰 Tools
🪛 actionlint (1.7.10)
29-29: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In @.github/workflows/release.yml at line 29, The workflow references the
deprecated runner action "uses: softprops/action-gh-release@v1" which relies on
an old Node runtime; update that step to a maintained release action (for
example "softprops/action-gh-release@v2" or another maintained alternative like
"ncipollo/release-action") and adjust any inputs per the new action's docs;
after swapping the "uses: softprops/action-gh-release@v1" line to the chosen
supported tag, run a workflow test to confirm there are no breaking input/name
changes.
| void construct() | ||
| { | ||
| _dataEditing = static_cast<uint8_t*>(malloc(_sizeData)); | ||
| // data cleared later in Begin() | ||
|
|
||
| _dataSending = static_cast<uint8_t*>(malloc(_sizeData)); | ||
| // no need to initialize it, it gets overwritten on every send | ||
| } |
There was a problem hiding this comment.
Missing null-check after malloc allocations.
If malloc fails and returns nullptr, subsequent operations on _dataEditing or _dataSending will cause undefined behavior. Consider adding error handling or at least an assertion.
Suggested fix
void construct()
{
_dataEditing = static_cast<uint8_t*>(malloc(_sizeData));
+ if (!_dataEditing) return; // allocation failed
// data cleared later in Begin()
_dataSending = static_cast<uint8_t*>(malloc(_sizeData));
+ if (!_dataSending) {
+ free(_dataEditing);
+ _dataEditing = nullptr;
+ return;
+ }
// no need to initialize it, it gets overwritten on every send
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h around lines 173 - 180, The
construct() method currently calls malloc for _dataEditing and _dataSending
without checking for failure; add null checks after each allocation (for
_dataEditing and _dataSending using _sizeData) and handle allocation failures by
either freeing the other buffer if allocated and setting a clear error
state/return value or asserting/logging and aborting initialization so no
further code dereferences nullptr; ensure the chosen error handling is
consistent with the class initialization pattern and prevents use of these
buffers if allocation failed.
| esp_err_t NeoEsp32RmtHiMethodDriver::WaitForTxDone(rmt_channel_t channel, TickType_t wait_time) { | ||
| if ((channel >= RMT_CHANNEL_MAX) || !driverState || !driverState[channel]) return ESP_ERR_INVALID_ARG; | ||
|
|
||
| NeoEsp32RmtHIChannelState& state = *driverState[channel]; | ||
| // yield-wait until wait_time | ||
| esp_err_t rv = ESP_OK; | ||
| uint32_t status; | ||
| while(1) { | ||
| status = rmt_ll_tx_get_channel_status(&RMT, channel); | ||
| if (!_RmtStatusIsTransmitting(channel, status)) break; | ||
| if (wait_time == 0) { rv = ESP_ERR_TIMEOUT; break; }; | ||
|
|
||
| TickType_t sleep = std::min(wait_time, (TickType_t) 5); | ||
| vTaskDelay(sleep); | ||
| wait_time -= sleep; | ||
| }; | ||
|
|
||
| return rv; | ||
| } |
There was a problem hiding this comment.
Potential timeout underflow in WaitForTxDone.
If wait_time is less than sleep (5 ticks), the subtraction at line 501 could underflow since wait_time is TickType_t (unsigned). This would cause a very long wait instead of timing out.
🐛 Proposed fix
while(1) {
status = rmt_ll_tx_get_channel_status(&RMT, channel);
if (!_RmtStatusIsTransmitting(channel, status)) break;
if (wait_time == 0) { rv = ESP_ERR_TIMEOUT; break; };
TickType_t sleep = std::min(wait_time, (TickType_t) 5);
vTaskDelay(sleep);
- wait_time -= sleep;
+ if (wait_time >= sleep) wait_time -= sleep;
+ else wait_time = 0;
};| if node_ex is None: | ||
| print('\x1b[0;31;43m' + 'Node.js is not installed or missing from PATH html css js will not be processed check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m') | ||
| exitCode = env.Execute("null") | ||
| exit(exitCode) |
There was a problem hiding this comment.
Cross-platform issue with env.Execute("null").
The command "null" is not a valid command on Windows systems, where it would be "cmd /c exit 1" or similar. This will cause the build to fail with a confusing error on Windows instead of the intended graceful failure.
Consider using Python's sys.exit(1) directly instead:
🐛 Proposed fix
+import sys
+
node_ex = shutil.which("node")
# Check if Node.js is installed and present in PATH if it failed, abort the build
if node_ex is None:
print('\x1b[0;31;43m' + 'Node.js is not installed or missing from PATH html css js will not be processed check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m')
- exitCode = env.Execute("null")
- exit(exitCode)
+ sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if node_ex is None: | |
| print('\x1b[0;31;43m' + 'Node.js is not installed or missing from PATH html css js will not be processed check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m') | |
| exitCode = env.Execute("null") | |
| exit(exitCode) | |
| if node_ex is None: | |
| print('\x1b[0;31;43m' + 'Node.js is not installed or missing from PATH html css js will not be processed check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m') | |
| sys.exit(1) |
🧰 Tools
🪛 Ruff (0.14.10)
8-8: Undefined name env
(F821)
🤖 Prompt for AI Agents
In @pio-scripts/build_ui.py around lines 6 - 9, The current failure path uses
env.Execute("null") which is invalid on Windows; update the block that checks
node_ex (the Node.js detection branch) to perform a proper Python exit: import
sys if not already present, then call sys.exit(1) (or exit(1)) instead of
env.Execute("null"); remove the env.Execute("null") call and ensure the printed
error message for node_ex is preserved before exiting so the script fails
gracefully cross-platform.
| #include "bus_wrapper.h" | ||
| #include "bus_manager.h" | ||
| #include "bus_wrapper.h" | ||
| #include <bits/unique_ptr.h> |
There was a problem hiding this comment.
Non-portable internal header included.
<bits/unique_ptr.h> is an internal GCC/libstdc++ implementation header. This will fail to compile on other toolchains (e.g., LLVM/libc++, MSVC). Since std::unique_ptr is commented out anyway, this include should be removed. If needed in the future, use the standard <memory> header instead.
Suggested fix
#include "bus_manager.h"
#include "bus_wrapper.h"
-#include <bits/unique_ptr.h>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <bits/unique_ptr.h> | |
| #include "bus_manager.h" | |
| #include "bus_wrapper.h" |
🤖 Prompt for AI Agents
In @wled00/bus_manager.cpp at line 23, Remove the non-portable internal header
inclusion "#include <bits/unique_ptr.h>" from bus_manager.cpp; since the code
currently comments out std::unique_ptr usage, simply delete that include and do
not replace it with an internal header—if you later need std::unique_ptr or
other smart pointers, include the standard header "<memory>" and reference
std::unique_ptr explicitly (check the commented-out unique_ptr usage to decide
if adding <memory> is required).
| Segment &Segment::setName(const char *newName) { | ||
| if (newName) { | ||
| const int newLen = min(strlen(newName), (size_t)WLED_MAX_SEGNAME_LEN); | ||
| if (newLen) { | ||
| if (name) name = static_cast<char*>(realloc(name, newLen+1)); | ||
| else name = static_cast<char*>(malloc(newLen+1)); | ||
| if (name) strlcpy(name, newName, newLen+1); | ||
| name[newLen] = 0; | ||
| return *this; | ||
| } | ||
| } | ||
| return clearName(); | ||
| } |
There was a problem hiding this comment.
Segment::setName uses malloc/realloc with delete[] and can dereference null
Segment::setName() currently:
- Allocates with
malloc/realloc, but other code paths (copy assignment, JSON name handling, clearName) usenew[]/delete[]forname, which is undefined behavior when combined. - Writes
name[newLen] = 0;unconditionally, which will dereference a null pointer ifmalloc/reallocfails.
This combination is a correctness and stability bug and should be fixed before release.
Suggested fix: use new[] consistently and guard allocation
-Segment &Segment::setName(const char *newName) {
- if (newName) {
- const int newLen = min(strlen(newName), (size_t)WLED_MAX_SEGNAME_LEN);
- if (newLen) {
- if (name) name = static_cast<char*>(realloc(name, newLen+1));
- else name = static_cast<char*>(malloc(newLen+1));
- if (name) strlcpy(name, newName, newLen+1);
- name[newLen] = 0;
- return *this;
- }
- }
- return clearName();
-}
+Segment &Segment::setName(const char *newName) {
+ if (newName) {
+ size_t newLen = strnlen(newName, WLED_MAX_SEGNAME_LEN + 1);
+ if (newLen > WLED_MAX_SEGNAME_LEN) newLen = WLED_MAX_SEGNAME_LEN;
+ if (newLen) {
+ char *buf = new (std::nothrow) char[newLen + 1];
+ if (!buf) {
+ // Allocation failed, keep existing name unchanged
+ return *this;
+ }
+ memcpy(buf, newName, newLen);
+ buf[newLen] = '\0';
+ if (name) delete[] name;
+ name = buf;
+ return *this;
+ }
+ }
+ return clearName();
+}This keeps allocation/deallocation consistent with the rest of Segment and avoids null dereference on failure.
🤖 Prompt for AI Agents
In @wled00/FX_fcn.cpp around lines 608 - 620, Segment::setName currently mixes
malloc/realloc with other paths that use new[]/delete[] and writes to name
without checking allocation success; change allocation to use new char[newLen+1]
(allocate a new buffer), copy the string (respecting newLen), delete[] the old
name pointer only after successful allocation (or delete[] before creating the
new buffer if you prefer RAII order), and ensure you check the new pointer for
nullptr before writing name[newLen] = 0; if allocation fails or newLen is zero
call clearName() and return. Ensure references: Segment::setName, clearName, and
the member name are updated consistently.
| unsigned digitalCount = 0; | ||
| #if defined(ARDUINO_ARCH_ESP32) && !defined(CONFIG_IDF_TARGET_ESP32C3) | ||
| // determine if it is sensible to use parallel I2S outputs on ESP32 (i.e. more than 5 outputs = 1 I2S + 4 RMT) | ||
| unsigned maxLedsOnBus = 0; | ||
| unsigned busType = 0; | ||
| for (const auto &bus : busConfigs) { | ||
| if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type)) { | ||
| digitalCount++; | ||
| if (busType == 0) busType = bus.type; // remember first bus type | ||
| if (busType != bus.type) { | ||
| DEBUG_PRINTF_P(PSTR("Mixed digital bus types detected! Forcing single I2S output.\n")); | ||
| useParallelI2S = false; // mixed bus types, no parallel I2S | ||
| } | ||
| if (bus.count > maxLedsOnBus) maxLedsOnBus = bus.count; | ||
| } | ||
| } | ||
| DEBUG_PRINTF_P(PSTR("Maximum LEDs on a bus: %u\nDigital buses: %u\n"), maxLedsOnBus, digitalCount); | ||
| // we may remove 600 LEDs per bus limit when NeoPixelBus is updated beyond 2.8.3 | ||
| if (digitalCount > 1 && maxLedsOnBus <= 600 && useParallelI2S) BusManager::useParallelOutput(); // must call before creating buses | ||
| else useParallelI2S = false; // enforce single I2S | ||
| digitalCount = 0; | ||
| #endif | ||
|
|
||
| // create buses/outputs | ||
| unsigned mem = 0; | ||
| for (const auto &bus : busConfigs) { | ||
| mem += bus.memUsage(Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type) ? digitalCount++ : 0); // includes global buffer | ||
| if (mem <= MAX_LED_MEMORY) { | ||
| if (BusManager::add(bus) == -1) break; | ||
| } else DEBUG_PRINTF_P(PSTR("Out of LED memory! Bus %d (%d) #%u not created."), (int)bus.type, (int)bus.count, digitalCount); | ||
| } | ||
| busConfigs.clear(); | ||
| busConfigs.shrink_to_fit(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the structure of the code around lines 1202-1234 and 1317-1321
cd wled00 && sed -n '1200,1240p' FX_fcn.cppRepository: wled/WLED
Length of output: 1986
🏁 Script executed:
# Let's also check what memUsage() does and how it's defined
rg -A 10 "memUsage\(" . -t cpp | head -50Repository: wled/WLED
Length of output: 3236
🏁 Script executed:
# Check if there are any comments or discussions about bus memory management
rg -B 3 -A 3 "MAX_LED_MEMORY" wled00/FX_fcn.cppRepository: wled/WLED
Length of output: 446
🏁 Script executed:
# Check the Bus class to understand memUsage implementation
fd -e h -e hpp | xargs rg -l "class Bus" | head -5Repository: wled/WLED
Length of output: 73
🏁 Script executed:
# Check the BusManager::add() implementation to see if it does any clamping
rg -A 30 "BusManager::add" wled00/bus_manager.cpp | head -60Repository: wled/WLED
Length of output: 1512
🏁 Script executed:
# Look at lines 1317-1321 mentioned as also applying
sed -n '1310,1325p' wled00/FX_fcn.cppRepository: wled/WLED
Length of output: 898
🏁 Script executed:
# Check git history for recent changes to this memory gating logic
git log --oneline -n 20 -- wled00/FX_fcn.cppRepository: wled/WLED
Length of output: 68
🏁 Script executed:
# Check if there are any comments or issues about memory handling
rg "degraded\|fallback\|reduce.*memory\|MAX_LED_MEMORY" wled00/ -t cpp | grep -i "memory\|bus\|led"Repository: wled/WLED
Length of output: 35
🏁 Script executed:
# Check BusDigital constructor to see if it clamps counts
rg -A 20 "BusDigital::" wled00/bus_manager.cpp | head -80Repository: wled/WLED
Length of output: 3385
🏁 Script executed:
# Look for any count validation or clamping in the Bus class
rg -B 3 -A 10 "class BusDigital" wled00/bus_manager.hRepository: wled/WLED
Length of output: 484
🏁 Script executed:
# Check if there's any count reduction logic in initializers
rg "count.*=" wled00/bus_manager.cpp | grep -i "clamp\|min\|max\|reduce"Repository: wled/WLED
Length of output: 209
🏁 Script executed:
# Search for how the prior "degraded" behavior was implemented - look for skipAmount usage
rg -B 5 -A 5 "skipAmount" wled00/bus_manager.cppRepository: wled/WLED
Length of output: 966
Hard-gating on MAX_LED_MEMORY may regress from intentional fallback behavior
The current code skips entire buses when accumulated mem exceeds MAX_LED_MEMORY rather than creating them with reduced LED counts. Prior design (per versioning notes) intentionally allowed memory overruns with reduced counts to maintain basic LED output after migrations, avoiding "no LEDs at all" failures.
The constructor and BusManager::add() do not clamp or reduce bus counts—buses are created exactly as configured or skipped entirely. The skipAmount parameter affects only memory estimation in memUsage(), not actual bus creation.
If the fallback behavior is still intended, this loop needs to adjust bus counts downward when memory constraints are hit, rather than refusing to create subsequent buses. Confirm whether hard gating is the new intended design across all platforms, or if degraded-but-functional fallback should be restored.
This also applies to the default bus creation loop at lines 1317–1321.
🤖 Prompt for AI Agents
In @wled00/FX_fcn.cpp around lines 1202 - 1234, The loop currently aborts
creating further buses when the running mem + bus.memUsage(...) exceeds
MAX_LED_MEMORY; instead, clamp each bus's LED count down so the bus can still be
created in a degraded state: when mem + bus.memUsage(fullCount) >
MAX_LED_MEMORY, compute the largest reducedCount such that mem +
bus.memUsage(reducedCount) <= MAX_LED_MEMORY (binary search or decrement loop),
construct a temporary BusConfig with that reduced count and call
BusManager::add() with it, update mem with the actual memUsage used, and
continue to the next bus rather than skipping all remaining ones; apply the same
clamping logic to the other default bus-creation loop that also uses mem and
MAX_LED_MEMORY.
| #include <esp_ota_ops.h> | ||
| #include <esp_spi_flash.h> | ||
| #include <mbedtls/sha256.h> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -20 wled00/ota_update.cpp | cat -nRepository: wled/WLED
Length of output: 1082
🌐 Web query:
ESP-IDF 5.0 esp_spi_flash.h deprecated esp_flash.h replacement
💡 Result:
Short answer: esp_spi_flash.h was removed in ESP‑IDF 5.x — use the esp_flash API headers instead (esp_flash.h for the esp_flash_* APIs) and spi_flash_mmap.h for the memory‑mapping functionality. [1][2]
Sources:
- ESP‑IDF migration notes (removed esp_spi_flash.h; use spi_flash_mmap.h). [1]
- SPI flash API docs showing esp_flash.h (esp_flash_* APIs). [2]
🏁 Script executed:
rg -n "esp_spi_flash" wled00/ota_update.cppRepository: wled/WLED
Length of output: 81
🏁 Script executed:
sed -n '290,300p' wled00/ota_update.cpp | cat -nRepository: wled/WLED
Length of output: 704
🏁 Script executed:
# Check for any version guards or conditional includes in the file
grep -n "ifdef.*IDF\|ifdef.*VERSION\|if.*ESP_IDF" wled00/ota_update.cpp | head -20Repository: wled/WLED
Length of output: 108
🏁 Script executed:
# Check for any SPI_FLASH constants or types that might come from esp_spi_flash.h
grep -n "SPI_FLASH\|spi_flash_mmap" wled00/ota_update.cppRepository: wled/WLED
Length of output: 35
🏁 Script executed:
# Let's see the entire includes section and any usage of what might be defined there
sed -n '1,100p' wled00/ota_update.cpp | cat -nRepository: wled/WLED
Length of output: 4622
🌐 Web query:
ESP-IDF esp_spi_flash.h what was it used for API reference
💡 Result:
Short answer: esp_spi_flash.h was the ESP-IDF SPI flash (raw flash) API header used to initialize and perform low-level operations on the on‑chip SPI flash: init, read, write, erase (sectors/regions/chip), get chip size, and memory‑mapping. It was the API surface in older IDF releases (v3.x) and later evolved into the esp_flash_* / esp_partition_* APIs; current ESP-IDF docs recommend using esp_partition_* for partition-aware access and esp_flash_* for newer raw‑chip access. [1][2][3]
Typical functions exposed (legacy / v3.x esp_spi_flash.h):
- spi_flash_init(), spi_flash_get_chip_size()
- spi_flash_read(), spi_flash_write()
- spi_flash_erase_sector(), spi_flash_erase_region(), spi_flash_erase_chip()
- memory mapping and partition helpers (later moved/expanded into esp_flash.h and esp_partition.h). [1]
Where to read more / current equivalents:
- Legacy SPI flash API (esp_spi_flash.h) — ESP-IDF v3.x docs. [1]
- Current SPI flash API (esp_flash_, esp_partition_) — ESP-IDF stable docs. [2][3]
References:
[1] ESP-IDF v3.x SPI Flash API (spi_flash/include/esp_spi_flash.h).
[2] ESP-IDF stable (esp_flash / spi_flash component) — API reference.
[3] ESP-IDF v4.x spi_flash / partition documentation (notes about migration and recommended esp_partition_*).
Remove or conditionally guard the unused include of esp_spi_flash.h for ESP-IDF 5.x compatibility.
esp_spi_flash.h was removed (not deprecated) in ESP-IDF 5.x and will cause compilation failures on that version. The include on line 6 appears to be unused—no functions from it are called in this file. The actual flash read operations already use version-guarded calls to esp_flash_read() (IDF 4.4.0+) and spi_flash_read() (older IDF), which come from different headers. Either remove the unused include or conditionally guard it based on ESP_IDF_VERSION.
🤖 Prompt for AI Agents
In @wled00/ota_update.cpp around lines 5 - 7, The include of esp_spi_flash.h is
unused and breaks builds on ESP-IDF 5.x; remove it or guard it with an
IDF-version macro. Replace the bare #include <esp_spi_flash.h> with either no
include at all, or wrap it in a preprocessor check such as only including
esp_spi_flash.h when ESP-IDF major version is less than 5 (e.g. use
ESP_IDF_VERSION_MAJOR), keeping the existing includes of esp_ota_ops.h and
mbedtls/sha256.h intact so version-guarded calls to
esp_flash_read()/spi_flash_read() (and any references to esp_flash_read or
spi_flash_read) continue to compile on both older and newer IDF versions.
|
|
||
| if ((strncmp_P(PSTR("ESP32-U4WDH"), ESP.getChipModel(), 11) == 0) || // this is the correct identifier, but.... | ||
| (strncmp_P(PSTR("ESP32-PICO-D2"), ESP.getChipModel(), 13) == 0)) { // https://github.com/espressif/arduino-esp32/issues/10683 | ||
| // this chip has 4 MB of internal Flash and different packaging, so available pins are different! | ||
| if (((gpio > 5) && (gpio < 9)) || (gpio == 11)) | ||
| return false; | ||
| } else { | ||
| // for classic ESP32 (non-mini) modules, these are the SPI flash pins | ||
| if (gpio > 5 && gpio < 12) return false; //SPI flash pins | ||
| } | ||
|
|
||
| if (((strncmp_P(PSTR("ESP32-PICO"), ESP.getChipModel(), 10) == 0) || | ||
| (strncmp_P(PSTR("ESP32-U4WDH"), ESP.getChipModel(), 11) == 0)) | ||
| && (gpio == 16 || gpio == 17)) return false; // PICO-D4/U4WDH: gpio16+17 are in use for onboard SPI FLASH | ||
| if (gpio == 16 || gpio == 17) return !psramFound(); //PSRAM pins on ESP32 (these are IO) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
sed -n '217,231p' wled00/pin_manager.cpp | cat -nRepository: wled/WLED
Length of output: 1109
🏁 Script executed:
# Check the full context and indentation around lines 217-231
sed -n '210,235p' wled00/pin_manager.cpp | cat -nRepository: wled/WLED
Length of output: 1696
🏁 Script executed:
# Search for all getChipModel usages in the codebase
rg "getChipModel" --type cpp -B2 -A2Repository: wled/WLED
Length of output: 2458
🏁 Script executed:
# Check what chip models are mentioned in comments or strings
rg "ESP32.*PICO|ESP32.*U4WDH" --type cppRepository: wled/WLED
Length of output: 666
🏁 Script executed:
# Let me verify the exact spacing by checking the raw bytes
sed -n '218p' wled00/pin_manager.cpp | od -c | head -20Repository: wled/WLED
Length of output: 93
🏁 Script executed:
# Check line 218 character by character to confirm indentation
sed -n '218p' wled00/pin_manager.cpp | sed 's/^/[START]/; s/$/[END]/'Repository: wled/WLED
Length of output: 183
🏁 Script executed:
# Get the complete context with all 4 lines to verify indentation consistency
sed -n '218,231p' wled00/pin_manager.cpp | sed 's/^/|/; s/ /··/g'Repository: wled/WLED
Length of output: 1017
Fix indentation: lines 218–232 use 4-space indentation but should use 2-space per coding guidelines.
The chip-specific pin handling logic is correct:
- ESP32-U4WDH and ESP32-PICO-D2: Blocks pins 6-8 and 11 (internal flash layout)
- ESP32-PICO-D4/U4WDH: Blocks gpio16/17 (onboard SPI flash)
- Classic ESP32: Blocks pins 6-11 (SPI flash)
The chip model strings match the expected Arduino-ESP32 identifiers, and the comment referencing issue #10683 appropriately documents the PICO-D2 workaround.
However, all lines in this block use 4-space indentation instead of the required 2-space indentation specified in the coding guidelines for wled00/**/*.cpp files.
🤖 Prompt for AI Agents
In @wled00/pin_manager.cpp around lines 217 - 231, The issue is inconsistent
indentation in the chip-specific pin handling block around the checks using
ESP.getChipModel(), strncmp_P, and the psramFound() check; reformat the entire
if/else block (the two strncmp_P branches, the else, and the subsequent
gpio==16/17 checks that reference ESP32-U4WDH, ESP32-PICO(-D2/-D4), and
psramFound()) to use 2-space indentation per project guidelines, preserving
existing logic, comments, and line breaks while adjusting leading spaces for all
lines in that block.
| const wled_metadata_t __attribute__((section(BUILD_METADATA_SECTION))) WLED_BUILD_DESCRIPTION = { | ||
| WLED_CUSTOM_DESC_MAGIC, // magic | ||
| WLED_CUSTOM_DESC_VERSION, // version | ||
| TOSTRING(WLED_VERSION), | ||
| WLED_RELEASE_NAME, // release_name | ||
| std::integral_constant<uint32_t, djb2_hash_constexpr(WLED_RELEASE_NAME)>::value, // hash - computed at compile time; integral_constant enforces this | ||
| { 0, 0, 0 }, // All other platforms can update safely | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for BUILD_METADATA_SECTION definition
rg -n "BUILD_METADATA_SECTION" wled00/ --type cpp --type hRepository: wled/WLED
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Check the actual indentation and context in wled_metadata.cpp around lines 56-63
sed -n '50,70p' wled00/wled_metadata.cpp | cat -ARepository: wled/WLED
Length of output: 985
🏁 Script executed:
#!/bin/bash
# Search for where BUILD_METADATA_SECTION might be defined in config or headers
rg -n "define.*BUILD_METADATA_SECTION" --type cpp --type hRepository: wled/WLED
Length of output: 196
Fix indentation to match coding guidelines.
The BUILD_METADATA_SECTION macro is properly defined in wled00/ota_update.h, and the std::integral_constant technique for enforcing compile-time hash computation is sound. However, the struct initialization uses 4-space indentation instead of the required 2-space indentation for C++ files in wled00/. Lines 57-62 need to be adjusted to use 2 spaces per indentation level.
🤖 Prompt for AI Agents
In @wled00/wled_metadata.cpp around lines 56 - 63, The struct initializer for
WLED_BUILD_DESCRIPTION (type wled_metadata_t, placed in BUILD_METADATA_SECTION)
uses 4-space indentation; reformat the initializer lines so each nested entry is
indented with 2 spaces per coding guidelines (affect the lines containing
WLED_CUSTOM_DESC_MAGIC, WLED_CUSTOM_DESC_VERSION, TOSTRING(WLED_VERSION),
WLED_RELEASE_NAME, the std::integral_constant<uint32_t,
djb2_hash_constexpr(WLED_RELEASE_NAME)>::value entry, and the { 0, 0, 0 } array)
so the block aligns with the rest of the C++ files in the wled00 directory.
Comment out unused code related to DynamicBuffer.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/build-esp32.yml (2)
1-4: Hardcoded version in workflow name.The workflow name includes "v0.15.3-no-v" which will need updating for future releases. Consider using dynamic version extraction if this workflow is intended to be reusable, or add a comment clarifying this is a branch-specific build workflow.
18-19: Consider caching PlatformIO dependencies for faster builds.Adding cache steps for PlatformIO packages and pip dependencies would speed up subsequent workflow runs.
⚡ Proposed caching improvement
Add after the Python setup step:
- name: Cache PlatformIO uses: actions/cache@v4 with: path: | ~/.platformio ~/.cache/pip key: ${{ runner.os }}-pio-${{ hashFiles('**/platformio.ini') }} restore-keys: | ${{ runner.os }}-pio-
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-esp32.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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-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: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-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: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 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: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED noise function aliases (`inoise8`, `inoise16`, etc.) are not used. These should be replaced with the proper WLED functions (`perlin8`, `perlin16`, etc.) as the `inoise*` functions are only aliases kept for compatibility reasons.
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: willmmiles
Repo: wled/WLED PR: 4998
File: wled00/ota_update.cpp:120-124
Timestamp: 2025-10-11T17:56:24.923Z
Learning: In the WLED OTA update implementation (wled00/ota_update.cpp), the parameter name for bypassing OTA validation is "skipValidation", not "ignoreRelease". The intent evolved from the original spec in issue #4929 to disable all validation checks, not just release name checks, which is why the broader parameter name "skipValidation" is used.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:58.047Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
.github/workflows/build-esp32.yml
Outdated
| - name: Build firmware (esp32) | ||
| run: pio run -e esp32 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Extract all ESP32 environments from platformio.ini
rg -n '^\[env:esp32' platformio.ini | head -30Repository: wled/WLED
Length of output: 312
🏁 Script executed:
#!/bin/bash
# Show full build-esp32.yml workflow
cat .github/workflows/build-esp32.ymlRepository: wled/WLED
Length of output: 504
🏁 Script executed:
#!/bin/bash
# Check if there are other build workflows
ls -la .github/workflows/ | grep buildRepository: wled/WLED
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Search for [env:esp32] without suffixes in platformio.ini
rg -n '^\[env:esp32\]' platformio.iniRepository: wled/WLED
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Show the build.yml workflow
cat .github/workflows/build.ymlRepository: wled/WLED
Length of output: 2302
🏁 Script executed:
#!/bin/bash
# Check the beginning of platformio.ini for base configurations
head -100 platformio.ini | grep -A 5 '\[env:esp32\]'Repository: wled/WLED
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Search for esp32 anywhere in platformio.ini to understand the structure
rg 'esp32' platformio.ini | head -20Repository: wled/WLED
Length of output: 1902
🏁 Script executed:
#!/bin/bash
# Check the platformio.ini structure around environments
sed -n '420,450p' platformio.iniRepository: wled/WLED
Length of output: 1396
Fix invalid build environment name and use dynamic environment matrix instead.
The build command references pio run -e esp32, but esp32 is not a buildable environment—it's a shared configuration section in platformio.ini. The actual buildable ESP32 environments are: esp32dev, esp32dev_V4, esp32_eth, esp32_wrover, esp32c3dev, esp32s3dev_16MB_opi, esp32s3dev_8MB_opi, and esp32s3_4M_qspi.
Additionally, the existing build.yml workflow already implements the correct approach: it dynamically discovers all default environments, builds them in a matrix strategy, and uploads artifacts per environment. Consider using that workflow instead of this manual approach, or align build-esp32.yml with the same pattern.
Updated the workflow to build the ESP32dev version and added caching for PlatformIO. Included steps to show firmware info and upload the firmware artifact.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build-esp32.yml (1)
38-42: Consider parameterizing the artifact name.The artifact name "WLED_0.15.3_ESP32dev_no_v" is hardcoded with version-specific information. This relates to the same concern as the workflow name. If this workflow is intended for reuse across versions, consider using workflow inputs or GitHub context variables to make the artifact name dynamic.
Example using workflow inputs
+on: + workflow_dispatch: + inputs: + version: + description: 'Version string for artifact naming' + required: false + default: 'latest' + jobs: build: runs-on: ubuntu-latest steps: # ... existing steps ... - name: Upload firmware artifact uses: actions/upload-artifact@v4 with: - name: WLED_0.15.3_ESP32dev_no_v + name: WLED_${{ github.event.inputs.version }}_ESP32dev path: .pio/build/esp32dev/firmware.bin
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-esp32.yml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-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: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-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: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 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: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED noise function aliases (`inoise8`, `inoise16`, etc.) are not used. These should be replaced with the proper WLED functions (`perlin8`, `perlin16`, etc.) as the `inoise*` functions are only aliases kept for compatibility reasons.
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: willmmiles
Repo: wled/WLED PR: 4998
File: wled00/ota_update.cpp:120-124
Timestamp: 2025-10-11T17:56:24.923Z
Learning: In the WLED OTA update implementation (wled00/ota_update.cpp), the parameter name for bypassing OTA validation is "skipValidation", not "ignoreRelease". The intent evolved from the original spec in issue #4929 to disable all validation checks, not just release name checks, which is why the broader parameter name "skipValidation" is used.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:58.047Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Build at least one hardware environment locally (e.g., `pio run -e esp32dev`) before marking work complete
Applied to files:
.github/workflows/build-esp32.yml
📚 Learning: 2025-11-22T20:33:14.840Z
Learnt from: softhack007
Repo: wled/WLED PR: 5107
File: boards/adafruit_matrixportal_esp32s3.json:1-66
Timestamp: 2025-11-22T20:33:14.840Z
Learning: In WLED, board JSON files in the boards/ directory may reference partition files that don't exist or have slightly different names than actual files in tools/. This is intentional because platformio.ini or platformio_override.sample.ini explicitly override the partition file using the board_build.partitions setting, which takes precedence over the board JSON partition reference. This pattern exists in multiple local board definitions and should not be flagged as an issue.
Applied to files:
.github/workflows/build-esp32.yml
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
.github/workflows/build-esp32.yml
🔇 Additional comments (1)
.github/workflows/build-esp32.yml (1)
1-1: Clarify intent: version-specific workflow name in main branch.The workflow name includes the hardcoded version string "v0.15.3-no-v". Since this PR targets the
mainbranch, having version-specific naming could lead to confusion and maintenance issues. Consider either:
- Using a generic name like "Build WLED ESP32dev (Manual)" if this is intended as a reusable workflow
- Parameterizing the version using workflow inputs or environment variables
- Confirming if this workflow should only exist on the release branch rather than main
Could you clarify whether this is intended to be a version-specific workflow that lives in main, or if the naming should be made generic?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/build-esp32.yml:
- Line 37: Replace the Russian inline comment "# Обычно итоговый файл здесь:"
with an equivalent English comment such as "# Usually the final file is here:"
to match the repository's English-language comments and maintain consistency.
🧹 Nitpick comments (2)
.github/workflows/build-esp32.yml (2)
39-39: Consider parameterizing the hardcoded version string.The version "0.15.3" is hardcoded in the output filename. While this may be intentional for a branch-specific workflow, consider using a workflow variable or environment variable for easier maintenance if this workflow is reused or updated.
♻️ Example parameterization approach
Add an environment variable at the workflow level:
env: WLED_VERSION: "0.15.3"Then reference it in the filename:
- cp ".pio/build/esp32dev/firmware.bin" "out/WLED_0.15.3_ESP32dev_no_v.bin" + cp ".pio/build/esp32dev/firmware.bin" "out/WLED_${WLED_VERSION}_ESP32dev_no_v.bin"And update the artifact name similarly at line 51.
46-46: Persist the SHA256 checksum for artifact validation.The SHA256 checksum is computed and logged but not saved to a file. Consider writing it to a checksum file and including it in the artifact upload for integrity verification.
💾 Proposed enhancement
- sha256sum out/*.bin + sha256sum out/*.bin | tee out/checksums.txtThen update the artifact upload to include the checksum file:
path: out/*.bin + # or: path: out/*
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-esp32.yml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-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: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-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: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 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: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED noise function aliases (`inoise8`, `inoise16`, etc.) are not used. These should be replaced with the proper WLED functions (`perlin8`, `perlin16`, etc.) as the `inoise*` functions are only aliases kept for compatibility reasons.
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: willmmiles
Repo: wled/WLED PR: 4998
File: wled00/ota_update.cpp:120-124
Timestamp: 2025-10-11T17:56:24.923Z
Learning: In the WLED OTA update implementation (wled00/ota_update.cpp), the parameter name for bypassing OTA validation is "skipValidation", not "ignoreRelease". The intent evolved from the original spec in issue #4929 to disable all validation checks, not just release name checks, which is why the broader parameter name "skipValidation" is used.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:58.047Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T16:42:26.035Z
Learning: In WLED PR #5267, the bus allocation strategy for single I2S mode was changed: ESP32 now uses single I2S on bus 8 (the last bus, digitalCount=9) instead of bus 0; ESP32-S2 uses single I2S on bus 4 (digitalCount=5). Memory calculation logic must account for which specific bus uses I2S in single mode, not just check digitalCount <= 8.
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Build at least one hardware environment locally (e.g., `pio run -e esp32dev`) before marking work complete
Applied to files:
.github/workflows/build-esp32.yml
📚 Learning: 2025-11-22T20:33:14.840Z
Learnt from: softhack007
Repo: wled/WLED PR: 5107
File: boards/adafruit_matrixportal_esp32s3.json:1-66
Timestamp: 2025-11-22T20:33:14.840Z
Learning: In WLED, board JSON files in the boards/ directory may reference partition files that don't exist or have slightly different names than actual files in tools/. This is intentional because platformio.ini or platformio_override.sample.ini explicitly override the partition file using the board_build.partitions setting, which takes precedence over the board JSON partition reference. This pattern exists in multiple local board definitions and should not be flagged as an issue.
Applied to files:
.github/workflows/build-esp32.yml
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
.github/workflows/build-esp32.yml
| run: | | ||
| set -e | ||
| mkdir -p out | ||
| # Обычно итоговый файл здесь: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace Russian comment with English.
The comment "# Обычно итоговый файл здесь:" (Usually the final file is here) should be in English for consistency with the rest of the codebase.
📝 Proposed fix
- # Обычно итоговый файл здесь:
+ # The firmware.bin output is typically found here:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Обычно итоговый файл здесь: | |
| # The firmware.bin output is typically found here: |
🤖 Prompt for AI Agents
In @.github/workflows/build-esp32.yml at line 37, Replace the Russian inline
comment "# Обычно итоговый файл здесь:" with an equivalent English comment such
as "# Usually the final file is here:" to match the repository's
English-language comments and maintain consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/build-esp32.yml (3)
1-1: Clarify if this workflow is temporary or should use dynamic versioning.The workflow name includes a hardcoded version "v0.15.3-no-v". If this workflow is intended to be maintained long-term in the main branch, the version should be extracted to an environment variable or derived dynamically from the codebase (e.g., from
package.json). If this is a temporary workflow for a specific release variant, consider:
- Adding a comment at the top explaining its temporary nature
- Placing it in a separate branch or removing it after the release
The same hardcoded version appears in lines 39 and 51, requiring updates in multiple locations for each version change.
Is this workflow intended to be temporary for the v0.15.3 release variant, or should it be made version-agnostic for long-term maintenance?
33-46: Translate Russian comment to English.Line 37 contains a Russian comment ("Обычно итоговый файл здесь:") which translates to "Usually the final file is here:". For maintainability in the WLED codebase, comments should be in English.
🌐 Proposed fix
- # Обычно итоговый файл здесь: + # The built firmware is typically located here:
53-54: Remove trailing whitespace.The file ends with trailing whitespace. While this doesn't affect functionality, it's good practice to remove it for code hygiene.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-esp32.yml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-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: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-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: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 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: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED noise function aliases (`inoise8`, `inoise16`, etc.) are not used. These should be replaced with the proper WLED functions (`perlin8`, `perlin16`, etc.) as the `inoise*` functions are only aliases kept for compatibility reasons.
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: willmmiles
Repo: wled/WLED PR: 4998
File: wled00/ota_update.cpp:120-124
Timestamp: 2025-10-11T17:56:24.923Z
Learning: In the WLED OTA update implementation (wled00/ota_update.cpp), the parameter name for bypassing OTA validation is "skipValidation", not "ignoreRelease". The intent evolved from the original spec in issue #4929 to disable all validation checks, not just release name checks, which is why the broader parameter name "skipValidation" is used.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:58.047Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T16:42:26.035Z
Learning: In WLED PR #5267, the bus allocation strategy for single I2S mode was changed: ESP32 now uses single I2S on bus 8 (the last bus, digitalCount=9) instead of bus 0; ESP32-S2 uses single I2S on bus 4 (digitalCount=5). Memory calculation logic must account for which specific bus uses I2S in single mode, not just check digitalCount <= 8.
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Build at least one hardware environment locally (e.g., `pio run -e esp32dev`) before marking work complete
Applied to files:
.github/workflows/build-esp32.yml
📚 Learning: 2025-11-22T20:33:14.840Z
Learnt from: softhack007
Repo: wled/WLED PR: 5107
File: boards/adafruit_matrixportal_esp32s3.json:1-66
Timestamp: 2025-11-22T20:33:14.840Z
Learning: In WLED, board JSON files in the boards/ directory may reference partition files that don't exist or have slightly different names than actual files in tools/. This is intentional because platformio.ini or platformio_override.sample.ini explicitly override the partition file using the board_build.partitions setting, which takes precedence over the board JSON partition reference. This pattern exists in multiple local board definitions and should not be flagged as an issue.
Applied to files:
.github/workflows/build-esp32.yml
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
.github/workflows/build-esp32.yml
🔇 Additional comments (1)
.github/workflows/build-esp32.yml (1)
48-52: LGTM with the versioning caveat.The artifact upload configuration is correct. The hardcoded artifact name on line 51 is part of the broader versioning concern already flagged in the workflow name comment.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.