DMX Output - Runtime Config & replace SparkFunDMX with esp_dmx#5287
Closed
DMX Output - Runtime Config & replace SparkFunDMX with esp_dmx#5287
Conversation
Many of these functions are internal, and we can reduce RAM by caching the binary value instead.
Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Add a field to the OTA metadata structure indicating the oldest base version it's safe to install this update /from/. This provides a clear path forward in case there are incompatibilities, eg. some case (bootloader compatibility) where 0.16.0 cannot be installed safely from 0.15.2, but a transitional 0.15.3 can arrange the groundwork.
If we cap the acceptable version, it becomes impossible to increase it: older firmwares will reject it. Instead we must guarantee backwards compatibility so long as the magic number is the same. If a breaking change is required in the future, a new magic number should be used to identify it.
Report the older structure version until sufficient penetration of the previous patch.
Contributor
WalkthroughLarge refactor and feature bundle: adds DMX pin-configurable output and UI, introduces dynarray linker-based usermod registration, rewrites bus/PolyBus allocation and memory accounting, converts many FX/effect functions to void, tightens symbol linkage, replaces numeric JSON buffer lock IDs with named constants, updates many web UI pages and build scripts, and adjusts OTA/bootloader metadata and verification. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
netmindz
commented
Jan 10, 2026
| -D DECODE_LG=true | ||
| -DWLED_USE_MY_CONFIG | ||
| -D WLED_PS_DONT_REPLACE_FX ; PS replacement FX are purely a flash memory saving feature, do not replace classic FX until we run out of flash | ||
| -D WLED_ENABLE_DMX |
Member
Author
There was a problem hiding this comment.
TODO: remove after testing, unless we can safely enable by default now it can be enabled at runtime?
Member
Author
|
I still need to test with actual hardware, but I think this should be pretty close to the required code |
netmindz
changed the title
Clarify language requirement for code comments and documentation.
- Remove unnecessary checks - Remove unnecessary constructions - Fix unsafe buffer-to-String
Allow OTA upgrade between ESP32_V4 and ESP32 release names
…f requested port is 2 on device with only 1
…utput-esp_dmx # Conflicts: # wled00/data/settings_sync.htm # wled00/dmx_output.cpp # wled00/xml.cpp
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/json.cpp (1)
813-817:⚠️ Potential issue | 🟠 MajorKeep
info.archstable; expose chip detail under a new key instead.
info.archused to be a stable family identifier. ReturningESP.getChipModel()only on some ESP32 targets turns it into mixed-format values like"esp32"vs"ESP32-S3", which breaks exact-comparison clients. Preservearchas the family value and publish the detailed chip model separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/json.cpp` around lines 813 - 817, Preserve info.arch as a stable family identifier and add a separate key for the detailed chip model: always set root[F("arch")] to the family string (e.g., "esp32" for ESP32 targets) instead of conditionally using ESP.getChipModel(); then publish the full chip model under a new key such as root[F("chip")] using ESP.getChipModel() (or equivalent) so clients can still do exact comparisons on arch while accessing detailed model via the new key.
♻️ Duplicate comments (3)
wled00/set.cpp (1)
476-478:⚠️ Potential issue | 🟠 MajorChanging the DMX output pin still doesn't take effect at runtime.
This only updates
dmxOutputPin; it doesn't release/reallocate the pin or reinitialize the DMX output driver, so the live session keeps using the old pin until reboot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/set.cpp` around lines 476 - 478, The code only updates dmxOutputPin but does not reinitialize the DMX driver, so change the handler that sets dmxOutputPin (where request->arg(F("IDMO")).toInt() is used) to also reconfigure the DMX output: stop/disable the current DMX instance, release the old pin, and initialize/start the DMX driver on the new pin. Locate the DMX init/teardown logic (e.g. functions used elsewhere like startDmxOutput()/stopDmxOutput(), dmx.begin()/dmx.stop(), or the DMX driver init in setup) and invoke the teardown then startup sequence after assigning dmxOutputPin; if such helper functions don’t exist, add a small reinit routine that calls the driver’s stop and begin/init with the new dmxOutputPin so the change takes effect immediately without reboot.wled00/dmx_output.cpp (2)
95-96:⚠️ Potential issue | 🔴 Critical
uint8_ttruncates half the DMX universe here.
handleDMXOutput()computesDMXAddras anint, butwrite()narrows it touint8_tbefore indexingdmxdata. Addresses above 255 will wrap to the wrong slot, and there is still no bounds guard on the array access.🐛 Proposed fix
-void DMXOutput::write(uint8_t channel, uint8_t value) { - dmxdata[channel] = value; +void DMXOutput::write(uint16_t channel, uint8_t value) { + if (channel == 0 || channel >= DMX_PACKET_SIZE) return; + dmxdata[channel] = value; }Update the declaration in
wled00/dmx_output.hto match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.cpp` around lines 95 - 96, The write method currently narrows the DMX address and can wrap addresses >255 and lacks bounds checking; change DMXOutput::write signature to accept a wider integer type (e.g., int or size_t) to match how DMXAddr is computed in handleDMXOutput(), update the declaration in wled00/dmx_output.h accordingly, and inside DMXOutput::write validate the incoming address against the dmxdata buffer size (use the buffer length or a constant like DMX_UNIVERSE_SIZE) before indexing to avoid out-of-bounds writes.
72-75:⚠️ Potential issue | 🟠 MajorPass a string to
%sin the allocation failure log.
PinManager::getPinOwner()returns the enum, not a C string, so this varargs call is undefined behavior. This PR already addsPinManager::getPinOwnerName(), which fits the log directly.🐛 Proposed fix
if (!pinAllocated) { DEBUG_PRINTF("DMXOutput: Error: Failed to allocate pin for DMX_OUTPUT. Pin already in use:\n"); - DEBUG_PRINTF("In use by: %s\n", PinManager::getPinOwner(outputPin)); + DEBUG_PRINTF("In use by: %s\n", PinManager::getPinOwnerName(outputPin)); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.cpp` around lines 72 - 75, The allocation-failure log is currently passing an enum to a "%s" format which is undefined behavior; update the second DEBUG_PRINTF call to pass the string-returning helper instead by replacing PinManager::getPinOwner(outputPin) with PinManager::getPinOwnerName(outputPin) (keep the "%s" format), or alternatively use the new getPinOwnerName() in any combined message so the log receives a C-string; modify the DEBUG_PRINTF invocation in the DMXOutput allocation failure branch accordingly.
🟡 Minor comments (7)
usermods/pixels_dice_tray/BLE_REQUIREMENT.md-17-20 (1)
17-20:⚠️ Potential issue | 🟡 MinorFix the sample copy command path.
As written, this assumes the current directory already contains
platformio_override.ini.sample, but then climbs three levels for the destination. Fromusermods/pixels_dice_tray/, repo root is only two levels up, so the command overshoots and copies to the wrong place.🛠️ Suggested doc fix
-1. **Copy the sample configuration:** +1. **From the repository root, copy the sample configuration:** ```bash - cp platformio_override.ini.sample ../../../platformio_override.ini + cp usermods/pixels_dice_tray/platformio_override.ini.sample platformio_override.ini ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md` around lines 17 - 20, The cp command in BLE_REQUIREMENT.md incorrectly uses ../../../ which overshoots the repo root; update the sample copy command so it references the actual sample file path and copies it into the intended destination (replace the existing cp line that mentions ../../../ with a command that reads the sample from usermods/pixels_dice_tray/platformio_override.ini.sample and copies it to platformio_override.ini) so readers running the example from the repo root or from within usermods/pixels_dice_tray get the correct file.wled00/data/index.css-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorFix CSS lint error on
url()argument quoting.Line 3 currently violates
function-url-quotes(Stylelint). Please quote theurl(...)argument.Suggested lint-safe change
- src: url(data:font/woff2;charset=utf-8;base64,...); + src: url("data:font/woff2;charset=utf-8;base64,...");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/index.css` at line 3, The src declaration uses an unquoted data URL in the url(...) call which triggers Stylelint's function-url-quotes rule; fix it by updating the src: url(...) token (the data:font/woff2... entry) to use quoted URL syntax (i.e., url("...")) so the entire data URL argument is wrapped in quotes while leaving the rest of the `@font-face` declaration unchanged.wled00/data/settings_sec.htm-69-69 (1)
69-69:⚠️ Potential issue | 🟡 MinorPotential orphaned
</span>tag.Line 69 has a closing
</span>tag, but there's no corresponding opening<span>tag visible in the OTA section. This appears to be a remnant from the old markup structure that should be removed.Proposed fix
<i class="warn">⚠ If you are using multiple VLANs (i.e. IoT or guest network) either set PIN or disable this option.<br> - Disabling this option will make your device less secure.</i><br></span> + Disabling this option will make your device less secure.</i><br>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_sec.htm` at line 69, The closing </span> in the OTA section is orphaned (see the HTML fragment "Disabling this option will make your device less secure.</i><br></span>"); remove the stray </span> so the markup properly closes only the <i> tag and the <br>, or if intended, add the corresponding opening <span> with the needed attributes—prefer removing the stray </span> to restore valid HTML in settings_sec.htm.wled00/data/common.js-140-160 (1)
140-160:⚠️ Potential issue | 🟡 MinorRemove misleading
asyncmarker or implement proper promise completion.The
uploadFile()function is markedasyncbut doesn't return a meaningful promise—it resolves immediately afterreq.send()rather than when the upload completes. While current callers rely on the callback pattern (which still works), this creates a confusing contract for future maintainers and could lead to bugs if someone tries toawaitthe function. Either remove theasynckeyword to preserve the synchronous callback-only semantics, or implement a proper promise that resolves when the upload succeeds or fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/common.js` around lines 140 - 160, uploadFile is marked async but currently returns immediately; convert it into a proper promise-returning async function by returning a new Promise that resolves/rejects when the XMLHttpRequest completes: create and return new Promise((resolve,reject)=>{ set req.addEventListener('load', ...) to call resolve(this.status < 400) (and still call callback if provided), set req.addEventListener('error', ...) to call reject(false) or resolve(false) consistently (and call callback), and remove the implicit immediate completion—ensure fileObj.value = '' runs after resolution/rejection; keep existing await usage (e.g., await file.text()) so uploadFile remains async but now correctly represents upload completion.usermods/user_fx/README.md-269-273 (1)
269-273:⚠️ Potential issue | 🟡 MinorUse
SEGENV.callin this example, notSEGMENT.call.
calllives on the segment environment, so the current wording points readers at a nonexistent API member.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/user_fx/README.md` around lines 269 - 273, The README example incorrectly references SEGMENT.call; update the snippet and text to use SEGENV.call (the call counter lives on the segment environment) so replace any mention of SEGMENT.call with SEGENV.call and ensure the explanatory sentence refers to SEGENV.call incrementing on each function call.wled00/ota_update.cpp-403-442 (1)
403-442:⚠️ Potential issue | 🟡 MinorKeep the bootloader SHA cache invalid on read/parse failures.
calculateBootloaderSHA256()ignores failedesp_flash_read()calls andBootloaderImageSizererrors, but still setsbootloaderSHA256CacheValid = true. That turns a short/corrupt readback into the authoritative hash until reboot, which is misleading for the UI and any code consuming this value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 403 - 442, calculateBootloaderSHA256 currently ignores esp_flash_read failures and BootloaderImageSizer parse errors and always sets bootloaderSHA256CacheValid = true; change it to track any failure (e.g., set a bool readFailed if esp_flash_read(...) != ESP_OK, and a bool parseFailed if the sizer detects a corrupt/impossible size such that totalSize == 0 or totalSize > BOOTLOADER_SIZE or other sizer-specific error), and if either flag is true or totalHashLen == 0 then do not mark the cache valid: clear/zero bootloaderSHA256Cache, ensure mbedtls_sha256_free(&ctx) is called, and leave bootloaderSHA256CacheValid = false (return early). Update calculateBootloaderSHA256 to only set bootloaderSHA256CacheValid = true when no read/parsing failures occurred and totalHashLen > 0.wled00/bus_manager.cpp-229-242 (1)
229-242:⚠️ Potential issue | 🟡 MinorABL reintroduces the CCT-from-RGB / accurate-white bug.
This loop reads back the already auto-white-processed pixel and then calls
calculateCCT(c, ...). When_cct < 0andRGBW_MODE_AUTO_ACCURATEis active, that RGB no longer matches the original color, so WW/CW shifts as soon as ABL repaints the bus. The normalsetPixelColor()path fixed this by preserving the pre-auto-white color; this path needs the same source data or cached WW/CW values.Based on learnings: When "Calculate CCT from RGB" mode (_cct < 0) is combined with "Accurate white" mode (RGBW_MODE_AUTO_ACCURATE),
approximateKelvinFromRGB()receives RGB values that have already been modified byautoWhiteCalc(), so the original color must be preserved.
🧹 Nitpick comments (9)
CONTRIBUTING.md (2)
61-66: Fix blank line inside blockquote.Line 66 creates a blank line inside the caution blockquote, which triggers markdownlint MD028. This can cause inconsistent rendering across markdown parsers.
📝 Proposed formatting fix
> [!CAUTION] > Do not use "force-push" while your PR is open! > It has many subtle and unexpected consequences on our GitHub repository. > For example, we regularly lose review comments when the PR author force-pushes code changes. Our review bot (coderabbit) may become unable to properly track changes, it gets confused or stops responding to questions. > So, pretty please, do not force-push. -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 61 - 66, The blockquote starting with "[!CAUTION]" contains an extra blank line before "Do not use "force-push" while your PR is open!" which triggers markdownlint MD028; remove the empty line so the blockquote content is contiguous (i.e., ensure "[!CAUTION]" is immediately followed by the warning text) to fix the formatting and prevent inconsistent Markdown rendering.
17-21: Minor markdown formatting: Extra spaces in blockquote.The blockquote has multiple spaces after the
>symbol, which triggers markdownlint MD027 warnings. While this may be intentional for visual formatting in GitHub's renderer, standard markdown style uses a single space.📝 Proposed formatting fix
> [!TIP] -> **The easiest way to start your first PR** -> When viewing a file in `wled/WLED`, click on the "pen" icon and start making changes. -> When you choose to 'Commit changes', GitHub will automatically create a PR from your fork. -> +> **The easiest way to start your first PR** +> When viewing a file in `wled/WLED`, click on the "pen" icon and start making changes. +> When you choose to 'Commit changes', GitHub will automatically create a PR from your fork. +>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 17 - 21, The blockquote lines starting with "> " (the line containing "**The easiest way to start your first PR**" and the following lines including the image tag) have extra spaces after the ">" which triggers markdownlint MD027; update those blockquote lines to use a single space after ">" (e.g., change "> **The easiest..." to "> **The easiest..." and similarly remove the extra spaces before the image tag and subsequent lines) so the blockquote follows standard markdown spacing.usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp (1)
132-132: Replace the hardcoded240throttle with a named constant.This literal is meaningful behavior and should be self-describing.
♻️ Suggested cleanup
+static constexpr unsigned long AUTOSAVE_UPDATE_GUARD_MS = 240UL; ... - if (!autoSaveAfterSec || !enabled || (autoSaveIgnorePresets && currentPreset>0) || (strip.isUpdating() && now - lastRun < 240)) return; + if (!autoSaveAfterSec || !enabled || (autoSaveIgnorePresets && currentPreset>0) || (strip.isUpdating() && now - lastRun < AUTOSAVE_UPDATE_GUARD_MS)) return;Based on learnings: In WLED reviews, meaningful magic numbers should be replaced with defined constants to improve maintainability and reduce inconsistencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp` at line 132, Replace the magic number 240 in the autosave throttle check with a named constant (e.g., AUTO_SAVE_UPDATE_THROTTLE_SEC) to make the meaning explicit; declare the constant at the top of usermod_v2_auto_save.cpp (or alongside other constants) and update the condition that uses strip.isUpdating() && now - lastRun < 240 to use the new constant (refer to autoSaveAfterSec, enabled, autoSaveIgnorePresets, currentPreset, strip.isUpdating(), lastRun, now) so the throttle interval is self-documenting and easy to change.wled00/data/icons-ui/style.css (1)
4-6: Verify woff2 format removal is intentional.The
woff2font format was removed from the@font-facesources. Whilewoffandttfprovide broad compatibility,woff2offers better compression. If this was unintentional, consider re-adding it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/icons-ui/style.css` around lines 4 - 6, The `@font-face` src list in style.css (the lines listing url('fonts/wled122.ttf...'), url('fonts/wled122.woff...'), url('fonts/wled122.svg...')) no longer includes a woff2 entry; confirm this removal was intentional and, if not, restore the woff2 source (e.g., add a url('fonts/wled122.woff2?...') format('woff2') entry) within the same `@font-face` src order and ensure the referenced woff2 file exists in the fonts assets and any cache-busting query string matches your build artifacts.usermods/pov_display/pov_display.cpp (1)
38-38: Nit: Redundant return statement.The trailing
return;at the end of a void function is unnecessary since the function will return implicitly.Optional cleanup
s_lastLoadAttemptMs = nowMs; s_pov.loadImage(segName); - return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/pov_display/pov_display.cpp` at line 38, Remove the redundant trailing `return;` from the end of the void function in pov_display.cpp (the final statement shown as `return;` in the diff); simply delete that line so the function returns implicitly rather than explicitly.wled00/button.cpp (1)
379-382: Avoid hard-wiring relay enable tobeginStrip()ordering.The relay-on path now works only because an earlier
handleOnOff(true)already put the pin into output mode. Re-establishing the mode here keeps relay enable self-contained and avoids future breakage if the init order or pin ownership changes.♻️ Suggested tweak
if (rlyPin>=0) { - // note: pinMode is set in first call to handleOnOff(true) in beginStrip() digitalWrite(rlyPin, rlyMde); // set to on state + pinMode(rlyPin, rlyOpenDrain ? OUTPUT_OPEN_DRAIN : OUTPUT); delay(RELAY_DELAY); // let power stabilize before sending LED data (`#346` `#812` `#3581` `#3955`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/button.cpp` around lines 379 - 382, The relay enable assumes pinMode was already set elsewhere; make the relay setup self-contained by calling pinMode(rlyPin, OUTPUT) before digitalWrite(rlyPin, rlyMde) in the block that checks rlyPin>=0 (so the relay is reliably driven even if handleOnOff(true) or beginStrip() order changes), then keep the existing delay(RELAY_DELAY) afterwards; reference rlyPin, rlyMde, RELAY_DELAY, digitalWrite(), and the related beginStrip()/handleOnOff setup when updating the code.wled00/data/settings_pininfo.htm (1)
16-30: Consider polling interval for resource-constrained devices.The 250ms polling interval provides responsive updates but may be aggressive for ESP8266 or devices under load. Since pin state typically doesn't change rapidly, a longer interval (e.g., 500ms-1000ms) could reduce CPU/network overhead while still providing adequate responsiveness.
💡 Optional: Use a longer polling interval
- pinsTimer = setInterval(loadPins, 250); + pinsTimer = setInterval(loadPins, 500);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_pininfo.htm` around lines 16 - 30, The 250ms polling in S() (pinsTimer = setInterval(loadPins, 250)) is too aggressive for resource-constrained devices; change the interval used by setInterval to a longer value (e.g., 500–1000 ms) or make it configurable, and keep the call site referencing pinsTimer and loadPins so the existing start/stop logic remains intact.usermods/TetrisAI_v2/TetrisAI_v2.cpp (1)
8-8: MakenoFlashOnClearfile-local.It is only used inside
usermods/TetrisAI_v2/TetrisAI_v2.cpp, so exporting it as a global symbol adds namespace pollution for no benefit.static(or aTetrisAIUsermodmember) keeps the linkage tight. Based on learnings: when C-style functions or variables are added in WLED and are not declared in a global header, prefer making themstaticunless they are intentionally global.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/TetrisAI_v2/TetrisAI_v2.cpp` at line 8, The global variable noFlashOnClear should be made file-local to avoid exporting an unnecessary symbol: change its declaration in TetrisAI_v2.cpp to have internal linkage (e.g., prepend static: "static bool noFlashOnClear = false;") or move it into the TetrisAIUsermod class as a private member and update all uses accordingly; update any references in the file to use the new member name if moved into the class.usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp (1)
88-296: These effect shims can be file-local.Every
mode_*wrapper in this block is only registered from this translation unit, so exporting all of them as global symbols unnecessarily widens the namespace. Mark themstaticor wrap the block in an anonymous namespace, and normalize the touched block to the repo's 2-space C++ style while you're here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp` around lines 88 - 296, This block of mode_* wrapper functions (e.g., mode_Module_Experiment10, mode_Zoom, mode_RGB_Blobs, mode_Rotating_Blob) are only used in this translation unit and should be file-local to avoid exporting globals; make them static (or enclose the entire block in an anonymous namespace) and reformat the block to the project's 2-space C++ style while preserving the existing calls like anim.initEffect() and anim.<EffectName>().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pio-scripts/load_usermods.py`:
- Around line 57-93: _predict_dep_name currently ignores symlink:// and file://
(and other non-hosted URL schemes) and thus returns None for those entries;
update it so that after the existing hosted-Git branch (the
_URL_SCHEME_RE.match(entry) check) you still parse any scheme with
urlparse(entry) and extract the final path component (Path(parsed.path).name),
stripping trailing '/' and a '.git' suffix and returning None if empty; retain
the special-case branch that handles github.com/gitlab.com/bitbucket.com by
using the second path segment when available, and ensure the SSH branch
(_SSH_URL_RE) and '@' / plain registry branches keep their current behavior
(still strip fragments, versions, and '.git' as currently done).
In `@pio-scripts/validate_modules.py`:
- Around line 42-50: The current subprocess.run call invoking nm (in the block
that assigns result = subprocess.run(...) and nm_output = result.stdout) only
handles exceptions, so if nm exits with a non-zero returncode we may parse empty
stdout and erroneously fail; update the logic to check result.returncode after
the subprocess.run call and treat non-zero as a tool failure: log a warning
including returncode and stderr, then return the conservative pass set (same as
in the exception branch) instead of proceeding to parse nm_output; ensure you
reference result.returncode and result.stderr when building the warning message
and preserve the existing conservative-return behavior used for
subprocess.TimeoutExpired/FileNotFoundError/OSError.
In `@usermods/PS_Comet/PS_Comet.cpp`:
- Line 4: The global nextCometCreationTime must be moved into segment-local
effect state so each segment running mode_pscomet() has its own throttle; stop
using the translation-unit global nextCometCreationTime and instead store the
deadline in SEGENV.step (or in the per-segment extra bytes after PSdataEnd) and
initialize it in the effect initialization path, then reference and update that
per-segment field inside mode_pscomet() wherever nextCometCreationTime is
currently read or written so reinitialization and separate segment instances do
not share the same timer.
In `@usermods/TetrisAI_v2/tetrisbag.h`:
- Around line 89-92: The loop in queuePiece() shifts elements then assigns to
piecesQueue[piecesQueue.size() - 1], which underflows when piecesQueue is empty;
add an early guard at the start of queuePiece() (e.g., if (piecesQueue.empty())
return;) or otherwise ensure queueLength > 0 before running the loop and the
final assignment so you never compute size()-1 on an empty vector; update the
function that contains the for-loop and the call to getNextPiece() to use this
guard.
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1131-1147: The 4-bit hash in SEGENV.aux1 is too small and causes
frequent collisions; compute and keep a larger fingerprint (e.g., full 16-bit
textHash) and persist it in a wider field (use SEGENV.data or another available
SEGENV field) instead of truncating to 4 bits. Change the code that reads
lastHashBits from SEGENV.aux1 to read the full stored hash from SEGENV.data (or
the chosen field), compute a 16-bit textHash in the loop (keep textHash as
uint16_t), set currentHash = textHash (or a simple reduced-but-still-large slice
like top 12 bits), compare that against the stored value when computing
textChanged, and ensure when rebuilding you update the stored hash in
SEGENV.data (and stop packing the hash into SEGENV.aux1). Update references
around textHash, currentHashBits/lastHashBits, needsRebuild and where
SEGENV.aux1 was previously written so the new larger hash is saved and loaded
consistently (affects mode_morsecode / SEGMENT.name change detection).
In `@usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp`:
- Around line 132-133: When the early-return path triggered by
autoSaveIgnorePresets && currentPreset>0 runs, it must cancel any pending
autosave and resync the tracked state before returning; update the block around
the condition that checks autoSaveAfterSec, enabled, autoSaveIgnorePresets,
currentPreset, strip.isUpdating(), now and lastRun to clear the pending
autoSaveAfter timer/flag and refresh the known* tracking variables (e.g., known
color/state variables the autosaver uses) prior to returning so a
previously-armed autosave cannot later persist the preset state unexpectedly.
In `@wled00/bus_manager.cpp`:
- Around line 1190-1199: The current code inserts BusPlaceholder instances into
BusManager::busses which preserves the original bus type and causes unsafe
downcasts (e.g., static_cast<BusDigital*>(bus.get())->isI2S()) and premature
loop termination on checks like bus->isOk(); fix by either (A) never pushing
BusPlaceholder into BusManager::busses (create placeholders in a separate
container or only keep real Bus* in busses) or (B) add a reliable discriminator
on Bus (e.g., enum BusKind or bool isPlaceholder()) and update every downcast
and loop to check that discriminator before casting or treating the bus as real;
audit and guard all uses such as BusPlaceholder, BusManager::busses,
Bus::isVirtual, Bus::isDigital, the static_cast<BusDigital*> sites, the
virtual-cast to BusNetwork, and FX_fcn.cpp checks of bus->isOk() so placeholders
are skipped or handled safely.
In `@wled00/bus_wrapper.h`:
- Around line 1389-1398: The current logic in the I2S assignment block
(_i2sChannelsAssigned, offset) force-coerces later bus pixel type t to the first
lane's type via _parallelBusItype and flips _useParallelI2S, which silently
creates mismatched pixel-type buses; instead detect when a later request's t
differs from _parallelBusItype and reject/abort creation (or fall back to RMT)
rather than overwriting t: update the branch that handles offset==1 to compare t
vs _parallelBusItype and, on mismatch, return an error/false or switch to the
RMT path, keeping _useParallelI2S only for compatible types and preserving the
original t when compatible.
In `@wled00/cfg.cpp`:
- Around line 110-119: The current code always writes default/empty
WPA-Enterprise values into multiWiFi[n], causing silent downgrades when the JSON
omits those keys; change deserialize logic so encType, enterpriseAnonIdentity
and enterpriseIdentity are only overwritten when the corresponding JSON keys are
present/non-empty—use the existing wifi JSON checks (e.g., test wifi["e_ident"],
wifi["e_anon_ident"], and wifi[F("enc_type")] or CJSON result) and only call
multiWiFi[n].encryptionType = encType and strlcpy(...) when a value was actually
provided by getStringFromJson/CJSON, otherwise leave
multiWiFi[n].enterpriseAnonIdentity, multiWiFi[n].enterpriseIdentity and
multiWiFi[n].encryptionType unchanged.
In `@wled00/data/cpal/cpal.htm`:
- Around line 117-121: When toggling preview via the gId('chkPreview') change
handler, the current logic calls applyLED() or sends
requestJson({seg:{frz:false}}), which only unfreezes currently selected segments
and leaves the previously previewed main segment frozen; modify the handler and
related preview code to cache the previewed segment IDs (e.g., store in a
variable like previewedSegIds when applyLED() is used for previewing) and when
turning preview off (or on page leave) call requestJson for each cached id with
explicit objects like {seg:{id:<cachedId>, frz:false}} (or a single request with
an array of ids if the API supports it), then clear the cache; apply the same
change at the other occurrence noted (around lines 693–695) to ensure previewed
segments are explicitly unfrozen.
In `@wled00/data/edit.htm`:
- Around line 111-136: The timeout branch in loadFiles currently calls cb() as
if the script loaded, which can start dependent work prematurely; change the
timeout handler (tm) inside loadFiles so it invokes the script's error path
instead of cb() — e.g., call the same onerror logic (or invoke s.onerror()) so
the retry/failure logic runs (and eventually calls cb() only when appropriate).
Update references in loadFiles (tm, s.onload, s.onerror, cb) so the timeout
triggers the error/retry flow and thus prevents loadResources/S from being
called prematurely.
In `@wled00/data/index.js`:
- Around line 46-62: The current loadIro IIFE sets l.src = 'iro.js' before
onLoad populates location info, causing file: mode to try a local file and retry
indefinitely; change the flow so the page initializes first (wait for onLoad to
run or check d.readyState) and only then call a refactored loadIro that builds
l.src from the resolved location/IP (use the populated loc/locip or a canonical
absolute URL) and creates cpick; remove the tight infinite retry by adding a
graceful fallback (stop after a few retries or detect file: scheme and skip
loading the remote script), and ensure errors call a single failure handler
instead of continuously retrying so the IP prompt can appear when appropriate.
In `@wled00/data/settings_leds.htm`:
- Around line 147-150: The current save path contains a hard-stop when bquot >
100 (the block that alerts "Too many LEDs! Can't handle that!" and returns
false), which prevents saving oversized but recoverable configs; remove that
early return and instead only show warnings (preserve the existing alerts for
bquot > 80 and the additional messages referencing maxM) so the form can proceed
and allow finalizeInit() logic to trim bus lengths; ensure no other code paths
rely on that return to avoid proceeding with completely invalid data.
In `@wled00/dmx_input.cpp`:
- Around line 151-158: The early return on invalid inputPortNum leaks previously
allocated GPIO pins; either validate inputPortNum before any pin allocation
(move the inputPortNum check above the pin-allocation code) or, if you keep the
current order, free/deallocate the allocated pins before the early return (call
the existing pin cleanup routine or add a deallocatePins()/releasePins() call)
so that when the block around this->inputPortNum sets default and returns the
pins are not left reserved.
In `@wled00/dmx_output.cpp`:
- Around line 82-100: The DMX API calls use the v4.1.0 signatures: replace the
current three-arg dmx_driver_install call in DMXOutput::init with the four-arg
form expected by esp_dmx (pass dmxPort, config (not &config), a personalities
pointer (use nullptr or an empty array) and a personality count (0) to satisfy
the new signature), and change the dmx_send call in DMXOutput::update to the
single-argument form dmx_send(dmxPort); keep using dmx_write(dmxPort, dmxdata,
DMX_PACKET_SIZE) and dmx_set_pin(dmxPort, outputPin, -1, -1) as before but
ensure the types match the updated API.
In `@wled00/FX_fcn.cpp`:
- Around line 1165-1188: The code counts i2sBusCount from raw
BusConfig.driverType before fallbacks and then later double-counts bus memory
(memUsage + getBusSize), causing use_placeholder to trigger incorrectly; fix by
(1) moving the I2S detection to use the resolved runtime driver (call
BusManager::getI(...) or resolve the bus via BusManager before counting) and
derive useParallelI2S from that resolved driver instead of BusConfig.driverType,
and (2) remove the duplicate size addition (do not add getBusSize() on top of
BusConfig::memUsage() or adjust memUsage so it only counts each bus once) so
that mem and use_placeholder are computed from the same, non-duplicated values
(refer to busConfigs, i2sBusCount, Bus::isDigital, Bus::is2Pin, driverType,
BusManager::getI, useParallelI2S, BusConfig::memUsage, getBusSize,
use_placeholder).
In `@wled00/wled_metadata.cpp`:
- Around line 131-137: The bufToString template leaves sbuf uninitialized past
real_len causing nondeterministic reads; in the bufToString(const char
(&buf)[len]) function, after memcpy(..., real_len) set sbuf[real_len] = '\0'
(instead of sbuf[len]) so the copied string is properly null-terminated and
String(sbuf) only reads the intended bytes.
- Around line 56-66: The metadata struct WLED_BUILD_DESCRIPTION is emitting a
hard-coded version of 1 which prevents shouldAllowOTA() from checking
safe_update_version; change the second field (currently
"/*WLED_CUSTOM_DESC_VERSION*/ 1") to the actual v2 metadata version (e.g.,
replace with WLED_CUSTOM_DESC_VERSION or the literal 2) so binaries produced
include desc_version > 1 and trigger the safe-update gate (update the
initializer in WLED_BUILD_DESCRIPTION).
---
Outside diff comments:
In `@wled00/json.cpp`:
- Around line 813-817: Preserve info.arch as a stable family identifier and add
a separate key for the detailed chip model: always set root[F("arch")] to the
family string (e.g., "esp32" for ESP32 targets) instead of conditionally using
ESP.getChipModel(); then publish the full chip model under a new key such as
root[F("chip")] using ESP.getChipModel() (or equivalent) so clients can still do
exact comparisons on arch while accessing detailed model via the new key.
---
Minor comments:
In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md`:
- Around line 17-20: The cp command in BLE_REQUIREMENT.md incorrectly uses
../../../ which overshoots the repo root; update the sample copy command so it
references the actual sample file path and copies it into the intended
destination (replace the existing cp line that mentions ../../../ with a command
that reads the sample from
usermods/pixels_dice_tray/platformio_override.ini.sample and copies it to
platformio_override.ini) so readers running the example from the repo root or
from within usermods/pixels_dice_tray get the correct file.
In `@usermods/user_fx/README.md`:
- Around line 269-273: The README example incorrectly references SEGMENT.call;
update the snippet and text to use SEGENV.call (the call counter lives on the
segment environment) so replace any mention of SEGMENT.call with SEGENV.call and
ensure the explanatory sentence refers to SEGENV.call incrementing on each
function call.
In `@wled00/data/common.js`:
- Around line 140-160: uploadFile is marked async but currently returns
immediately; convert it into a proper promise-returning async function by
returning a new Promise that resolves/rejects when the XMLHttpRequest completes:
create and return new Promise((resolve,reject)=>{ set
req.addEventListener('load', ...) to call resolve(this.status < 400) (and still
call callback if provided), set req.addEventListener('error', ...) to call
reject(false) or resolve(false) consistently (and call callback), and remove the
implicit immediate completion—ensure fileObj.value = '' runs after
resolution/rejection; keep existing await usage (e.g., await file.text()) so
uploadFile remains async but now correctly represents upload completion.
In `@wled00/data/index.css`:
- Line 3: The src declaration uses an unquoted data URL in the url(...) call
which triggers Stylelint's function-url-quotes rule; fix it by updating the src:
url(...) token (the data:font/woff2... entry) to use quoted URL syntax (i.e.,
url("...")) so the entire data URL argument is wrapped in quotes while leaving
the rest of the `@font-face` declaration unchanged.
In `@wled00/data/settings_sec.htm`:
- Line 69: The closing </span> in the OTA section is orphaned (see the HTML
fragment "Disabling this option will make your device less
secure.</i><br></span>"); remove the stray </span> so the markup properly closes
only the <i> tag and the <br>, or if intended, add the corresponding opening
<span> with the needed attributes—prefer removing the stray </span> to restore
valid HTML in settings_sec.htm.
In `@wled00/ota_update.cpp`:
- Around line 403-442: calculateBootloaderSHA256 currently ignores
esp_flash_read failures and BootloaderImageSizer parse errors and always sets
bootloaderSHA256CacheValid = true; change it to track any failure (e.g., set a
bool readFailed if esp_flash_read(...) != ESP_OK, and a bool parseFailed if the
sizer detects a corrupt/impossible size such that totalSize == 0 or totalSize >
BOOTLOADER_SIZE or other sizer-specific error), and if either flag is true or
totalHashLen == 0 then do not mark the cache valid: clear/zero
bootloaderSHA256Cache, ensure mbedtls_sha256_free(&ctx) is called, and leave
bootloaderSHA256CacheValid = false (return early). Update
calculateBootloaderSHA256 to only set bootloaderSHA256CacheValid = true when no
read/parsing failures occurred and totalHashLen > 0.
---
Duplicate comments:
In `@wled00/dmx_output.cpp`:
- Around line 95-96: The write method currently narrows the DMX address and can
wrap addresses >255 and lacks bounds checking; change DMXOutput::write signature
to accept a wider integer type (e.g., int or size_t) to match how DMXAddr is
computed in handleDMXOutput(), update the declaration in wled00/dmx_output.h
accordingly, and inside DMXOutput::write validate the incoming address against
the dmxdata buffer size (use the buffer length or a constant like
DMX_UNIVERSE_SIZE) before indexing to avoid out-of-bounds writes.
- Around line 72-75: The allocation-failure log is currently passing an enum to
a "%s" format which is undefined behavior; update the second DEBUG_PRINTF call
to pass the string-returning helper instead by replacing
PinManager::getPinOwner(outputPin) with PinManager::getPinOwnerName(outputPin)
(keep the "%s" format), or alternatively use the new getPinOwnerName() in any
combined message so the log receives a C-string; modify the DEBUG_PRINTF
invocation in the DMXOutput allocation failure branch accordingly.
In `@wled00/set.cpp`:
- Around line 476-478: The code only updates dmxOutputPin but does not
reinitialize the DMX driver, so change the handler that sets dmxOutputPin (where
request->arg(F("IDMO")).toInt() is used) to also reconfigure the DMX output:
stop/disable the current DMX instance, release the old pin, and initialize/start
the DMX driver on the new pin. Locate the DMX init/teardown logic (e.g.
functions used elsewhere like startDmxOutput()/stopDmxOutput(),
dmx.begin()/dmx.stop(), or the DMX driver init in setup) and invoke the teardown
then startup sequence after assigning dmxOutputPin; if such helper functions
don’t exist, add a small reinit routine that calls the driver’s stop and
begin/init with the new dmxOutputPin so the change takes effect immediately
without reboot.
---
Nitpick comments:
In `@CONTRIBUTING.md`:
- Around line 61-66: The blockquote starting with "[!CAUTION]" contains an extra
blank line before "Do not use "force-push" while your PR is open!" which
triggers markdownlint MD028; remove the empty line so the blockquote content is
contiguous (i.e., ensure "[!CAUTION]" is immediately followed by the warning
text) to fix the formatting and prevent inconsistent Markdown rendering.
- Around line 17-21: The blockquote lines starting with "> " (the line
containing "**The easiest way to start your first PR**" and the following lines
including the image tag) have extra spaces after the ">" which triggers
markdownlint MD027; update those blockquote lines to use a single space after
">" (e.g., change "> **The easiest..." to "> **The easiest..." and similarly
remove the extra spaces before the image tag and subsequent lines) so the
blockquote follows standard markdown spacing.
In `@usermods/pov_display/pov_display.cpp`:
- Line 38: Remove the redundant trailing `return;` from the end of the void
function in pov_display.cpp (the final statement shown as `return;` in the
diff); simply delete that line so the function returns implicitly rather than
explicitly.
In `@usermods/TetrisAI_v2/TetrisAI_v2.cpp`:
- Line 8: The global variable noFlashOnClear should be made file-local to avoid
exporting an unnecessary symbol: change its declaration in TetrisAI_v2.cpp to
have internal linkage (e.g., prepend static: "static bool noFlashOnClear =
false;") or move it into the TetrisAIUsermod class as a private member and
update all uses accordingly; update any references in the file to use the new
member name if moved into the class.
In `@usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp`:
- Around line 88-296: This block of mode_* wrapper functions (e.g.,
mode_Module_Experiment10, mode_Zoom, mode_RGB_Blobs, mode_Rotating_Blob) are
only used in this translation unit and should be file-local to avoid exporting
globals; make them static (or enclose the entire block in an anonymous
namespace) and reformat the block to the project's 2-space C++ style while
preserving the existing calls like anim.initEffect() and anim.<EffectName>().
In `@usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp`:
- Line 132: Replace the magic number 240 in the autosave throttle check with a
named constant (e.g., AUTO_SAVE_UPDATE_THROTTLE_SEC) to make the meaning
explicit; declare the constant at the top of usermod_v2_auto_save.cpp (or
alongside other constants) and update the condition that uses strip.isUpdating()
&& now - lastRun < 240 to use the new constant (refer to autoSaveAfterSec,
enabled, autoSaveIgnorePresets, currentPreset, strip.isUpdating(), lastRun, now)
so the throttle interval is self-documenting and easy to change.
In `@wled00/button.cpp`:
- Around line 379-382: The relay enable assumes pinMode was already set
elsewhere; make the relay setup self-contained by calling pinMode(rlyPin,
OUTPUT) before digitalWrite(rlyPin, rlyMde) in the block that checks rlyPin>=0
(so the relay is reliably driven even if handleOnOff(true) or beginStrip() order
changes), then keep the existing delay(RELAY_DELAY) afterwards; reference
rlyPin, rlyMde, RELAY_DELAY, digitalWrite(), and the related
beginStrip()/handleOnOff setup when updating the code.
In `@wled00/data/icons-ui/style.css`:
- Around line 4-6: The `@font-face` src list in style.css (the lines listing
url('fonts/wled122.ttf...'), url('fonts/wled122.woff...'),
url('fonts/wled122.svg...')) no longer includes a woff2 entry; confirm this
removal was intentional and, if not, restore the woff2 source (e.g., add a
url('fonts/wled122.woff2?...') format('woff2') entry) within the same `@font-face`
src order and ensure the referenced woff2 file exists in the fonts assets and
any cache-busting query string matches your build artifacts.
In `@wled00/data/settings_pininfo.htm`:
- Around line 16-30: The 250ms polling in S() (pinsTimer = setInterval(loadPins,
250)) is too aggressive for resource-constrained devices; change the interval
used by setInterval to a longer value (e.g., 500–1000 ms) or make it
configurable, and keep the call site referencing pinsTimer and loadPins so the
existing start/stop logic remains intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d1610ede-c6d8-4616-be92-169479d0f724
⛔ Files ignored due to path filters (5)
package-lock.jsonis excluded by!**/package-lock.jsonwled00/data/icons-ui/fonts/wled122.svgis excluded by!**/*.svgwled00/data/icons-ui/fonts/wled122.ttfis excluded by!**/*.ttfwled00/data/icons-ui/fonts/wled122.woffis excluded by!**/*.woffwled00/data/icons-ui/fonts/wled122.woff2is excluded by!**/*.woff2
📒 Files selected for processing (118)
.github/ISSUE_TEMPLATE/bug.yml.github/copilot-instructions.md.github/workflows/nightly.yml.github/workflows/release.yml.gitignoreCONTRIBUTING.mdpackage.jsonpio-scripts/dynarray.pypio-scripts/load_usermods.pypio-scripts/validate_modules.pyplatformio.iniplatformio_override.sample.initools/cdata.jstools/dynarray_espressif32.ldusermods/Analog_Clock/Analog_Clock.cppusermods/PS_Comet/PS_Comet.cppusermods/PS_Comet/README.mdusermods/PS_Comet/library.jsonusermods/Temperature/Temperature.cppusermods/TetrisAI_v2/TetrisAI_v2.cppusermods/TetrisAI_v2/gridbw.husermods/TetrisAI_v2/gridcolor.husermods/TetrisAI_v2/pieces.husermods/TetrisAI_v2/readme.mdusermods/TetrisAI_v2/tetrisai.husermods/TetrisAI_v2/tetrisbag.husermods/audioreactive/audio_reactive.cppusermods/audioreactive/audio_source.husermods/deep_sleep/deep_sleep.cppusermods/pixels_dice_tray/BLE_REQUIREMENT.mdusermods/pixels_dice_tray/led_effects.husermods/pixels_dice_tray/library.jsonusermods/pov_display/pov_display.cppusermods/rgb-rotary-encoder/rgb-rotary-encoder.cppusermods/user_fx/README.mdusermods/user_fx/user_fx.cppusermods/usermod_v2_RF433/usermod_v2_RF433.cppusermods/usermod_v2_animartrix/usermod_v2_animartrix.cppusermods/usermod_v2_auto_save/usermod_v2_auto_save.cppwled00/FX.cppwled00/FX.hwled00/FX_2Dfcn.cppwled00/FX_fcn.cppwled00/FXparticleSystem.cppwled00/FXparticleSystem.hwled00/NodeStruct.hwled00/bus_manager.cppwled00/bus_manager.hwled00/bus_wrapper.hwled00/button.cppwled00/cfg.cppwled00/colors.cppwled00/const.hwled00/data/404.htmwled00/data/common.jswled00/data/cpal/cpal.htmwled00/data/dmxmap.htmwled00/data/edit.htmwled00/data/icons-ui/HowTo_AddNewIcons.txtwled00/data/icons-ui/Read Me.txtwled00/data/icons-ui/demo-files/demo.csswled00/data/icons-ui/demo.htmlwled00/data/icons-ui/selection.jsonwled00/data/icons-ui/style.csswled00/data/index.csswled00/data/index.htmwled00/data/index.jswled00/data/liveview.htmwled00/data/liveviewws2D.htmwled00/data/msg.htmwled00/data/pixart/pixart.htmwled00/data/pixelforge/pixelforge.htmwled00/data/settings.htmwled00/data/settings_2D.htmwled00/data/settings_leds.htmwled00/data/settings_pin.htmwled00/data/settings_pininfo.htmwled00/data/settings_sec.htmwled00/data/settings_sync.htmwled00/data/settings_time.htmwled00/data/settings_ui.htmwled00/data/settings_um.htmwled00/data/settings_wifi.htmwled00/data/style.csswled00/data/update.htmwled00/data/usermod.htmwled00/data/welcome.htmwled00/dmx_input.cppwled00/dmx_output.cppwled00/dynarray.hwled00/e131.cppwled00/fcn_declare.hwled00/file.cppwled00/improv.cppwled00/ir.cppwled00/json.cppwled00/mqtt.cppwled00/network.cppwled00/ntp.cppwled00/ota_update.cppwled00/ota_update.hwled00/overlay.cppwled00/pin_manager.cppwled00/pin_manager.hwled00/presets.cppwled00/remote.cppwled00/set.cppwled00/udp.cppwled00/um_manager.cppwled00/util.cppwled00/wled.cppwled00/wled.hwled00/wled_metadata.cppwled00/wled_metadata.hwled00/wled_serial.cppwled00/wled_server.cppwled00/ws.cppwled00/xml.cpp
💤 Files with no reviewable changes (4)
- usermods/TetrisAI_v2/pieces.h
- usermods/Analog_Clock/Analog_Clock.cpp
- usermods/pixels_dice_tray/library.json
- wled00/ota_update.h
✅ Files skipped from review due to trivial changes (24)
- .gitignore
- wled00/data/msg.htm
- wled00/data/liveview.htm
- wled00/data/dmxmap.htm
- wled00/data/usermod.htm
- wled00/data/settings_pin.htm
- wled00/data/404.htm
- wled00/data/icons-ui/HowTo_AddNewIcons.txt
- wled00/data/settings.htm
- wled00/data/liveviewws2D.htm
- usermods/PS_Comet/library.json
- wled00/data/welcome.htm
- wled00/network.cpp
- wled00/data/style.css
- wled00/data/pixart/pixart.htm
- usermods/deep_sleep/deep_sleep.cpp
- .github/ISSUE_TEMPLATE/bug.yml
- wled00/data/icons-ui/demo-files/demo.css
- wled00/data/settings_time.htm
- package.json
- wled00/data/settings_um.htm
- wled00/NodeStruct.h
- wled00/e131.cpp
- usermods/TetrisAI_v2/readme.md
🚧 Files skipped from review as they are similar to previous changes (5)
- wled00/pin_manager.h
- wled00/data/settings_sync.htm
- wled00/xml.cpp
- platformio.ini
- wled00/wled.cpp
Comment on lines
+57
to
+93
| def _predict_dep_name(entry: str) -> str | None: | ||
| """Predict the library name PlatformIO will assign to this dep (best-effort). | ||
|
|
||
| Accuracy relies on the library's manifest "name" matching the repo/package | ||
| name in the spec. This holds for well-authored libraries; the libArchive | ||
| check (which requires library.json) provides an early-failure safety net. | ||
| """ | ||
| return usermod_dir in Path(dep.src_dir).parents or str(dep.name).startswith("wled-") | ||
|
|
||
| ## Script starts here | ||
| # Process usermod option | ||
| usermods = env.GetProjectOption("custom_usermods","") | ||
| entry = entry.strip() | ||
| # "LibName = <spec>" — name is given explicitly; always use it | ||
| m = _NAME_EQ_RE.match(entry) | ||
| if m: | ||
| return m.group(1).strip() | ||
| # URL scheme: extract name from path | ||
| if _URL_SCHEME_RE.match(entry): | ||
| parsed = urlparse(entry) | ||
| if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'): | ||
| parts = [p for p in parsed.path.split('/') if p] | ||
| if len(parts) >= 2: | ||
| name = parts[1] | ||
| else: | ||
| name = Path(parsed.path.rstrip('/')).name.strip() | ||
| if name.endswith('.git'): | ||
| name = name[:-4] | ||
| return name or None | ||
| # SSH git URL: git@github.com:user/repo.git#tag → repo | ||
| if _SSH_URL_RE.match(entry): | ||
| path_part = entry.split(':', 1)[1].split('#')[0].rstrip('/') | ||
| name = Path(path_part).name | ||
| return (name[:-4] if name.endswith('.git') else name) or None | ||
| # Versioned registry: "owner/Name @ version" → Name | ||
| if '@' in entry: | ||
| name_part = entry.split('@')[0].strip() | ||
| return name_part.split('/')[-1].strip() if '/' in name_part else name_part | ||
| # Plain registry: "owner/Name" → Name | ||
| if re.match(r'^[^/\s]+/[^/\s]+$', entry): | ||
| return entry.split('/')[-1].strip() | ||
| return None |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from urllib.parse import urlparse
from pathlib import Path
samples = [
"symlink:///tmp/example_usermod",
"file:///tmp/example_usermod",
"https://github.com/org/example_usermod.git",
]
for entry in samples:
parsed = urlparse(entry)
name = None
if parsed.netloc in ("github.com", "gitlab.com", "bitbucket.com"):
parts = [p for p in parsed.path.split("/") if p]
if len(parts) >= 2:
name = parts[1]
else:
name = Path(parsed.path.rstrip("/")).name.strip()
if name.endswith(".git"):
name = name[:-4]
print(f"{entry} -> {name}")
PYRepository: wled/WLED
Length of output: 189
🏁 Script executed:
# Search for symlink:// and file:// references in the codebase
rg "symlink://" --type py
rg "file://" --type pyRepository: wled/WLED
Length of output: 357
🏁 Script executed:
# Check how _custom_usermod_names is used and populated
rg "_custom_usermod_names" --type py -A 3 -B 3Repository: wled/WLED
Length of output: 2207
🏁 Script executed:
# Examine the full context around _predict_dep_name usage (lines around 57-93, 96-102, 114-125)
cat -n pio-scripts/load_usermods.py | sed -n '50,130p'Repository: wled/WLED
Length of output: 3569
🏁 Script executed:
# Check if there's documentation about custom_usermods format
rg -i "custom_usermod|symlink|file://" --type mdRepository: wled/WLED
Length of output: 4918
Fix _predict_dep_name() to handle symlink:// and file:// paths.
The code generates and uses symlink:// URLs internally (lines 131, 136), yet _predict_dep_name() fails to extract names from them, returning None instead. This prevents those modules from being registered in _custom_usermod_names and breaks the is_wled_module() check. The function should extract the directory name from all URL schemes, not just hosted Git providers.
The suggested fix extracts the final path component for symlink://, file://, and other schemes while preserving special handling for hosted Git repos:
Suggested fix
if _URL_SCHEME_RE.match(entry):
parsed = urlparse(entry)
- if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
- parts = [p for p in parsed.path.split('/') if p]
- if len(parts) >= 2:
- name = parts[1]
- else:
- name = Path(parsed.path.rstrip('/')).name.strip()
- if name.endswith('.git'):
- name = name[:-4]
- return name or None
+ path = parsed.path.split('#', 1)[0].rstrip('/')
+ parts = [p for p in path.split('/') if p]
+ if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com') and len(parts) >= 2:
+ name = parts[1]
+ else:
+ name = Path(path).name.strip()
+ if name.endswith('.git'):
+ name = name[:-4]
+ return name or NoneAffects lines 96–102 and 114–125 as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pio-scripts/load_usermods.py` around lines 57 - 93, _predict_dep_name
currently ignores symlink:// and file:// (and other non-hosted URL schemes) and
thus returns None for those entries; update it so that after the existing
hosted-Git branch (the _URL_SCHEME_RE.match(entry) check) you still parse any
scheme with urlparse(entry) and extract the final path component
(Path(parsed.path).name), stripping trailing '/' and a '.git' suffix and
returning None if empty; retain the special-case branch that handles
github.com/gitlab.com/bitbucket.com by using the second path segment when
available, and ensure the SSH branch (_SSH_URL_RE) and '@' / plain registry
branches keep their current behavior (still strip fragments, versions, and
'.git' as currently done).
Comment on lines
+42
to
+50
| try: | ||
| result = subprocess.run( | ||
| [nm_path, "--defined-only", "-l", str(elf_path)], | ||
| capture_output=True, text=True, errors="ignore", timeout=120, | ||
| ) | ||
| nm_output = result.stdout | ||
| except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e: | ||
| secho(f"WARNING: nm failed ({e}); skipping per-module validation", fg="yellow", err=True) | ||
| return {Path(b.build_dir).name for b in module_lib_builders} # conservative pass |
Contributor
There was a problem hiding this comment.
Handle non-zero nm exits before parsing stdout.
Only exceptions take the conservative-pass path today. If nm exists but exits non-zero, stdout can be empty and this turns into a false "No symbols..." build failure instead of being treated as a tool failure.
🐛 Proposed fix
try:
result = subprocess.run(
[nm_path, "--defined-only", "-l", str(elf_path)],
capture_output=True, text=True, errors="ignore", timeout=120,
)
+ if result.returncode != 0:
+ secho(
+ f"WARNING: nm failed with exit code {result.returncode}; skipping per-module validation",
+ fg="yellow",
+ err=True,
+ )
+ return {Path(b.build_dir).name for b in module_lib_builders}
nm_output = result.stdout
except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e:
secho(f"WARNING: nm failed ({e}); skipping per-module validation", fg="yellow", err=True)
return {Path(b.build_dir).name for b in module_lib_builders} # conservative pass🧰 Tools
🪛 Ruff (0.15.6)
[error] 43-43: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pio-scripts/validate_modules.py` around lines 42 - 50, The current
subprocess.run call invoking nm (in the block that assigns result =
subprocess.run(...) and nm_output = result.stdout) only handles exceptions, so
if nm exits with a non-zero returncode we may parse empty stdout and erroneously
fail; update the logic to check result.returncode after the subprocess.run call
and treat non-zero as a tool failure: log a warning including returncode and
stderr, then return the conservative pass set (same as in the exception branch)
instead of proceeding to parse nm_output; ensure you reference result.returncode
and result.stderr when building the warning message and preserve the existing
conservative-return behavior used for
subprocess.TimeoutExpired/FileNotFoundError/OSError.
| #include "wled.h" | ||
| #include "FXparticleSystem.h" | ||
|
|
||
| unsigned long nextCometCreationTime = 0; |
Contributor
There was a problem hiding this comment.
Keep comet spawn timing in segment-local state.
nextCometCreationTime is a translation-unit global, so every segment running mode_pscomet() shares the same spawn throttle and the deadline survives effect reinitialization. This needs to live in segment-local effect state (SEGENV.step or extra bytes after PSdataEnd), not in a global.
Also applies to: 47-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PS_Comet/PS_Comet.cpp` at line 4, The global nextCometCreationTime
must be moved into segment-local effect state so each segment running
mode_pscomet() has its own throttle; stop using the translation-unit global
nextCometCreationTime and instead store the deadline in SEGENV.step (or in the
per-segment extra bytes after PSdataEnd) and initialize it in the effect
initialization path, then reference and update that per-segment field inside
mode_pscomet() wherever nextCometCreationTime is currently read or written so
reinitialization and separate segment instances do not share the same timer.
Comment on lines
+89
to
92
| for (uint8_t i = 1; i < piecesQueue.size(); i++) { | ||
| piecesQueue[i - 1] = piecesQueue[i]; | ||
| } | ||
| piecesQueue[piecesQueue.size() - 1] = getNextPiece(); |
Contributor
There was a problem hiding this comment.
Guard queuePiece() against empty queues before indexing.
If piecesQueue is empty, Line 92 computes piecesQueue.size() - 1, underflows, and performs out-of-bounds access. Add an early return (or enforce queueLength > 0 before use).
Suggested fix
void queuePiece()
{
+ if (piecesQueue.empty()) return;
//move vector to left
- for (uint8_t i = 1; i < piecesQueue.size(); i++) {
+ for (uint8_t i = 1; i < piecesQueue.size(); i++) {
piecesQueue[i - 1] = piecesQueue[i];
}
piecesQueue[piecesQueue.size() - 1] = getNextPiece();
}📝 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.
Suggested change
| for (uint8_t i = 1; i < piecesQueue.size(); i++) { | |
| piecesQueue[i - 1] = piecesQueue[i]; | |
| } | |
| piecesQueue[piecesQueue.size() - 1] = getNextPiece(); | |
| if (piecesQueue.empty()) return; | |
| for (uint8_t i = 1; i < piecesQueue.size(); i++) { | |
| piecesQueue[i - 1] = piecesQueue[i]; | |
| } | |
| piecesQueue[piecesQueue.size() - 1] = getNextPiece(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/TetrisAI_v2/tetrisbag.h` around lines 89 - 92, The loop in
queuePiece() shifts elements then assigns to piecesQueue[piecesQueue.size() -
1], which underflows when piecesQueue is empty; add an early guard at the start
of queuePiece() (e.g., if (piecesQueue.empty()) return;) or otherwise ensure
queueLength > 0 before running the loop and the final assignment so you never
compute size()-1 on an empty vector; update the function that contains the
for-loop and the call to getNextPiece() to use this guard.
Comment on lines
+1131
to
+1147
| // SEGENV.aux1 stores: [bit 15: check2] [bit 14: check3] [bits 10-13: text hash (4 bits)] [bits 0-9: pattern length] | ||
| bool lastCheck2 = (SEGENV.aux1 & 0x8000) != 0; | ||
| bool lastCheck3 = (SEGENV.aux1 & 0x4000) != 0; | ||
| uint16_t lastHashBits = (SEGENV.aux1 >> 10) & 0xF; // 4 bits of hash | ||
| uint16_t patternLength = SEGENV.aux1 & 0x3FF; // Lower 10 bits for length (up to 1023) | ||
|
|
||
| // Compute text hash | ||
| uint16_t textHash = 0; | ||
| for (char *p = text; *p; p++) { | ||
| textHash = ((textHash << 5) + textHash) + *p; | ||
| } | ||
| uint16_t currentHashBits = (textHash >> 12) & 0xF; // Use upper 4 bits of hash | ||
|
|
||
| bool textChanged = (currentHashBits != lastHashBits) && (SEGENV.call > 0); | ||
|
|
||
| // Check if we need to rebuild the pattern | ||
| bool needsRebuild = (SEGENV.call == 0) || textChanged || (SEGMENT.check2 != lastCheck2) || (SEGMENT.check3 != lastCheck3); |
Contributor
There was a problem hiding this comment.
A 4-bit rename fingerprint will miss too many name changes.
Using only four hash bits gives roughly a 1-in-16 collision rate, so mode_morsecode() can keep scrolling the previous message after SEGMENT.name changes. Store a larger hash in SEGENV.data or another field instead of truncating it this aggressively.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/user_fx/user_fx.cpp` around lines 1131 - 1147, The 4-bit hash in
SEGENV.aux1 is too small and causes frequent collisions; compute and keep a
larger fingerprint (e.g., full 16-bit textHash) and persist it in a wider field
(use SEGENV.data or another available SEGENV field) instead of truncating to 4
bits. Change the code that reads lastHashBits from SEGENV.aux1 to read the full
stored hash from SEGENV.data (or the chosen field), compute a 16-bit textHash in
the loop (keep textHash as uint16_t), set currentHash = textHash (or a simple
reduced-but-still-large slice like top 12 bits), compare that against the stored
value when computing textChanged, and ensure when rebuilding you update the
stored hash in SEGENV.data (and stop packing the hash into SEGENV.aux1). Update
references around textHash, currentHashBits/lastHashBits, needsRebuild and where
SEGENV.aux1 was previously written so the new larger hash is saved and loaded
consistently (affects mode_morsecode / SEGMENT.name change detection).
Comment on lines
+151
to
+158
| if (inputPortNum <= (SOC_UART_NUM - 1) && inputPortNum > 0) { | ||
| this->inputPortNum = inputPortNum; | ||
| } | ||
| else { | ||
| DEBUG_PRINTF("DMXInput: Error: invalid inputPortNum: %d, default to 1\n", inputPortNum); | ||
| this->inputPortNum = 1; | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Resource leak on invalid inputPortNum.
When inputPortNum is out of range, the function returns at line 157 after pins have already been allocated (line 143). However, the pins are never deallocated, causing a resource leak—the pins remain reserved but the DMX driver is never initialized.
Consider deallocating the pins before returning, or move the port validation before pin allocation.
Proposed fix: Deallocate pins on early return
if (inputPortNum <= (SOC_UART_NUM - 1) && inputPortNum > 0) {
this->inputPortNum = inputPortNum;
}
else {
DEBUG_PRINTF("DMXInput: Error: invalid inputPortNum: %d, default to 1\n", inputPortNum);
- this->inputPortNum = 1;
+ PinManager::deallocateMultiplePins(pins, 3, PinOwner::DMX_INPUT);
return;
}Alternatively, move the validation before pin allocation:
Alternative fix: Validate before allocation
void DMXInput::init(uint8_t rxPin, uint8_t txPin, uint8_t enPin, uint8_t inputPortNum)
{
+ // Validate port number first
+ if (inputPortNum > (SOC_UART_NUM - 1) || inputPortNum == 0) {
+ DEBUG_PRINTF("DMXInput: Error: invalid inputPortNum: %d\n", inputPortNum);
+ return;
+ }
if (rxPin > 0 && enPin > 0 && txPin > 0) {
const managed_pin_type pins[] = {📝 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.
Suggested change
| if (inputPortNum <= (SOC_UART_NUM - 1) && inputPortNum > 0) { | |
| this->inputPortNum = inputPortNum; | |
| } | |
| else { | |
| DEBUG_PRINTF("DMXInput: Error: invalid inputPortNum: %d, default to 1\n", inputPortNum); | |
| this->inputPortNum = 1; | |
| return; | |
| } | |
| if (inputPortNum <= (SOC_UART_NUM - 1) && inputPortNum > 0) { | |
| this->inputPortNum = inputPortNum; | |
| } | |
| else { | |
| DEBUG_PRINTF("DMXInput: Error: invalid inputPortNum: %d, default to 1\n", inputPortNum); | |
| PinManager::deallocateMultiplePins(pins, 3, PinOwner::DMX_INPUT); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/dmx_input.cpp` around lines 151 - 158, The early return on invalid
inputPortNum leaks previously allocated GPIO pins; either validate inputPortNum
before any pin allocation (move the inputPortNum check above the pin-allocation
code) or, if you keep the current order, free/deallocate the allocated pins
before the early return (call the existing pin cleanup routine or add a
deallocatePins()/releasePins() call) so that when the block around
this->inputPortNum sets default and returns the pins are not left reserved.
Comment on lines
+82
to
+100
| void DMXOutput::init(uint8_t outputPin) { | ||
| dmx_config_t config = DMX_CONFIG_DEFAULT; | ||
| const bool installOk = dmx_driver_install(dmxPort, &config, DMX_INTR_FLAGS_DEFAULT); | ||
| if (!installOk) { | ||
| DEBUG_PRINTF("DMXOutput: Error: Failed to install dmx driver\n"); | ||
| return; | ||
| } | ||
| const bool setPin = dmx_set_pin(dmxPort, outputPin, -1, -1); | ||
| if (!setPin) { | ||
| DEBUG_PRINTF("DMXOutput: Error: Failed to set DMX output pin\n"); | ||
| return; | ||
| } | ||
| } | ||
| void DMXOutput::write(uint8_t channel, uint8_t value) { | ||
| dmxdata[channel] = value; | ||
| } | ||
| void DMXOutput::update() { | ||
| dmx_write(dmxPort, dmxdata, DMX_PACKET_SIZE); | ||
| dmx_send(dmxPort, DMX_PACKET_SIZE); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "dmx_output.cpp" -o -name "*dmx*" -type f | head -20Repository: wled/WLED
Length of output: 198
🏁 Script executed:
cat -n wled00/dmx_output.cpp | head -110 | tail -50Repository: wled/WLED
Length of output: 1845
🏁 Script executed:
find . -type f \( -name "idf_component.yml" -o -name "idf_component.yaml" -o -name "*.txt" -o -name "*.json" \) | xargs grep -l "esp_dmx" 2>/dev/null | head -5Repository: wled/WLED
Length of output: 35
🏁 Script executed:
find . -type f \( -name "platformio.ini" -o -name "CMakeLists.txt" -o -name "*.cmake" \) | head -10Repository: wled/WLED
Length of output: 69
🏁 Script executed:
cat platformio.ini | grep -A 5 -B 5 -i "esp_dmx\|dmx"Repository: wled/WLED
Length of output: 895
🌐 Web query:
esp_dmx someweisguy commit 47db25d esp_dmx.h dmx_driver_install
💡 Result:
esp_dmx is an open-source ESP32 library by GitHub user someweisguy (likely Mitch Weisbrod) for implementing ANSI-ESTA E1.11 DMX-512A and E1.20 RDM protocols. It supports transmitting/receiving DMX/RDM via any hardware UART port. The header file esp_dmx.h declares dmx_driver_install, the key function to initialize a DMX driver: dmx_driver_install(dmx_port_t dmx_num, const dmx_config_t *config, const dmx_personality_t personalities[], int personality_count); Usage example from README.md (release/v4.1): const dmx_port_t dmx_num = DMX_NUM_1; dmx_config_t config = DMX_CONFIG_DEFAULT; const int personality_count = 1; dmx_personality_t personalities[] = { {.footprint = 1, .slots = 16} }; dmx_driver_install(dmx_num, &config, personalities, personality_count); Then set pins: dmx_set_pin(dmx_num, tx_pin, rx_pin, rts_pin); Commit 47db25d could not be located in searches of the repository. It may be from a fork, private repo, or deleted. No matching commit found in main repo history (2,838 commits noted). Known issues: Compatibility problems with newer ESP-IDF versions (e.g., 5.3+), causing periph_module errors on UART2/LP-UART (ESP32-C6). Fixes proposed in PR #187 and issues #150/#204/#164, but not merged as of 2026-02. Use updated uart.c patches for ESP-IDF 5.4+. Library requires ESP-IDF v4.4.1+ or Arduino-ESP32 v2.0.3+. Latest release v4.1.0 (2024-03-21).
Citations:
- 1: https://github.com/someweisguy/esp_dmx
- 2: https://github.com/someweisguy/esp_dmx/blob/release/v4.1/README.md
- 3: https://github.com/someweisguy/esp_dmx/blob/release%2Fv4.1/src%2Fdmx%2Fhal%2Fuart.c
- 4: fixed periph.module bug someweisguy/esp_dmx#187
- 5: Error trying to compile DMX Receiver in custom UART Pins someweisguy/esp_dmx#204
- 6: Problem with installing DMX on UART2 someweisguy/esp_dmx#150
🌐 Web query:
esp_dmx dmx_send dmx_send_num API signature v4.1.0
💡 Result:
In the esp_dmx library version 4.1.0 (https://github.com/someweisguy/esp_dmx/tree/release/v4.1), the API functions for sending DMX data are: - dmx_send(dmx_port_t dmx_num): Initiates sending the current DMX buffer to the bus (full packet). - dmx_send_num(dmx_port_t dmx_num, int num_slots): Sends a specified number of slots from the buffer (seen in Arduino example usage). These follow dmx_write(dmx_num, data, size) to populate the buffer. Use dmx_wait_sent(dmx_num, ticks) to wait for completion. Note: esp_dmx is a third-party ESP32 DMX512/RDM library for ESP-IDF/Arduino, not official ESP-IDF. Official ESP-IDF v4.1 has no esp_dmx component. Library requires ESP-IDF v4.4.1+, but v4.1 branch exists with recent compatibility fixes (PR #187). Arduino examples confirm dmx_send_num(dmxPort, DMX_PACKET_SIZE).
Citations:
- 1: https://github.com/someweisguy/esp_dmx/blob/release/v4.1/README.md
- 2: https://github.com/someweisguy/esp_dmx/blob/release%2Fv4.1/src%2Fdmx%2Fhal%2Fuart.c
- 3: https://github.com/someweisguy/esp_dmx
- 4: https://forum.arduino.cc/t/timing-for-dmx-on-an-esp32/1333698
🏁 Script executed:
# Let's verify the commit exists by checking if it's in a specific branch
cd . && git log --oneline | grep "47db25d" || echo "Commit not in current history"Repository: wled/WLED
Length of output: 82
Fix API calls to match esp_dmx v4.1.0 signatures.
The pinned esp_dmx library (commit 47db25d) requires four arguments for dmx_driver_install(dmx_num, config, personalities[], personality_count) and one argument for dmx_send(dmx_num). The current code passes incorrect argument counts and types:
- Line 84:
dmx_driver_install(dmxPort, &config, DMX_INTR_FLAGS_DEFAULT)has 3 args; the third should be a personalities array pointer and a fourth argument specifying the count - Line 100:
dmx_send(dmxPort, DMX_PACKET_SIZE)has 2 args but only one is accepted
Fix
void DMXOutput::init(uint8_t outputPin) {
dmx_config_t config = DMX_CONFIG_DEFAULT;
- const bool installOk = dmx_driver_install(dmxPort, &config, DMX_INTR_FLAGS_DEFAULT);
+ const bool installOk = dmx_driver_install(dmxPort, &config, nullptr, 0);
if (!installOk) {
DEBUG_PRINTF("DMXOutput: Error: Failed to install dmx driver\n");
return;
}
const bool setPin = dmx_set_pin(dmxPort, outputPin, -1, -1);
@@ void DMXOutput::update() {
dmx_write(dmxPort, dmxdata, DMX_PACKET_SIZE);
- dmx_send(dmxPort, DMX_PACKET_SIZE);
+ dmx_send(dmxPort);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/dmx_output.cpp` around lines 82 - 100, The DMX API calls use the
v4.1.0 signatures: replace the current three-arg dmx_driver_install call in
DMXOutput::init with the four-arg form expected by esp_dmx (pass dmxPort, config
(not &config), a personalities pointer (use nullptr or an empty array) and a
personality count (0) to satisfy the new signature), and change the dmx_send
call in DMXOutput::update to the single-argument form dmx_send(dmxPort); keep
using dmx_write(dmxPort, dmxdata, DMX_PACKET_SIZE) and dmx_set_pin(dmxPort,
outputPin, -1, -1) as before but ensure the types match the updated API.
Comment on lines
+1165
to
+1188
| // validate the bus config: count I2S buses and check if they meet requirements | ||
| unsigned i2sBusCount = 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; | ||
| if (bus.driverType == 1) | ||
| i2sBusCount++; | ||
| } | ||
| } | ||
| 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 (maxLedsOnBus <= 600 && useParallelI2S) BusManager::useParallelOutput(); // must call before creating buses | ||
| else useParallelI2S = false; // enforce single I2S | ||
| digitalCount = 0; | ||
| DEBUG_PRINTF_P(PSTR("Digital buses: %u, I2S buses: %u\n"), digitalCount, i2sBusCount); | ||
|
|
||
| // Determine parallel vs single I2S usage (used for memory calculation only) | ||
| bool useParallelI2S = false; | ||
| #if defined(CONFIG_IDF_TARGET_ESP32S3) | ||
| // ESP32-S3 always uses parallel LCD driver for I2S | ||
| if (i2sBusCount > 0) { | ||
| useParallelI2S = true; | ||
| } | ||
| #else | ||
| if (i2sBusCount > 1) { | ||
| useParallelI2S = true; | ||
| } | ||
| #endif |
Contributor
There was a problem hiding this comment.
The new RAM gate can placeholder buses that actually fit.
i2sBusCount is derived from driverType before BusManager::getI() resolves any fallback to RMT, so useParallelI2S can stay enabled for a bus set that is no longer actually parallel-I2S. Then Line 1225 adds getBusSize() on top of BusConfig::memUsage(), which already counted the bus object + driver buffer. That inflates mem twice and makes use_placeholder fire too early.
Possible direction
- unsigned i2sBusCount = 0;
-
- for (const auto &bus : busConfigs) {
- if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type)) {
- digitalCount++;
- if (bus.driverType == 1)
- i2sBusCount++;
- }
- }
+ unsigned i2sBusCount = 0;
+ for (auto &bus : busConfigs) {
+ if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type)) digitalCount++;
+ bus.iType = BusManager::getI(bus.type, bus.pins, bus.driverType);
+ if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type) && (bus.iType & 0x01) == 0) i2sBusCount++;
+ }
- for (auto &bus : busConfigs) {
- // assign bus types...
- bus.iType = BusManager::getI(bus.type, bus.pins, bus.driverType);
- }
for (auto &bus : busConfigs) {
bool use_placeholder = false;
unsigned busMemUsage = bus.memUsage();
mem += busMemUsage;
...
- if (BusManager::add(bus, use_placeholder) != -1) {
- mem += BusManager::busses.back()->getBusSize();
+ if (BusManager::add(bus, use_placeholder) != -1) {
if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type) && BusManager::busses.back()->isPlaceholder()) digitalCount--;
}
}Also applies to: 1193-1226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX_fcn.cpp` around lines 1165 - 1188, The code counts i2sBusCount from
raw BusConfig.driverType before fallbacks and then later double-counts bus
memory (memUsage + getBusSize), causing use_placeholder to trigger incorrectly;
fix by (1) moving the I2S detection to use the resolved runtime driver (call
BusManager::getI(...) or resolve the bus via BusManager before counting) and
derive useParallelI2S from that resolved driver instead of BusConfig.driverType,
and (2) remove the duplicate size addition (do not add getBusSize() on top of
BusConfig::memUsage() or adjust memUsage so it only counts each bus once) so
that mem and use_placeholder are computed from the same, non-duplicated values
(refer to busConfigs, i2sBusCount, Bus::isDigital, Bus::is2Pin, driverType,
BusManager::getI, useParallelI2S, BusConfig::memUsage, getBusSize,
use_placeholder).
Comment on lines
56
to
+66
| const wled_metadata_t __attribute__((section(BUILD_METADATA_SECTION))) WLED_BUILD_DESCRIPTION = { | ||
| WLED_CUSTOM_DESC_MAGIC, // magic | ||
| WLED_CUSTOM_DESC_VERSION, // version | ||
| /*WLED_CUSTOM_DESC_VERSION*/ 1, // structure version. Currently set to 1 to allow OTA from broken original version. FIXME before 0.16 release. | ||
| 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 | ||
| #if defined(ESP32) && defined(CONFIG_IDF_TARGET_ESP32) | ||
| { 0, 15, 3 }, // Some older ESP32 might have bootloader issues; assume we'll have it sorted by 0.15.3 | ||
| #else | ||
| { 0, 15, 2 }, // All other platforms can update safely | ||
| #endif |
Contributor
There was a problem hiding this comment.
Emit the actual v2 metadata version here, or the safe-update gate never runs.
shouldAllowOTA() only checks safe_update_version when the uploaded image reports desc_version > 1. With Line 58 hard-coded to 1, every binary built from this branch bypasses the new guard, so the protection stays disabled until someone remembers to remove the FIXME.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/wled_metadata.cpp` around lines 56 - 66, The metadata struct
WLED_BUILD_DESCRIPTION is emitting a hard-coded version of 1 which prevents
shouldAllowOTA() from checking safe_update_version; change the second field
(currently "/*WLED_CUSTOM_DESC_VERSION*/ 1") to the actual v2 metadata version
(e.g., replace with WLED_CUSTOM_DESC_VERSION or the literal 2) so binaries
produced include desc_version > 1 and trigger the safe-update gate (update the
initializer in WLED_BUILD_DESCRIPTION).
Comment on lines
+131
to
+137
| template<size_t len> | ||
| static inline String bufToString(const char (&buf)[len]) { | ||
| char sbuf[len+1]; | ||
| size_t real_len = strnlen(buf, len); | ||
| memcpy(sbuf, buf, real_len); | ||
| sbuf[len] = '\0'; | ||
| return sbuf; |
Contributor
There was a problem hiding this comment.
Terminate the copied string at real_len.
When the source name is shorter than the fixed buffer, sbuf[real_len..len-1] stays uninitialized and String(sbuf) reads past the copied text until it happens to find '\0'. That makes OTA release-name comparisons nondeterministic.
💡 Minimal fix
template<size_t len>
static inline String bufToString(const char (&buf)[len]) {
- char sbuf[len+1];
+ char sbuf[len + 1];
size_t real_len = strnlen(buf, len);
memcpy(sbuf, buf, real_len);
- sbuf[len] = '\0';
+ sbuf[real_len] = '\0';
return sbuf;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/wled_metadata.cpp` around lines 131 - 137, The bufToString template
leaves sbuf uninitialized past real_len causing nondeterministic reads; in the
bufToString(const char (&buf)[len]) function, after memcpy(..., real_len) set
sbuf[real_len] = '\0' (instead of sbuf[len]) so the copied string is properly
null-terminated and String(sbuf) only reads the intended bytes.
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.
Allow the DMX Output pin to be set at runtime, not need the user to hack around in the source code
Also swap the DMX Output to the same driver as DMX Input, leaving only 8266 to use ESPDMX
Summary by CodeRabbit
New Features
Refactor
Documentation