Adding DDP over WS, moving duplicate WS-connection to common.js #4997
Conversation
- Enabling DDP over WebSocket: this allows for UI or html tools to stream data to the LEDs much faster than through the JSON API. - Moved the duplicate function to establish a WS connection from the live-view htm files to common.js
WalkthroughAdds client WebSocket helpers ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wled00/data/liveviewws2D.htm (1)
36-36: Pre-existing bug: Parameter name mismatch in message handler.The event listener parameter is named
e, but line 36 referencesevent.datainstead ofe.data. While this may work in some browsers due to the globaleventobject, it's not standard and could fail in strict mode or other environments.Apply this diff to fix the parameter reference:
- let leds = new Uint8Array(event.data); + let leds = new Uint8Array(e.data);Note: This bug exists in the original code, not introduced by this PR.
wled00/data/liveview.htm (1)
72-72: Pre-existing bug: Parameter name mismatch in message handler.Same issue as in
liveviewws2D.htm: the event listener parameter ise, but the code referencesevent.datainstead ofe.data.Apply this diff:
- let leds = new Uint8Array(event.data); + let leds = new Uint8Array(e.data);Note: This is a pre-existing bug, not introduced by this PR.
🧹 Nitpick comments (2)
wled00/ws.cpp (1)
74-83: Consider adding DDP header validation.The current implementation validates packet length but does not verify the DDP magic number or version fields. While the length checks prevent buffer overflows, adding validation of bytes 0-3 would improve robustness and help reject malformed packets early.
For example, verify the DDP header magic bytes before forwarding:
}else if (info->opcode == WS_BINARY) { // interpreted binary data using DDP protocol if (len < 10) return; // DDP header is 10 bytes + // Validate DDP magic number (0x41 = 'A', typically followed by version/flags) + if (data[0] != 0x41) return; size_t ddpDataLen = (data[8] << 8) | data[9]; // data length in bytes from DDP header if (len < (10 + ddpDataLen)) return; // not enough data, prevent out of bounds read // could be a valid DDP packet, forward to handler IPAddress clientIP = client->remoteIP(); e131_packet_t *p = (e131_packet_t*)data; handleE131Packet(p, clientIP, P_DDP); }wled00/data/common.js (1)
147-169: Consider adding error handling and return value propagation.The
sendDDPfunction correctly chunks data and constructs DDP headers, but could be more robust:
- The function returns
trueon success but doesn't catch exceptions fromws.send(), which could throw if the WebSocket is closing.- WebSocket send operations can fail silently when the buffer is full.
Consider wrapping the send operation in a try-catch:
pkt.set(colors.subarray(cOff, cOff + dLen), 10); - ws.send(pkt.buffer); + try { + ws.send(pkt.buffer); + } catch (err) { + console.error("DDP send failed:", err); + return false; + } } return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/data/common.js(1 hunks)wled00/data/liveview.htm(3 hunks)wled00/data/liveviewws2D.htm(2 hunks)wled00/ws.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/common.jswled00/data/liveviewws2D.htmwled00/data/liveview.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/common.jswled00/data/liveviewws2D.htmwled00/data/liveview.htm
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/ws.cpp
🧬 Code graph analysis (2)
wled00/data/common.js (1)
wled00/data/index.js (13)
e(1501-1501)l(223-223)pathn(233-233)paths(234-234)url(1415-1415)i(632-632)i(766-766)i(941-941)i(1473-1473)i(2212-2212)i(2219-2219)i(2236-2236)i(2819-2819)
wled00/ws.cpp (1)
wled00/e131.cpp (2)
handleE131Packet(57-120)handleE131Packet(57-57)
⏰ 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). (11)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (4)
wled00/data/common.js (2)
119-140: LGTM! WebSocket connection helper is well-designed.The
connectWsfunction properly handles:
- Reusing existing parent WebSocket connections
- Reverse proxy path detection (consistent with existing code in index.js)
- Binary frame support via
arraybuffertype- Optional initialization callbacks
153-153: DDP byte offset calculation is correctThe JS
off = (start + i) * 3sends the byte offset (pixel index × 3 bytes), and the server’shtonl(p->channelOffset)/ddpChannelsPerLedconversion aligns with the DDP header interpretation. No changes required.wled00/data/liveviewws2D.htm (1)
30-32: LGTM! Clean integration with connectWs.The refactored WebSocket initialization properly uses the new
connectWshelper fromcommon.jsand sends the liveview message immediately upon connection.wled00/data/liveview.htm (1)
65-68: LGTM! Consistent refactoring to use connectWs.The WebSocket initialization has been properly refactored to use the new
connectWshelper, making the code more maintainable and eliminating duplication.
wled00/ws.cpp
Outdated
| size_t ddpDataLen = (data[8] << 8) | data[9]; // data length in bytes from DDP header | ||
| if (len < (10 + ddpDataLen)) return; // not enough data, prevent out of bounds read | ||
| // could be a valid DDP packet, forward to handler | ||
| IPAddress clientIP = client->remoteIP(); |
There was a problem hiding this comment.
realtimeIP = client->remoteIP();
wled00/ws.cpp
Outdated
| // could be a valid DDP packet, forward to handler | ||
| IPAddress clientIP = client->remoteIP(); | ||
| e131_packet_t *p = (e131_packet_t*)data; | ||
| handleE131Packet(p, clientIP, P_DDP); |
There was a problem hiding this comment.
handleDDPPacket((e131_packet_t*)data);
There was a problem hiding this comment.
I did that at first, then wanted to support the other protocols and realized they cannot be destinguished reliably just by the data and additional variables over JSON would be needed. Since I only have use for DDP I dropped the others but kept the function call to set the IP, but setting the IP directly and calling handleDDPPacket makes a lot more sense. Thanks for the review.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/data/common.js(1 hunks)wled00/data/liveview.htm(3 hunks)wled00/e131.cpp(1 hunks)wled00/ws.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/data/common.js
- wled00/ws.cpp
🧰 Additional context used
📓 Path-based instructions (3)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/e131.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/liveview.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/liveview.htm
🧠 Learnings (2)
📚 Learning: 2025-10-10T16:33:28.649Z
Learnt from: DedeHai
PR: wled/WLED#4997
File: wled00/ws.cpp:74-83
Timestamp: 2025-10-10T16:33:28.649Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), malformed packets must be rejected with comprehensive validation: check timecode flag requires ≥14 bytes total, enforce DDP spec limit of 1440 bytes max data length, and validate that loop iterations won't exceed available data by calculating max index (c + (numLeds-1)*channelsPerLed + channelsPerLed - 1) before accessing the data array.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-08-21T00:00:17.196Z
Learnt from: willmmiles
PR: wled/WLED#4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
Applied to files:
wled00/e131.cpp
⏰ 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). (1)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (3)
wled00/data/liveview.htm (3)
72-72: LGTM! Correct usage of ArrayBuffer.The conversion from
e.datatoUint8Arrayis correct, assuming thebinaryTypeverification passes (see previous comment).
69-80: No action required: binaryType is set
common.js setsws.binaryType = "arraybuffer", so the ArrayBuffer type check in liveview.htm will work as expected.
20-20: common.js verification completecommon.js is present at wled00/data/common.js, exports the
connectWsfunction, and correctly setsws.binaryType = "arraybuffer". No further action needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
wled00/data/liveviewws2D.htm (2)
30-32: Avoid capturing ws in onOpen; prefer passing socket and JSON.stringify.If connectWs invokes the callback synchronously, ws may still be undefined. Also prefer JSON.stringify to avoid JSON syntax issues.
Consider this, if connectWs can pass the socket:
- var ws = connectWs(()=>{ - ws.send('{"lv":true}'); - }); + var ws = connectWs((socket)=>{ + socket.send(JSON.stringify({ lv: true })); + });If connectWs does not pass the socket, please confirm the callback is always deferred until after ws is assigned (onopen). Based on learnings.
36-36: Ensure binaryType='arraybuffer' and avoid shadowing.Require connectWs to set ws.binaryType = 'arraybuffer' so e.data is an ArrayBuffer. Also avoid shadowing the outer leds variable for clarity.
- let leds = new Uint8Array(e.data); - if (leds[0] != 76 || leds[1] != 2 || !ctx) return; //'L', set in ws.cpp - let mW = leds[2]; // matrix width - let mH = leds[3]; // matrix height + const buf = new Uint8Array(e.data); + if (buf[0] != 76 || buf[1] != 2 || !ctx) return; //'L', set in ws.cpp + let mW = buf[2]; // matrix width + let mH = buf[3]; // matrix height let pPL = Math.min(c.width / mW, c.height / mH); // pixels per LED (width of circle) let lOf = Math.floor((c.width - pPL*mW)/2); //left offset (to center matrix) var i = 4; for (y=0.5;y<mH;y++) for (x=0.5; x<mW; x++) { - ctx.fillStyle = `rgb(${leds[i]},${leds[i+1]},${leds[i+2]})`; + ctx.fillStyle = `rgb(${buf[i]},${buf[i+1]},${buf[i+2]})`; ctx.beginPath(); ctx.arc(x*pPL+lOf, y*pPL, pPL*0.4, 0, 2 * Math.PI); ctx.fill(); i+=3; }wled00/data/liveview.htm (2)
20-20: Indent with tabs in web UI files.The new line is indented with spaces. Please switch to tabs for files under wled00/data.
As per coding guidelines.
65-68: Open-callback pattern + JSON formattingGood: JSON now uses double quotes. To avoid any race with ws assignment and ensure correctness, prefer passing the socket and using JSON.stringify.
- ws = connectWs(function () { - //console.info("Peek WS open"); - ws.send('{"lv":true}'); - }); + ws = connectWs(function (socket) { + //console.info("Peek WS open"); + socket.send(JSON.stringify({ lv: true })); + });If connectWs doesn’t pass the socket, please confirm the callback is guaranteed to run after ws is assigned (onopen).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/liveview.htm(3 hunks)wled00/data/liveviewws2D.htm(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/liveviewws2D.htmwled00/data/liveview.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/liveviewws2D.htmwled00/data/liveview.htm
⏰ 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). (2)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (3)
wled00/data/liveviewws2D.htm (1)
13-13: Good move to centralize WS logic; remember the UI build step.Including common.js to dedupe WS connection is 👍. Please run npm run build to regenerate embedded headers before firmware build.
As per coding guidelines.
wled00/data/liveview.htm (2)
72-72: Confirm binaryType is set to arraybuffer.Ensure connectWs sets ws.binaryType = 'arraybuffer'; otherwise e.data may be a Blob and the ArrayBuffer path won’t run.
87-87: No issues.Closing tag change is fine.
wled00/data/common.js
Outdated
| return ws; | ||
| } | ||
|
|
||
| // send led colors to ESP using WebSocket and DDP protocol (RGB) |
wled00/ws.cpp
Outdated
| }else if (info->opcode == WS_BINARY) { | ||
| // interpreted binary data using DDP protocol | ||
| if (len < 10) return; // DDP header is 10 bytes | ||
| size_t ddpDataLen = (data[8] << 8) | data[9]; // data length in bytes from DDP header | ||
| uint8_t flags = data[0]; | ||
| if ((flags & DDP_TIMECODE_FLAG) ) ddpDataLen += 4; // timecode flag adds 4 bytes to data length | ||
| if (len < (10 + ddpDataLen)) return; // not enough data, prevent out of bounds read | ||
| // could be a valid DDP packet, forward to handler | ||
| handleE131Packet((e131_packet_t*)data, client->remoteIP(), P_DDP); |
There was a problem hiding this comment.
This is the only bit that makes me twitchy, the assumption that any binary messages are going to be DDP
I would prefer we had a way to indicate what this data is, with DDP being the first (and only) supported type rather than assume that we can "detect" the format and do some magic later when there are more than DDP being send as binary
There was a problem hiding this comment.
I am no expert in protocol definition. How to solve it?
Detecting protocols by header as it is (crudely and incompletely) implemented is the simplest version. A AR sync packet could be detected that way as well. Adding another "header" or "request" layer on top could do more harm than good: what happens if two clients connect, one sending DDP one sending AR data for example?
edit:
if in the future, custom needs arise for a completely different protocol, it could also use a header OR do a UI config override: if "some special protocol" is used, disable all other protocol support.
if past experience has tought me anything it is not to try to predict the future of data usage ;)
There was a problem hiding this comment.
I've just checked the WebSocket protocol docs and you can send a Sec-WebSocket-Protocol http header, which if we used that then you also have a mechanism to know if the server supports what you are trying to send.
That doesn't however resolve the challenge of knowing when you get a binary message, which function to delegate to.
While I take your point about predicting the future is hard, at the same point, writing something in a way that prevents further extension later is bad. I still think it would be worth adding a single byte at the start that indicates what the message type is
There was a problem hiding this comment.
i.e the first byte maps to the function, which might itself support multiple messages
There was a problem hiding this comment.
adding a header byte would work for me and since no applications exist yet that use binary streaming over WS there are no compatibility issues either. should I implement this approach? say something like:
0 = generic (future implementation or auto detect from following bytes?)
1 = DDP
2 - 9 = reserved for other LED streaming protocols
11 = AR packet (not yet supported)
There was a problem hiding this comment.
Not quite sure how auto detection would work, but reserving 0 for that sounds a good idea, beyond that do we already have any constant that identifiers DDP?
I would avoid trying to allocate any particular values for future use, the AR example was just the first hypothetical that came to mind, I'm not sure it would be a good idea as latency is more important than guaranteed delivery, so possibly inadvisable, but possibly only option of anyone was to want to try and do something entirely in the browser.
There might be other usermods however that would benefit from high speed TCP rather than hammering the API
There was a problem hiding this comment.
Not quite sure how auto detection would work
for DDP or AR packets for example by looking at each packets header and finding unique identifiers, but this was just a thought, no real usecase in mind. I will go ahead and implement a minimal version
There was a problem hiding this comment.
You definitely could do some form of auto detection, my query was more how to ensure proper encapsulation and separation of concerns within the code if we did that.
For now let's just keep to the simple if ID btye indicates DDP, delegate to the relevant function, otherwise error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/ws.cpp (1)
84-99: Add break statement and default case to switch.The switch statement has two style issues:
- Missing
break;after the DDP case (line 99) - while not a bug here, explicit break statements improve code clarity and prevent future errors- Missing
default:case - unknown protocol bytes are silently ignoredApply this diff:
case BINARY_PROTOCOL_DDP: if (len < 10 + offset) return; // DDP header is 10 bytes (+1 protocol byte) size_t ddpDataLen = (data[8+offset] << 8) | data[9+offset]; // data length in bytes from DDP header uint8_t flags = data[0+offset]; if ((flags & DDP_TIMECODE_FLAG) ) ddpDataLen += 4; // timecode flag adds 4 bytes to data length if (len < (10 + offset + ddpDataLen)) return; // not enough data, prevent out of bounds read // could be a valid DDP packet, forward to handler handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_DDP); + break; + default: + DEBUG_PRINTF_P(PSTR("WS binary: unsupported protocol %u\n"), data[0]); + return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/common.js(1 hunks)wled00/ws.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/data/common.js
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/ws.cpp
🧠 Learnings (2)
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
PR: wled/WLED#4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/ws.cpp
📚 Learning: 2025-08-21T00:00:17.196Z
Learnt from: willmmiles
PR: wled/WLED#4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
Applied to files:
wled00/ws.cpp
🧬 Code graph analysis (1)
wled00/ws.cpp (1)
wled00/e131.cpp (2)
handleE131Packet(65-128)handleE131Packet(65-65)
⏰ 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). (14)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (4)
wled00/ws.cpp (4)
34-34: LGTM: Improved documentation of frame size limits.The updated comment clarifies platform-specific WebSocket frame size limits, which is helpful for understanding the constraints.
91-98: LGTM: DDP bounds validation logic is sound.The bounds checking correctly prevents out-of-bounds memory access:
- Line 92 ensures minimum header size (10 bytes + protocol byte)
- Lines 93-95 compute total data length, accounting for optional timecode (4 bytes)
- Line 96 validates the full packet length before forwarding to the handler
This aligns with the established approach of preventing out-of-bounds access rather than enforcing DDP spec compliance.
Based on learnings.
102-102: LGTM: Enhanced debug logging for multipart messages.The additional debug information (final, index, len, total) will be helpful for troubleshooting multipart message handling.
8-12: Validate input bounds for E131/ARTNET handlers; add default case for protocol switch.The switch statement handles binary protocols at lines 84-99. While the DDP case includes bounds validation (lines 92, 96), the E131 and ARTNET cases directly invoke handlers without checking packet size first. Given these protocols are marked untested and per prior learnings to validate packet size before access, verify that
handleE131Packet()performs adequate input validation for all three protocol types.Additionally, the switch lacks a default case to handle unknown protocol bytes—any unrecognized value silently falls through. Add a default case to log unexpected protocols:
switch (data[0]) { case BINARY_PROTOCOL_E131: handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_E131); break; case BINARY_PROTOCOL_ARTNET: handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_ARTNET); break; case BINARY_PROTOCOL_DDP: if (len < 10 + offset) return; size_t ddpDataLen = (data[8+offset] << 8) | data[9+offset]; uint8_t flags = data[0+offset]; if ((flags & DDP_TIMECODE_FLAG)) ddpDataLen += 4; if (len < (10 + offset + ddpDataLen)) return; handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_DDP); break; default: DEBUG_PRINTF_P(PSTR("WS binary: unsupported protocol %u\n"), data[0]); return; }
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/common.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/common.js
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/common.js
🧬 Code graph analysis (1)
wled00/data/common.js (1)
wled00/data/index.js (10)
e(1501-1501)url(1415-1415)i(632-632)i(766-766)i(941-941)i(1473-1473)i(2212-2212)i(2219-2219)i(2236-2236)i(2819-2819)
🔇 Additional comments (1)
wled00/data/common.js (1)
136-175: No issues found—server-side DDP packet handling is already correct.The verification confirms that ws.cpp and e131.cpp correctly handle the packet structure from sendDDP:
- ws.cpp (lines 83-98) correctly skips the protocol byte at offset 0 and casts
&data[offset]as the 10-byte DDP header (e131_packet_t*).- ws.cpp line 92 validates minimum packet size:
if (len < 10 + offset) return;covers the header.- e131.cpp handleDDPPacket (lines 40-44) performs proper bounds checking:
if (maxDataIndex > dataLen) return;and correctly handles the timecode flag offset.The client sends 11 bytes (1 protocol indicator + 10-byte DDP header) followed by RGB data, and the server-side parsing correctly interprets this structure. No changes are needed.
|
Last two rabbit suggestions look fair, but otherwise I think we are good to merge |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
wled00/data/common.js (4)
121-135: Handle CONNECTING reuse and clear stale reference on close.Avoid spawning a second WS while an existing one is CONNECTING; also clear top.window.ws when that socket closes. Prefer addEventListener over onopen to not clobber handlers.
function connectWs(onOpen) { try { - if (top.window.ws && top.window.ws.readyState === WebSocket.OPEN) { - if (onOpen) onOpen(); - return top.window.ws; - } + if (top.window.ws) { + if (top.window.ws.readyState === WebSocket.OPEN) { + if (onOpen) onOpen(); + return top.window.ws; + } + if (top.window.ws.readyState === WebSocket.CONNECTING) { + if (onOpen) top.window.ws.addEventListener('open', onOpen, { once:true }); + return top.window.ws; + } + } } catch (e) {} getLoc(); // ensure globals (loc, locip, locproto) are up to date - let url = loc ? getURL('/ws').replace("http","ws") : "ws://"+window.location.hostname+"/ws"; - let ws = new WebSocket(url); - ws.binaryType = "arraybuffer"; - if (onOpen) { ws.onopen = onOpen; } + const url = loc ? getURL('/ws').replace("http","ws") : "ws://"+window.location.hostname+"/ws"; + const ws = new WebSocket(url); + ws.binaryType = "arraybuffer"; + if (onOpen) ws.addEventListener('open', onOpen, { once:true }); + ws.addEventListener('close', () => { try { if (top.window.ws === ws) top.window.ws = null; } catch(e) {} }); try { top.window.ws = ws; } catch (e) {} // store in parent for reuse return ws; }
142-147: Validate arguments and enforce Uint8Array input.Guard against negative/zero lengths and wrong buffer types; keep existing length check.
function sendDDP(ws, start, len, colors) { - if (!colors || colors.length < len * 3) return false; // not enough color data - let maxDDPpx = 472; // must fit into one WebSocket frame of 1428 bytes, DDP header is 10+1 bytes -> 472 RGB pixels + if (!Number.isInteger(start) || start < 0 || !Number.isInteger(len) || len <= 0) return false; + if (!(colors instanceof Uint8Array) || colors.length < len * 3) return false; // not enough color data + const maxDDPpx = 472; // must fit into one WebSocket frame of 1428 bytes, DDP header is 10+1 bytes -> 472 RGB pixels //let maxDDPpx = 172; // ESP8266: must fit into one WebSocket frame of 528 bytes -> 172 RGB pixels TODO: add support for ESP8266? if (!ws || ws.readyState !== WebSocket.OPEN) return false;Based on learnings.
154-165: Typos in comments and small const nit.Fix minor typos; prefer const where values don’t change.
- let pkt = new Uint8Array(11 + dLen); // DDP header is 10 bytes, plus 1 byte for WLED websocket protocol indicator + const pkt = new Uint8Array(11 + dLen); // DDP header is 10 bytes, plus 1 byte for WLED WebSocket protocol indicator pkt[0] = 0x02; // DDP protocol indicator for WLED websocket. Note: below DDP protocol bytes are offset by 1 pkt[1] = 0x40; // flags: 0x40 = no push, 0x41 = push (i.e. render), note: this is DDP protocol byte 0 pkt[2] = 0x00; // reserved pkt[3] = 0x01; // 1 = RGB (currently only supported mode) pkt[4] = 0x01; // destination id (not used but 0x01 is default output) - pkt[5] = (off >> 24) & 255; // DDP protocl 4-7 is offset + pkt[5] = (off >> 24) & 255; // DDP protocol 4-7 is offset pkt[6] = (off >> 16) & 255; pkt[7] = (off >> 8) & 255; pkt[8] = off & 255; - pkt[9] = (dLen >> 8) & 255; // DDP protocol 8-9 is data length + pkt[9] = (dLen >> 8) & 255; // DDP protocol 8-9 is data length pkt[10] = dLen & 255;
119-177: Remember to rebuild embedded web assets.After changing files under wled00/data, run: npm run build before firmware build so headers get regenerated. As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/common.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/common.js
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/common.js
🧠 Learnings (1)
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
PR: wled/WLED#4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/data/common.js
🧬 Code graph analysis (1)
wled00/data/common.js (1)
wled00/data/index.js (10)
e(1501-1501)url(1415-1415)i(632-632)i(766-766)i(941-941)i(1473-1473)i(2212-2212)i(2219-2219)i(2236-2236)i(2819-2819)
⏰ 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). (17)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/data/common.js (1)
142-177: Bounds validation properly added; consider optional type-safety improvement.The previous bounds-checking issue has been correctly addressed at line 143. The function now validates sufficient color data before processing.
The DDP packet construction, chunking logic, and error handling are all correct. The implementation properly sets the push flag on the final packet to trigger rendering.
Optional improvement: The validation at line 143 checks
colors.lengthbut doesn't verify thatcolorshas asubarraymethod. While the function documentation specifies a Uint8Array parameter, adding a simple check liketypeof colors?.subarray !== 'function'would provide a clearer error message if the wrong type is passed, rather than throwing at line 165.Optional type-safety improvement
function sendDDP(ws, start, len, colors) { - if (!colors || colors.length < len * 3) return false; // not enough color data + if (!colors || typeof colors.subarray !== 'function' || colors.length < len * 3) return false; // not enough color data
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/common.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/common.js
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/common.js
🧠 Learnings (1)
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
PR: wled/WLED#4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/data/common.js
🧬 Code graph analysis (1)
wled00/data/common.js (1)
wled00/data/index.js (10)
e(1501-1501)url(1415-1415)i(632-632)i(766-766)i(941-941)i(1473-1473)i(2212-2212)i(2219-2219)i(2236-2236)i(2819-2819)
🔇 Additional comments (1)
wled00/data/common.js (1)
119-135: LGTM! WebSocket reuse properly implemented.The previous critical issue has been correctly fixed. The WebSocket is now stored in
top.window.wsat line 133, enabling proper connection reuse across subsequent calls. The try-catch blocks appropriately handle potential cross-origin access restrictions.
3 participants
uses only 100bytes of additional flash and is a feature I could make use of in the matrix tool.
Also the new custom palette editor has a "preview" feature that currently uses the JSON API to do a live-preview on the LEDs. Using DDP instead would make it much faster.
I ran some experiments doing video streaming from a ESP-Hosted webpage to the LEDs and this also did work quite well.
Summary by CodeRabbit
New Features
Performance
Compatibility
Bug Fixes
Refactor