Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
WalkthroughAdds many new LiveScript effects and three board layouts, deletes an old SE16 script, updates LiveScript docs, and modifies the LiveScript runtime to route calls per FreeRTOS task (task→node registry) while exposing new external APIs (RGB/HSV, 2D/3D setters, palette helpers, drawing primitives) and time-of-day exports. Changes
Sequence Diagram(s)sequenceDiagram
participant Task as Script Task
participant Registry as Task→Node Registry
participant Node as LiveScriptNode
participant HW as Pixel Layer
Task->>Registry: registerNodeForTask(taskHandle, node)
Registry-->>Task: mapped
Task->>Node: call external (e.g., setRGBXY(x,y,color))
Node->>HW: perform pixel operation in node context
HW-->>Node: ack
Task->>Registry: unregisterNodeForTask(taskHandle) on kill
Registry-->>Task: unmapped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@livescripts/Layouts/L_LC16.sc`:
- Around line 6-10: The slider for ledsPerPin allows values up to 255 which
leads to y coordinates up to 509 and overflow of uint8_t in addLight; change the
control and runtime clamping so ledsPerPin cannot exceed the safe maximum (128).
Specifically, update the addControl call that creates the "ledsPerPin" slider to
set the max to 128, and add a small guard where ledsPerPin is used (e.g., before
computing y ranges or calling addLight) to clamp the value to 128 so runtime
values cannot overflow uint8_t; reference the variable ledsPerPin and the
addControl(...) line as the change points and ensure any code paths producing y
= ledsPerPin .. 2*ledsPerPin-1 respect the clamp.
In `@livescripts/Layouts/L_R16.sc`:
- Around line 5-8: The slider allows scale up to 10 which causes coordinate
overflow when passed to addLight; either cap the slider's maximum to the safe
value (change addControl(&scale, "scale", "slider", 1, 3) so scale never exceeds
3) or clamp the computed x/y in addRing()/before calling addLight() to fall
within 0..255 (e.g., compute scaledX = min(max(rawX*scale, 0), 255) and same for
Y) so addLight() always receives valid uint8_t coordinates; update references to
the global variable scale, the addControl call, and the addRing/addLight call
sites accordingly.
In `@livescripts/Layouts/L_SE16.sc`:
- Around line 3-8: The y-coordinate can overflow uint8_t when ledsPerPin > 128
in mirroredPins mode; update the slider control for ledsPerPin in setup() (the
addControl call that currently uses max 255) to cap the maximum at 128 so users
cannot select values that cause coordinate overflow; keep the ledsPerPin
variable and mirroredPins logic unchanged and only adjust the slider's max
argument in the addControl for "ledsPerPin".
🪄 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: d1ff1af7-0bf8-4b4e-90bb-44a5b096c6df
📒 Files selected for processing (18)
docs/moonbase/inputoutput.mddocs/moonlight/livescripts.mdlivescripts/Effects/E_GEQ2D.sclivescripts/Effects/E_GyroBall.sclivescripts/Effects/E_PanTilt.sclivescripts/Effects/E_lines.sclivescripts/Effects/E_noise.sclivescripts/Effects/E_octo.sclivescripts/Effects/E_random.sclivescripts/Layouts/L_LC16.sclivescripts/Layouts/L_R16.sclivescripts/Layouts/L_SE16.sclivescripts/Layouts/L_panel.sclivescripts/Layouts/L_panels.sclivescripts/Layouts/L_rings241.sclivescripts/Palettes/P_Fire.sclivescripts/Palettes/P_Shift.scmisc/livescripts/L_SE16.sc
💤 Files with no reviewable changes (1)
- misc/livescripts/L_SE16.sc
Live Scripts: - add ColorFromPalette, getRGB, setRGBXY(Z), drawLine, drawCircle, time - fix gNode - Add arti-fx effects
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
src/MoonBase/LiveScriptNode.cpp (2)
376-404: Consider extracting duplicated unregistration logic.The task unregistration code (lines 379-384 and 395-400) is identical in
kill()andkillAndDelete(). A small helper function could reduce duplication.♻️ Optional helper extraction
// Add as private helper void LiveScriptNode::unregisterCurrentTask() { Executable* exec = scriptRuntime.findExecutable(animation.c_str()); if (exec && exec->__run_handle_index != 9999) { TaskHandle_t h = *runningPrograms.getHandleByIndex(exec->__run_handle_index); if (h) unregisterNodeForTask(h); } }Then in
kill()andkillAndDelete():void LiveScriptNode::kill() { EXT_LOGV(MB_TAG, "%s", animation.c_str()); hasLoopTask = false; - // 🌙 Unregister task → node mapping before the task is deleted. - Executable* exec = scriptRuntime.findExecutable(animation.c_str()); - if (exec && exec->__run_handle_index != 9999) { - TaskHandle_t h = *runningPrograms.getHandleByIndex(exec->__run_handle_index); - if (h) unregisterNodeForTask(h); - } + unregisterCurrentTask(); // 🌙 Unregister task → node mapping before deletion scriptRuntime.kill(animation.c_str()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/LiveScriptNode.cpp` around lines 376 - 404, Extract the duplicated unregistration block from LiveScriptNode::kill and LiveScriptNode::killAndDelete into a private helper (e.g., LiveScriptNode::unregisterCurrentTask): move the logic that calls scriptRuntime.findExecutable(animation.c_str()), checks exec->__run_handle_index != 9999, obtains TaskHandle_t h via runningPrograms.getHandleByIndex(exec->__run_handle_index) and calls unregisterNodeForTask(h) if h is non-null; then replace the duplicated blocks in kill() and killAndDelete() with a call to unregisterCurrentTask().
81-85: Consider usinglocaltime_r()for thread-safety.
localtime()returns a pointer to a static buffer that can be overwritten by subsequent calls. While currently_updateTime()is only called fromloop()on the effectTask, using the reentrantlocaltime_r()would be safer for future changes.🛡️ Proposed thread-safe alternative
static void _updateTime() { time_t now = time(nullptr); - struct tm* t = localtime(&now); - if (t) { _lsHour = t->tm_hour; _lsMinute = t->tm_min; _lsSecond = t->tm_sec; } + struct tm t; + if (localtime_r(&now, &t)) { _lsHour = t.tm_hour; _lsMinute = t.tm_min; _lsSecond = t.tm_sec; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/LiveScriptNode.cpp` around lines 81 - 85, The _updateTime() function uses localtime(&now) which is not thread-safe; replace it with the reentrant localtime_r variant by creating a local struct tm (e.g., struct tm local_tm), call localtime_r(&now, &local_tm), check the return value, and then assign _lsHour = local_tm.tm_hour, _lsMinute = local_tm.tm_min, _lsSecond = local_tm.tm_sec; update the function body accordingly so _updateTime() uses localtime_r instead of localtime to avoid static-buffer races.livescripts/Effects/Arti-FX/E_Mover.sc (1)
1-18: Clean moving segment effect with proper modulo wrapping.The effect correctly clears the strip each frame and draws three colored segments that move smoothly. The modulo wrapping ensures positions stay valid regardless of strip length. Using
fadeToBlackBy()instead of the manual clear loop could be slightly more efficient:♻️ Optional: Use fadeToBlackBy for clearing
void loop() { - for (int i = 0; i < NUM_LEDS; i++) { - setRGB(i, CRGB(0, 0, 0)); - } + fadeToBlackBy(255); // instant clear int locn = millis() / 100;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@livescripts/Effects/Arti-FX/E_Mover.sc` around lines 1 - 18, Current per-frame clearing loop in loop() that calls setRGB for each index is inefficient; replace that manual clear with a single fadeToBlackBy call to operate on the whole LED buffer (using NUM_LEDS) for better performance and optional trailing fade. In other words, remove the for-loop that zeroes LEDs and call fadeToBlackBy with the LED array/buffer and NUM_LEDS, choosing a fade amount (255 for immediate clear or a smaller value for trails) so the subsequent setHSV calls (p1, p2, p3) still overlay correctly.livescripts/Effects/Arti-FX/E_RainbowFonts2.sc (1)
11-11: Unused variable:offsetLis computed but never used.The
offsetLvariable is calculated but not referenced in the loop. Consider removing it or using it if it was intended for the effect.💡 Remove unused variable
float offset = sin(timeOff * PI2) * NUM_LEDS / 10.0; - float offsetL = offset / NUM_LEDS; for (int i = 0; i < NUM_LEDS; i++) {Note: If
offsetLwas intended to be used in line 18 instead of a constant, verify the original effect's behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@livescripts/Effects/Arti-FX/E_RainbowFonts2.sc` at line 11, The variable offsetL (computed as offset / NUM_LEDS) is unused; either remove the declaration or apply it where intended (likely in the hue/position math inside the loop that currently uses a constant). Update the code around the loop that computes colors (referencing offset, NUM_LEDS and offsetL) to replace the constant with offsetL if the effect should be scaled by LED count, or simply delete the offsetL declaration if it was unnecessary.livescripts/Effects/Arti-FX/E_GlitchBands.sc (1)
52-52: Redundant expression:1.0 - (1.0 - stt)simplifies tostt.The expression on line 52 is mathematically equivalent to just
stt. This appears to be leftover from a different formula or copy-paste artifact.💡 Simplified
- float s = 1.0 - (1.0 - stt); + float s = stt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@livescripts/Effects/Arti-FX/E_GlitchBands.sc` at line 52, The assignment to variable s uses a redundant expression "1.0 - (1.0 - stt)"; in the E_GlitchBands.sc snippet replace that redundant formula so s is directly derived from stt (i.e., set s equal to stt)—locate the statement initializing s and simplify it to remove the unnecessary arithmetic.livescripts/Effects/Arti-FX/E_SpinCycle.sc (1)
7-10: Redundant variables:t1 == t2andw1 == w2.Lines 7-8 compute identical values, making
t2and consequentlyw2redundant. If this is intentional for readability/future expansion, consider adding a comment. Otherwise, simplify or use different periods.💡 Simplified version
float t1 = (millis() % 6553) / 6553.0; - float t2 = (millis() % 6553) / 6553.0; float w1 = (1.0 + sin(t1 * PI2)) / 2.0; - float w2 = (1.0 + sin(t2 * PI2)) / 2.0;Then replace
w2withw1on line 14.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@livescripts/Effects/Arti-FX/E_SpinCycle.sc` around lines 7 - 10, The variables t1 and t2 (and therefore w1 and w2) are computed identically so t2/w2 are redundant; either remove t2 and w2 and reuse t1/w1 (update the use on the later line that currently references w2 to use w1) or intentionally differentiate them by changing the modulo/divisor or phase for t2 (e.g., use a different period) and document the intent with a comment; locate the calculations of t1/t2 and w1/w2 and the later usage of w2 in the SpinCycle effect to apply the chosen fix.livescripts/Effects/Arti-FX/E_ColorTwinkleBounce.sc (1)
7-8: Redundant computation:t1andt2are identical.Both variables use the same formula
(millis() % 3276) / 3276.0 * PI2, makingt2redundant. This appears to be a copy-paste artifact from the original conversion. Consider whethert2should use a different period for added variation, or remove it if intentional.💡 Option: use different periods for distinct behavior
float t1 = (millis() % 3276) / 3276.0 * PI2; - float t2 = (millis() % 3276) / 3276.0 * PI2; + float t2 = (millis() % 6553) / 6553.0 * PI2; // or simply reuse t1 float tb = (millis() % 6553) / 6553.0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@livescripts/Effects/Arti-FX/E_ColorTwinkleBounce.sc` around lines 7 - 8, t2 is computed identically to t1 (both use (millis() % 3276) / 3276.0 * PI2) so it is redundant; either remove t2 and replace its uses with t1, or give t2 a distinct period to produce different behavior (e.g., change the modulus/divisor for t2 or multiply by a different scalar) so t1 and t2 are not identical—update all uses of t2 accordingly (look for variables named t1, t2 and the millis()/PI2 expressions in this script).livescripts/Effects/Arti-FX/E_SlowColorShift.sc (1)
12-12: Unused variable:ilis declared but never used.The normalized position
ilis computed but not referenced anywhere in the loop body. This appears to be leftover from the original effect or a copy-paste artifact.💡 Remove unused variable
for (int i = 0; i < NUM_LEDS; i++) { - float il = i * 1.0 / NUM_LEDS; float a1 = i / 2.0 + 5.0 * sin(t1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@livescripts/Effects/Arti-FX/E_SlowColorShift.sc` at line 12, The variable 'il' is declared but not used anywhere in the loop in the E_SlowColorShift effect. To fix this, remove the declaration and assignment of 'il' to clean up the code and avoid unused variable warnings.livescripts/Effects/Arti-FX/E_BlockReflections.sc (1)
34-35: Unnecessary absolute value:mis always positive.Since
m = 0.3 + trit1 * 0.2(line 22) andtrit1 ∈ [0, 1],mis always in range[0.3, 0.5]. The absolute value computation on lines 34-35 is redundant.💡 Simplified
- float vm = m; - if (vm < 0.0) vm = 0.0 - vm; - float v = (vv + vm + t1); + float v = (vv + m + t1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@livescripts/Effects/Arti-FX/E_BlockReflections.sc` around lines 34 - 35, The code computes vm as an absolute value of m but m is already guaranteed positive (m = 0.3 + trit1 * 0.2 with trit1 in [0,1]); remove the redundant absolute-value logic and assign vm directly from m (replace the vm = m; if (vm < 0.0) vm = 0.0 - vm; block with a simple vm = m;), keeping references to the variables m and vm so the surrounding logic uses the direct value.livescripts/Effects/Arti-FX/E_Smiley.sc (1)
11-14: Use the smaller axis for face radius on rectangular panels.Line 13 scales radius from
widthonly, which can clip vertically on non-square layouts. Basing radius onmin(width, height)will keep the face contained.💡 Suggested refactor
- int radius = width * 2 / 5; + int minDim = width; + if (height < minDim) minDim = height; + int radius = minDim * 2 / 5;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@livescripts/Effects/Arti-FX/E_Smiley.sc` around lines 11 - 14, The face radius is computed from width only (radius = width * 2 / 5) and can be clipped on non-square panels; update the radius calculation used before drawCircle(cx, cy, radius, faceColor) to use the smaller axis (e.g., radius based on min(width, height) * 2 / 5) so the circle always fits within the panel while keeping cx and cy as width/2 and height/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 `@livescripts/Effects/Arti-FX/E_Beatmania.sc`:
- Around line 17-31: The variable locn3 is calculated but never used; add a
third draw call using setRGBPal to render that mover. Modify the block that
currently calls setRGBPal for locn12, locn1 and locn2 to include a fourth/set
additional call like setRGBPal(locn3, <choose color value such as colr1 or colr2
or a combined colr12>, <choose brightness such as bri1 or bri2 or bri12>) so
locn3 is actually written to the LED buffer; use the existing symbols locn3,
colr1/colr2/colr12 and bri1/bri2/bri12 to pick the color/brightness you want.
- Line 31: Remove the redundant and unsafe modulo in the call to setRGBPal so it
uses locn2 directly (setRGBPal(locn2, colr1, bri2)), since locn2 is already
constrained by beatsin8 and the modulo can drop the final LED and divide-by-zero
when NUM_LEDS == 1; also add the missing third render using locn3 (e.g., a
setRGBPal(locn3, colrX, briY) call) where the other two renders occur so the
computed locn3 is actually used. Ensure references: locn2, locn3, setRGBPal,
NUM_LEDS, and beatsin8.
In `@livescripts/Effects/Arti-FX/E_Drip.sc`:
- Around line 24-31: The drop rendering/reseting has an off-by-one: it skips LED
index 0 because rendering only occurs when dripLocn > 0.0 and the reset uses <=
0.0; change the logic so index 0 is drawn and only reset when the drop goes
strictly below 0.0. Concretely, update the render check around
setRGBPal((int)dripLocn, colr, bri) to include 0 (use >= 0.0) and change the
reset branch to trigger on dripLocn < 0.0 so dripLocn is set to NUM_LEDS - 1 and
dripSpd = 0.0 only after it passes below zero.
In `@livescripts/Effects/Arti-FX/E_FireworkSparks.sc`:
- Around line 26-29: The value channel passed to setHSV can exceed 255 because
(sv + vout) may be >1.0; clamp the computed value before scaling to bytes by
computing a clampedValue = clamp(sv + vout, 0.0, 1.0) (using the project
clamp/CLAMP utility or a min/max) and then call setHSV(i, h * 255, sv * 255,
clampedValue * 255) so the uint8_t conversion cannot wrap; update the block
around variables sv, vout and the setHSV call accordingly.
In `@livescripts/Effects/Arti-FX/E_Opposites.sc`:
- Around line 27-29: Brightness computation for v can exceed 1.0 and overflow
when multiplying by 255 before setHSV; clamp v into the valid [0.0,1.0] range
after computing it (using e.g. clamp/min/max on the variable v computed from
w1,w2,w3) so that setHSV(i, h * 255, 255, v * 255) never receives a value >1.0;
update the code around the v calculation (the float v = ... line and right
before setHSV) to enforce v = clamp(v, 0.0, 1.0) or v = min(v, 1.0).
In `@livescripts/Effects/Arti-FX/E_PerlinMove.sc`:
- Line 15: The fade behavior is inverted here: change the call to
fadeToBlackBy(255 - fade) so it passes the fade variable directly (use fade) to
match other effects; update the call to fadeToBlackBy(fade) in this script (the
fadeToBlackBy call using the fade variable) so higher fade values increase
fading consistently with E_Sinelon.sc, E_Ripple.sc, E_Drip.sc, etc.
In `@livescripts/Effects/Arti-FX/E_RainbowMelt.sc`:
- Around line 9-15: The code computes hl = NUM_LEDS / 2 and then divides by hl
when computing c1, which will divide by zero if NUM_LEDS is 0 or 1; guard
against that by ensuring hl is nonzero before the loop (e.g., set hl = max(1,
NUM_LEDS/2) or return/skip processing when NUM_LEDS < 2) and adjust the c1
calculation accordingly; modify the logic around hl and the for-loop in
E_RainbowMelt.sc so that hl, NUM_LEDS, and the c1 = 1.0 - diff * 1.0 / hl
expression cannot trigger a zero-denominator.
In `@livescripts/Effects/Arti-FX/E_Subpixel.sc`:
- Around line 19-22: bri can reach 256 and overflow the uint8_t parameter of
setHSV; clamp bri to the valid range before calling setHSV by ensuring bri =
min(bri, 255) (and keep the existing lower clamp bri = 0) where bri is computed
from diff and reverseSlider; update the code around the bri calculation (the
block that sets int bri = 256 - (int)(diff * reverseSlider); if (bri < 0) bri =
0;) to enforce an upper bound of 255 before calling setHSV(i, 0, 255, bri).
In `@livescripts/Effects/Arti-FX/E_WaveSins.sc`:
- Line 21: The code uses uint8_t arithmetic for custom1 + custom2 which can
overflow and make beatsin8's highest < lowest; change the computation to use a
wider type (e.g., uint16_t or int) to compute sum = (uint16_t)custom1 +
(uint16_t)custom2, clamp the sum to 255 (or ensure sum >= custom1) before
calling beatsin8, and then pass the clamped value as the highest parameter to
beatsin8 when computing palIdx so beatsin8 always receives a valid
lowest<=highest range.
In `@misc/upstream-prs/ESPLiveScript-Coord3D.md`:
- Around line 19-25: The fenced code block starting at the crash snippet lacks a
language tag and contains a typo; update the snippet to use a fenced block with
an explicit language (e.g., ```text) and correct "foudn" to "found" so the
snippet reads "member not found in Coord3D", and ensure the block is properly
closed with a trailing ```; this will satisfy MD040 and fix the typo in the
crash output shown (references: the snippet lines showing "member not foudn in
Coord3D" and the following Guru Meditation stack trace).
---
Nitpick comments:
In `@livescripts/Effects/Arti-FX/E_BlockReflections.sc`:
- Around line 34-35: The code computes vm as an absolute value of m but m is
already guaranteed positive (m = 0.3 + trit1 * 0.2 with trit1 in [0,1]); remove
the redundant absolute-value logic and assign vm directly from m (replace the vm
= m; if (vm < 0.0) vm = 0.0 - vm; block with a simple vm = m;), keeping
references to the variables m and vm so the surrounding logic uses the direct
value.
In `@livescripts/Effects/Arti-FX/E_ColorTwinkleBounce.sc`:
- Around line 7-8: t2 is computed identically to t1 (both use (millis() % 3276)
/ 3276.0 * PI2) so it is redundant; either remove t2 and replace its uses with
t1, or give t2 a distinct period to produce different behavior (e.g., change the
modulus/divisor for t2 or multiply by a different scalar) so t1 and t2 are not
identical—update all uses of t2 accordingly (look for variables named t1, t2 and
the millis()/PI2 expressions in this script).
In `@livescripts/Effects/Arti-FX/E_GlitchBands.sc`:
- Line 52: The assignment to variable s uses a redundant expression "1.0 - (1.0
- stt)"; in the E_GlitchBands.sc snippet replace that redundant formula so s is
directly derived from stt (i.e., set s equal to stt)—locate the statement
initializing s and simplify it to remove the unnecessary arithmetic.
In `@livescripts/Effects/Arti-FX/E_Mover.sc`:
- Around line 1-18: Current per-frame clearing loop in loop() that calls setRGB
for each index is inefficient; replace that manual clear with a single
fadeToBlackBy call to operate on the whole LED buffer (using NUM_LEDS) for
better performance and optional trailing fade. In other words, remove the
for-loop that zeroes LEDs and call fadeToBlackBy with the LED array/buffer and
NUM_LEDS, choosing a fade amount (255 for immediate clear or a smaller value for
trails) so the subsequent setHSV calls (p1, p2, p3) still overlay correctly.
In `@livescripts/Effects/Arti-FX/E_RainbowFonts2.sc`:
- Line 11: The variable offsetL (computed as offset / NUM_LEDS) is unused;
either remove the declaration or apply it where intended (likely in the
hue/position math inside the loop that currently uses a constant). Update the
code around the loop that computes colors (referencing offset, NUM_LEDS and
offsetL) to replace the constant with offsetL if the effect should be scaled by
LED count, or simply delete the offsetL declaration if it was unnecessary.
In `@livescripts/Effects/Arti-FX/E_SlowColorShift.sc`:
- Line 12: The variable 'il' is declared but not used anywhere in the loop in
the E_SlowColorShift effect. To fix this, remove the declaration and assignment
of 'il' to clean up the code and avoid unused variable warnings.
In `@livescripts/Effects/Arti-FX/E_Smiley.sc`:
- Around line 11-14: The face radius is computed from width only (radius = width
* 2 / 5) and can be clipped on non-square panels; update the radius calculation
used before drawCircle(cx, cy, radius, faceColor) to use the smaller axis (e.g.,
radius based on min(width, height) * 2 / 5) so the circle always fits within the
panel while keeping cx and cy as width/2 and height/2.
In `@livescripts/Effects/Arti-FX/E_SpinCycle.sc`:
- Around line 7-10: The variables t1 and t2 (and therefore w1 and w2) are
computed identically so t2/w2 are redundant; either remove t2 and w2 and reuse
t1/w1 (update the use on the later line that currently references w2 to use w1)
or intentionally differentiate them by changing the modulo/divisor or phase for
t2 (e.g., use a different period) and document the intent with a comment; locate
the calculations of t1/t2 and w1/w2 and the later usage of w2 in the SpinCycle
effect to apply the chosen fix.
In `@src/MoonBase/LiveScriptNode.cpp`:
- Around line 376-404: Extract the duplicated unregistration block from
LiveScriptNode::kill and LiveScriptNode::killAndDelete into a private helper
(e.g., LiveScriptNode::unregisterCurrentTask): move the logic that calls
scriptRuntime.findExecutable(animation.c_str()), checks exec->__run_handle_index
!= 9999, obtains TaskHandle_t h via
runningPrograms.getHandleByIndex(exec->__run_handle_index) and calls
unregisterNodeForTask(h) if h is non-null; then replace the duplicated blocks in
kill() and killAndDelete() with a call to unregisterCurrentTask().
- Around line 81-85: The _updateTime() function uses localtime(&now) which is
not thread-safe; replace it with the reentrant localtime_r variant by creating a
local struct tm (e.g., struct tm local_tm), call localtime_r(&now, &local_tm),
check the return value, and then assign _lsHour = local_tm.tm_hour, _lsMinute =
local_tm.tm_min, _lsSecond = local_tm.tm_sec; update the function body
accordingly so _updateTime() uses localtime_r instead of localtime to avoid
static-buffer races.
🪄 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: 1de7e072-a9cc-4f47-97bf-ca4effcc9b1a
📒 Files selected for processing (44)
docs/moonlight/livescripts.mdlivescripts/Effects/Arti-FX/E_Beatmania.sclivescripts/Effects/Arti-FX/E_BlockReflections.sclivescripts/Effects/Arti-FX/E_BrightPulse.sclivescripts/Effects/Arti-FX/E_Clock.sclivescripts/Effects/Arti-FX/E_Clock2D.sclivescripts/Effects/Arti-FX/E_ColorFadePulse.sclivescripts/Effects/Arti-FX/E_ColorRandom.sclivescripts/Effects/Arti-FX/E_ColorTwinkleBounce.sclivescripts/Effects/Arti-FX/E_ColorTwinkles.sclivescripts/Effects/Arti-FX/E_Drip.sclivescripts/Effects/Arti-FX/E_EdgeBurst.sclivescripts/Effects/Arti-FX/E_FFTBands.sclivescripts/Effects/Arti-FX/E_FastPulse.sclivescripts/Effects/Arti-FX/E_FireworkDust.sclivescripts/Effects/Arti-FX/E_FireworkSparks.sclivescripts/Effects/Arti-FX/E_GlitchBands.sclivescripts/Effects/Arti-FX/E_GreenRipple.sclivescripts/Effects/Arti-FX/E_HalloweenTwinkles.sclivescripts/Effects/Arti-FX/E_Kitt.sclivescripts/Effects/Arti-FX/E_MarchingRainbow.sclivescripts/Effects/Arti-FX/E_Matrix2DPulse.sclivescripts/Effects/Arti-FX/E_Millipede.sclivescripts/Effects/Arti-FX/E_Mover.sclivescripts/Effects/Arti-FX/E_Opposites.sclivescripts/Effects/Arti-FX/E_PerlinMove.sclivescripts/Effects/Arti-FX/E_PhaseShift.sclivescripts/Effects/Arti-FX/E_RainbowFonts.sclivescripts/Effects/Arti-FX/E_RainbowFonts2.sclivescripts/Effects/Arti-FX/E_RainbowMelt.sclivescripts/Effects/Arti-FX/E_RainbowPinwheel.sclivescripts/Effects/Arti-FX/E_Ripple.sclivescripts/Effects/Arti-FX/E_Shift.sclivescripts/Effects/Arti-FX/E_Sinelon.sclivescripts/Effects/Arti-FX/E_SlowColorShift.sclivescripts/Effects/Arti-FX/E_Smiley.sclivescripts/Effects/Arti-FX/E_Snake.sclivescripts/Effects/Arti-FX/E_SpinCycle.sclivescripts/Effects/Arti-FX/E_Subpixel.sclivescripts/Effects/Arti-FX/E_TwinkleUp.sclivescripts/Effects/Arti-FX/E_WaveSins.sclivescripts/Effects/E_GEQ2D.scmisc/upstream-prs/ESPLiveScript-Coord3D.mdsrc/MoonBase/LiveScriptNode.cpp
✅ Files skipped from review due to trivial changes (6)
- livescripts/Effects/Arti-FX/E_ColorRandom.sc
- livescripts/Effects/Arti-FX/E_RainbowPinwheel.sc
- livescripts/Effects/Arti-FX/E_Matrix2DPulse.sc
- livescripts/Effects/Arti-FX/E_RainbowFonts.sc
- livescripts/Effects/Arti-FX/E_EdgeBurst.sc
- livescripts/Effects/Arti-FX/E_Kitt.sc
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/moonlight/livescripts.md
Comment on lines
+17
to
+31
| uint8_t locn3 = beatsin8(speed / 5 + 1, 0, NUM_LEDS / 2 + NUM_LEDS / 3, 0, 0); | ||
|
|
||
| uint8_t colr1 = beatsin8(intensity / 6 + 1, 0, 255, 0, 0); | ||
| uint8_t colr2 = beatsin8(intensity / 7 + 1, 0, 255, 0, 0); | ||
|
|
||
| uint8_t bri1 = beatsin8(intensity / 6 + 1, 32, 255, 0, 0); | ||
| uint8_t bri2 = beatsin8(intensity / 7 + 1, 32, 255, 0, 0); | ||
|
|
||
| int locn12 = (locn1 + locn2) % NUM_LEDS; | ||
| uint8_t colr12 = colr1 + colr2; | ||
| uint8_t bri12 = bri1 + bri2; | ||
|
|
||
| setRGBPal(locn12, colr12, bri12); | ||
| setRGBPal(locn1, colr2, bri1); | ||
| setRGBPal(locn2 % (NUM_LEDS - 1), colr1, bri2); |
There was a problem hiding this comment.
locn3 is computed but never rendered.
At Line 17, locn3 is unused in output writes (Lines 29-31). If the intent is three beat movers, add a third setRGBPal call for locn3.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@livescripts/Effects/Arti-FX/E_Beatmania.sc` around lines 17 - 31, The
variable locn3 is calculated but never used; add a third draw call using
setRGBPal to render that mover. Modify the block that currently calls setRGBPal
for locn12, locn1 and locn2 to include a fourth/set additional call like
setRGBPal(locn3, <choose color value such as colr1 or colr2 or a combined
colr12>, <choose brightness such as bri1 or bri2 or bri12>) so locn3 is actually
written to the LED buffer; use the existing symbols locn3, colr1/colr2/colr12
and bri1/bri2/bri12 to pick the color/brightness you want.
| void loop() { | ||
| for (int i = 0; i < NUM_LEDS; i++) { | ||
| float bri = sin(millis() / 4.0 + i * intensity) * 128.0 + 128.0; | ||
| uint8_t palIdx = beatsin8(speed, custom1, custom1 + custom2, 0, i * custom3); |
There was a problem hiding this comment.
Potential uint8_t overflow in custom1 + custom2.
When custom1 and custom2 are both uint8_t, their sum can exceed 255 and wrap around. For example, if custom1=200 and custom2=128, the result wraps to 72 instead of 328. This causes beatsin8's highest parameter to be less than lowest, which may produce unexpected oscillation behavior.
Consider clamping the sum or adjusting the slider range:
🛡️ Option: clamp the sum to 255
- uint8_t palIdx = beatsin8(speed, custom1, custom1 + custom2, 0, i * custom3);
+ uint8_t highest = (custom1 + custom2 > 255) ? 255 : custom1 + custom2;
+ uint8_t palIdx = beatsin8(speed, custom1, highest, 0, i * custom3);📝 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.
Suggested change
| uint8_t palIdx = beatsin8(speed, custom1, custom1 + custom2, 0, i * custom3); | |
| uint16_t sum = (uint16_t)custom1 + (uint16_t)custom2; | |
| uint8_t highest = (sum > 255) ? 255 : (uint8_t)sum; | |
| uint8_t palIdx = beatsin8(speed, custom1, highest, 0, i * custom3); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@livescripts/Effects/Arti-FX/E_WaveSins.sc` at line 21, The code uses uint8_t
arithmetic for custom1 + custom2 which can overflow and make beatsin8's highest
< lowest; change the computation to use a wider type (e.g., uint16_t or int) to
compute sum = (uint16_t)custom1 + (uint16_t)custom2, clamp the sum to 255 (or
ensure sum >= custom1) before calling beatsin8, and then pass the clamped value
as the highest parameter to beatsin8 when computing palIdx so beatsin8 always
receives a valid lowest<=highest range.
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.
Summary by CodeRabbit
New Features
Documentation