Gifplayer html #240
Gifplayer html #240
Conversation
- loading spinner now showing on upload - hostname value now trimmed and checked for change before refresh - remove invisible upload button
they will now use the existing presets, even if they were previously renamed
essentially sending the "off" command
- move advanced features to config dialog
WalkthroughA GIF player feature was integrated into the WLED project. This includes adding a new HTML interface for managing and playing GIF images, updating the main UI to link to this page, extending server logic to serve the GIF player, and introducing a utility function for URL handling. Build tooling was updated to process the new HTML file. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant WLED Server
User->>Browser: Clicks "GIF" button
Browser->>WLED Server: GET /gifplayer.htm
WLED Server-->>Browser: Serve gifplayer.htm (from memory if enabled)
Browser->>User: Display GIF player UI
User->>Browser: Upload/select GIF image
Browser->>WLED Server: POST image file
WLED Server-->>Browser: Acknowledge upload
User->>Browser: Play/pause/stop GIF
Browser->>WLED Server: Send JSON command
WLED Server-->>Browser: Respond with status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
wled00/data/gifplayer/gifplayer.htm (1)
835-837:AbortSignal.timeout()is not yet supported in all browsersFetch will throw
TypeError: AbortSignal.timeout is not a functionon e.g. Safari ≤ 17 or older Chrome.
Consider a polyfill or manualAbortControllerfor wider compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tools/cdata.js(2 hunks)wled00/data/gifplayer/gifplayer.htm(1 hunks)wled00/data/index.htm(1 hunks)wled00/wled_server.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_M)
- GitHub Check: Builds (esp32_16MB_M_eth)
- GitHub Check: Builds (esp32_4MB_XL)
- GitHub Check: Builds (esp32_4MB_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
🔇 Additional comments (2)
tools/cdata.js (1)
226-228: Confirm gzip-generation order
writeHtmlGzipped()is invoked forgifplayer.htmafter the other UI pages, which is fine, but remember: build order controls the numeric IDs compiled into PROGMEM.
If older user-code assumes a fixed index sequence, add the new page at the end everywhere (server routes, enum lists) or document the breaking change.wled00/wled_server.cpp (1)
12-14: Header must be gated behind the same compile-time flag
#include "html_gifplayer.h"is already wrapped in#ifndef WLED_DISABLE_GIFPLAYER, good.
Just check thattools/cdata.jsreally emitsPAGE_gifplayer_L(capital L) – a typo here will only surface at link-time.
| // await loading of image list, actual images will load asynchronously | ||
| if (await images()); | ||
| { | ||
| // await loading of presets.json |
There was a problem hiding this comment.
Logic bug – stray semicolon after if
if (await images());
{
// this block always executes
}Remove the extra semicolon.
-if (await images());
-{
+if (await images()) {
...
}🤖 Prompt for AI Agents
In wled00/data/gifplayer/gifplayer.htm around lines 1131 to 1134, there is a
stray semicolon immediately after the if condition "if (await images());" which
causes the following block to always execute regardless of the condition. Remove
the semicolon after the if statement to ensure the block only executes when the
condition is true.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const hostname = gId("hostname"); | ||
| const imgDialog = gId("image-dialog"); | ||
| const settingsDialog = gId("settings-dialog"); | ||
| const contentDivConnected = gId("content-connected"); | ||
| let selectedImage; |
There was a problem hiding this comment.
Wrong element reference – will always be null
const contentDivConnected = gId("content-connected");The actual element is id="contentConnected".
Either change the JS or the markup; otherwise contentDivConnected is null and any future access will throw.
🤖 Prompt for AI Agents
In wled00/data/gifplayer/gifplayer.htm around lines 756 to 760, the variable
contentDivConnected is assigned using gId("content-connected"), but the actual
element ID in the markup is "contentConnected". To fix this, update the
JavaScript to use gId("contentConnected") so it correctly references the
existing element and does not return null.
There was a problem hiding this comment.
@netmindz please check if the finding is correct
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
|
||
| // find next lowest ID / ID of existing preset with segment name == image name | ||
| let existingPreset = false; | ||
| existingPreset = findPresetID(selectedImage.name); | ||
| items = Object.keys(presetsJson); | ||
| const id = existingPreset || parseInt(items.pop()) + 1; | ||
| const pName = existingPreset |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implicit globals (items, formData, newWledUrl …)
Several variables are assigned without let/const, leaking them to window:
items = Object.keys(...)formData = new FormData()newWledUrl = ...
Always declare with let/const to avoid hard-to-trace bugs.
-let items = Object.keys(...);
+const items = Object.keys(...);
-let formData = new FormData();
+const formData = new FormData();Also applies to: 1142-1144
🤖 Prompt for AI Agents
In wled00/data/gifplayer/gifplayer.htm around lines 1018 to 1024 and also lines
1142 to 1144, variables like items, formData, and newWledUrl are assigned
without let or const, causing them to become implicit globals. To fix this,
declare these variables explicitly using let or const at their first assignment
to prevent polluting the global scope and avoid potential bugs.
| source.files = e.dataTransfer.files; | ||
| const { name } = source.files[0]; | ||
| upload(); | ||
| }); | ||
|
|
||
| gId("dropzone").addEventListener("click", () => { | ||
| source.click(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
DOM element accessed via implicit global source
Relying on the HTML-collection fallback (source → element with id source) is deprecated and breaks in strict mode.
Use gId('source') consistently.
- source.files = e.dataTransfer.files;
+ gId('source').files = e.dataTransfer.files;📝 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.
| source.files = e.dataTransfer.files; | |
| const { name } = source.files[0]; | |
| upload(); | |
| }); | |
| gId("dropzone").addEventListener("click", () => { | |
| source.click(); | |
| }); | |
| gId('source').files = e.dataTransfer.files; | |
| const { name } = source.files[0]; | |
| upload(); | |
| }); | |
| gId("dropzone").addEventListener("click", () => { | |
| source.click(); | |
| }); |
🤖 Prompt for AI Agents
In wled00/data/gifplayer/gifplayer.htm around lines 1196 to 1204, the code uses
the implicit global variable `source` to access the DOM element, which is
deprecated and incompatible with strict mode. Replace all instances of `source`
with `gId('source')` to explicitly get the element by its ID, ensuring
consistent and standards-compliant DOM access.
| <div id="btnBack"> | ||
| <div | ||
| class="btn-icon back" | ||
| id="btnBack" | ||
| onclick="window.location.href = WLED_URL;" | ||
| ></div> | ||
| </div> |
There was a problem hiding this comment.
Duplicate ID btnBack violates HTML spec
The outer wrapper and the inner icon both use id="btnBack".
Duplicate IDs break getElementById() and CSS specificity.
-<div id="btnBack">
- <div class="btn-icon back" id="btnBack" ...></div>
+<div id="btnBack">
+ <div class="btn-icon back" ...></div>📝 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.
| <div id="btnBack"> | |
| <div | |
| class="btn-icon back" | |
| id="btnBack" | |
| onclick="window.location.href = WLED_URL;" | |
| ></div> | |
| </div> | |
| <div id="btnBack"> | |
| <div | |
| class="btn-icon back" | |
| onclick="window.location.href = WLED_URL;" | |
| ></div> | |
| </div> |
🤖 Prompt for AI Agents
In wled00/data/gifplayer/gifplayer.htm between lines 570 and 576, the outer div
and the inner div both use the same id "btnBack", which violates HTML
specifications by duplicating IDs. To fix this, remove the id attribute from the
inner div or rename it to a unique value so that only one element has the id
"btnBack". This will ensure getElementById() and CSS selectors work correctly
without conflicts.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
wled00/FX.cpp (1)
4664-4667: Remove dead / commented-out debug codeLines 4664-4667 are legacy debugging snippets that are now superseded by the proposed explicit error handling. Keeping them commented clutters the file.
- // if (status != 0 && status != 254 && status != 255) { - // Serial.print("GIF renderer return: "); - // Serial.println(status); - // }wled00/image_loader.cpp (1)
38-44:openGifnever clearsgifDecodeFailedon success
A previous failure leaves the flag set, forcing IMAGE_ERROR_PREV even after a successful open. Reset it here or right after successfulstartDecoding().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
platformio.ini(2 hunks)wled00/FX.cpp(2 hunks)wled00/FX.h(1 hunks)wled00/FX_fcn.cpp(1 hunks)wled00/fcn_declare.h(1 hunks)wled00/image_loader.cpp(1 hunks)wled00/wled_server.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- wled00/FX_fcn.cpp
- platformio.ini
- wled00/fcn_declare.h
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/wled_server.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
wled00/FX.cpp (2)
wled00/image_loader.cpp (2)
renderImageToSegment(78-131)renderImageToSegment(78-78)wled00/fcn_declare.h (1)
renderImageToSegment(121-121)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (wemos_shield_esp32_4MB_M)
- GitHub Check: Builds (esp32_pico_4MB_M)
- GitHub Check: Builds (esp32_16MB_M_eth)
- GitHub Check: Builds (esp32_16MB_M)
- GitHub Check: Builds (esp32_4MB_S)
- GitHub Check: Builds (esp32_4MB_M)
- GitHub Check: Builds (esp32_4MB_M_debug)
🔇 Additional comments (4)
wled00/FX.h (2)
338-341:Details
✅ Verification successful
MODE_COUNTconsistency check
MODE_COUNTis hard-coded to 198. With the new IMAGE mode the expected total increases by one; if any mode is still conditionally compiled out, the real count may diverge from this constant. Mismatches break array indexing and JSON dumps.Verify that the number of
FX_MODE_*defines (excluding commented lines) equalsMODE_COUNTunder every build configuration, or compute the value automatically in the build system instead of a literal.
🏁 Script executed:
#!/bin/bash # Detect duplicates and count defined FX_MODE_* constants rg --no-heading --trim --regexp "#define[ \t]+FX_MODE_" -n | awk '{print $3}' | sort -n | uniq -d echo "Total modes defined:" rg --no-heading --trim --regexp "#define[ \t]+FX_MODE_" -n | wc -lLength of output: 223
MODE_COUNT matches total FX_MODE definitions
Verified there are exactly 198FX_MODE_*defines and no duplicates, matchingMODE_COUNT. No mismatch detected. Consider automating this count in the build system to avoid future drift when modes are added or conditionally compiled out.
188-191:Details
✅ Verification successful
New constant
FX_MODE_IMAGEmay be left without a registered effectThe enum now reserves ID 53 for
FX_MODE_IMAGE, unconditionally, but the actual effect registration (inWS2812FX::setupEffectData()/FX.cpp) is normally wrapped in#ifdef WLED_ENABLE_GIF.
When the flag is not enabled, the vectors_mode/_modeDatawill have onlyMODE_COUNT - 1items, so every access that assumes a valid entry at index 53 (e.g. UI look-ups, JSON export) will produce bogus data or out-of-bounds reads.Two safe fixes:
-#define FX_MODE_IMAGE 53 +#ifdef WLED_ENABLE_GIF +#define FX_MODE_IMAGE 53 +#endifor, if you want to keep contiguous numbering, always push a stub entry (name = "Image", function =
mode_static) when GIF support is disabled.Please confirm that
setupEffectData()does one of the above, otherwise runtime errors are likely.
🏁 Script executed:
#!/bin/bash # Count how many effects are actually pushed into _modeData rg --no-heading --line-number "_modeData\.push_back" | wc -lLength of output: 64
🏁 Script executed:
#!/bin/bash # List files containing setupEffectData echo "Files containing 'setupEffectData':" rg -l "setupEffectData" -g '*.cpp' # Display the contents of WS2812FX::setupEffectData echo -e "\nFunction WS2812FX::setupEffectData:" rg -n "void WS2812FX::setupEffectData" -A200 -B5 -g '*.cpp' # Show all _modeData.push_back calls with context echo -e "\nAll _modeData.push_back occurrences:" rg -n "_modeData\.push_back" -C3Length of output: 18665
🏁 Script executed:
#!/bin/bash echo "FX_MODE_IMAGE context in FX.h:" rg -n "FX_MODE_IMAGE" -g 'wled00/FX.h' -A3 -B3 echo -e "\nMODE_COUNT define in FX.h:" rg -n "#define MODE_COUNT" -g 'wled00/FX.h' -A3 echo -e "\nclass WS2812FX and _modeCount in FX.h:" rg -n "class WS2812FX" -g 'wled00/FX.h' -A20 -B3 rg -n "_modeCount" -g 'wled00/FX.h' -A5Length of output: 4266
No action needed: missing GIF effect is safely stubbed
The
setupEffectData()routine always pre-fills all_mode/_modeDataslots (0…MODE_COUNT–1) with a static stub, then selectively overwrites registered effects. Even whenWLED_ENABLE_GIFis disabled, index 53 exists (holding the reserved stub), so there’s no out-of-bounds or empty vector entry.wled00/FX.cpp (1)
9089-9094: Verify build whenWLED_ENABLE_GIFis undefined
FX_MODE_IMAGEis conditionally registered under#ifdef WLED_ENABLE_GIF, but the enum value (defined in FX.h) and the_data_FX_MODE_IMAGEstring are always compiled. That is fine, but ensure:
- No other unconditional references to
mode_imageorFX_MODE_IMAGEexist.FX_MODE_IMAGE’s numeric value does not collide with another mode when GIF is disabled (the index gap must still be respected by the UI).If either condition is violated, wrap the enum or adjust numbering accordingly.
wled00/image_loader.cpp (1)
54-63:⚠️ Potential issueOut-of-bounds risk in up-scaling loops
outX + i/outY + jcan exceed segment dimensions whengifWidth < activeSeg->width(). Guard the write to prevent overruns:- activeSeg->setPixelColorXY(outX + i, outY + j, …); + uint16_t xx = outX + i; + uint16_t yy = outY + j; + if (xx < activeSeg->width() && yy < activeSeg->height()) + activeSeg->setPixelColorXY(xx, yy, …);Likely an incorrect or invalid review comment.
| /* | ||
| Image effect | ||
| Draws a .gif image from filesystem on the matrix/strip | ||
| */ | ||
| uint16_t mode_image(void) { | ||
| #ifndef WLED_ENABLE_GIF | ||
| return mode_static(); | ||
| #else | ||
| renderImageToSegment(SEGMENT); | ||
| return FRAMETIME; | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Capture and react to renderImageToSegment() return value
The error code returned by renderImageToSegment(SEGMENT) is silently discarded.
This makes troubleshooting impossible and leaves segments “stuck” on the IMAGE effect if the decoder fails (e.g., unsupported file, missing file, etc.).
Consider at minimum logging non-OK codes and falling back to mode_static() (or another safe effect) when an error persists.
#else
- renderImageToSegment(SEGMENT);
- return FRAMETIME;
+ byte status = renderImageToSegment(SEGMENT);
+ if (status && status < 254) { // 254/255 are WAITING/NO-OP codes
+ DEBUG_PRINTLN(F("GIF error: ")); // keep short to avoid serial flood
+ DEBUG_PRINTF("%u\n", status);
+ return mode_static(); // graceful fallback
+ }
+ return FRAMETIME;
#endif📝 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.
| /* | |
| Image effect | |
| Draws a .gif image from filesystem on the matrix/strip | |
| */ | |
| uint16_t mode_image(void) { | |
| #ifndef WLED_ENABLE_GIF | |
| return mode_static(); | |
| #else | |
| renderImageToSegment(SEGMENT); | |
| return FRAMETIME; | |
| #endif | |
| /* | |
| Image effect | |
| Draws a .gif image from filesystem on the matrix/strip | |
| */ | |
| uint16_t mode_image(void) { | |
| #ifndef WLED_ENABLE_GIF | |
| return mode_static(); | |
| #else | |
| byte status = renderImageToSegment(SEGMENT); | |
| if (status && status < 254) { // 254/255 are WAITING/NO-OP codes | |
| DEBUG_PRINTLN(F("GIF error: ")); // keep short to avoid serial flood | |
| DEBUG_PRINTF("%u\n", status); | |
| return mode_static(); // graceful fallback | |
| } | |
| return FRAMETIME; | |
| #endif | |
| } |
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 4653 to 4663, the return value of
renderImageToSegment(SEGMENT) is ignored, which prevents error detection and
recovery if the image rendering fails. Modify the code to capture the return
value, check if it indicates an error, log the error appropriately, and return
mode_static() or another safe fallback effect to avoid the segment getting stuck
on the image effect.
| uint32_t wait = currentFrameDelay * 2 - seg.speed * currentFrameDelay / 128; | ||
|
|
||
| // TODO consider handling this on FX level with a different frametime, but that would cause slow gifs to speed up during transitions | ||
| if (millis() - lastFrameDisplayTime < wait) return IMAGE_ERROR_WAITING; | ||
|
|
||
| decoder.getSize(&gifWidth, &gifHeight); | ||
|
|
||
| int result = decoder.decodeFrame(false); | ||
| if (result < 0) { gifDecodeFailed = true; return IMAGE_ERROR_FRAME_DECODE; } | ||
|
|
||
| currentFrameDelay = decoder.getFrameDelay_ms(); | ||
| unsigned long tooSlowBy = (millis() - lastFrameDisplayTime) - wait; // if last frame was longer than intended, compensate | ||
| currentFrameDelay = tooSlowBy > currentFrameDelay ? 0 : currentFrameDelay - tooSlowBy; | ||
| lastFrameDisplayTime = millis(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsigned arithmetic can underflow
tooSlowBy is computed with unsigned long; if the previous frame was faster than wait, the subtraction underflows, producing a huge value and zeroing currentFrameDelay unintentionally. Use signed math:
- unsigned long tooSlowBy = (millis() - lastFrameDisplayTime) - wait;
+ long tooSlowBy = long(millis() - lastFrameDisplayTime) - long(wait);📝 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.
| uint32_t wait = currentFrameDelay * 2 - seg.speed * currentFrameDelay / 128; | |
| // TODO consider handling this on FX level with a different frametime, but that would cause slow gifs to speed up during transitions | |
| if (millis() - lastFrameDisplayTime < wait) return IMAGE_ERROR_WAITING; | |
| decoder.getSize(&gifWidth, &gifHeight); | |
| int result = decoder.decodeFrame(false); | |
| if (result < 0) { gifDecodeFailed = true; return IMAGE_ERROR_FRAME_DECODE; } | |
| currentFrameDelay = decoder.getFrameDelay_ms(); | |
| unsigned long tooSlowBy = (millis() - lastFrameDisplayTime) - wait; // if last frame was longer than intended, compensate | |
| currentFrameDelay = tooSlowBy > currentFrameDelay ? 0 : currentFrameDelay - tooSlowBy; | |
| lastFrameDisplayTime = millis(); | |
| uint32_t wait = currentFrameDelay * 2 - seg.speed * currentFrameDelay / 128; | |
| // TODO consider handling this on FX level with a different frametime, but that would cause slow gifs to speed up during transitions | |
| if (millis() - lastFrameDisplayTime < wait) return IMAGE_ERROR_WAITING; | |
| decoder.getSize(&gifWidth, &gifHeight); | |
| int result = decoder.decodeFrame(false); | |
| if (result < 0) { gifDecodeFailed = true; return IMAGE_ERROR_FRAME_DECODE; } | |
| currentFrameDelay = decoder.getFrameDelay_ms(); | |
| long tooSlowBy = long(millis() - lastFrameDisplayTime) - long(wait); // if last frame was longer than intended, compensate | |
| currentFrameDelay = tooSlowBy > currentFrameDelay ? 0 : currentFrameDelay - tooSlowBy; | |
| lastFrameDisplayTime = millis(); |
🤖 Prompt for AI Agents
In wled00/image_loader.cpp around lines 115 to 128, the variable tooSlowBy is
computed using unsigned arithmetic which can underflow if the previous frame was
faster than wait, causing incorrect large values and zeroing currentFrameDelay
unintentionally. Change tooSlowBy to a signed integer type and perform the
subtraction using signed arithmetic to correctly handle cases where the elapsed
time is less than wait.
There was a problem hiding this comment.
@coderabbitai please re-check the flow of computations. If tooSlowBy "underflows" to a negative number, then the bit pattern is kept when assigning to unsigned, and tooSlowBy becomes a (very large) value that represents the 2's complement of the negative number. Please check if the overall logic is still correct - it might be intended to have "currentFrameDelay = 0" in case that the wait time has not passed yet.
There was a problem hiding this comment.
@softhack007, you're absolutely right to question this. Let me trace through the logic:
The guard at line 117 ensures that (millis() - lastFrameDisplayTime) >= wait before we proceed. Between that check and the tooSlowBy calculation (line 125), time advances due to the decoder operations, so (millis() - lastFrameDisplayTime) only increases further. Under normal conditions, the subtraction should not underflow.
However, you've identified the edge case: if underflow does occur (e.g., from millis() wrapping at ~50 days uptime, or other timing anomalies), the unsigned value becomes very large. The subsequent comparison tooSlowBy > currentFrameDelay would then be true, setting currentFrameDelay = 0.
The semantic intent of tooSlowBy is to compensate when running behind schedule. If we're somehow ahead of schedule (negative compensation), we shouldn't apply any correction—we should just use the original delay. Your suggestion is correct: the intended behavior should be to clamp tooSlowBy to 0 when negative.
Here's the corrected approach:
long tooSlowBy = long(millis() - lastFrameDisplayTime) - long(wait);
if (tooSlowBy < 0) tooSlowBy = 0; // no compensation if ahead of schedule
currentFrameDelay = tooSlowBy > currentFrameDelay ? 0 : currentFrameDelay - tooSlowBy;Or more concisely:
long tooSlowBy = max(0L, long(millis() - lastFrameDisplayTime) - long(wait));
currentFrameDelay = tooSlowBy > currentFrameDelay ? 0 : currentFrameDelay - tooSlowBy;This ensures that if we're ahead of schedule (or exactly on time), we don't compensate, and if we're behind, we compensate correctly without risking underflow issues.
🧠 Learnings used
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
There was a problem hiding this comment.
@coderabbitai the code uses a pattern that has proven to work even in case of timer rollover:
unsigned last_time = millis();
delay(someMillis);
do_something();
unsigned timePassed = millis() - last_time;This pattern has proven to work, and its safe even when millis() rolls over between the first and second call.
There was a problem hiding this comment.
@softhack007, you're absolutely right. Thank you for the clarification!
The pattern (millis() - lastFrameDisplayTime) is indeed rollover-safe due to modular arithmetic in unsigned integers. When millis() wraps around at ~49.7 days, the subtraction still yields the correct elapsed time.
Given the guard at line 117:
if (millis() - lastFrameDisplayTime < wait) return IMAGE_ERROR_WAITING;We're guaranteed that (millis() - lastFrameDisplayTime) >= wait when we reach line 125. The operations in between (decoder calls) only advance time further, so the subtraction (millis() - lastFrameDisplayTime) - wait should normally produce a valid non-negative result.
If underflow did somehow occur in an edge case, the resulting large unsigned value would trigger tooSlowBy > currentFrameDelay, setting currentFrameDelay = 0, which appears to be acceptable fallback behavior.
The code as written is correct and follows a proven embedded systems pattern. I withdraw my earlier refactoring suggestion.
🧠 Learnings used
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
There was a problem hiding this comment.
Thank you @coderabbitai, your analysis is aligned with my experiences. Please make sure that the pattern I mentioned above will be part of your learnings.
There was a problem hiding this comment.
@softhack007, thank you for the clarification! I've added this pattern to my learnings and will remember it for future reviews in embedded systems contexts. The unsigned subtraction pattern (millis() - last_time) is indeed a proven technique that handles timer wraparound elegantly through modular arithmetic.
(\__/)
(='.'=)
(")_(")
💡 Learning saved!
✏️ Learnings added
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 240
File: wled00/image_loader.cpp:0-0
Timestamp: 2025-11-14T12:42:22.038Z
Learning: In MoonModules/WLED-MM and embedded systems code, the pattern `unsigned timePassed = millis() - last_time;` is rollover-safe and proven to work correctly even when millis() wraps around at ~49.7 days uptime. This works due to modular arithmetic in unsigned integers. The subtraction yields the correct elapsed time regardless of rollover. This pattern should not be flagged as problematic or refactored to use signed arithmetic.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/index.js(1 hunks)wled00/wled_server.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/wled_server.cpp
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (wemos_shield_esp32_4MB_M)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_16MB_M_eth)
- GitHub Check: Builds (esp32_pico_4MB_M)
- GitHub Check: Builds (esp32_4MB_M_debug)
| function getURL(path) { | ||
| return (loc ? locproto + "//" + locip : "") + path; | ||
| } |
There was a problem hiding this comment.
Undefined locproto triggers a runtime exception
locproto is not declared anywhere in this file (and a quick scan shows no earlier definition).
When loc === true, calling getURL() will throw ReferenceError: locproto is not defined, breaking every navigation that relies on this helper.
- function getURL(path) {
- return (loc ? locproto + "//" + locip : "") + path;
+// Builds a URL that honours the page’s current protocol.
+function getURL(path) {
+ const proto = (typeof location !== 'undefined') ? location.protocol : 'http:';
+ return (loc ? proto + "//" + locip : "") + path;
}📝 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.
| function getURL(path) { | |
| return (loc ? locproto + "//" + locip : "") + path; | |
| } | |
| // Builds a URL that honours the page’s current protocol. | |
| function getURL(path) { | |
| const proto = (typeof location !== 'undefined') ? location.protocol : 'http:'; | |
| return (loc ? proto + "//" + locip : "") + path; | |
| } |
🤖 Prompt for AI Agents
In wled00/data/index.js around lines 208 to 210, the variable locproto used in
getURL is not defined, causing a ReferenceError when loc is true. To fix this,
declare and initialize locproto appropriately before using it in the return
statement, ensuring it holds the correct protocol string (e.g., "http:" or
"https:") based on the context or environment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@netmindz please have a look at the finding from rabbit. I have no idea about JS/HTML coding, so if you feel the code is good, just go ahead and merge into mdev. |
|
Are we going to use this one or swap to the more advanced tool from dedehi ? |
I think we can use the advanced tool from dedehi - i've done a quick test and it works, just the "scrolling text" tool is not working because WLED-MM does not support all the "tags" offered by upstream. |
3 participants
Port of wled#3921 to MoonModules WLED-MM
Also includes #226 for ease of testing
Summary by CodeRabbit
New Features
Improvements