Add JSON data handling over ESP-NOW in espNowReceiveCB #5161
Conversation
WalkthroughThe ESP-NOW receive callback in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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: 0
🧹 Nitpick comments (2)
wled00/udp.cpp (2)
956-956: Add defensive length check before accessing data[0].While the callback is unlikely to be invoked with
len == 0, add an explicit check for defensive programming consistency with similar checks elsewhere in this file (e.g., lines 502, 605).Apply this diff:
// handle JSON data over ESP-NOW - if (data[0] == '{') { + if (len > 0 && data[0] == '{') { if (requestJSONBufferLock(18)) {
955-966: Consider adding debug output for JSON handling.For consistency with the WiZ Mote handler (line 928 area) and to aid troubleshooting, consider adding debug output when JSON is processed.
Example addition:
// handle JSON data over ESP-NOW if (len > 0 && data[0] == '{') { + DEBUG_PRINTLN(F("ESP-NOW: Processing JSON")); if (requestJSONBufferLock(18)) { DeserializationError error = deserializeJson(*pDoc, data, len); JsonObject root = pDoc->as<JsonObject>(); if (!error && !root.isNull()) { deserializeState(root); + } else { + DEBUG_PRINTLN(F("ESP-NOW: JSON parse failed")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/udp.cpp(1 hunks)
🧰 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/udp.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: willmmiles
Repo: wled/WLED PR: 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.
Learnt from: DedeHai
Repo: wled/WLED PR: 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.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: In WLED projects, ArduinoJson.h is not directly included via #include <ArduinoJson.h> - the ArduinoJson symbols are made available through the WLED build system and wled.h transitive includes, so explicitly adding #include <ArduinoJson.h> is not necessary and may not work.
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 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/udp.cpp
📚 Learning: 2025-08-21T00:00:17.196Z
Learnt from: willmmiles
Repo: wled/WLED PR: 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/udp.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). (13)
- 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 (usermods)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (2)
wled00/udp.cpp (2)
955-967: JSON over ESP-NOW implementation follows secure pattern.The implementation correctly:
- Relies on the trusted sender check (lines 936-947) for security
- Passes
lentodeserializeJson, avoiding null termination issues per learnings- Acquires the JSON buffer lock before parsing
- Returns early to prevent double-processing
This is actually more secure than the UDP JSON API (line 654), which lacks authentication.
However, see separate comments for minor defensive improvements and design verification.
955-966: Verify that JSON packets intentionally bypass sync mode requirements.The early return at line 965 means JSON packets bypass the checks at line 969 (broadcast flag, 'W' magic byte,
useESPNowSyncsetting, WiFi connection state). This differs from WLED sync packets but matches the WiZ Mote pattern (lines 950-953).Given the PR objectives (custom remote, similar to WiZMote), this is likely intentional, but please confirm this design choice is correct.
netmindz
added
enhancement
connectivity
|
Sounds like a good addition. What I am not too sure about is to use "{" as an indicator and the lack of suporting longer JSONs. For a simle single command there is already the way to go through a remote.json file which does exactly what you want to achieve, just not in a dynamic way. |
|
remote.json is to remaps a wizmote button to a commande ( on the wled side ) right ? |
correct, you can have up to 256 button functions to map to any JSON command. |
|
I already looked at your repo and really like it. 
|
|
the advantage of using remote.json is you can change remote functionality without changing the firmware on the remote. Disadvantage is that you need that file on each WLED device. @blazoncek would such an implementation clash with any of your ESPNow work? What are your thoughts on this in general? |
|
For purposes like this (home made ESP-NOW remote) you have usermod callback which can be used for handling custom payloads. Write a usermod and it can be included. If you have found readily available (generic?) remote that is sending JSON payloads (and preferably WLED's JSON API) then I would request to implement split packets as well. Otherwise this is enhancement just for (one) specific use. Be mindful that 99% of WLED users are not electronics tinkerers and they will prefer to buy off the shelf remote (like Wizmote) which don't benefit from this enhancement. However this is just my view on the topic. |
|
I do see some use-cases like writing custom sync sequence for art installations, though that could also be done using the remote.json. I am not opposed to adding this if its done properly. Its not much code for a feature some may find useful. |
I would require split packet processing as ~250 bytes is really very little for JSON.
I do agree. |
|
I'm not fan of the way UsermodManager::onEspNowMessage is done right now, because he don't check if the remote is in the whitelist. One of the problem with the Wizmote, to send 1 pqt it send a espnow pqt to every channels. I think be able to send a "small" pqt is important so a remote can do the same. ( spam all channel ) But yes I should also think about a way send the "cmd" in a multiple pqt ( probally only on one channel ), but both should be possible |
You can do that in usermod.
That is not a problem. That's a benefit. |
4 participants
I'm building a custom remote and I want to control WLED with it.
The WIZMote packet works great, but it isn’t easily customizable for my needs.
This PR adds support for sending JSON API commands over ESP-NOW.
ESP-NOW limits packets to 255 bytes, but this is more than enough for remote commands.
For security, JSON parsing only happens when the sender (the remote) is present in the trusted device list, and only when the received payload starts with
{Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.