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

drivers/nrf5x: do not assign IRQ prio when building for RIOT #483

Merged
merged 2 commits into from Jun 25, 2019

Conversation

haukepetersen
Copy link
Member

RIOT does not allow for nested interrupts and initializes all IRQs to the same priority during system initialization. Assigning different priorities in the driver leads to system crashes.

For some reasons, the differing IRQ prios had no effect on nrf52-based platforms. We found it however causing hard-faults on nrf51-based boards. My guess is, that the different interrupt grouping on the Cortex-M0 and M4 are causing this difference?!

RIOT does not allow for nested interrupts and initializes all IRQs
to the same priority during system initialization. Assigning
different priorities in the driver leads to system crashes.
RIOT does not allow for nested interrupts and initializes all IRQs
to the same priority during system initialization. Assigning
different priorities in the driver leads to system crashes.
@haukepetersen
Copy link
Member Author

rebased.

@andrzej-kaczmarek any chance you could have a look at this? Thx!

Copy link
Contributor

@andrzej-kaczmarek andrzej-kaczmarek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this could crash tbh.
iirc there was problem some time ago when RIOT used basepri to disable interrupts and higher priority interrupt like radio was indeed unexpected, but now I see that primask is used to disable interrupts so this should not matter, but I may also confuse this with some other issue.

since this affects only RIOT and is tested I'll approve, but would be good to have radio interrupt at higher priority since this needs to be handled quickly, otherwise we risk losing some packets.

@andrzej-kaczmarek andrzej-kaczmarek merged commit 1dfda9d into apache:master Jun 25, 2019
@haukepetersen
Copy link
Member Author

Thanks for merging.

not sure why this could crash tbh.

Same here, my guess is that the scheduler is getting confused while its interrupted, but did not find out the exact details. However, with this merged the issue is gone for now, so for now I think its the best we can do.

since this affects only RIOT and is tested I'll approve, but would be good to have radio interrupt at higher priority since this needs to be handled quickly, otherwise we risk losing some packets.

Unfortunately, this is not possible with the current way RIOT handles interrupts on Cortex-M platforms. But likely we will we revisit this at some point, allowing for more elaborate interrupt prio handling...

@andrzej-kaczmarek
Copy link
Contributor

@haukepetersen do you have some easy way to reproduce crash on nrf51? ideally if I can just build some test app and run to trigger crash I could look into this some time

@haukepetersen
Copy link
Member Author

Easy is relative :-)

Here is what you can do:

  • checkout @aabadie's branch enabling NimBLE support for nRF51 boards in RIOT -> according PR is pkg/nimble: adapt to nrf51 family RIOT-OS/RIOT#11463
  • build RIOT/examples/nimble_gatt against any NimBLE version prio to merging this PR -> you won't be able to connect to the device and do a GATT service discovery
  • build the same example against any NimBLE version after this PR was merged -> everything should work fine

tip: the currently simplest way in RIOT to have quick control over package versions is to add PKG_SOURCE_LOCAL = /local/path/to/your/nimble/repo to the packages Makefile (pkg/nimble/Makefile). This skips the checkout of the packages configured repository and simply copies the contents of the referenced folder into the RIOT build path, omitting the need to commit any changes to the packages code...

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

Successfully merging this pull request may close these issues.

None yet

2 participants