nodemcu / nodemcu-firmware

Lua based interactive firmware for ESP8266, ESP8285 and ESP32
https://nodemcu.readthedocs.io
MIT License
7.62k stars 3.12k forks source link

Make sodium.random API behave consistently on all build types #3562

Closed tomsci closed 1 year ago

tomsci commented 1 year ago

The sodium.random APIs didn't behave consistently on all build types - on 5.1-integral you'd get negative numbers returned, due to casting a uint32_t to a lua_Number (which is int32_t), but you wouldn't on 5.1-doublefp. On 5.3-xxx builds you'd get numbers returned instead of ints, and on 5.3-intXX-singlefp the API was basically unusable due to 32-bit floats only being able to represent ~1% of the 32-bit integer space.

This change makes the result of random() return potentially-negative integers between INT32_MIN and INT32_MAX on all configurations. Which is a change in behaviour on 5.1-doublefp but one that I think is worthwhile, to get consistency across all build configs.

I updated the documentation for this function, and also clarified the randomness constraints (ie you don't need wifi on if you're OK with only having pseudorandom guarantees) and fixed some links.

tomsci commented 1 year ago

I've never had any issues raised against the sodium module so I might be the only one using it! It certainly could break code written only for 5.1-doublefp (5.1-integral behaviour is unchanged), but it's a relatively unusual API to be calling, most of the time sodium.random.uniform() is a better option. I can't think of any particularly good reason for ever calling it, I only exposed it to Lua for completeness really. The fact that it was already documented to behave differently depending on build config, plus the fact that something had to change on 5.3 because of having the integer number subtype, means I don't feel too bad about changing it.

jmattsson commented 1 year ago

Fair enough. I'll give Philip a few days to chime in if he wishes, otherwise I'll merge.