Conversation
Add explicit guidance explaining that a unique USERMOD_ID_* is only needed when a usermod uses inter-mod communication, pin ownership via pinManager, or needs to be identifiable in JSON info output. Updates both AGENTS.md and the const.h comment block to reflect this.
On BOARD_HAS_PSRAM builds, DEBUG_* macros always route to a 32 KB PSRAM ring buffer (LogBuffer) regardless of WLED_DEBUG. The buffer is exposed via /json/log (plain-text streaming, ?clear support) and a /log web UI page with refresh, auto-refresh, copy and clear controls. A 'Diagnostic Log' button is shown on the Settings menu on PSRAM builds.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
🧹 Nitpick comments (1)
wled00/log_buffer.h (1)
39-41: ⚡ Quick winUse project allocator wrapper for PSRAM allocation
init()usesps_mallocdirectly. Repository guidance prefersp_malloc()/d_malloc()in C++ to keep allocation policy centralized.Proposed fix
- _buf = static_cast<char*>(ps_malloc(CAPACITY)); + _buf = static_cast<char*>(p_malloc(CAPACITY));As per coding guidelines: “Use
d_malloc()(DRAM-preferred) /p_malloc()(PSRAM-preferred) for allocation in C++.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/log_buffer.h` around lines 39 - 41, The init() function currently calls ps_malloc directly to allocate _buf; replace that call with the project allocator wrapper p_malloc (for PSRAM-preferred) or d_malloc as appropriate. Update the allocation line in init() to use p_malloc(CAPACITY) (or d_malloc(CAPACITY) if DRAM is intended) and keep the existing null check on _buf, ensuring _buf remains a char* and CAPACITY is used unchanged; reference symbols: init(), _buf, CAPACITY, p_malloc(), d_malloc().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/data/log.htm`:
- Around line 51-58: The clearLog function currently ignores the HTTP response
status and always clears the textarea and shows "Log cleared."; update clearLog
to check the fetch response (r.ok) before mutating DOM: if r.ok then clear
document.getElementById('log').value, update
document.getElementById('sz').textContent and call setStatus('Log cleared.'),
otherwise do not clear and call setStatus with an error message (include
response.status/text) and add a .catch handler to surface network errors; locate
the logic inside function clearLog and the fetch(getURL('/json/log?clear'))
promise chain to implement this.
In `@wled00/json.cpp`:
- Around line 1388-1402: serveLog allows unauthenticated access; protect it like
other sensitive endpoints by checking the settings PIN at the start of serveLog
(before reading/clearing wledLogBuffer). Call the same helper used elsewhere
(e.g., isSettingsAccessible(request) / checkSettingsPin(request) or the
project's equivalent) and if it fails send an appropriate HTTP error (401/403)
and return; ensure the ?clear branch is also gated by that check so clearing
requires a valid PIN.
In `@wled00/log_buffer.h`:
- Around line 61-71: printTo() currently reads _used, _head and buffer contents
without synchronization while write()/clear() use _mux, leading to torn
snapshots; fix by acquiring the same mutex (_mux) at the start of printTo(),
then snapshot needed state (e.g. local copies of _used and _head) and perform
the out.write() reads while the lock is held (or alternatively copy the relevant
buffer region into a temporary while holding _mux and then release before
writing to out), ensuring all accesses to _used, _head and _buf are synchronized
with write()/clear() to avoid inconsistent wrapped/non-wrapped outputs.
---
Nitpick comments:
In `@wled00/log_buffer.h`:
- Around line 39-41: The init() function currently calls ps_malloc directly to
allocate _buf; replace that call with the project allocator wrapper p_malloc
(for PSRAM-preferred) or d_malloc as appropriate. Update the allocation line in
init() to use p_malloc(CAPACITY) (or d_malloc(CAPACITY) if DRAM is intended) and
keep the existing null check on _buf, ensuring _buf remains a char* and CAPACITY
is used unchanged; reference symbols: init(), _buf, CAPACITY, p_malloc(),
d_malloc().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4e79fcc-5bb7-442f-9ff3-f08b2f0be1f5
📒 Files selected for processing (10)
tools/cdata.jswled00/data/log.htmwled00/data/settings.htmwled00/fcn_declare.hwled00/json.cppwled00/log_buffer.hwled00/wled.cppwled00/wled.hwled00/wled_server.cppwled00/xml.cpp
Comment on lines
+51
to
+58
| function clearLog() { | ||
| fetch(getURL('/json/log?clear')) | ||
| .then(r => r.text()) | ||
| .then(() => { | ||
| document.getElementById('log').value = ''; | ||
| document.getElementById('sz').textContent = '0 KB'; | ||
| setStatus('Log cleared.'); | ||
| }) |
Contributor
There was a problem hiding this comment.
Check HTTP status before showing “Log cleared.”
On Line 53, clearLog() ignores r.ok, so failed clear requests still wipe the textarea and show success.
Proposed fix
function clearLog() {
fetch(getURL('/json/log?clear'))
- .then(r => r.text())
- .then(() => {
+ .then(r => {
+ if (!r.ok) {
+ setStatus('Clear failed: HTTP ' + r.status);
+ return null;
+ }
+ return r.text();
+ })
+ .then(txt => {
+ if (txt === null) return;
document.getElementById('log').value = '';
document.getElementById('sz').textContent = '0 KB';
setStatus('Log cleared.');
})
.catch(e => setStatus('Clear error: ' + e));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/data/log.htm` around lines 51 - 58, The clearLog function currently
ignores the HTTP response status and always clears the textarea and shows "Log
cleared."; update clearLog to check the fetch response (r.ok) before mutating
DOM: if r.ok then clear document.getElementById('log').value, update
document.getElementById('sz').textContent and call setStatus('Log cleared.'),
otherwise do not clear and call setStatus with an error message (include
response.status/text) and add a .catch handler to surface network errors; locate
the logic inside function clearLog and the fetch(getURL('/json/log?clear'))
promise chain to implement this.
Comment on lines
+1388
to
+1402
| void serveLog(AsyncWebServerRequest* request) | ||
| { | ||
| #ifdef BOARD_HAS_PSRAM | ||
| if (!wledLogBuffer.available()) { | ||
| // PSRAM detected at compile time but not found / not initialised at runtime | ||
| request->send(503, FPSTR(CONTENT_TYPE_PLAIN), F("Log buffer unavailable (no PSRAM detected)")); | ||
| return; | ||
| } | ||
|
|
||
| // Check for ?clear query parameter to wipe the buffer | ||
| if (request->hasParam(F("clear"))) { | ||
| wledLogBuffer.clear(); | ||
| request->send(200, FPSTR(CONTENT_TYPE_PLAIN), F("Log buffer cleared.")); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Protect /json/log with the same PIN gate as other sensitive endpoints.
serveLog() currently allows unauthenticated read/clear access. On devices with a settings PIN, this can leak diagnostic details and lets any reachable client wipe evidence via ?clear.
🔐 Suggested minimal guard
void serveLog(AsyncWebServerRequest* request)
{
`#ifdef` BOARD_HAS_PSRAM
+ if (strlen(settingsPIN) > 0 && !correctPIN) {
+ serveJsonError(request, 401, ERR_DENIED);
+ return;
+ }
+
if (!wledLogBuffer.available()) {
// PSRAM detected at compile time but not found / not initialised at runtime
request->send(503, FPSTR(CONTENT_TYPE_PLAIN), F("Log buffer unavailable (no PSRAM detected)"));
return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/json.cpp` around lines 1388 - 1402, serveLog allows unauthenticated
access; protect it like other sensitive endpoints by checking the settings PIN
at the start of serveLog (before reading/clearing wledLogBuffer). Call the same
helper used elsewhere (e.g., isSettingsAccessible(request) /
checkSettingsPin(request) or the project's equivalent) and if it fails send an
appropriate HTTP error (401/403) and return; ensure the ?clear branch is also
gated by that check so clearing requires a valid PIN.
Comment on lines
+61
to
+71
| size_t printTo(Print& out) const { | ||
| if (!_buf || !_used) return 0; | ||
| if (_used < CAPACITY) { | ||
| // Buffer has not wrapped yet — data starts at index 0. | ||
| out.write(reinterpret_cast<const uint8_t*>(_buf), _used); | ||
| return _used; | ||
| } | ||
| // Buffer has wrapped — oldest byte is at _head. | ||
| out.write(reinterpret_cast<const uint8_t*>(_buf + _head), CAPACITY - _head); | ||
| out.write(reinterpret_cast<const uint8_t*>(_buf), _head); | ||
| return CAPACITY; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "log_buffer.h" -type fRepository: wled/WLED
Length of output: 75
🏁 Script executed:
cat -n wled00/log_buffer.hRepository: wled/WLED
Length of output: 4446
🏁 Script executed:
rg "p_malloc|ps_malloc|d_malloc" -t cpp -t h --max-count=20Repository: wled/WLED
Length of output: 3367
🏁 Script executed:
rg -A 3 -B 1 "p_malloc|ps_malloc" wled00/Repository: wled/WLED
Length of output: 7029
Synchronize printTo() reads with writer updates to prevent torn snapshots
printTo() reads _used, _head, and buffer regions without locking, while write() and clear() modify them under _mux. Between the initial checks and subsequent reads, concurrent writes can change _used from below-capacity to capacity, or alter _head, causing printTo() to emit an inconsistent or partially-wrapped log snapshot.
Proposed fix (snapshot indices under lock)
size_t printTo(Print& out) const {
if (!_buf || !_used) return 0;
+ size_t used;
+ size_t head;
+ portENTER_CRITICAL(const_cast<portMUX_TYPE*>(&_mux));
+ used = _used;
+ head = _head;
+ portEXIT_CRITICAL(const_cast<portMUX_TYPE*>(&_mux));
- if (_used < CAPACITY) {
+ if (used < CAPACITY) {
// Buffer has not wrapped yet — data starts at index 0.
- out.write(reinterpret_cast<const uint8_t*>(_buf), _used);
- return _used;
+ out.write(reinterpret_cast<const uint8_t*>(_buf), used);
+ return used;
}
// Buffer has wrapped — oldest byte is at _head.
- out.write(reinterpret_cast<const uint8_t*>(_buf + _head), CAPACITY - _head);
- out.write(reinterpret_cast<const uint8_t*>(_buf), _head);
+ out.write(reinterpret_cast<const uint8_t*>(_buf + head), CAPACITY - head);
+ out.write(reinterpret_cast<const uint8_t*>(_buf), head);
return CAPACITY;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/log_buffer.h` around lines 61 - 71, printTo() currently reads _used,
_head and buffer contents without synchronization while write()/clear() use
_mux, leading to torn snapshots; fix by acquiring the same mutex (_mux) at the
start of printTo(), then snapshot needed state (e.g. local copies of _used and
_head) and perform the out.write() reads while the lock is held (or
alternatively copy the relevant buffer region into a temporary while holding
_mux and then release before writing to out), ensuring all accesses to _used,
_head and _buf are synchronized with write()/clear() to avoid inconsistent
wrapped/non-wrapped outputs.
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
1 participant
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.
This pull request introduces a PSRAM-backed diagnostic log buffer for WLED, enabling users to view, copy, and clear runtime logs from the web UI on devices with PSRAM, without requiring serial access or debug builds. It also updates usermod ID documentation, improves debug macro routing.
Diagnostic Log Buffer and Web UI Integration:
Added a PSRAM-backed ring buffer (
log_buffer.h) for capturing log output via newwledLogBufferandwledLogobjects; logs are exposed through a new/json/logHTTP endpoint and a/logweb UI page, available only on devices with PSRAM. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Added a log viewer page (
log.htm) with refresh, auto-refresh, copy, and clear functionality, and integrated it into the settings UI (hidden unless PSRAM is present). [1] [2] [3]Debug Output Routing:
WLED_DEBUGis enabled); on non-PSRAM builds, behavior is unchanged or disabled as appropriate. [1] [2]Usermod ID Documentation:
AGENTS.mdandconst.hthat usermod IDs are only required for inter-usermod communication, pin ownership, or JSON info identification, reducing unnecessary ID assignments. [1] [2]Summary by CodeRabbit
Release Notes