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

randint() not random when bluetooth enabled #5409

Closed
Amerlander opened this issue Nov 17, 2023 · 11 comments
Closed

randint() not random when bluetooth enabled #5409

Amerlander opened this issue Nov 17, 2023 · 11 comments

Comments

@Amerlander
Copy link
Contributor

Describe the bug
When pxt is set to have bluetooth anabled the randint() block always produces the same number. Regardless of v1 or v2.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://makecode.microbit.org
  2. Add bluetooth extension
  3. Set code to basic.showNumber(randint(0, 9))
  4. See every time the microbit starts, the same number (one) is displayed

Alternative check this shared project where ble was enabled without loading the bluetooth extension: https://makecode.microbit.org/S91179-28567-56186-10264

Expected behavior
I expect the microbit to show a random number that is not predictable.

micro:bit version (please complete the following information):
Tested on 1.3, but happens on v1 and v2 afaik.

Additional context
On v1 the seedrandom function checks if ble is enabled. If so it tries to use nrf functions for generating a seed and if that fails it is a fix seed. On v2 it is always a fix seed if ble is enabled: https://github.com/lancaster-university/microbit-dal/blob/602153e9199c28c08fd2561ce7b43bf64e9e7394/source/core/MicroBitDevice.cpp#L340

The init function cals seedrandom(): https://github.com/lancaster-university/microbit/blob/1d1baa561f82ab0d16d1916b0026955c67d95e74/source/MicroBit.cpp#L118C18-L118C18
But since it gets called before ble is started, it should not matter what value in BLE_ENABLED is set.

  • I tried to reproduce this error by enabling bluetooth in the microbit samples, but I was not able to reproduce it. I always get random numbers on the samples.
  • I tried to replace the static seed fall back with random numbers generated by the sensors, but that still fails.
  • randint() is defined in pxt, but I didn't figure out how this actually calls the microbit random number functions. Since I was not able to reproduce this issue in the samples, I wonder if there is something on the pxt side I'm missing.
@abchatra
Copy link
Collaborator

@carlosperate @JohnVidler

@abchatra
Copy link
Collaborator

I can reproduce the bug on v2 hardware. The sequence of numbers are always 1,7,8 etc when the hardware is reset and random number is generated on button A pressed.

https://makecode.microbit.org/_EaXWh9CzjFWp

@microbit-carlos
Copy link
Collaborator

@Amerlander would you be able to provide a C++ example that replicates the issue in CODAL? that will help us replicating it and debugging it.

@Amerlander
Copy link
Contributor Author

@microbit-carlos unfortunately no, that's why I think this is a microbit-pxt and not a DAL/CODAL issue, as mentioned in my initial post:

  • I tried to reproduce this error by enabling bluetooth in the microbit samples, but I was not able to reproduce it. I always get random numbers on the samples.

It produces non-random ints, when "MICROBIT_BLE_ENABLED" is set to 1 in microbit-pxt, the same program in C++ is working fine.

The code, that should - but actually doesn't - replicate this issue in CODAL would be:

#include "MicroBit.h"

MicroBit uBit;

int main()
{
    uBit.init();
    int randomInt = uBit.random(9);
    uBit.display.print(randomInt);
}
{
    "target": {
        "name": "codal-microbit-v2",
        "url": "https://github.com/lancaster-university/codal-microbit-v2",
        "branch": "master",
        "type": "git"
    },
    "config":{
        "MICROBIT_BLE_ENABLED" : 1
    }
}

@microbit-carlos
Copy link
Collaborator

microbit-carlos commented Feb 15, 2024

Thanks @Amerlander!

Would adding this code to the CODAL example make a difference?

void platform_init() {
microbit_seed_random();
int seed = microbit_random(0x7fffffff);
DMESG("random seed: %d", seed);
seedRandom(seed);
}

seedRandom() is probably from here, but assuming the seed is different every time it should technically work:
https://github.com/microsoft/pxt-common-packages/blob/5148fecf59dbbef876ac53c3a877d814da6de0dd/libs/base/core.cpp#L333

