(wip) AC minor patch cluster #2, and some goodies for 8266 #152
Conversation
Add a parameter for blur amount in the Fire 2012 effect
softhack007
changed the title
Improve jsonBufferLock management
Add stress testing scripts
2.7.9 has improved stability of BitBang (resolves some wdt resets when using non-accelerated pins) on 8266
Fixes wled#3809 Loxone JSON parser doesn't handle lx=0 correctly
Rename wled00.ino to wled_main.cpp
…d#4184) * fixed the positioning of the download button * fixed space after "Download the latest binary:" disapering after building * fixed typo --------- Co-authored-by: maxi4329 <maxi4329>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughIntroduces a comprehensive ESP8266 PWM/waveform engine with NMI-safe ISR handling, refactors JSON buffer locking with mutex/timeout support, adds testing infrastructure, updates preset state management to include cycling state, and includes minor effects/UI adjustments. Changes
Sequence Diagram(s)sequenceDiagram
actor Main as Main Code
participant ISR as Timer1 ISR
participant WVF as Waveform State
participant Pin as GPIO Pin
Main->>ISR: Initialize timer1 interrupt
Note over Main,ISR: PWM Cycle Loop
loop Until PWM stopped
ISR->>WVF: Check mailbox for config changes
WVF-->>ISR: High/low times, freq update
ISR->>WVF: Update state machine position
ISR->>Pin: Toggle pin based on phase
alt Pin in HIGH phase
Pin->>Pin: Hold HIGH for N cycles
else Pin in LOW phase
Pin->>Pin: Hold LOW for N cycles
end
ISR->>WVF: Advance to next entry in sequence
end
Main->>ISR: Stop PWM (via _stopPWM_weak)
ISR->>Pin: Final pin toggle/settle
sequenceDiagram
participant App as Application
participant Lock as requestJSONBufferLock()
participant Mutex as Semaphore/Flag
participant Cleanup as Lock Holder ID
App->>Lock: Request JSON buffer access
alt ESP32 Path
Lock->>Mutex: xSemaphoreTakeRecursive (1100ms)
alt Timeout
Mutex-->>Lock: timeout
Lock->>Cleanup: Log error, holder ID
Lock-->>App: return false
else Success
Mutex-->>Lock: acquired
Lock->>Cleanup: Set jsonBufferLock to module ID
Lock-->>App: return true
end
else ESP8266 Path
Lock->>Mutex: Poll lock flag (1100ms)
alt Lock held beyond timeout
Lock->>Cleanup: Log error
Lock-->>App: return false
else Lock available
Lock->>Cleanup: Set jsonBufferLock
Lock-->>App: return true
end
end
Note over Cleanup: App uses JSON buffer
App->>Lock: releaseJSONBufferLock()
Lock->>Cleanup: Clear jsonBufferLock, log release
Lock->>Mutex: Release semaphore/mutex
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The PWM engine implementation introduces substantial low-level ISR/NMI-safe logic with intricate state management, crash workarounds, and timer orchestration. Multiple concurrent state changes across presets, locking mechanisms, and effect logic require cross-file reasoning. While many changes are localized improvements (util.cpp locking, const.h buffer size), the overall heterogeneity and density of new low-level functionality elevate complexity. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wled00/FX.cpp (1)
4117-4135: Don’t pack two 16-bit counters intoSEGENV.step.
SEGENV.stepis still auint16_t(seeSegment_runtimeinFX.h), so writing(sCIStart4 << 16) | ...truncates the upper half every frame. On the next iterationsCIStart4 = (SEGENV.step >> 16)always yields zero, meaning the fourth phase never advances and the Pacifica animation regresses. Please keepsCIStart4in a 16-bit slot (e.g., one of the aux fields) or widenstepeverywhere before merging this.wled00/util.cpp (2)
162-177: Fix off-by-one overflow in oappend() null terminator checkstrcpy() writes a trailing '\0'. The current capacity check doesn’t reserve space for it; when olen + len == SETTINGS_STACK_BUF_SIZE - 1, the '\0' writes out of bounds.
Apply:
- if ((obuf == nullptr) || (olen + len >= SETTINGS_STACK_BUF_SIZE)) { // sanity checks + if ((obuf == nullptr) || (olen + len + 1 > SETTINGS_STACK_BUF_SIZE)) { // reserve room for '\0'Optionally use strlcpy:
- strcpy(obuf + olen, finalTxt); - olen += len; + olen += strlcpy(obuf + olen, finalTxt, SETTINGS_STACK_BUF_SIZE - olen);
380-387: Off-by-one write in extractModeSlider() default branchdest[maxLen] = '\0' writes past the buffer. Use maxLen-1.
- dest[maxLen] = '\0'; // strncpy does not necessarily null terminate string + dest[maxLen-1] = '\0'; // ensure termination within bounds
🧹 Nitpick comments (6)
wled00/wled.cpp (1)
85-87: Make PWM fix entry weak/guarded to avoid link issues.If ESP8266PWM isn’t linked in some envs, a hard extern risks an unresolved symbol.
Apply this to harden it:
-extern "C" void usePWMFixedNMI(); // PWM bugfix for 8266 +#ifdef ESP8266 +extern "C" void usePWMFixedNMI() __attribute__((weak)); // PWM bugfix for 8266 (weak to allow missing lib) +#endifAnd guard the call (see below).
wled00/presets.cpp (1)
128-137: Dead code: consider removal or add rationale.
The disabled applyPresetFromPlaylist() adds maintenance noise. If kept, add a short comment why; otherwise remove.wled00/data/update.htm (1)
20-22: Add rel and alt for security/accessibility.
Opening new tabs should use rel, and images need alt text.Apply:
-Download the latest binary: <a href="https://github.com/MoonModules/WLED/releases" target="_blank" - style="vertical-align: text-bottom; display: inline-flex;"> - <img src="https://img.shields.io/github/release/MoonModules/WLED.svg?style=flat-square"></a><br> +Download the latest binary: <a href="https://github.com/MoonModules/WLED/releases" target="_blank" rel="noopener noreferrer" + style="vertical-align: text-bottom; display: inline-flex;"> + <img src="https://img.shields.io/github/release/MoonModules/WLED.svg?style=flat-square" alt="Latest WLED-MM release badge"></a><br>tools/stress_test.sh (2)
26-33: Harden word-splitting and array handlingAvoid unquoted command substitution and array re-expansion. Use mapfile/readarray and preserve elements.
-read -a JSON_TINY_TARGETS <<< $(replicate "json/nodes") -read -a JSON_SMALL_TARGETS <<< $(replicate "json/info") -read -a JSON_LARGE_TARGETS <<< $(replicate "json/si") -read -a JSON_LARGER_TARGETS <<< $(replicate "json/fxdata") +readarray -t JSON_TINY_TARGETS < <(replicate "json/nodes") +readarray -t JSON_SMALL_TARGETS < <(replicate "json/info") +readarray -t JSON_LARGE_TARGETS < <(replicate "json/si") +readarray -t JSON_LARGER_TARGETS < <(replicate "json/fxdata") -# Expand target URLS to full arguments for curl -TARGETS=(${TARGET_STR[@]}) +# Expand target URLs to full arguments for curl +TARGETS=("${TARGET_STR[@]}") -#echo "${TARGETS[@]}" -FULL_TGT_OPTIONS=$(printf "http://${HOST}/%s -o /dev/null " "${TARGETS[@]}") +FULL_TGT_OPTIONS=$(printf "http://%s/%s -o /dev/null " "${HOST}" "${TARGETS[@]}")Also quote replicate’s printf format:
- printf "${1}?%d " $(seq 1 ${REPLICATE_COUNT}) + printf "%s?%d " "${1}" $(seq 1 "${REPLICATE_COUNT}")
18-19: Use -w to print status, add -sS, and actually apply the formatCURL_PRINT_RESPONSE_ARGS is unused. Add it to the curl call for visibility and reduce noise.
-CURL_ARGS="--compressed --parallel --parallel-immediate --parallel-max ${PARALLEL_MAX}" -CURL_PRINT_RESPONSE_ARGS="-w %{http_code}\n" +CURL_ARGS="--compressed --parallel --parallel-immediate --parallel-max ${PARALLEL_MAX} -sS" +CURL_PRINT_RESPONSE_ARGS="-w \"%{http_code} %{url_effective}\n\"" -time curl ${CURL_ARGS} ${FULL_TGT_OPTIONS} +time bash -c "curl ${CURL_ARGS} ${CURL_PRINT_RESPONSE_ARGS} ${FULL_TGT_OPTIONS}"If running on older bash (<4.3), declare -n may not be available. Confirm target environment or gate with a version check.
Also applies to: 34-37
tools/udp_test.py (1)
24-29: Add num_pixels validation to prevent silent overflow of 1-byte LED indicesThe WARLS protocol correctly uses 1-byte indices (0–255) and your code implementation matches the spec exactly: header byte 0 = protocol (1), byte 1 = timeout; each LED entry is 1 byte index + R + G + B. Your test at NUM_PIXELS=255 is within the limit.
However, the constructor doesn't validate or enforce the 256-LED ceiling. If
num_pixelsexceeds 256 (e.g., 257+), LED indices will silently overflow the 1-byte field. Add an assertion or cap in__init__:assert num_pixels <= 256, "WARLS protocol supports max 256 LEDs (1-byte index 0–255)"For larger LED counts, switch to DRGB/DNRGB modes which use implicit indexing or a 2-byte start index.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
lib/ESP8266PWM/src/core_esp8266_waveform_pwm.cpp(1 hunks)platformio.ini(1 hunks)tools/stress_test.sh(1 hunks)tools/udp_test.py(1 hunks)wled00/FX.cpp(3 hunks)wled00/const.h(1 hunks)wled00/data/update.htm(1 hunks)wled00/json.cpp(1 hunks)wled00/presets.cpp(1 hunks)wled00/util.cpp(2 hunks)wled00/wled.cpp(2 hunks)wled00/wled.h(2 hunks)wled00/wled_main.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-04T10:32:30.457Z
Learnt from: DedeHai
PR: MoonModules/WLED-MM#237
File: wled00/FXparticleSystem.cpp:678-743
Timestamp: 2025-05-04T10:32:30.457Z
Learning: In the Particle System implementation for WLED, stack allocation is preferred over heap allocation for temporary rendering buffers because there's enough stack memory available and heap allocations can cause memory fragmentation.
Applied to files:
wled00/const.h
🪛 Clang (14.0.6)
wled00/wled_main.cpp
[error] 1-1: 'Arduino.h' file not found
(clang-diagnostic-error)
lib/ESP8266PWM/src/core_esp8266_waveform_pwm.cpp
[error] 49-49: 'Arduino.h' file not found
(clang-diagnostic-error)
🪛 Shellcheck (0.11.0)
tools/stress_test.sh
[warning] 18-18: CURL_PRINT_RESPONSE_ARGS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 20-20: JSON_TARGETS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 20-20: Use spaces, not commas, to separate array elements.
(SC2054)
[warning] 21-21: FILE_TARGETS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 26-26: JSON_TINY_TARGETS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 26-26: Quote this to prevent word splitting.
(SC2046)
[warning] 27-27: JSON_SMALL_TARGETS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: JSON_LARGE_TARGETS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: JSON_LARGER_TARGETS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 32-32: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
🔇 Additional comments (10)
wled00/wled_main.cpp (2)
1-1: Good: include Arduino core for .cpp entry.
Converting from .ino requires this include. Change looks correct.If clang still flags 'Arduino.h' missing in static analysis, adjust your analysis job to use PlatformIO’s compile_commands.json or add Arduino framework includes.
8-10: Doc update reads well.
Clear guidance that PlatformIO is required; OK to merge.wled00/presets.cpp (1)
142-144: Syncing cycling state with applyPreset is sensible.
This should resolve preset cycling inconsistencies.Confirm presetCycCurr is defined and used consistently with currentPreset (no duplicate UI updates). A quick grep should show its declaration and primary consumers.
wled00/wled.cpp (1)
641-644: Incorrect assumption: usePWMFixedNMI is not a weak symbol.Verification shows
usePWMFixedNMIhas no weak attributes in either its declaration (line 86) or definition (lib/ESP8266PWM/src/core_esp8266_waveform_pwm.cpp:58). It's a regular extern symbol with a concrete implementation. The existing#ifdef ESP8266guard is sufficient—when ESP8266 is targeted, the symbol is guaranteed to be available from the linked library.The suggested runtime guard (
if (&usePWMFixedNMI)) is unnecessary and should not be applied.Likely an incorrect or invalid review comment.
wled00/const.h (1)
424-424: ESP8266 stack pressure from SETTINGS_STACK_BUF_SIZE increase is real; existing safeguards are partial.Confirmed: ESP8266 allocation increased from 2048→2560 bytes (+512), consuming ~63% of typical 4KB task stack in a single local variable in serveSettingsJS() (wled00/wled_server.cpp:565).
Existing protections prevent buffer overflow (util.cpp:163 sanity check, plus debug stack monitoring at wled00/wled_server.cpp:583–586) but do not guard against stack exhaustion from deeper call chains. No heap/PSRAM fallback currently exists for this buffer—PSRAM support is limited to JSON documents only.
Before merging, verify that:
- Worst-case call depth from AsyncWebServer request handler through serveSettingsJS() leaves >1KB headroom, or
- Build includes CONFIG_ASYNC_TCP_TASK_STACK_SIZE boost (noted in const.h:429 for audioreactive), or
- Accept the tighter margin with continued stack monitoring (already implemented).
If margin is unacceptable, revert the +512 increase or implement heap/PSRAM fallback with checked allocation.
wled00/json.cpp (1)
270-274: LGTM - Coordinate parsing now accepts zero values.The change from
> 0to>= 0correctly allows coordinate value 0 to be processed byparseLxJson. This provides consistent behavior for both x and y coordinates.platformio.ini (2)
297-297: NeoPixelBus library update improves ESP8266 stability.The upgrade from 2.7.5 to 2.7.9 addresses WDT crashes on bitbang pins as noted in the PR objectives.
298-298: ESP8266PWM library integration verified.The library is properly integrated with correct extern declaration at
wled00/wled.cpp:86and function call at line 642. TheusePWMFixedNMI()implementation exists inlib/ESP8266PWM/src/core_esp8266_waveform_pwm.cpp:58with matching C-style linkage.wled00/wled.h (1)
375-379: LGTM - Conditional initialization of autoSegments.The
#ifdef WLED_AUTOSEGMENTSconditional allows build-time configuration of the auto-segments feature while maintaining backward compatibility (defaults to false).wled00/util.cpp (1)
219-241: Review identifies valid architectural inconsistency: recursive mutex contradicts non-reentrant implementationThe review comment is correct. The code uses
xSemaphoreTakeRecursive()but implements non-reentrant behavior. Even when the same module holdsjsonBufferLock, a secondrequestJSONBufferLock()call will fail because the check at line 235 (if (jsonBufferLock)) returns false unconditionally, ignoring ownership.However, the suggested fix is incomplete:
releaseJSONBufferLock()unconditionally clears the lock with no depth tracking. Enabling re-entrancy without per-task depth tracking would break paired release sequences. Before implementing the suggested change, add depth tracking per task to ensurereleaseJSONBufferLock()only clearsjsonBufferLockwhen depth reaches zero.Alternatively, clarify the comment at lines 220–222 to reflect the actual non-reentrant behavior and explain why the recursive mutex is used (e.g., to prevent deadlock if the same system task needs the buffer during large JSON operations).
| extern "C" { | ||
|
|
||
| // Linker magic | ||
| void usePWMFixedNMI() {}; | ||
|
|
||
| // Maximum delay between IRQs |
There was a problem hiding this comment.
extern "C" scope wraps C++ class/definitions — invalid in C++
The extern "C" block starts at Line 55 and only closes at Line 717, enclosing class and C++ definitions. That’s not allowed and will fail to compile.
Minimal fix: restrict extern "C" to declarations only.
-extern "C" {
-// Linker magic
-void usePWMFixedNMI() {};
+extern "C" void usePWMFixedNMI();
...
-};
+// define with C++ linkage outside
+void usePWMFixedNMI() {}Alternatively, close the extern block immediately after the declarations:
-extern "C" {
-// Linker magic
-void usePWMFixedNMI() {};
+extern "C" {
+void usePWMFixedNMI();
+}
+void usePWMFixedNMI() {}Also applies to: 717-717
🤖 Prompt for AI Agents
In lib/ESP8266PWM/src/core_esp8266_waveform_pwm.cpp around lines 55 to 60, the
extern "C" block currently wraps C++ class/definition code (it opens at line 55
and closes only at line 717), which is invalid; restrict the extern "C" linkage
to only the C-compatible declarations by moving the closing brace immediately
after those declarations (or remove extern "C" and apply it only to individual
function prototypes), ensuring all C++ classes and methods are defined outside
the extern "C" scope so the file compiles.
| static void _setPWMFreq_bound(uint32_t freq) __attribute__((weakref("_setPWMFreq_weak"))); | ||
| void _setPWMFreq(uint32_t freq) { | ||
| _setPWMFreq_bound(freq); | ||
| } | ||
| */ | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Weak wrapper mismatch: calls use non‑weak names; wrappers commented out
startWaveformClockCycles_weak() calls _stopPWM(), and _setPWM_weak() calls stopWaveform(). _setPWMFreq() is called but only _setPWMFreq_weak() is defined here. With wrappers commented, these can be unresolved or bind to unintended externals.
Enable the weakref wrappers or call the _weak variants directly. Example enabling wrappers:
-/*
static void _setPWMFreq_bound(uint32_t freq) __attribute__((weakref("_setPWMFreq_weak")));
void _setPWMFreq(uint32_t freq) {
_setPWMFreq_bound(freq);
}
-*/
...
-/*
static bool _stopPWM_bound(uint8_t pin) __attribute__((weakref("_stopPWM_weak")));
IRAM_ATTR bool _stopPWM(uint8_t pin) {
return _stopPWM_bound(pin);
}
-*/
...
-/*
static int stopWaveform_bound(uint8_t pin) __attribute__((weakref("stopWaveform_weak")));
IRAM_ATTR int stopWaveform(uint8_t pin) {
return stopWaveform_bound(pin);
}
-*/Or switch call sites:
- _stopPWM(pin); // Make sure there's no PWM live here
+ _stopPWM_weak(pin); // Make sure there's no PWM live here
...
- _setPWMFreq(_pwmFreq);
+ _setPWMFreq_weak(_pwmFreq);Please confirm target ESP8266 core version provides strong _setPWM/_stopPWM/stopWaveform; otherwise the above change is required to avoid link errors.
Also applies to: 274-279, 418-423, 444-448, 476-480, 327-356, 353-353, 383-385
I'll verify this review comment by examining the actual file content and tracking the weak wrapper definitions and call sites.
🏁 Script executed:
#!/bin/bash
# Examine the file structure and check lines mentioned in the review
echo "=== Checking file existence and basic info ==="
file_path="lib/ESP8266PWM/src/core_esp8266_waveform_pwm.cpp"
if [ -f "$file_path" ]; then
wc -l "$file_path"
else
echo "File not found at $file_path"
find . -name "core_esp8266_waveform_pwm.cpp" 2>/dev/null
fi
echo -e "\n=== Lines 221-226 ==="
sed -n '221,226p' "$file_path"
echo -e "\n=== Lines 274-279 ==="
sed -n '274,279p' "$file_path"
echo -e "\n=== Lines 418-423 ==="
sed -n '418,423p' "$file_path"
echo -e "\n=== Lines 444-448 ==="
sed -n '444,448p' "$file_path"
echo -e "\n=== Lines 476-480 ==="
sed -n '476,480p' "$file_path"
echo -e "\n=== Lines 327-356 ==="
sed -n '327,356p' "$file_path"
echo -e "\n=== Lines 383-385 ==="
sed -n '383,385p' "$file_path"Length of output: 2312
I need to examine the surrounding context to verify if these wrappers are truly commented out. Let me check for opening comment markers and trace the actual definitions.
🏁 Script executed:
#!/bin/bash
file_path="lib/ESP8266PWM/src/core_esp8266_waveform_pwm.cpp"
echo "=== Context around lines 221-226 (check for /* opening) ==="
sed -n '210,230p' "$file_path"
echo -e "\n=== Context around lines 274-279 ==="
sed -n '263,283p' "$file_path"
echo -e "\n=== Context around lines 444-448 ==="
sed -n '433,453p' "$file_path"
echo -e "\n=== Context around lines 476-480 ==="
sed -n '465,485p' "$file_path"
echo -e "\n=== Search for _weak function definitions ==="
rg -n "^\w+.*_weak\s*\(" "$file_path" | head -20
echo -e "\n=== Search for calls to _stopPWM (non-weak) ==="
rg -n "_stopPWM\s*\(" "$file_path" | grep -v "_stopPWM_weak"
echo -e "\n=== Search for calls to _setPWMFreq (non-weak) ==="
rg -n "_setPWMFreq\s*\(" "$file_path" | grep -v "_setPWMFreq_weak"
echo -e "\n=== Search for calls to stopWaveform (non-weak) ==="
rg -n "stopWaveform\s*\(" "$file_path" | grep -v "stopWaveform_weak" | grep -v "startWaveformClockCycles"Length of output: 3721
I need to verify the context of line 383 and check on the startWaveformClockCycles wrapper status to complete the verification.
🏁 Script executed:
#!/bin/bash
file_path="lib/ESP8266PWM/src/core_esp8266_waveform_pwm.cpp"
echo "=== Context around line 383 (which function contains _stopPWM call) ==="
sed -n '365,395p' "$file_path"
echo -e "\n=== Full context of startWaveformClockCycles wrapper (lines 410-430) ==="
sed -n '410,430p' "$file_path"
echo -e "\n=== Search for calls to startWaveformClockCycles (non-weak) ==="
rg -n "startWaveformClockCycles\s*\(" "$file_path" | grep -v "startWaveformClockCycles_weak" | grep -v "_bound"Length of output: 2950
Weak wrapper mismatch confirmed: non‑weak wrappers commented out, but call sites use non‑weak names and _bound variants
Your analysis is correct. The weakref wrapper definitions are commented out across multiple functions:
- Lines 220–226:
_setPWMFreq_boundwrapper commented - Lines 273–279:
_stopPWM_boundwrapper commented - Lines 415–423:
startWaveformClockCycles_boundwrapper commented - Lines 443–448:
setTimer1Callback_boundwrapper commented - Lines 475–480:
stopWaveform_boundwrapper commented
Call sites depend on these missing wrappers:
- Line 328:
stopWaveform(pin)from_setPWM_weak() - Line 353:
_setPWMFreq(_pwmFreq)from_setPWM_weak() - Line 383:
_stopPWM(pin)fromstartWaveformClockCycles_weak() - Line 427:
startWaveformClockCycles_bound()fromstartWaveform()— this_boundvariant doesn't exist (commented)
This creates unresolved references at link time. The wrapper definitions must be uncommented, or all call sites must switch to _weak variants:
- _stopPWM(pin);
+ _stopPWM_weak(pin);- _setPWMFreq(_pwmFreq);
+ _setPWMFreq_weak(_pwmFreq);- stopWaveform(pin);
+ stopWaveform_weak(pin);And uncomment the wrapper blocks to restore:
-/*
static void _setPWMFreq_bound(uint32_t freq) __attribute__((weakref("_setPWMFreq_weak")));
void _setPWMFreq(uint32_t freq) {
_setPWMFreq_bound(freq);
}
-*/Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/ESP8266PWM/src/core_esp8266_waveform_pwm.cpp (wrappers around lines
220–226, 273–279, 415–423, 443–448, 475–480) the weakref wrapper definitions
have been commented out, causing unresolved references at link time because call
sites use the non‑weak names and the _bound variants; restore the missing
symbols by uncommenting each weak wrapper block so the _bound functions (e.g.
_setPWMFreq_bound, _stopPWM_bound, startWaveformClockCycles_bound,
setTimer1Callback_bound, stopWaveform_bound) are defined with the correct
signatures and __attribute__((weakref("..."))) annotations, or alternatively
change all call sites to call the corresponding _weak variants
consistently—prefer uncommenting the wrapper blocks to restore original linkage
and ensure the wrapper names match the referenced weak names.
| extern void _NMILevelVector(); | ||
| extern void _UserExceptionVector_1(); // the next function after _NMILevelVector | ||
| static inline IRAM_ATTR void nmiCrashWorkaround() { | ||
| static uintptr_t epc3_backup, eps3_backup; | ||
|
|
||
| uintptr_t epc3, eps3; | ||
| __asm__ __volatile__("rsr %0,epc3; rsr %1,eps3":"=a"(epc3),"=a" (eps3)); | ||
| if ((epc3 < (uintptr_t) &_NMILevelVector) || (epc3 >= (uintptr_t) &_UserExceptionVector_1)) { |
There was a problem hiding this comment.
Declare NMI vector symbols with C linkage
These are ROM/SDK symbols; declare them with extern "C" to avoid mangling.
-extern void _NMILevelVector();
-extern void _UserExceptionVector_1(); // the next function after _NMILevelVector
+extern "C" void _NMILevelVector();
+extern "C" void _UserExceptionVector_1(); // the next function after _NMILevelVector📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extern void _NMILevelVector(); | |
| extern void _UserExceptionVector_1(); // the next function after _NMILevelVector | |
| static inline IRAM_ATTR void nmiCrashWorkaround() { | |
| static uintptr_t epc3_backup, eps3_backup; | |
| uintptr_t epc3, eps3; | |
| __asm__ __volatile__("rsr %0,epc3; rsr %1,eps3":"=a"(epc3),"=a" (eps3)); | |
| if ((epc3 < (uintptr_t) &_NMILevelVector) || (epc3 >= (uintptr_t) &_UserExceptionVector_1)) { | |
| extern "C" void _NMILevelVector(); | |
| extern "C" void _UserExceptionVector_1(); // the next function after _NMILevelVector | |
| static inline IRAM_ATTR void nmiCrashWorkaround() { | |
| static uintptr_t epc3_backup, eps3_backup; | |
| uintptr_t epc3, eps3; | |
| __asm__ __volatile__("rsr %0,epc3; rsr %1,eps3":"=a"(epc3),"=a" (eps3)); | |
| if ((epc3 < (uintptr_t) &_NMILevelVector) || (epc3 >= (uintptr_t) &_UserExceptionVector_1)) { |
🤖 Prompt for AI Agents
In lib/ESP8266PWM/src/core_esp8266_waveform_pwm.cpp around lines 509 to 516, the
ROM/SDK symbols _NMILevelVector and _UserExceptionVector_1 are declared as C++
extern functions which may be name-mangled; change their declarations to use C
linkage (extern "C") so the linker finds the correct ROM symbols, e.g. declare
them as extern "C" void _NMILevelVector(); and extern "C" void
_UserExceptionVector_1(); and keep the nmiCrashWorkaround usage unchanged.
| CURL_ARGS="--compressed --parallel --parallel-immediate --parallel-max ${PARALLEL_MAX}" | ||
| CURL_PRINT_RESPONSE_ARGS="-w %{http_code}\n" | ||
|
|
||
| JSON_TARGETS=('json/state' 'json/info' 'json/si', 'json/palettes' 'json/fxdata' 'settings/s.js?p=2') |
There was a problem hiding this comment.
Comma in Bash array splits an element; remove it
This makes 'json/si,' a literal entry.
-JSON_TARGETS=('json/state' 'json/info' 'json/si', 'json/palettes' 'json/fxdata' 'settings/s.js?p=2')
+JSON_TARGETS=('json/state' 'json/info' 'json/si' 'json/palettes' 'json/fxdata' 'settings/s.js?p=2')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| JSON_TARGETS=('json/state' 'json/info' 'json/si', 'json/palettes' 'json/fxdata' 'settings/s.js?p=2') | |
| JSON_TARGETS=('json/state' 'json/info' 'json/si' 'json/palettes' 'json/fxdata' 'settings/s.js?p=2') |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 20-20: JSON_TARGETS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 20-20: Use spaces, not commas, to separate array elements.
(SC2054)
🤖 Prompt for AI Agents
In tools/stress_test.sh around line 20, the Bash array contains a stray comma
causing the element 'json/si,' to include the comma; remove the comma so entries
are space-separated (e.g., 'json/si' 'json/palettes') ensuring no punctuation
inside the quoted elements and then save the file.
| n_packets = (num_pixels + self.max_pixels_per_packet - 1) // self.max_pixels_per_packet | ||
| idx_split = np.array_split(idx, n_packets) | ||
|
|
||
| header = bytes([1, 2]) # WARLS protocol header | ||
| for packet_indices in idx_split: | ||
| data = bytearray(header) | ||
| for i in packet_indices: | ||
| data.extend([i, *p[:, i]]) # Index and RGB values | ||
| self._sock.sendto(bytes(data), (self.wled_controller_ip, self.udp_port)) |
There was a problem hiding this comment.
Guard zero-changes path; array_split(0) raises
When no pixels change, n_packets becomes 0 and np.array_split() throws.
- n_packets = (num_pixels + self.max_pixels_per_packet - 1) // self.max_pixels_per_packet
- idx_split = np.array_split(idx, n_packets)
+ if num_pixels == 0:
+ return
+ n_packets = (num_pixels + self.max_pixels_per_packet - 1) // self.max_pixels_per_packet
+ idx_split = np.array_split(idx, n_packets)Also consider early-return before building header for the no-change case.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| n_packets = (num_pixels + self.max_pixels_per_packet - 1) // self.max_pixels_per_packet | |
| idx_split = np.array_split(idx, n_packets) | |
| header = bytes([1, 2]) # WARLS protocol header | |
| for packet_indices in idx_split: | |
| data = bytearray(header) | |
| for i in packet_indices: | |
| data.extend([i, *p[:, i]]) # Index and RGB values | |
| self._sock.sendto(bytes(data), (self.wled_controller_ip, self.udp_port)) | |
| if num_pixels == 0: | |
| return | |
| n_packets = (num_pixels + self.max_pixels_per_packet - 1) // self.max_pixels_per_packet | |
| idx_split = np.array_split(idx, n_packets) | |
| header = bytes([1, 2]) # WARLS protocol header | |
| for packet_indices in idx_split: | |
| data = bytearray(header) | |
| for i in packet_indices: | |
| data.extend([i, *p[:, i]]) # Index and RGB values | |
| self._sock.sendto(bytes(data), (self.wled_controller_ip, self.udp_port)) |
🤖 Prompt for AI Agents
In tools/udp_test.py around lines 21 to 29, n_packets can be zero when there are
no changed pixels which makes np.array_split(idx, n_packets) raise; fix this by
guarding the zero-changes path: check if the index array (idx) or num_pixels is
zero and return early before creating the header or splitting (e.g., if idx.size
== 0: return), otherwise compute n_packets and proceed; this prevents the
np.array_split error and avoids building/sending empty packets.
| #if defined(ARDUINO_ARCH_ESP32) | ||
| WLED_GLOBAL SemaphoreHandle_t jsonBufferLockMutex _INIT(xSemaphoreCreateRecursiveMutex()); | ||
| #endif |
There was a problem hiding this comment.
Potential initialization order issue with mutex creation.
Creating the recursive mutex with xSemaphoreCreateRecursiveMutex() at global scope (static initialization time) may occur before FreeRTOS is fully initialized on ESP32. This could lead to:
- Mutex creation failure (returns NULL)
- Undefined behavior if the scheduler isn't ready
- Race conditions during early initialization
Consider initializing the mutex explicitly in setup() or using a lazy initialization pattern (create on first use with a check).
// Suggested approach 1: Initialize in setup()
#if defined(ARDUINO_ARCH_ESP32)
WLED_GLOBAL SemaphoreHandle_t jsonBufferLockMutex _INIT(NULL);
#endif
// In setup() or initInterfaces():
void WLED::setup() {
#if defined(ARDUINO_ARCH_ESP32)
if (jsonBufferLockMutex == NULL) {
jsonBufferLockMutex = xSemaphoreCreateRecursiveMutex();
}
#endif
// ... rest of setup
}// Suggested approach 2: Lazy initialization with helper
#if defined(ARDUINO_ARCH_ESP32)
WLED_GLOBAL SemaphoreHandle_t jsonBufferLockMutex _INIT(NULL);
inline void ensureJsonMutexInit() {
if (jsonBufferLockMutex == NULL) {
jsonBufferLockMutex = xSemaphoreCreateRecursiveMutex();
}
}
#endif🤖 Prompt for AI Agents
In wled00/wled.h around lines 827-829, the global creation of the recursive
mutex via xSemaphoreCreateRecursiveMutex() can run before FreeRTOS is ready;
change the global initialization to NULL (do not call
xSemaphoreCreateRecursiveMutex() at static init time) and create the mutex
explicitly during WLED setup (or use a small helper for lazy init that checks
for NULL and calls xSemaphoreCreateRecursiveMutex() on first use), ensuring you
guard creation with a NULL check and store the returned handle back into
jsonBufferLockMutex.
There was a problem hiding this comment.
I think the bunny is right here, calling xSemaphoreCreateRecursiveMutex(); before we have entered main() and setup() may or may not work.
There was a problem hiding this comment.
@softhack007, thanks for confirming! You're absolutely right—calling xSemaphoreCreateRecursiveMutex() during static initialization is risky since FreeRTOS may not be ready yet.
The safest fix is to initialize the mutex handle to NULL at global scope and then create it explicitly in WLED::setup():
In wled00/wled.h (around line 827):
#if defined(ARDUINO_ARCH_ESP32)
WLED_GLOBAL SemaphoreHandle_t jsonBufferLockMutex _INIT(NULL);
#endifIn wled00/wled.cpp (in the setup() function):
void WLED::setup() {
#if defined(ARDUINO_ARCH_ESP32)
if (jsonBufferLockMutex == NULL) {
jsonBufferLockMutex = xSemaphoreCreateRecursiveMutex();
}
#endif
// ... rest of setup code
}This ensures the mutex is created after FreeRTOS is fully initialized and avoids any potential race conditions or initialization failures. You should also add a check in the lock/unlock functions (in wled00/util.cpp) to handle the case where mutex creation might fail, though that should be rare.
There was a problem hiding this comment.
The things I have learned since I wrote that code!
In practice it works: this is because xSemaphoreCreateRecursiveMutex() happens to need only malloc() to be available, it doesn't interact with any of FreeRTOS's internal data structures. malloc() must be ready to use as part of the C runtime before any static initializers are run.
That said, better practice is to use a pre-allocated StaticSemaphore_t and xSemaphoreCreateRecursiveMutexStatic() in setup(); it can't fail if the memory is valid, so there's no need to consider the failure case at point of usage. (Also saves 8 bytes of heap overhead.)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
this is not true (AI hallucination???) . SEGENV.step is defined as 32bit in fx.h (
this could be a real, really old bug. need to double-check --> #272
|
4 participants
pending merge:
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Improvements
UI/UX