Fix blank area issue with Twinkle #5005
Conversation
WalkthroughChanged local PRNG16 in mode_twinkle (wled00/FX.cpp) from type unsigned to uint16_t; seed still initialized from SEGENV.aux1 and updated using 16-bit arithmetic, altering PRNG width but not control flow. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
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 |
|
good call, I wonder how no one noticed this before, this bug was introduced a year ago. |
wled00/FX.cpp
Outdated
| { | ||
| SEGENV.aux0 = 0; | ||
| SEGENV.aux1 = hw_random(); //new seed for our PRNG | ||
| SEGENV.aux1 = hw_random16(); //new seed for our PRNG (16-bit for better coverage) |
There was a problem hiding this comment.
this is not the issue. please revert.
There was a problem hiding this comment.
Interestingly, in the 0_15_x branch, the method used here is random16, which I'm assuming is the 16-bit version of random.
I'm not disagreeing with you. PRs should be as small as possible and I have reverted that change.
There was a problem hiding this comment.
I noticed because I got conflicts when I was cherry-picking.
There was a problem hiding this comment.
I was wrong. There is no random or random32 method in the FastLED lib. So random16 was the largest number available.
There was a problem hiding this comment.
yes, I replaced all fastled pseudo random numbers (or almost all) with an implementation that uses the hardware random register that is available in all ESPs giveing almost true random numbers
There was a problem hiding this comment.
Very nice! Thanks for the info.
| } | ||
|
|
||
| unsigned PRNG16 = SEGENV.aux1; | ||
| uint16_t PRNG16 = SEGENV.aux1; |
|
Should I just edit this PR to merge into NOPE. That didn't work. I'll create a new PR for that branch. |
3 participants
There was an issue with my setup where the first ~100 lights in my ~500 light string would not twinkle and would stay as the background color.
This fixes that issue on my setup.
AI seems to think that because the
hw_randommethod creates a 32 bit number, when it's combined and truncated, it somehow leaves out a portion of the segment when the segment size is much smaller than 2^32.Switching to a 16 bit random number fixed it. I'm not sure why, but I've built and tested it, and it works as expected with the same segments as the previously broken one.
Summary by CodeRabbit