Add JSON data handling over ESP-NOW in espNowReceiveCB#5161
Add JSON data handling over ESP-NOW in espNowReceiveCB#5161spectrenoir06 wants to merge 8 commits intowled:mainfrom
Conversation
WalkthroughIntroduces a new ESP-NOW JSON Handler Usermod that reassembles fragmented JSON payloads received via ESP-NOW protocol, implements message deduplication, and applies deserialized JSON to WLED state. Includes module implementation, configuration file, documentation, and a new usermod identifier constant. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. 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. |
…in UDP ( need to me moved in usermod )
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@usermods/espnow_json_handler/espnow_json_handler.cpp`:
- Around line 12-16: The header comment incorrectly states a 4-byte fragment
header while the implementation and constant ESPNOW_FRAGMENT_HEADER_SIZE use 3
bytes; update the comment to reflect the actual 3-byte layout (Byte 0:
message_id, Byte 1: fragment_index, Byte 2: total_fragments, Byte 3+: JSON
payload) OR alternatively change ESPNOW_FRAGMENT_HEADER_SIZE and the code paths
that parse/construct the header (search for ESPNOW_FRAGMENT_HEADER_SIZE and any
code using bytes 0-2 for message_id/fragment_index/total_fragments) so they
consistently use a 4-byte header—pick one approach and make the code, constant,
and header comment consistent.
- Around line 88-93: Add a timeout for incomplete fragment reassembly by
recording a start timestamp when beginning a new message in onEspNowMessage()
(set reassemblyStartTime = millis() when allocating/initializing
reassemblyBuffer and setting lastMsgId), define a REASSEMBLY_TIMEOUT_MS
constant, and in loop() check if reassemblyBuffer is non-null and millis() -
reassemblyStartTime > REASSEMBLY_TIMEOUT_MS then call cleanupReassembly() (and
DEBUG_PRINTLN a timeout message) to free stale state; ensure cleanupReassembly()
clears reassemblyBuffer and resets reassemblyStartTime/lastMsgId appropriately.
- Around line 54-64: The onEspNowMessage handler currently accepts fragmented
JSON without validating the sender MAC, so add a whitelist check: inside
onEspNowMessage(uint8_t* sender, uint8_t* payload, uint8_t len) (before
returning true for handled messages) verify the sender MAC against the core
linked_remotes list (or call the same core validation routine used for other
ESP-NOW handlers) and return false if the sender is not authorized; ensure you
reference the sender pointer and use the same comparison semantics/format as
linked_remotes to avoid bypassing core checks.
🧹 Nitpick comments (3)
usermods/espnow_json_handler/espnow_json_handler.cpp (3)
23-29: Inconsistent indentation within the class.The private member declarations use extra leading whitespace compared to other sections of the class. Consider aligning indentation consistently throughout the file.
♻️ Suggested fix
private: - // Fragment reassembly state - uint8_t lastMsgId = 0; - uint8_t lastProcessedMsgId = 0; - uint8_t fragmentsReceived = 0; - uint8_t* reassemblyBuffer = nullptr; - size_t reassemblySize = 0; + // Fragment reassembly state + uint8_t lastMsgId = 0; + uint8_t lastProcessedMsgId = 0; + uint8_t fragmentsReceived = 0; + uint8_t* reassemblyBuffer = nullptr; + size_t reassemblySize = 0;
78-86: Deduplication tracks only the most recent message ID.The current deduplication approach using a single
lastProcessedMsgIdworks for multi-channel broadcast from the same sender but may incorrectly filter out messages from different senders using the same ID. This is likely acceptable for the intended use case (single remote broadcasting on multiple channels) but worth documenting as a known limitation.
130-140: Consider adding a named constant for the JSON buffer lock ID.The magic number
18inrequestJSONBufferLock(18)could be documented or defined as a constant for clarity. Based on learnings, this pattern is used throughout WLED for lock identification.
…mbly timeout handling + use whitelist for remote
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@usermods/espnow_json_handler/espnow_json_handler.cpp`:
- Around line 26-42: The code treats 0 as a valid messageId but also uses 0 as
the initial lastProcessedMsgId, causing messageId 0 to be wrongly skipped and
leaving lastMsgId unchanged after cleanup prevents retries; change the initial
values of lastMsgId and lastProcessedMsgId to a sentinel (e.g., UINT8_MAX)
instead of 0 and update cleanupReassembly() to reset lastMsgId to that sentinel
as well; locate the variables lastMsgId, lastProcessedMsgId and the
cleanupReassembly() function and apply the sentinel initialization and reset so
messageId 0 is processed and retries restart the timeout.
- Around line 63-75: The onEspNowMessage method currently relies on the external
last_signal_src string; instead, build a local 12-char hex MAC string from the
sender parameter (e.g., using sprintf with "%02x" for sender[0]..sender[5]) and
use that local sender string when iterating linked_remotes to set knownRemote
and to log/return decisions; update comparisons that reference last_signal_src
to use your new local senderStr so the function is self-contained (symbols:
onEspNowMessage, sender parameter, linked_remotes, last_signal_src).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@usermods/espnow_json_handler/espnow_json_handler.cpp`:
- Around line 140-149: Add a hard cap on the total reassembly buffer size (e.g.,
define MAX_REASSEMBLY_SIZE = 4096 or 8192) and check it before calling realloc:
compute newSize = reassemblySize + fragmentDataSize and if newSize >
MAX_REASSEMBLY_SIZE log an error, call cleanupReassembly(), and return true to
fail gracefully; otherwise proceed with the existing realloc on reassemblyBuffer
and the +1 null terminator. Ensure the cap check is placed immediately before
the realloc in the fragment handling path so reassemblySize, fragmentDataSize,
reassemblyBuffer, and cleanupReassembly() are used as shown.
♻️ Duplicate comments (2)
usermods/espnow_json_handler/espnow_json_handler.cpp (2)
26-27: Use a sentinel value for message ID tracking.With
lastProcessedMsgIdinitialized to0, a legitimate message withmessageId == 0will be incorrectly skipped as "already processed" on first reception. Initialize both to a sentinel like0xFFand resetlastMsgIdincleanupReassembly().🛠️ Suggested fix
- uint8_t lastMsgId = 0; - uint8_t lastProcessedMsgId = 0; + uint8_t lastMsgId = 0xFF; + uint8_t lastProcessedMsgId = 0xFF;And in
cleanupReassembly():void cleanupReassembly() { if (reassemblyBuffer) { free(reassemblyBuffer); reassemblyBuffer = nullptr; } + lastMsgId = 0xFF; fragmentsReceived = 0;
78-89: Derive sender MAC string from the function parameter instead of relying onlast_signal_src.The whitelist check depends on the global
last_signal_srcbeing set by the caller before this callback runs. This creates a fragile coupling to implicit state. Format the sender MAC locally from thesenderparameter to make the function self-contained.🛠️ Suggested fix
bool onEspNowMessage(uint8_t* sender, uint8_t* payload, uint8_t len) override { - + // Format sender MAC for whitelist comparison + char senderStr[13]; + sprintf_P(senderStr, PSTR("%02x%02x%02x%02x%02x%02x"), + sender[0], sender[1], sender[2], sender[3], sender[4], sender[5]); + bool knownRemote = false; for (const auto& mac : linked_remotes) { - if (strlen(mac.data()) == 12 && strcmp(last_signal_src, mac.data()) == 0) { + if (strlen(mac.data()) == 12 && strcmp(senderStr, mac.data()) == 0) { knownRemote = true; break; } } if (!knownRemote) { DEBUG_PRINT(F("ESP Now Message Received from Unlinked Sender: ")); - DEBUG_PRINTLN(last_signal_src); + DEBUG_PRINTLN(senderStr); return false; // Not handled }
🧹 Nitpick comments (1)
usermods/espnow_json_handler/espnow_json_handler.cpp (1)
167-177: Consider defining a named constant for the JSON buffer lock ID.The magic number
18inrequestJSONBufferLock(18)should ideally be a named constant for clarity and maintainability. Also, whenroot.isNull()is true buterrorsucceeded, the error message on line 174 would print an empty/misleading error string.♻️ Suggested improvement
+// At file scope or in class +static constexpr uint8_t JSON_LOCK_ESPNOW = 18; // Process the complete JSON message - if (requestJSONBufferLock(18)) { + if (requestJSONBufferLock(JSON_LOCK_ESPNOW)) { DeserializationError error = deserializeJson(*pDoc, reassemblyBuffer, reassemblySize); JsonObject root = pDoc->as<JsonObject>(); - if (!error && !root.isNull()) { - deserializeState(root); - } - else { - DEBUG_PRINTF_P(PSTR("ESP-NOW JSON deserialization error: %s\n"), error.c_str()); + if (error) { + DEBUG_PRINTF_P(PSTR("ESP-NOW JSON deserialization error: %s\n"), error.c_str()); + } else if (root.isNull()) { + DEBUG_PRINTLN(F("ESP-NOW JSON: empty or non-object payload")); + } else { + deserializeState(root); } releaseJSONBufferLock(); }Based on learnings, magic numbers should be replaced with meaningful constants when they represent specific identifiers.
| // Allocate or reallocate buffer | ||
| size_t fragmentDataSize = len - ESPNOW_FRAGMENT_HEADER_SIZE; | ||
| size_t newSize = reassemblySize + fragmentDataSize; | ||
|
|
||
| uint8_t* newBuffer = (uint8_t*)realloc(reassemblyBuffer, newSize + 1); // +1 for null terminator | ||
| if (!newBuffer) { | ||
| DEBUG_PRINTLN(F("ESP-NOW fragment reassembly: memory allocation failed")); | ||
| cleanupReassembly(); | ||
| return true; // Handled by failing gracefully | ||
| } |
There was a problem hiding this comment.
Add an upper bound on total reassembly buffer size to prevent memory exhaustion.
A malicious or buggy sender could specify totalFragments=255 with ~250-byte payloads, causing up to ~62KB of heap allocation. On memory-constrained ESP devices, this could exhaust available heap. Add a reasonable cap (e.g., 4KB or 8KB) for total reassembled message size.
🛠️ Suggested fix
Add a constant and check:
`#define` ESPNOW_FRAGMENT_HEADER_SIZE 3
`#define` ESPNOW_REASSEMBLY_TIMEOUT_MS 5000
+#define ESPNOW_MAX_REASSEMBLY_SIZE 4096 // 4KB max JSON payload size_t fragmentDataSize = len - ESPNOW_FRAGMENT_HEADER_SIZE;
size_t newSize = reassemblySize + fragmentDataSize;
+ if (newSize > ESPNOW_MAX_REASSEMBLY_SIZE) {
+ DEBUG_PRINTLN(F("ESP-NOW fragment reassembly: max size exceeded"));
+ cleanupReassembly();
+ return true; // Handled by failing gracefully
+ }
+
uint8_t* newBuffer = (uint8_t*)realloc(reassemblyBuffer, newSize + 1);🤖 Prompt for AI Agents
In `@usermods/espnow_json_handler/espnow_json_handler.cpp` around lines 140 - 149,
Add a hard cap on the total reassembly buffer size (e.g., define
MAX_REASSEMBLY_SIZE = 4096 or 8192) and check it before calling realloc: compute
newSize = reassemblySize + fragmentDataSize and if newSize > MAX_REASSEMBLY_SIZE
log an error, call cleanupReassembly(), and return true to fail gracefully;
otherwise proceed with the existing realloc on reassemblyBuffer and the +1 null
terminator. Ensure the cap check is placed immediately before the realloc in the
fragment handling path so reassemblySize, fragmentDataSize, reassemblyBuffer,
and cleanupReassembly() are used as shown.
|
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.
Add support for sending JSON API commands over ESP-NOW.
Packet size limits (250B v1 / 1470B v2) are sufficient for most cases, with optional fragmentation for larger commands.
Packets can be broadcast on multiple channels without duplicate processing.
For security, JSON parsing only happens when the sender (the remote) is present in the trusted device list
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.