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

mozzi_rand cannot go full range #220

Open
eclab opened this issue Dec 19, 2023 · 7 comments
Open

mozzi_rand cannot go full range #220

eclab opened this issue Dec 19, 2023 · 7 comments

Comments

@eclab
Copy link

eclab commented Dec 19, 2023

uint8_t rand(uint8_t maxval)

The largest value that can be passed into this function is 255, which means that the largest possible random number that can be generated is 254.

unsigned int rand(unsigned int maxval)

The largest value that can be passed into this function is 65535, which means that the largest possible random number that can be generated is 65534.

The alternative is to call xorshift directly and mask out the value. But there ought to be versions of these functions which provide full range for the datatypes. Furthermore, calling xorshift means I have to do a bit conversion to get an int8_t or int16_t, so I get to build a union.

@tomcombriat
Copy link
Collaborator

What range would you like to have? 16 bits on 8bitters already seems to be plenty!

@eclab
Copy link
Author

eclab commented Dec 19, 2023

I'd like to have 0...255 or -128...127 on 8 bit and 0...65535 or -32768 ... 32767 on 16 bit. The simplest solution would be something like

union _secret { uint8_t u; int8_t i; } secret;
uint8_t randu8() { return xorshift() & 0xFF; }
int8_t randi8() { _secret.u = randu8(); return _secret.i; }

... and similarly for 16 bit. [I'm sure there's a C++ convention for bit-converting unsigned to signed but I don't know what it is].

@tomcombriat
Copy link
Collaborator

I don't understand, why not using

int8_t rand(int8_t minval, int8_t maxval);
int rand(int minval, int maxval);

which are respectively 8 and 16 bits on AVR.

@eclab
Copy link
Author

eclab commented Dec 19, 2023

Because the range of rand is minval to maxval-1, not to maxval.

@tomcombriat
Copy link
Collaborator

tomcombriat commented Dec 19, 2023

Ah, I get it. Well, this is coherent with Arduino's random() function.

I am not at the origins of these but if you look at the implementation this is done to avoid type promotion. Of course, it would be quite easy to add overloaded functions without maxval as a parameter which would return in the full type range, for instance (untested):

int8_t rand()
{
	return (int8_t)  lowByte(xorshift96());
}

Welcome to submit a PR ;), otherwise will try to do it soon… If @tfry-git and @sensorium are not against this could go in.

Edit: I guess this version would not work as the compiler will not know which one to use between the different return types, with no parameter, hence the name rand() can probably not be used, but rand_uint8 could.

(Still wondering if having the last value, which will not pop-up very frequently indeed, is actually of any use, but thsese "bare" functions would be faster which sounds appealing).

@tfry-git
Copy link
Collaborator

tfry-git commented Jan 1, 2024

Maybe we don't really need all of these. Essentially, all these would do is call xorshift96() and then cast to a type that needs to be named, explicitly, anyway. (Note that the existing overloads perform additional computation with a given precision, and therefore these do make sense, indeed).

Brainstorming some ideas:

  • we could create a parameter-less long rand() simply as a friendly-sounding alias to xorshift96()
  • bit-casting between signed and unsigned is the only really cumbersome exercise, so maybe have one randUnsigned() and one randSigned() variant for convenience (the latter, again, being a mere alias to xorshift96()).
  • isn't this a great opportunity for templating? rand<int8_t>(), etc. Except of course, it may be overkill.

And then of course, while we're at it, I suppose it will make sense to convert "int" and "long" to int16_t and int32_t?

@tomcombriat
Copy link
Collaborator

isn't this a great opportunity for templating? rand<int8_t>(), etc. Except of course, it may be overkill.

And to port that for direct, U/SFixMath random numbers :P ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants