Skip to content
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

Open
wants to merge 1 commit into
base: 0_15
Choose a base branch
from

Conversation

DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Oct 25, 2024

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

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.

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
@softhack007
Copy link
Collaborator

softhack007 commented Oct 25, 2024

Hi @DedeHai,

I have two points we maybe need to discuss:

A) on esp32, is there a benefit compared to esp_random() ?

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?

@softhack007 softhack007 added enhancement optimization re-working an existing feature to be faster, or use less memory labels Oct 25, 2024
@DedeHai
Copy link
Collaborator Author

DedeHai commented Oct 25, 2024

thanks for the feedback

  • esp_random() uses a delay loop to ensure entropy, that is what I first used and found it is not necessary for WLED but probably required for encryption purposes. also that function is not available on ESP8266.
  • I did read that PR discussion, these changes would not affect that if I am not mistaken: the random seed of the PRNG at bootup makes all calls to random8() and random16() different on different devices. The FX that require sync I left untouched (the ones that call get seed and set seed) but please correct me if I am mistaken.
  • I did not test performance and I would not expect it to be any different: in my test, calling both random16() and hw_random16() gave a maybe 5% speed boost (a few microseconds difference at 512 calls IIRC) so not noticeable if you do not call it thousands of times. But replacing all calls with the hw version makes code smaller by 300 bytes and gives true randomness.

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.

Copy link
Collaborator

@blazoncek blazoncek left a 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.

wled00/fcn_declare.h Show resolved Hide resolved
@softhack007
Copy link
Collaborator

softhack007 commented Oct 25, 2024

the fastled PRNG is just a few lines of code. could put that in util.cpp as prandom16() or something similar and then explicitly distinct between PRNG and RNG. I can easily do that if someone can point me towards the ideas behind super-sync.

@DedeHai the topic was actually split over different "threads" of discussion. I think these are most important parts

  • Seed FastLED's RNG #3552
    • --> Introduced the idea to always use strip.now (instead of millis()) and replaced all remaining random() calls with fastled random functions
    • This means AC WLED only implemented a "lightweight" version of super sync. If I remember correctly, the main objective was to have segments in sync as much as we can, plus allow effect to do "almost the same thing" when running on different devices that are "synced"
  • The full "super sync" idea originated in the MM fork. I have to say this activity was never 100% competed - we mainly tried to sync single effects to understand what's needed to make them "super sync".

@blazoncek blazoncek dismissed their stale review October 25, 2024 12:08

Ignore requirement to use preprocessor.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Oct 25, 2024

So the supersync idea using strip.now is still at a concept stage, is that correct?
IMHO the idea is flawed. Sync would need to be frame based, not time based between different controllers. My reasoning is this: the random16() functions are called by frame, if one controller runs at 60FPS and another doing more segments for example runs at 50FPS, they will not be in sync. Or what is the idea?
To me, that is a seperate topic and does not really have anything to do with this PR. The changes to FX are easy to revert back to a PRNG, the changes I made should not affect the current state of things.

@blazoncek
Copy link
Collaborator

My reasoning is this: the random16() functions are called by frame, if one controller runs at 60FPS and another doing more segments for example runs at 50FPS, they will not be in sync.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement optimization re-working an existing feature to be faster, or use less memory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants