Skip to content

nrf: remove critical section around sd_app_evt_wait() #5202

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

Merged

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented Aug 22, 2021

#5182 added a critical section around sd_app_evt_wait(), which prevents USB from coming up (and perhaps other things).

uint8_t is_nested_critical_region;
sd_nvic_critical_region_enter(&is_nested_critical_region);
if (!background_callback_pending()) {
sd_app_evt_wait();
}
sd_nvic_critical_region_exit(is_nested_critical_region);

This removes that critical section, which was not present before.

Other uses I see of sd_app_evt_wait() don't do anything special (no critical sections, etc.)

@tannewt I'm not sure if this the most correct fix, based on what your intention was, which was to shut off as many interrupts as possible and do the equivalent of a WFI. The documentation says the following.

uint32_t sd_app_evt_wait(void)

Waits for an application event.

An application event is either an application interrupt or a pended interrupt when the interrupt is disabled.

When the application waits for an application event by calling this function, an interrupt that is enabled will be taken immediately on pending since this function will wait in thread mode, then the execution will return in the application's main thread.

In order to wake up from disabled interrupts, the SEVONPEND flag has to be set in the Cortex-M MCU's System Control Register (SCR), CMSIS_SCB. In that case, when a disabled interrupt gets pended, this function will return to the application's main thread.

Note
The application must ensure that the pended flag is cleared using sd_nvic_ClearPendingIRQ in order to sleep using this function. This is only necessary for disabled interrupts, as the interrupt handler will clear the pending flag automatically for enabled interrupts.
If an application interrupt has happened since the last time sd_app_evt_wait was called this function will return immediately and not go to sleep. This is to avoid race conditions that can occur when a flag is updated in the interrupt handler and processed in the main loop.
Postcondition
An application interrupt has happened or a interrupt pending flag is set.

@dhalbert dhalbert requested a review from tannewt August 22, 2021 23:52
@jerryneedell
Copy link
Collaborator

confirmed that build fdfcaf0 also failed the same way on an nRF52840 Bluefruit Sense:
Compiled with this PR -- both the bluefruit sense and the raytac dongle boot normally.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

I'm fine just removing it. My intent was to prevent a background race like the other case but obviously it wasn't correct. :-) We can keep it in mind if we see issues w/it.

@tannewt tannewt merged commit 6f9078c into adafruit:main Aug 23, 2021
@dhalbert dhalbert deleted the nrf-background-callback-critical-section branch October 13, 2021 13:38
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.

nrf52840 builds not working
3 participants