-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace PRNG with hardware RNG #4225
Conversation
Both ESP8266 and ESP32 have a hardware random register. This update makes use of that. It is slightly faster than the fastled variants but mostly it is truly random, even when the timing limitations stated in the datasheet are disregarded. Also saves a bit on code size. - Replaced all random8() and random16() calls with new hw_random() versions - Not replaced in FX where PRNG is required
Hi @DedeHai, I have two points we maybe need to discuss: A) on esp32, is there a benefit compared to B) there was a design decision (time before you joined) to use fastled random functions in code related to effects. The reason behind was that some users wanted to have effects in sync on different devices (aka 'super-sync') which means all effects have the same timebase (strip.now) , plus they use a deterministic random function with the same seed. I see the benefit of a good random distribution , though. The previous decision for effects to use fastled random is not forever so let's discuss. Besides much better random distribution, did you measure if performance of effects improves? |
thanks for the feedback
as mentioned, I made this primarily for the particle system, where I found that random16() is just no random enough, it biases some of the effects (waterfall for example) and I have to use random() which is slower than this version. Still need to port this over and see if it makes any noticeable difference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing hundreds of lines, where preprocessor can do it for you, is prone to errors.
@DedeHai the topic was actually split over different "threads" of discussion. I think these are most important parts
|
Ignore requirement to use preprocessor.
So the supersync idea using strip.now is still at a concept stage, is that correct? |
Thank you for putting it this way. I could not figure out what I disliked about "super-sync" but it looks this is one of the things that bothered me. |
where do we stand on this PR? The way I see it the issues raised about random vs. pseudo random and supersync are something that needs to be addressed seperately and for the time being this PR improves true randomness of the current implementation. |
I'm happy to accept a faster random implementation. I think "Super-sync" might be better implemented by the "copy FX output" PR - it could be used to target network strip types. |
Hi @DedeHai, I think the PR is ok. To come back to the "supersync" discussion, there are several possible use cases
My summary is that only case #1 (and maybe #2) could still be relevant to users. Case 1 could still be implemented, if we implement a "switch" in hw_random() that falls back to fastled pseudo-random when the user explicitly wants this. Plus a bit of extra code to always set the same random seed before calling each effect function. So yes, let's proceed to merge. |
I thought about this and came up with a possible solution:
edit: Questions:
I could add this PRNG approach easily if requirements are clear, so additional comments and inputs are welcome. edit2: for the math enthusiasts: original paper about xorshift PRNG https://www.jstatsoft.org/article/download/v008i14/916 |
I'm curious - is there an example of an FX that uses randomness but will be "in sync" with different segment sizes? Even if using the same PRNG/seed for each frame, I'd expect that most FX will render distinctly differently if the segment sizes aren't identical, eg. particles will be placed at different locations because they modulo over the segment size, or the like. I definitely agree that "super-sync" cases 2 and 3 are best solved with mapping (either bus tools, copy FX, DDP, or the like). Re PRNG: My $0.02 is that -- if there is a use case for #1, above -- then we should probably always use the PRNG for FX (seeded by the hw RNG or sync'd value) rather than make dynamic function calls. Keep it simple. |
Yes there are some - like multi comet (random "launch" of comets), or drip (random "drop-off"), or fireworks starburst (random "bursts"). Plus some trivial ones that are not random, but strictly time/frame dependant - like twoDots, Akemi, DNA, GEQ, .... |
All of those have size-dependent RNG usage:
Synchronized rendering (ie. keeping frame counts in sync) is another ball of wax entirely. We'd either have to decouple the model calculation from the frame output timing (ie. separate physics from rendering - would change a lot of current FX); arrange bidirectional communication so no segment or node moves on to the next frame until everyone's up to date; or loop in the FX function until it's "caught up" if we notice we've fallen behind. All of which are doable but nontrivial. :( |
Both ESP8266 and ESP32 have a hardware random register. This update makes use of that. It is slightly faster than the fastled variants but mostly it is truly random, even when the timing limitations stated in the datasheet are disregarded. Also saves a bit on code size.
I started this for the particle system, where good randomness is required and made a more elaborate version with this PR.
As a test I filled an array of 128 with random numbers from both fastled random16() and hw_random() and found that the hardware variant gives a perfectly linear distribution while fastled PRNG does not: generate lots of numbers, sort them ascending in excel, plot and add a trend line to compare, see if the trendline goes (approximately) through (0,0). hw_random() results does, fastled random does not.
Please Review the changes to make sure I did not replace where not adequate. Also: if there is a better place than
fcn_declare.h
to add the#include
and#define
I can move it.