Open
Conversation
Eliminates size limit. Fixes wled#5458
Contributor
WalkthroughReplaced buffered JSON serialization for the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/json.cpp`:
- Around line 1196-1198: The truncation check after snprintf_P is off-by-one:
snprintf_P returns the number of characters that would be written (excluding
NUL), so if mode_bytes >= len the output was truncated; update the condition in
the block that uses dataPtr/mode_bytes/snprintf_P/len to test for mode_bytes >=
len (and break) instead of mode_bytes > len to avoid producing malformed JSON.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/json.cpp`:
- Around line 1190-1208: The loop can skip the first effect when
lineBuffer[0]==0 and later treat the next non-empty effect as non-first,
producing a leading comma; fix by tracking the actual first-written element
instead of relying on fx_index: introduce a boolean (e.g., first = true) before
the while, replace the check "if (fx_index == 0) *data = '[';" with "if (first)
{ *data = '['; first = false; }", and leave the rest of the logic (dataPtr,
mode_bytes, data/len/bytes_written updates, and fx_index++) unchanged so the '['
is written when the first non-empty mode is emitted.
- Line 1284: Remove the orphaned forward declaration "void
serializeModeData(JsonArray fxdata);" from the header fcn_declare.h; locate that
exact declaration and delete it so there is no unused prototype left in the
header, then rebuild to verify no references remain (no code changes required
elsewhere since there is no definition or callers).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/json.cpp (1)
1193-1193: Coupling to ArduinoJson internal namespace.
ARDUINOJSON_NAMESPACE::EscapeSequence::escapeCharis an internal API that could change between ArduinoJson versions. Since WLED vendors ArduinoJson, this is manageable, but worth noting for future maintenance.Consider adding a comment documenting this dependency, or if this streaming approach is generalized later (as mentioned in PR description), consider implementing a standalone escape function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/json.cpp` at line 1193, The code currently calls ARDUINOJSON_NAMESPACE::EscapeSequence::escapeChar(*p) which relies on ArduinoJson's internal API; update the usage by either (a) adding a clear comment above the call to ARDUINOJSON_NAMESPACE::EscapeSequence::escapeChar noting this internal dependency and the risk of API changes, or (b) replace the call with a small standalone helper escapeChar implementation (used by the streaming/JSON escape logic in json.cpp) and switch callers to that helper; reference the existing call site (the esc = ARDUINOJSON_NAMESPACE::EscapeSequence::escapeChar(*p) expression) when making the change so you modify the exact usage and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/json.cpp`:
- Line 1193: The code currently calls
ARDUINOJSON_NAMESPACE::EscapeSequence::escapeChar(*p) which relies on
ArduinoJson's internal API; update the usage by either (a) adding a clear
comment above the call to ARDUINOJSON_NAMESPACE::EscapeSequence::escapeChar
noting this internal dependency and the risk of API changes, or (b) replace the
call with a small standalone helper escapeChar implementation (used by the
streaming/JSON escape logic in json.cpp) and switch callers to that helper;
reference the existing call site (the esc =
ARDUINOJSON_NAMESPACE::EscapeSequence::escapeChar(*p) expression) when making
the change so you modify the exact usage and keep behavior identical.
Collaborator
|
I can confirm this working. |
Collaborator
|
now that the limit of number of FX is lifted, there is also no more reason to not include 2D particle system: the reason for the restriction was that the json would overflow.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eliminates size limit. Fixes #5458
The implementation is inspired by RapidJSON's SAX-style Writer, coupled with the
ChunkedResponsefeature of AsyncWebServer that allows streaming data with minimal state. This particular implementation is tightly coupled to the specific task at hand; I'm hoping to generalize and expand on this approach for wider usage as it can make parts of the web API much less resource intensive.Summary by CodeRabbit