correctly handle overflow, restore initial direction changing logic

[limpkin]

Summary by CodeRabbit

• Bug Fixes
  • Fixed star brightness calculations to prevent overflow and underflow during fading animations.
  • Standardized fade transition randomness for more consistent visual behavior.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)

Validation error: Unrecognized key(s) in object: 'review', 'context'

⚙️ Configuration instructions

• Please see the configuration documentation for more information.
• You can also validate your configuration using the online YAML validator.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

📝 Walkthrough

Walkthrough

Star brightness updates in the MoonLight effect are modified to prevent numeric overflow and underflow using explicit clamping guards. Random fade direction change thresholds are standardized from variable star_speed to a fixed value of 10 for both fading states.

Changes

Cohort / File(s)	Summary
Star Brightness Update Logic src/MoonLight/Nodes/Effects/E_MoonLight.h	Adds explicit boundary checks for fade-in brightness increments to prevent UINT8 overflow. Adds underflow guards for fade-out brightness decrements. Replaces variable star_speed threshold with fixed value (10) for random fade direction triggers in both fade states.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• starsky effect #96: Also modifies star-effect code in E_MoonLight.h regarding per-star brightness and update behavior.

Poem

🌟 The stars grew bright but overflowed,
Then dimmed to nothing when they slowed,
A canny coder came to mend,
With guards and clamps to comprehend,
Now starlight dances as it glowed! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes: fixing numeric overflow/underflow handling and restoring randomness logic for direction changes in star fading.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c200c3f and 12db236.

📒 Files selected for processing (1)

• src/MoonLight/Nodes/Effects/E_MoonLight.h

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.

Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.

Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.

📚 Learning: 2025-12-28T10:28:24.157Z

Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.

Applied to files:

• src/MoonLight/Nodes/Effects/E_MoonLight.h

📚 Learning: 2026-01-06T15:24:29.770Z

Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.

Applied to files:

• src/MoonLight/Nodes/Effects/E_MoonLight.h

📚 Learning: 2026-01-06T15:24:17.118Z

Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.

Applied to files:

• src/MoonLight/Nodes/Effects/E_MoonLight.h

🔇 Additional comments (2)

src/MoonLight/Nodes/Effects/E_MoonLight.h (2)

116-124: Overflow protection looks correct; consider documenting the random threshold change.

The overflow protection on line 117 correctly prevents stars_brightness[i] from exceeding UINT8_MAX when incremented by star_speed.

However, changing the random direction threshold from star_speed to the fixed value 10 on line 122 is a behavioral change. Previously, the probability of random direction changes varied with speed (0-42 out of 256, or 0-16.4%). Now it's fixed at ~3.9% (10/256) regardless of speed. This affects the "twinkling" behavior of stars.

Consider whether this standardized threshold was intentional. If star_speed ranged up to 42, fast speeds would have had ~16% chance of direction change per frame, creating more erratic twinkling. The fixed threshold of 10 makes behavior more consistent across speeds. Is this the desired effect?

---

125-135: Underflow protection implemented correctly.

The underflow protection on line 126 correctly prevents stars_brightness[i] from wrapping around when decremented by star_speed. If the current brightness is less than star_speed, it clamps to 0 instead of underflowing.

The random threshold change on line 132 is consistent with the fade-in branch (line 122), which is good for symmetry in star behavior.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}