new usermod hooks "onUdpPacket" #4859
Conversation
this new hooks will help you implement new and custom protocols in usermods. I've provided an example (see usermods/udp_name_sync). The example will help you share the main segment name across different WLED instances. The segment name can be useful to sync with some effects like GIF image or scrolling text. If you define new packet format in your usermod, make sure it will either not collide with already used version of wled udp packet : - 0 is for udp sync - 1 is for AudioReactive data - 2 is for udp_name_sync :) Also, the onUdpPacket will override "parseNotification" if it returns "true". Have fun!
WalkthroughAdds a UDP name-sync usermod and library manifest; introduces a Usermod UDP packet callback plus UsermodManager dispatcher using Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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). (18)
✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
this is a rework of a previous PR : #4468 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
usermods/udp_name_sync/library.json (1)
1-5: Add basic manifest metadata (version/description) to aid PIO caching and discoverabilityThe manifest works as-is, but adding a version and short description helps PlatformIO and future maintenance.
Apply this diff:
{ "name": "udp_name_sync", + "version": "0.1.0", + "description": "Usermod to synchronize the main segment name across WLED instances over UDP.", "build": { "libArchive": false }, "dependencies": {} }wled00/fcn_declare.h (1)
445-446: Optional: pass sender metadata to usermodsMany protocols need sender IP/port to filter or authenticate. Consider expanding the signature later to include remote IP/port while keeping the current overload for backwards compatibility.
wled00/udp.cpp (1)
560-562: Be explicit about override scope of the hookPlacing the hook here means a usermod that returns true will prevent subsequent handling of WLED notifier, TPM2.NET, UDP realtime (1..5), and UDP API parsing. If the intent is to allow overriding only WLED notifier parsing, consider moving the hook call to just before
parseNotifyPacket(...)and after the TPM2/Realtime branches. Otherwise, document the broader override effect so usermods return true only for their own packets.Proposed placement (outside this hunk for reference):
- After the TPM2.NET and realtime branches, but before the WLED notifier check (
udpIn[0] == 0 ...).usermods/udp_name_sync/udp_name_sync.cpp (1)
29-36: Respect the enabled flag; remove unused fields
enabledis never used;initDone/lastTimeare unused. Either gate behavior withenabledand implement rate limiting usinglastTime, or remove the fields.Minimal gating:
- void loop() override { - if (!WLED_CONNECTED) return; - if (!udpConnected) return; + void loop() override { + if (!enabled) return; + if (!WLED_CONNECTED) return; + if (!udpConnected) return;Optional: use
lastTimeto throttle broadcasts (e.g., min 100ms between sends).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
usermods/udp_name_sync/library.json(1 hunks)usermods/udp_name_sync/udp_name_sync.cpp(1 hunks)wled00/fcn_declare.h(2 hunks)wled00/udp.cpp(1 hunks)wled00/um_manager.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
wled00/um_manager.cpp (1)
usermods/udp_name_sync/udp_name_sync.cpp (2)
payload(62-70)payload(62-62)
wled00/udp.cpp (1)
wled00/um_manager.cpp (2)
onUdpPacket(71-74)onUdpPacket(71-71)
wled00/fcn_declare.h (2)
wled00/um_manager.cpp (2)
onUdpPacket(71-74)onUdpPacket(71-71)usermods/udp_name_sync/udp_name_sync.cpp (2)
payload(62-70)payload(62-62)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… empty Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/udp_name_sync/udp_name_sync.cpp (1)
69-77: Validate length and avoid trusting null termination from the wireAccessing payload[0] and calling setName((char*)&payload[1]) without validating len or bounding/copying can read past the UDP buffer and crash on malformed packets. Copy within len-1 into a local buffer, terminate, then setName().
- bool onUdpPacket(uint8_t * payload, size_t len) override { + bool onUdpPacket(uint8_t * payload, size_t len) override { DEBUG_PRINT(F("UdpNameSync: Received packet")); - if (receiveDirect) return false; - if (payload[0] != 200) return false; - //else - Segment& mainseg = strip.getMainSegment(); - mainseg.setName((char *)&payload[1]); + if (receiveDirect) return false; + if (len < 2) return false; // header + at least NUL + if (payload[0] != 200) return false; + Segment& mainseg = strip.getMainSegment(); + char tmp[WLED_MAX_SEGNAME_LEN] = {0}; + size_t copyLen = len - 1; + if (copyLen > sizeof(tmp) - 1) copyLen = sizeof(tmp) - 1; + memcpy(tmp, &payload[1], copyLen); + tmp[copyLen] = '\0'; + mainseg.setName(tmp); DEBUG_PRINT(F("UdpNameSync: set segment name")); return true; }
🧹 Nitpick comments (6)
wled00/fcn_declare.h (1)
485-485: Namespace declaration aligned; consider documenting short-circuit semanticsThe UsermodManager::onUdpPacket(size_t len) declaration matches the class API. Consider clarifying in a comment that returning true from any usermod bypasses core UDP parsing (parseNotification), to set expectations for implementers.
- bool onUdpPacket(uint8_t* payload, size_t len); + bool onUdpPacket(uint8_t* payload, size_t len); // if any usermod returns true, core UDP handling is skippedusermods/udp_name_sync/udp_name_sync.cpp (5)
35-35: Broadcast IP: base it on localIP, not gatewayUsing gatewayIP works only if the gateway shares the same subnet bits; localIP makes the intent clear and avoids surprises on atypical setups.
- IPAddress broadcastIp = ~uint32_t(Network.subnetMask()) | uint32_t(Network.gatewayIP()); + IPAddress broadcastIp = uint32_t(Network.localIP()) | ~uint32_t(Network.subnetMask());
37-37: Stale comment: 2 is not freeYou correctly use 200. Update the comment to reflect that 0..5 are used by core protocols; 200+ is for custom/usermod traffic.
- udpOut[0] = 200; // 0: wled notifier protocol, 1: warls protocol, 2 is free + udpOut[0] = 200; // custom usermod packet type (avoid 0..5 used by core protocols)
60-61: Drop unused variablelength is computed but not used.
- size_t length = strlen(mainseg.name); - strlcpy(segmentName, mainseg.name, sizeof(segmentName)); + strlcpy(segmentName, mainseg.name, sizeof(segmentName));
18-24: Small API polish: const-correctness and namingMark isEnabled() const and avoid parameter shadowing in enable().
- inline void enable(bool enable) { enabled = enable; } + inline void enable(bool value) { enabled = value; } @@ - inline bool isEnabled() { return enabled; } + inline bool isEnabled() const { return enabled; }
8-10: Unused membersinitDone and lastTime are currently unused. Either wire them into logic (e.g., rate limit broadcasts) or remove to reduce ROM/RAM.
- bool initDone = false; - unsigned long lastTime = 0;If you keep initDone, consider documenting its intended use or using it in loop().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
usermods/udp_name_sync/udp_name_sync.cpp(1 hunks)wled00/fcn_declare.h(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-21T00:00:17.164Z
Learnt from: willmmiles
PR: wled/WLED#4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.164Z
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/fcn_declare.husermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
wled00/fcn_declare.h
🧬 Code graph analysis (1)
wled00/fcn_declare.h (2)
wled00/um_manager.cpp (2)
onUdpPacket(71-74)onUdpPacket(71-71)usermods/udp_name_sync/udp_name_sync.cpp (2)
payload(69-78)payload(69-69)
⏰ 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). (16)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (4)
wled00/fcn_declare.h (2)
445-446: Good API: use size_t for UDP payload lengthSwitching to size_t for onUdpPacket avoids silent truncation and enables safe handling of full UDP frames. No issues here.
444-446: AllonUdpPacketoverrides use the updatedsize_tsignature
I searched throughusermodsandwled00for anyonUdpPacketoverrides using auint8_tlength parameter and found none—every override (includingusermods/udp_name_sync/udp_name_sync.cpp:69) now correctly usessize_tfor the payload length. No further changes required.usermods/udp_name_sync/udp_name_sync.cpp (2)
33-47: Edge case: initial state vs. cleared name logic is fine, but ensure cache update orderThe “name cleared” branch resets segmentName[0] before sending. That keeps the cache consistent—good. After the general send, you already copy the current name into segmentName—also good.
61-64: Good: bounded copies and length-aware sendUsing strlcpy into a fixed buffer and computing the write length with strnlen prevents overrun and preserves cross-platform max name length differences.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wled00/udp.cpp (1)
661-663: Hook call location is “observer of unconsumed packets”, not an override; also pass actual read length
- The callback is placed at the very end of handleNotifications(), after all core handlers return early. This means it cannot override parseNotifyPacket or realtime handling even if it returns true. That contradicts the PR summary’s “onUdpPacket will override parseNotification when it returns true.” If the intended behavior is “observe only packets not consumed by core,” please update the description/docs accordingly. Otherwise, you’ll need to invoke the hook before core handlers and honor its boolean return to short-circuit them.
- Minor: You currently pass packetSize; prefer the number of bytes actually read (len).
- UsermodManager::onUdpPacket(udpIn, packetSize); + // pass actual bytes read + UsermodManager::onUdpPacket(udpIn, len);Follow-up options:
- If “observer” semantics are intended: consider making the hook return void to avoid implying short-circuit behavior, and document that the payload pointer is valid only during the call and must not be retained.
- If “override” semantics are required: move a guarded call (e.g., only for udpIn[0]==0 notifier packets with custom callMode >=200) before parseNotifyPacket and return on true, while keeping this trailing call for all other unhandled packets to balance safety and flexibility. I can draft that variant if you want it in this PR.
Note: The prior bot warning about size_t→uint8_t truncation is resolved; um_manager.cpp already takes size_t len.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
usermods/udp_name_sync/udp_name_sync.cpp(1 hunks)wled00/udp.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/udp_name_sync/udp_name_sync.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-21T00:00:17.164Z
Learnt from: willmmiles
PR: wled/WLED#4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.164Z
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/udp.cpp
🧬 Code graph analysis (1)
wled00/udp.cpp (1)
wled00/um_manager.cpp (2)
onUdpPacket(71-74)onUdpPacket(71-71)
⏰ 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 (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- 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 (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- 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_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
|
have applied all suggestions and fixed comments, looks ready for a merge :) |
|
The majority of the nitpick comments I think are valid |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/udp_name_sync/udp_name_sync.cpp (1)
59-66: Fix unsafe UDP parsing: validate length and don’t trust null-terminationReads past buffer when len==0 and assumes payload+1 is NUL-terminated. Copy-bounds and terminate before setName(). Also future-proof by using a shared packet-type constant.
Apply:
@@ - bool onUdpPacket(uint8_t * payload, size_t len) override { + bool onUdpPacket(uint8_t * payload, size_t len) override { DEBUG_PRINT(F("UdpNameSync: Received packet")); - if (receiveDirect) return false; - if (payload[0] != 200) return false; - //else - Segment& mainseg = strip.getMainSegment(); - mainseg.setName((char *)&payload[1]); + if (!enabled) return false; + if (receiveDirect) return false; + if (len < 2) return false; // need type + at least 1 byte for name (can be 0) + if (payload[0] != kPacketType) return false; + Segment& mainseg = strip.getMainSegment(); + char tmp[WLED_MAX_SEGNAME_LEN] = {0}; + size_t copyLen = len - 1; + if (copyLen > sizeof(tmp) - 1) copyLen = sizeof(tmp) - 1; + memcpy(tmp, &payload[1], copyLen); + tmp[copyLen] = '\0'; + mainseg.setName(tmp); DEBUG_PRINT(F("UdpNameSync: set segment name")); return true; }
🧹 Nitpick comments (2)
usermods/udp_name_sync/udp_name_sync.cpp (2)
7-11: Replace magic number 200 with a single constantImproves readability and ensures sender/receiver stay in sync.
Apply:
@@ class UdpNameSync : public Usermod { private: + static constexpr uint8_t kPacketType = 200; // custom usermod packet type @@ - udpOut[0] = 200; // custom usermod packet type (avoid 0..5 used by core protocols) + udpOut[0] = kPacketType; // custom usermod packet type (avoid 0..5 used by core protocols) @@ - if (payload[0] != 200) return false; + if (payload[0] != kPacketType) return false;Also applies to: 34-35, 62-63
45-51: Avoid printing potential null pointer; reuse curNameUse the already null-safe curName for logging.
Apply:
@@ - notifierUdp.beginPacket(broadcastIp, udpPort); - DEBUG_PRINT(F("UdpNameSync: saving segment name ")); - DEBUG_PRINTLN(mainseg.name); + notifierUdp.beginPacket(broadcastIp, udpPort); + DEBUG_PRINT(F("UdpNameSync: saving segment name ")); + DEBUG_PRINTLN(curName); strlcpy(segmentName, mainseg.name, sizeof(segmentName));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
usermods/udp_name_sync/udp_name_sync.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 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:
usermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-02-19T12:41:22.676Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:343-347
Timestamp: 2025-02-19T12:41:22.676Z
Learning: The setName() method in WLED's Segment class (FX_fcn.cpp) handles all necessary validation internally, including:
1. Null pointer checks
2. Length validation against WLED_MAX_SEGNAME_LEN
3. Memory allocation/reallocation
No additional validation or name clearing is needed before calling setName().
Applied to files:
usermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
usermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
usermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
usermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-02-19T12:41:22.676Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:343-347
Timestamp: 2025-02-19T12:41:22.676Z
Learning: The setName() method in WLED handles segment name validation internally. No additional length checks or name clearing is needed before calling setName().
Applied to files:
usermods/udp_name_sync/udp_name_sync.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). (14)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (1)
usermods/udp_name_sync/udp_name_sync.cpp (1)
32-57: Sender path looks solid (size-bounded copies; explicit empty-name case)Use of strlcpy/strnlen with correct buffer sizes and explicit udpOut[1]=0 for empty name is good. Broadcast IP computation is standard for IPv4.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
usermods/udp_name_sync/udp_name_sync.cpp (5)
33-33: Micro: avoid strlen() in a boolean emptiness checkChecking the first char is cheaper and clearer.
- if (!strlen(segmentName) && !mainseg.name) return; //name was never set, do nothing + if (segmentName[0] == '\0' && !mainseg.name) return; // name was never set, do nothing
51-58: Trim debug noise and compute length onceReduce duplicate prints and avoid two passes over the payload.
- notifierUdp.beginPacket(broadcastIp, udpPort); - DEBUG_PRINT(F("UdpNameSync: saving segment name ")); - DEBUG_PRINTLN(mainseg.name); - DEBUG_PRINTLN(curName); - strlcpy(segmentName, mainseg.name, sizeof(segmentName)); - strlcpy((char *)&udpOut[1], segmentName, sizeof(udpOut) - 1); // leave room for header byte - notifierUdp.write(udpOut, 2 + strnlen((char *)&udpOut[1], sizeof(udpOut) - 1)); + notifierUdp.beginPacket(broadcastIp, udpPort); + DEBUG_PRINT(F("UdpNameSync: saving segment name ")); + DEBUG_PRINTLN(curName); + strlcpy(segmentName, curName, sizeof(segmentName)); + strlcpy((char *)&udpOut[1], segmentName, sizeof(udpOut) - 1); // leave room for header byte + size_t nameLen = strnlen((char *)&udpOut[1], sizeof(udpOut) - 1); + notifierUdp.write(udpOut, 2 + nameLen);
24-26: Clarify enablement model (compile-time vs runtime)setup() forces the mod enabled when compiled. If the PR intent is “not enabled by default” at runtime, consider defaulting to false and wiring a config toggle; otherwise, add a brief comment that it’s disabled by default at build-time (not included unless selected).
- void setup() override { - enable(true); - } + void setup() override { + // Enabled when this usermod is compiled in; flip to false if you prefer runtime opt-in. + enable(true); + }If you want a runtime toggle, I can add readFromConfig()/addToConfig() for “enabled”.
82-85: Remove or use PROGMEM strings_name and _enabled are currently unused. Either wire them into config/UI or drop to save a few bytes.
-const char UdpNameSync::_name[] PROGMEM = "UdpNameSync"; -const char UdpNameSync::_enabled[] PROGMEM = "enabled"; +// (Optional) Define only when used by config/UI to reduce flash usage. +// const char UdpNameSync::_name[] PROGMEM = "UdpNameSync"; +// const char UdpNameSync::_enabled[] PROGMEM = "enabled";
35-37: Optional: cache broadcast IP between sendsComputing broadcast each loop is fine; if you ever send more often, cache it and refresh on interface change to shave cycles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
usermods/udp_name_sync/udp_name_sync.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 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:
usermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-02-19T12:41:22.676Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:343-347
Timestamp: 2025-02-19T12:41:22.676Z
Learning: The setName() method in WLED's Segment class (FX_fcn.cpp) handles all necessary validation internally, including:
1. Null pointer checks
2. Length validation against WLED_MAX_SEGNAME_LEN
3. Memory allocation/reallocation
No additional validation or name clearing is needed before calling setName().
Applied to files:
usermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
usermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
usermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
usermods/udp_name_sync/udp_name_sync.cpp
📚 Learning: 2025-02-19T12:41:22.676Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:343-347
Timestamp: 2025-02-19T12:41:22.676Z
Learning: The setName() method in WLED handles segment name validation internally. No additional length checks or name clearing is needed before calling setName().
Applied to files:
usermods/udp_name_sync/udp_name_sync.cpp
🧬 Code graph analysis (1)
usermods/udp_name_sync/udp_name_sync.cpp (1)
usermods/wizlights/wizlights.cpp (1)
WLED_CONNECTED(83-103)
⏰ 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). (15)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (4)
usermods/udp_name_sync/udp_name_sync.cpp (4)
28-31: Good gating in loop()Early returns on enabled/connection state keep the hot path cheap. LGTM.
35-46: Empty-name send path is now well-initializedHeader set and payload byte zeroed avoids garbage on the wire. Nice.
48-50: Correct, bounded name equality checkUsing strncmp with a fixed bound avoids UB on null pointers and long names. Good.
63-77: Robust UDP parsing (length-first, bounded copy, no reliance on NUL on the wire)This follows WLED guidance on packet safety and uses Segment::setName() as intended. Solid.
|
hello, is there anything left to do here? I think I'm quite done resolving all conversations and nitpicks for CodeRabbit :) |
|
thank you so much for your approval! |
|
Can you also submit updates to the docs so that others know how to write their own UDP usermods? |
3 participants
this new hooks will help you implement new and custom protocols in usermods.
I've provided an example (see usermods/udp_name_sync). The example will help you share the main segment name across different WLED instances.
The segment name can be useful to sync with some effects like GIF image or scrolling text.
If you define new packet format in your usermod, make sure it will either not collide with already used version of wled udp packet :
Also, the onUdpPacket will override "parseNotification" if it returns "true". Have fun!
Summary by CodeRabbit
New Features
Behavior Change
Chores