Repository navigation
Conversation
- parseNotifyPacket() now receives the packet length: enforce a minimum length per protocol version, require a sane segment record stride and ignore segment data unless all declared segments fit in the packet (previously a short packet could make it read past the receive buffer) - notify() only sends the segments actually packed, and caps the UDP datagram at 1460 bytes (39 segments): arduino-esp32 WiFiUDP flushes every 1460 bytes, so PSRAM builds (64 segments, 2345 bytes) emitted a second datagram starting with arbitrary buffer content - retries no longer extend the 1 s receive hold-off, retries also run when syncing via ESP-NOW only, and a received notification for one of our send groups cancels pending retries - ESP-NOW: hand the reassembled message to the main loop instead of applying it from the QuickEspNow receive task (which on ESP32 runs concurrently with the main loop); reject a first fragment shorter than the 41-byte header Refs wled#5952. Partially AI generated (marked with // AI: comments). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Outbound packet sizing and retries wled00/udp.cpp |
Notifier sends cap segment records to those that fit in a 1460-byte datagram and advertise the packed record count. Retry timing is tracked separately from the initial-send time. |
Incoming packet and segment validation wled00/udp.cpp |
The parser checks packet length, supported versions, and segment record bounds before applying data. Matching sync-group notifications cancel pending retries. |
ESP-NOW reassembly and main-loop parsing wled00/udp.cpp |
Receive callbacks queue completed messages with their lengths for main-loop parsing. Invalid, replaced, or rate-limited buffers are freed. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant Callback as ESP-NOW receive callback
participant Buffer as Pending sync buffer
participant MainLoop as Main loop
participant Parser as parseNotifyPacket
Callback->>Buffer: Store completed message and length
MainLoop->>Buffer: Take pending message
MainLoop->>Parser: Parse message with stored length
MainLoop->>Buffer: Free parsed message
Possibly related issues
- wled/WLED#5942 — The PR validates packet lengths and ignores incomplete segment records, matching the issue’s short-packet concern.
- wled/WLED#5951 — The PR limits notifications to one 1460-byte datagram and sends only packed segments.
- wled/WLED#5952 — The PR changes the legacy UDP sync packet, retry, and ESP-NOW handling described in the issue.
Possibly related PRs
- wled/WLED#5932 — Both PRs harden UDP notifier receive parsing and ESP-NOW message assembly.
Suggested reviewers: dedehai
Merge Risk: 
68075
-
No new merge-blocking issue was established. Hardware testing of the notifier changes remains advisable.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title accurately and clearly summarizes the main change: hardening the legacy UDP sync notifier send and receive paths. The “POC” prefix is additional context but does not make the title misleadin… Full details: Title check
-
Explanation
The title accurately and clearly summarizes the main change: hardening the legacy UDP sync notifier send and receive paths. The “POC” prefix is additional context but does not make the title misleading.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
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.
Member
softhack007
marked this pull request as draft
November 3, 2026 16:12
1 task done
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.
This is Phase 0 of #5952. It hardens the existing (legacy v12) UDP sync notifier, independently of any future "sync v2". New firmware will keep receiving legacy packets, so this path has to be safe either way.
All changes are in
wled00/udp.cpp.Changes
Receive: bounds checking (
parseNotifyPacket)parseNotifyPacket()now receives the packet length. Before, it read fixed offsets up to byte 40, plus segment records whose count and stride come from the sender (udpIn[39]/udpIn[40]). That loop was bounded only againstUDP_IN_MAXSIZE, not the real packet size, so it could read past the receive buffer.read()returning -1.Send (
notify)WLEDPACKETSIZEbuffer went out regardless: 617 bytes on ESP8266, 1193 on ESP32 and 2345 on PSRAM builds, with uninitialised stack after the active segments.WiFiUDP::write()sends what it has as a datagram each time its 1460-byte tx buffer fills, then starts a new one.1–5) or UDP API input, depending on that byte.Retries
notificationSentTime, so with e.g. 29 retries a node ignored the whole group for about 8 s after its own change. A separatenotificationRetryTimenow drives the retry interval.udpConnected.ESP-NOW
handleNotifications()) instead of being applied directly inespNowReceiveCB().receive_handleFreeRTOS task. That task has the same priority and core asloopTask, so segments could be modified while the main loop was using them.std::atomicpointer exchange.os_timer, which never preempts the main loop, so a plain pointer swap is safe.User-visible behaviour changes
Deliberately not included
handleNotifications().udpIn[UDP_IN_MAXSIZE+1]made the function's stack frame grow from 176 to 1616 bytes on every call on ESP8266 (checked in the disassembly).packetSize > UDP_IN_MAXSIZEcheck just above it, so I left it as is.udpOut. Every byte that is sent is now written, so it is no longer needed.Testing
nodemcuv2,esp8266_2m_min(ESP-NOW disabled),esp32devandesp32s3dev_8MB_opi(PSRAM). There are no new warnings, andnpm testpasses.WLED_DEBUGserial log for crashes. I can share a small Python script that does this.AI disclosure
This PR is partially AI-generated with Claude Code. AI-written blocks are marked with
// AI: below section was generated by an AI/// AI: end. The analysis and the changes were reviewed against the source.🤖 Generated with Claude Code
Summary by CodeRabbit