bugfix to prevent "infinite" loop in palette blend #4841
Conversation
this fixes a very long loop when an overflow was happening in palette blending. - reset prevPaletteBlends to prevent overflow - add safety check in case overflow should still happen in another combination
WalkthroughUpdates in wled00/FX_fcn.cpp adjust transition handling and blending safety: resetting a per-transition palette-blend counter when copying an active transition, and capping FADE blending passes to a maximum of 255. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/FX_fcn.cpp (1)
371-375: Clamp is good; also guard against negative-diff underflow to avoid extra workCapping
noOfBlendsat 255 prevents pathological runtimes. To fully eliminate underflow and avoid doing 255 blends when the target decreased (e.g., mid-transition restarts), compute the target first and only blend if it increased since the last run.- unsigned noOfBlends = ((255U * prog) / 0xFFFFU) - _t->_prevPaletteBlends; - if(noOfBlends > 255) noOfBlends = 255; // safety check + unsigned targetBlends = (255U * prog) / 0xFFFFU; + unsigned noOfBlends = 0; + if (targetBlends > _t->_prevPaletteBlends) { + noOfBlends = targetBlends - _t->_prevPaletteBlends; + if (noOfBlends > 255U) noOfBlends = 255U; // cap work per frame + }This preserves the safety cap while ensuring we never over-blend due to unsigned wraparound when the target goes backwards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wled00/FX_fcn.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
🔇 Additional comments (1)
wled00/FX_fcn.cpp (1)
282-291: Confirmed:_prevPaletteBlendsand_palTdeclarations match build flags and our guard ensures safe compilationInspection of wled00/FX.h shows:
_prevPaletteBlendsis declared unconditionally (line 512)._palTis only declared under#ifndef WLED_SAVE_RAM(line 508).Because our new
loadPalette(_t->_palT, …)call is wrapped in the same#ifndef WLED_SAVE_RAMguard, it compiles correctly whether or notWLED_SAVE_RAMis defined. No further changes are needed.
| @@ -368,6 +369,7 @@ void Segment::beginDraw(uint16_t prog) { | |||
| // minimum blend time is 100ms maximum is 65535ms | |||
| #ifndef WLED_SAVE_RAM | |||
| unsigned noOfBlends = ((255U * prog) / 0xFFFFU) - _t->_prevPaletteBlends; | |||
| if(noOfBlends > 255) noOfBlends = 255; // safety check | |||
There was a problem hiding this comment.
No need for this check.
There was a problem hiding this comment.
are you 100% sure there are no possible combinations where transition time changes i.e. prog gets smaller and _prevPaletteBlends is not reset?
There was a problem hiding this comment.
If we trust in math then yes.
I.e. prog is a _t->_progress alias. _progress only increases with time. One exception is the one you found, when transition is restarted (_start and _dur change) to create a segment copy.
this fixes a very long loop when an overflow was happening in palette blending.
Summary by CodeRabbit