@microbit-carlos
Copy link
Collaborator

  • randint() is defined in pxt, but I didn't figure out how this actually calls the microbit random number functions. Since I was not able to reproduce this issue in the samples, I wonder if there is something on the pxt side I'm missing.

@abchatra I couldn't easily find the randint() implementation either, could somebody point to how this function is implemented? As @Amerlander has commented, it might be an issue in the pxt side.

@Amerlander
Copy link
Contributor Author

Amerlander commented Feb 15, 2024

Would adding this code to the CODAL example make a difference?

Yes. Using this code reproduces the issue, when BLE is enabled and is working when BLE is disabled

main.cpp in microbit-v2-samples:

#include "MicroBit.h"

MicroBit uBit;

int main()
{
    uBit.init();
     microbit_seed_random(); 
     int seed = microbit_random(0x7fffffff); 
     DMESG("random seed: %d", seed); 
     uBit.seedRandom(seed); 
     int randomInt = uBit.random(9);
     uBit.display.print(randomInt);
}

codal.json that produces random ints:

{
    "target": {
        "name": "codal-microbit-v2",
        "url": "https://github.com/lancaster-university/codal-microbit-v2",
        "branch": "master",
        "type": "git"
    },
    "config":{
        "MICROBIT_BLE_ENABLED" : 0
    }
}

Changing the Config to enable BLE will always give the same number as first random int:

"MICROBIT_BLE_ENABLED" : 1

@Amerlander
Copy link
Contributor Author

Uncommenting the line microbit_seed_random(); in libs/core/codal.cpp solves the issue for me:

microbit_seed_random();

It seems like if microbit_seed_random(); is called after BLE is initialized in the main init() function, the seed stops being random. Since the init() function already calls microbit_seed_random(); right before starting the BLE service it should not be required to call it in platform_init() again.

But I'm not that much into the CODAL functions, so I might be wrong.

@abchatra
Copy link
Collaborator

@microbit-carlos I am going to assign this bug to you and move to hotfix milestone as the fix seems to be on the codal side.

@abchatra abchatra assigned microbit-carlos and unassigned abchatra Aug 28, 2024
microbit-carlos added a commit that referenced this issue Sep 13, 2024
It is not needed, as the random number generator is already seeded
in uBit.init().
This also works around this issue until it is resolved in DAL &
CODAL:
#5409

Apart from that it is a costly call, specially in V1 where it
takes 50 microsec or more, and in theory can take up to 480 us.
@microbit-carlos
Copy link
Collaborator

We've been looking into this and @Amerlander is spot-on (thank you btw for the bug report, the debugging and analysis!).

uBit.init() already calls microbit_seed_random() and for that reason it is not needed in MakeCodes' platform_init():

microbit_seed_random();

That being said, calling microbit_seed_random() again without an argument should have worked and this is a bug in both CODAL and DAL.
The issue is that when it is called this second time, CODAL is missing the implementation that uses SoftDevice, so we need to add that, and DAL has the implementation but often fails, so we need to update it to retry it.

We'll do a CODAL tag with the fix soon, but realistically it will be a while before we release the next DAL tag. So, I've also created this PR in MakeCode which removes the unnecessary call to microbit_seed_random(). Which fixes the issue and has the added value that it will reduce the startup time, as generating a random number can be an expensive operation:

abchatra pushed a commit that referenced this issue Sep 20, 2024
It is not needed, as the random number generator is already seeded
in uBit.init().
This also works around this issue until it is resolved in DAL &
CODAL:
#5409

Apart from that it is a costly call, specially in V1 where it
takes 50 microsec or more, and in theory can take up to 480 us.
abchatra pushed a commit that referenced this issue Sep 23, 2024
It is not needed, as the random number generator is already seeded
in uBit.init().
This also works around this issue until it is resolved in DAL &
CODAL:
#5409

Apart from that it is a costly call, specially in V1 where it
takes 50 microsec or more, and in theory can take up to 480 us.
@microbit-carlos
Copy link
Collaborator

microbit-carlos commented Oct 10, 2024

@abchatra I can confirm that with #5945 this issue has been resolved in https://makecode.microbit.org/stable

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

No branches or pull requests

3 participants