Repository navigation
(WIP) esp32: always flush WiFiUDP rx buffer on early exit (fixes #5929) - #5932
softhack007 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
WalkthroughThe change adds platform-specific UDP receive-buffer cleanup for NTP handling and drains notification packets that are oversized, disabled, or self-originated before returning. ChangesUDP receive-buffer cleanup
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Possibly related issues
Suggested labels: Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main change: clearing or flushing the ESP32 WiFiUDP receive buffer on early exit to address issue #5929. The wording is concise and directly related to the changes.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.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
@coderabbitai please create a testplan for this change, also covering scenarios where message multiplication from multiple masters on the same subnet must be prevented. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr diff 5932 --repo wled/WLED
gh issue view 5929 --repo wled/WLED --json title,body
ast-grep outline wled00/udp.cpp --items all
ast-grep outline wled00/ntp.cpp --items all
rg -n -C5 'udpDropRXBuffer|CALL_MODE_NOTIFICATION|notificationSentTime|notificationCount|UDP_IN_MAXSIZE' wled00/udp.cpp wled00/ntp.cpp wled00/fcn_declare.hLength of output: 19641 🏁 Script executed: #!/bin/bash
sed -n '20,58p' wled00/udp.cpp
sed -n '175,206p' wled00/udp.cpp
sed -n '529,605p' wled00/udp.cpp
sed -n '197,254p' wled00/ntp.cpp
sed -n '255,354p' wled00/ntp.cpp
rg -n -C5 'void stateUpdated\(|notify\(callMode|udpNumRetries|NTP_PACKET_SIZE' wled00 --glob '*.{cpp,h}' | head -180Length of output: 21236
Status: Source inspection only. These tests have not been run. 1. SetupUse one isolated subnet with:
Test these firmware variants:
Run the main reproduction on the parent revision and the PR revision. Use the same hardware and configuration. Configure A and B to send and receive sync notifications. Configure C as receive-only. Use matching sync groups. Disable playlists, timers, MQTT automation, and other sources of state changes during packet-count tests. Disable ESP-NOW to isolate UDP. Record the actual ports. The source defaults are:
Capture traffic at the access point or another location that sees all node transmissions. A capture on an ordinary Wi-Fi client might not see all traffic. 2. Expected anti-loop behaviorThe supplied UDP-sync learning agrees with the inspected code:
The timeout is not the only protection against multiplication. A received notification must not cause a new sync transmission, even after the timeout expires. Each retry updates Node-info announcements can occur after brightness changes. Count those separately from sync notifications. 3. Receive-buffer recoveryFor each case, send the rejected packet first. Then send a known-good packet on the same socket, without a reboot or reconnect. Wait until any sync suppression window has expired before checking sync acceptance.
For RX-1 and RX-7, verify that the self-originated packet reaches the node. A test without self-delivery does not exercise the relevant guard. 4. Multiple-master testsStart each packet-count test after boot traffic and previous retries have stopped. First use For an isolated local change with Receiving nodes must add zero sync transmissions. Count packets by source IP and protocol, not only by destination port.
Do not require a deterministic final color after simultaneous conflicting changes. The suppression window can leave masters with different states. After the window expires, send one new change from a selected master and confirm that the other nodes receive it. For MM-7, do not apply the isolated-event packet-count formula. New local changes can replace an unfinished retry sequence. 5. NTP regression testsUse a controlled NTP server. Keep time sync pending when testing recovery.
6. Soak test and acceptance criteriaRun a one-hour mixed workload. Include multi-master changes, rejected datagrams, valid raw RGB frames, and NTP responses. Keep injection rates bounded so that queue overload does not hide the result. Record:
Accept the change when:
The original receive stall does not by itself prove a heap leak. Report heap behavior separately from receive recovery. 🧠 Learnings usedYou are interacting with an AI system. |
There was a problem hiding this comment.
🔵 Needs a closer look
ESP32 multi-device testing is still needed to confirm continued reception and effective UDP SYNC flood suppression.
No open issues 🎉
What changed in this PR
This PR addresses #5929 by clearing unread ESP32 UDP receive buffers that can block subsequent packets, while preserving ESP8266 behavior.
Changes:
- Adds a platform-aware helper using
clear()orflush()on ESP32. - Clears rejected packets in notification and realtime UDP handling.
- Clears retained NTP data before requests and after partially read responses.
| File | Description |
|---|---|
wled00/udp.cpp |
Discards receive buffers when rejecting UDP packets. |
wled00/ntp.cpp |
Clears retained NTP buffers and unread response bytes. |
wled00/fcn_declare.h |
Defines the ESP32-specific receive-buffer cleanup helper. |
Review effort: Balanced
Aircoookie
left a comment
Aircoookie
left a comment
There was a problem hiding this comment.
I believe this is a very valuable fix, though with initial testing I have been so far unable to reproduce an unpatched ESP32 sync master (both with 16.0.1 and main) entering a blocked state where it ignores incoming sync packets (and potentially leak memory)
| #ifdef ARDUINO_ARCH_ESP32 // I had problems using udp.flush() on 8266 | ||
| #if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 0) | ||
| while (ntpUdp.parsePacket() > 0) ntpUdp.clear(); // flush() is deprecated in arduino-esp32 3.x.y | ||
| ntpUdp.clear(); while (ntpUdp.parsePacket() > 0) ntpUdp.clear(); // flush() is deprecated in arduino-esp32 3.x.y |
There was a problem hiding this comment.
What is the reason for adding the unconditional clear() for clearing the ntpUdp socket? Logically, I don't understand it fully - the second one as part of the parse loop should suffice?
There was a problem hiding this comment.
I think the intent here is to flush any unread packets prior to sending a new NTP poll.
There was a problem hiding this comment.
Yes, unread (rejected or partially read) packets cause parsePacket()==0 so the while loop won't trigger. Calling clear() is always allowed even when there is no packet waiting.
I could change it into a do { clear(); } while parsePacket() > 0 to make the code a bit clearer.
| #else | ||
| if (cb > NTP_PACKET_SIZE) ntpUdp.flush(); | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
This should be if (cb > NTP_PACKET_SIZE) udpDropRXBuffer(ntpUdp);. In fact you could even do this without any #ifdefs as dead code elimination will remove it on ESP8266.
There was a problem hiding this comment.
True, ifdef soup is everywhere ... I'll change it
| size_t packetSize = notifierUdp.parsePacket(); | ||
| if (packetSize > UDP_IN_MAXSIZE) udpDropRXBuffer(notifierUdp); // too big |
There was a problem hiding this comment.
It might be good to wrap this pattern up in another little helper function. It might even be worth going all the way to a full read-to-buffer-if-ready call -- it'd clean up the logic here quite a bit, I think.
size_t udpRead(AsyncUDP& udp_socket, uint8_t* buf, size_t buf_size) {
size_t result = udp_socket.parsePacket();
if (result > buf_size) {
udpDropRXBuffer(udp_socket); // discard oversize packet
return 0;
}
if (result > 0) {
udp_socket.read(buf, buf_size);
}
return result;
}
|
@Aircoookie thanks, the main precondition for the blocked state is that your router allows broadcast-to-self, I think modern routers can filter this out. We had bug reports where all esp32 in the network act as sync masters, and stopped reacting on sync messages for example after being controlled by HyperHDR or FFP. The main idea of the fix is from wled-mm, however from a time when WiFiUDP used to leak memory when there was no "read" or "flush". We were hunting for leaks at the time, and adding "flush()" to every early return stabilised it. So the basic idea works, similar code is running in WLED-MM for some time already. I'm just not sure about the message flooding scenario, that requires several esp32 to multiply sync messages to each other. |
see analysis in #5929
needs testing, especially to confirm that the "broadcast ignore" timeout for UDP SYNC is always sufficient to prevent message floods. Reason: the bug also had a side-effect of making the sync master (esp32) ignore all future sync broadcasts after the first self-adressed broadcast was received.