Art-Net Improvements and Other Fixes #236
Conversation
WalkthroughThe changes span multiple areas of the codebase, focusing on enhancements to network and LED handling, UI updates, and robustness improvements. The audio UDP packet reception logic was refactored for better error handling and efficiency. The LED settings UI was updated to support a new "Art-Net RGBW (network)" type, with corresponding adjustments in validation and informational messages. Ethernet and WiFi connection management logic was refined to improve interface handling, including explicit WiFi disconnection when Ethernet is active. Art-Net universe handling was aligned with user-configurable settings. Several files received minor improvements, such as buffer size configuration for file operations and additional JavaScript variable exports for settings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsUI
participant LEDController
participant Network
participant WiFi
participant Ethernet
User->>SettingsUI: Selects "Art-Net RGBW (network)" LED type
SettingsUI->>SettingsUI: Shows Art-Net fields, validates input
SettingsUI->>LEDController: Submits settings (includes e131Universe)
LEDController->>Network: Updates Art-Net output universe
Network->>Ethernet: Handles SYSTEM_EVENT_ETH_GOT_IP
alt Ethernet active and AP off
Network->>WiFi: Disconnect WiFi
else Ethernet not active
Network->>WiFi: Keep WiFi as is
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)wled00/wled.cppTip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
wled00/data/settings_sync.htm (1)
83-83: Improved user interface headingChanged "Sync setup" to the more descriptive "Sync Interfaces" which better represents the purpose of this settings page.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
usermods/audioreactive/audio_reactive.h(2 hunks)wled00/data/settings_leds.htm(2 hunks)wled00/data/settings_sync.htm(3 hunks)wled00/file.cpp(4 hunks)wled00/network.cpp(2 hunks)wled00/udp.cpp(1 hunks)wled00/wled.cpp(1 hunks)wled00/xml.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wled00/xml.cpp (1)
wled00/fcn_declare.h (2)
oappend(377-377)oappendi(378-378)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32_16MB_M_eth)
- GitHub Check: Builds (wemos_shield_esp32_4MB_M)
- GitHub Check: Builds (esp32_4MB_S)
- GitHub Check: Builds (esp32_pico_4MB_M)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_4MB_XL)
- GitHub Check: Builds (esp32_4MB_M)
- GitHub Check: Builds (esp32_4MB_M_debug)
- GitHub Check: Builds (esp32_16MB_M)
🔇 Additional comments (15)
wled00/file.cpp (4)
69-71: Add buffer size configuration for ESP-IDF v4+This change sets a fixed buffer size (256 bytes) for file operations when running on ESP-IDF v4 or newer, preventing excessive RAM allocation during buffered file reads.
113-115: Consistent buffer size configuration for ESP-IDF v4+Applies the same buffer size fix to the
bufferedFindSpace()function for consistency with other similar functions.
159-161: Consistent buffer size configuration for ESP-IDF v4+Applies the same buffer size fix to the
bufferedFindObjectEnd()function.
186-188: Consistent buffer size configuration for ESP-IDF v4+Applies the same buffer size fix to the
writeSpace()function, completing the set of changes across all buffer-related file operations.wled00/network.cpp (2)
164-169: Add ESP-IDF v4 compatibility for Ethernet eventsMaps ESP-IDF v4 Ethernet event constants to maintain API compatibility between different versions. This allows the same code to work on both older and newer ESP-IDF versions.
179-190: Improve WiFi and Ethernet coexistenceAdds proper handling of the
ETH_GOT_IPevent to intelligently manage WiFi connections when Ethernet is active:
- If Ethernet is active and AP mode is not active, it disconnects WiFi to save resources
- If AP mode is active, it keeps WiFi enabled to maintain local access point functionality
This eliminates unnecessary dual network interfaces when using Ethernet.
wled00/wled.cpp (1)
1373-1381: Improve network interface management and user feedbackWhen connecting to a network, this change:
- Reports the connection type (Ethernet or WiFi) to the user
- When using Ethernet on ESP32, it actively disconnects WiFi to reduce resource usage
- Provides clearer feedback about which network interface is being used
This complements the changes in network.cpp, ensuring WiFi is properly disabled when Ethernet is active, improving system stability and resource utilization.
wled00/data/settings_sync.htm (2)
142-142: Simplified label for Network DMXChanged "Network DMX input" to simply "Network DMX" which is more accurate since this section can handle both input and output.
151-151: Added clarification for Start universe settingAdded the note "(Also used for sending Art-Net)" to clarify that this parameter serves a dual purpose - it's used for both receiving and sending Art-Net data. This supports the new Art-Net RGBW functionality mentioned in the PR objectives.
wled00/udp.cpp (1)
914-914: Improved Art-Net universe configuration by using e131Universe instead of hardcoded 0This change allows Art-Net output to start from the universe number defined in the Sync Setup rather than always starting at universe 0, providing better flexibility for users integrating with existing Art-Net setups.
wled00/xml.cpp (1)
490-493: Exposed e131Universe to JavaScript settingsThis addition exports the
e131Universevalue to the client-side JavaScript, making the Art-Net starting universe available to the LED settings page. This is essential for displaying the current universe configuration to users when setting up Art-Net outputs.wled00/data/settings_leds.htm (3)
426-426: Added Art-Net RGBW (network) LED typeAdded support for RGBW LEDs over Art-Net (value 83), complementing the existing RGB Art-Net option. This enhances the system's compatibility with RGBW LED strips over network control.
249-249: Improved clarity in Art-Net length mismatch warningThe warning message now explicitly shows both the total configured LEDs and the calculated number based on outputs × LEDs per output, making it easier for users to identify and fix configuration issues.
254-254: Added Art-Net universe configuration informationAdded an informational line showing the current Art-Net starting universe with a link to the Sync Interfaces settings page. This helps users understand the relationship between the Art-Net configuration and universe settings, improving the overall usability.
usermods/audioreactive/audio_reactive.h (1)
1881-1888:decodeAudioData_v1()lacks a success flag –haveFreshDatais alwaystrueFor v1 packets the method is
void, yet the caller setshaveFreshData = trueunconditionally.
IfdecodeAudioData_v1()later adds validation (or already silently fails), the caller will never know.
Recommend:-bool ok = decodeAudioData_v1(lastValidPacketSize, fftUdpBuffer); -haveFreshData = ok; +bool ok = decodeAudioData_v1(lastValidPacketSize, fftUdpBuffer); +haveFreshData = ok;(or give the v1 variant a boolean return).
| static uint8_t fftUdpBuffer[UDPSOUND_MAX_PACKET + 1] = {0}; | ||
| size_t lastValidPacketSize = 0; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Static UDP buffer can retain stale bytes and is 1 byte short for null‑termination
fftUdpBuffer is declared as UDPSOUND_MAX_PACKET + 1, which implies an intention to keep one extra byte for a terminating '\0'.
However, after every fftUdp.read() call the code never writes that trailing byte, so:
isValidUdpSyncVersion()receives a plainchar*that can over‑run into stale data beyondpacketSize.- Old payload data from a larger previous packet will remain in the tail when a smaller packet is received, potentially confusing later parsers/debuggers.
- fftUdp.read(fftUdpBuffer, packetSize);
+ fftUdp.read(fftUdpBuffer, packetSize);
+ if (packetSize < sizeof(fftUdpBuffer))
+ fftUdpBuffer[packetSize] = '\0'; // ensure clean termination
+ else
+ fftUdpBuffer[sizeof(fftUdpBuffer) - 1] = '\0';📝 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.
| static uint8_t fftUdpBuffer[UDPSOUND_MAX_PACKET + 1] = {0}; | |
| size_t lastValidPacketSize = 0; | |
| fftUdp.read(fftUdpBuffer, packetSize); | |
| if (packetSize < sizeof(fftUdpBuffer)) | |
| fftUdpBuffer[packetSize] = '\0'; // ensure clean termination | |
| else | |
| fftUdpBuffer[sizeof(fftUdpBuffer) - 1] = '\0'; |
| while (true) { | ||
| #if __cpp_exceptions | ||
| try { | ||
| packetSize = fftUdp.parsePacket(); | ||
| } catch(...) { | ||
| packetSize = 0; // low heap memory -> discard packet. | ||
| #ifdef ARDUINO_ARCH_ESP32 | ||
| fftUdp.flush(); // this does not work on 8266 | ||
| #endif | ||
| } catch (...) { | ||
| packetSize = 0; | ||
| #ifdef ARDUINO_ARCH_ESP32 | ||
| fftUdp.flush(); | ||
| #endif | ||
| DEBUG_PRINTLN(F("receiveAudioData: parsePacket out of memory exception caught!")); | ||
| USER_FLUSH(); | ||
| continue; // Skip to next iteration | ||
| } | ||
| #else | ||
| #else |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Packet‑drain loop can be simplified and avoids a latent sign/size bug
parsePacket()returns anint; assigning to the unsignedsize_t packetSizeand later comparing against0/UDPSOUND_MAX_PACKETrisks interpreting a negative error as a gigantic positive length.- The
while(true)pattern combined with the duplicatedparsePacket()call inside#elsemakes the flow harder to read.
Consider the canonical drain pattern:
-while (true) {
- #if __cpp_exceptions
- try {
- packetSize = fftUdp.parsePacket();
- } catch (...) { … }
- #else
- packetSize = fftUdp.parsePacket();
- #endif
- …
- if (packetSize == 0) break;
- …
-}
+int pLen;
+while ((pLen = fftUdp.parsePacket()) > 0) {
+ if (pLen > UDPSOUND_MAX_PACKET || pLen < 5) {
+ fftUdp.flush();
+ continue;
+ }
+ packetSize = static_cast<size_t>(pLen);
+ fftUdp.read(fftUdpBuffer, packetSize);
+ lastValidPacketSize = packetSize;
+}This removes the extra try, eliminates the unsigned cast pitfall, and clearly exits when the buffer is empty.
You can still keep the exception guard around the single parsePacket() call if exceptions are really enabled in your tool‑chain.
📝 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.
| while (true) { | |
| #if __cpp_exceptions | |
| try { | |
| packetSize = fftUdp.parsePacket(); | |
| } catch(...) { | |
| packetSize = 0; // low heap memory -> discard packet. | |
| #ifdef ARDUINO_ARCH_ESP32 | |
| fftUdp.flush(); // this does not work on 8266 | |
| #endif | |
| } catch (...) { | |
| packetSize = 0; | |
| #ifdef ARDUINO_ARCH_ESP32 | |
| fftUdp.flush(); | |
| #endif | |
| DEBUG_PRINTLN(F("receiveAudioData: parsePacket out of memory exception caught!")); | |
| USER_FLUSH(); | |
| continue; // Skip to next iteration | |
| } | |
| #else | |
| #else | |
| int pLen; | |
| while ((pLen = fftUdp.parsePacket()) > 0) { | |
| if (pLen > UDPSOUND_MAX_PACKET || pLen < 5) { | |
| fftUdp.flush(); | |
| continue; | |
| } | |
| packetSize = static_cast<size_t>(pLen); | |
| fftUdp.read(fftUdpBuffer, packetSize); | |
| lastValidPacketSize = packetSize; | |
| } |
| #ifdef ARDUINO_ARCH_ESP32 | ||
| if ((packetSize > 0) && ((packetSize < 5) || (packetSize > UDPSOUND_MAX_PACKET))) { | ||
| fftUdp.flush(); | ||
| continue; // Skip invalid packets | ||
| } | ||
| #endif |
There was a problem hiding this comment.
fftUdp.flush() guarded only for ESP32 – but 8266 needs it too
WiFiUDP::flush() exists on both cores.
When an over‑sized or too‑small packet arrives on an ESP8266 target, the branch is skipped, so the bogus packet will stay in the socket buffer and be re‑parsed every loop.
-#ifdef ARDUINO_ARCH_ESP32
- fftUdp.flush();
-#endif
+ fftUdp.flush(); // available on both ESP32 and ESP8266📝 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.
| #ifdef ARDUINO_ARCH_ESP32 | |
| if ((packetSize > 0) && ((packetSize < 5) || (packetSize > UDPSOUND_MAX_PACKET))) { | |
| fftUdp.flush(); | |
| continue; // Skip invalid packets | |
| } | |
| #endif | |
| if ((packetSize > 0) && ((packetSize < 5) || (packetSize > UDPSOUND_MAX_PACKET))) { | |
| fftUdp.flush(); // available on both ESP32 and ESP8266 | |
| continue; // Skip invalid packets | |
| } |
I really should clean my personal mdev, ha.
Improved Art-Net to have a starting universe and also improved some of the guidance in the Art-Net confg:
Summary of other things in this PR:
Summary by CodeRabbit
New Features
Improvements
Other Changes