NRF5x: rationalise sleep+critical #9358

kjbracey · 2019-01-11T15:49:25Z

Description

This follows up from previous closed PRs #8366 and #9211, in an attempt to fix the issue originally reported as #5647.

As discussed in those PRs, the sleep behaviour of the NRF5x family is apparently unsafe, at least for the cases where the SoftDevice is disabled.

This commit fixes it up by:

Collapsing down to 1 implementation, from 3.
Using standard PRIMASK disable when SoftDevice is disabled.
Using standard WFI when SoftDevice is disabled.

This commit shouldn't change the functionality for when it is enabled - it retains the "SEVONPEND" and "FPU pending" fudges.

If SoftDevice is enabled, we continue to hope that its sd_app_evt_wait() call does the Right Thing(TM). However, this seems unlikely, as documentation and the "nosd" stubs visible suggests they just do a
"WFE", when we are looking for a "WFI" equivalent. Or it's possible their real implementation does the unsafe and racy "SEV; WFE; WFE;" sequence.

Comments added to clarify what we are hoping for from SoftDevice.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@TacoGrandeTX, @deepikabhavnani, @pan-, @marcuschangarm, @scartmell-arm

kjbracey · 2019-01-11T15:54:42Z

This effectively completely replaces the sleep and critical code, and is kind of a "best-of" the various implementations around, with some of my own special sauce.

First draft, untested, but I want feedback.

I'm not clear how the SDK and glue stuff in FEATURE_BLE relates to the rest of the tree - I can't see they co-exist, so I've just deleted the "hal_patch" directory from there for now, and I'm assuming we're using either SDK11 or SDK14 from the targets directory.

One small "adaptation" header put in to cover up API differences between the SDKs.

I think we might have a problem in that the SoftDevice event wait API is unclear on whether it's a "WFE" or "WFI", and implementation may be too. I've added notes on how it might work for both purposes. But this PR doesn't change that mode - only the SoftDevice-disabled mode.

ciarmcom · 2019-01-11T16:00:35Z

@kjbracey-arm, thank you for your changes.
@scartmell-arm @marcuschangarm @deepikabhavnani @pan- @ARMmbed/mbed-os-pan @ARMmbed/mbed-os-maintainers please review.

marcuschangarm · 2019-01-11T18:23:41Z

FYI @TacoGrandeTX @naveenkaje @korjaa @gcloughley

targets/TARGET_NORDIC/TARGET_NRF5x/sleep.c

pan- · 2019-01-14T12:49:33Z

targets/TARGET_NORDIC/TARGET_NRF5x/nordic_critical.c

+    // Will likely recurse, as assert failure will try to take critical region
+    MBED_ASSERT(sd_result == NRF_SUCCESS);
+#else
+    (void) sd_nvic_critical_region_enter(&nested);


This function is not defined when softdevice is included.

When it is not included, you mean?

Can it ever not be included? Is there not SOFTDEVICE_NONE, at minimum, which gives us a stubbed implementation of that? (Not that it should ever be used, as mbed_softdevice_is_enabled will be a constant false in that case).

Yes, I made more investigation and the definition is not included on NRF52840_DK. When the soft device is on, the definition is in nrf_nvic.h

So could we turn on SDK14_2, NORDIC_SOFTDEVICE and SOFTDEVICE_NONE for that? Would that make sense?

pan- · 2019-01-14T12:49:46Z

targets/TARGET_NORDIC/TARGET_NRF5x/nordic_critical.c

+    uint32_t sd_result = sd_nvic_critical_region_exit(!critical_interrupts_enabled);
+    MBED_ASSERT(sd_result == NRF_SUCCESS);
+#else
+    (void) sd_nvic_critical_region_exit(!critical_interrupts_enabled);


This function is not defined when softdevice is included.

cmonr · 2019-01-14T19:52:42Z

Please rebase your PR.

A fix for the travis-ci/tools-py2.7 was brought in today (#9371) due to an update to the hypothesis module used in CI: https://github.com/HypothesisWorks/hypothesis/releases/tag/hypothesis-python-4.0.0

0xc0170 · 2019-01-28T10:29:13Z

Besides rebase to resolve travis failures (there was another tool breakage), needs reviews !

This follows up from previous closed PRs ARMmbed#8366 and ARMmbed#9211, in an attempt to fix the issue originally reported as ARMmbed#5647. As discussed in those PRs, the sleep behaviour of the NRF5x family is apparently unsafe, at least for the cases where the SoftDevice is disabled. This commit fixes it up by: * Collapsing down to 1 implementation, from 3. * Using standard PRIMASK disable when SoftDevice is disabled. * Using standard WFI when SoftDevice is disabled. This commit shouldn't change the functionality for when it is enabled - it retains the "SEVONPEND" and "FPU pending" fudges. If SoftDevice is enabled, we continue to hope that its sd_app_evt_wait() call does the Right Thing(TM). However, this seems unlikely, as documentation and the "nosd" stubs visible suggests they just do a "WFE", when we are looking for a "WFI" equivalent. Or it's possible their real implementation does the unsafe and racy "SEV; WFE; WFE;" sequence. Comments added to clarify what we are hoping for from SoftDevice.

kjbracey · 2019-01-28T10:54:58Z

Will also need to coordinate with #9019 - that no doubt will affect this. Seems like it might be increasing the number of HW/SDK/how-to-drive-it permutations further :/

0xc0170 · 2019-02-13T13:40:25Z

Will also need to coordinate with #9019 - that no doubt will affect this. Seems like it might be increasing the number of HW/SDK/how-to-drive-it permutations further :/

The update went in to the feature branch , anything to do there?

This still needs reviews

TacoGrandeTX · 2019-02-13T15:36:53Z

@0xc0170 As this was still under review it was not brought into the nrf52-sdk15 feature branch. What level of testing has been performed on this?

kjbracey · 2019-02-13T15:48:38Z

This is still at discussion stage - this is a proposal, and I was awaiting feedback and answers to some questions above. Particularly:

can we use "SOFTDEVICE_NONE" to get stubbed functions, rather than kill the entire SDK,
why does the BLE code have its own "hal patches"?

I will no doubt need to update this for SDK15, but not sure how to co-ordinate that if you're on a feature branch. Haven't looked at it yet, so I don't know how it changes anything.

There should be a follow-up discussion about how sd_app_evt_wait really works for when SoftDevice is on, as I'm not yet convinced from documentation that it's race-free the way we're using it, but for now this only changes the cases where we're not using it. (I hope).

cmonr · 2019-02-20T19:32:54Z

@kjbracey-arm Any progress?

TacoGrandeTX · 2019-02-20T23:32:35Z

@kjbracey-arm

can we use "SOFTDEVICE_NONE" to get stubbed functions, rather than kill the entire SDK,

I'm not sure what functions you are referring to, but yes building for SOFTDEVICE_NONE doesn't kill the SDK - it prevents the SoftDevice and MBR from being built into the final image. Some helper utility functions are located here for SOFTDEVICE_NONE:
https://github.com/ARMmbed/mbed-os/tree/master/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_SDK_14_2/TARGET_SOFTDEVICE_NONE/nrf_soc_nosd

I just checked your branch and it doesn't yet build for SOFTDEVICE_NONE.

why does the BLE code have its own "hal patches"?

Not sure - perhaps @desmond-blue knows?

desmond-blue · 2019-02-21T08:10:39Z

@TacoGrandeTX
I have no idea.
The hal_patch folder is only under target nRF51822, so my guess is that those two files are there because they include some SoftDevice functions and SoftDevice was bonded with BLE, and we just haven't updated them.

cmonr · 2019-03-19T01:53:02Z

@kjbracey-arm @ARMmbed/mbed-os-pan Any thoughts on how to progress this PR?

cmonr · 2019-03-27T20:37:29Z

@kjbracey-arm @ARMmbed/mbed-os-pan ^^^

mbed-ci · 2019-04-17T02:15:00Z

Test run: FAILED

Summary: 3 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-ARM

adbridge · 2019-04-25T11:33:34Z

@kjbracey-arm as this has seen no update for 29 days I am closing this. Please re-open once there are any new changes.

ciarmcom requested review from deepikabhavnani, marcuschangarm, pan- and a user January 11, 2019 16:00

ciarmcom added the needs: review label Jan 11, 2019

pan- reviewed Jan 14, 2019

View reviewed changes

targets/TARGET_NORDIC/TARGET_NRF5x/sleep.c Show resolved Hide resolved

pan- reviewed Jan 14, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jan 15, 2019

kjbracey force-pushed the nrf_sleep branch from 6c15bc7 to 1dc2856 Compare January 16, 2019 14:10

kjbracey force-pushed the nrf_sleep branch from 1dc2856 to f098bae Compare January 28, 2019 10:50

cmonr mentioned this pull request Jan 30, 2019

NRF52 SDK15 update #9019

Merged

cmonr added needs: review and removed needs: work labels Jan 30, 2019

0xc0170 removed the request for review from a user April 15, 2019 13:48

adbridge closed this Apr 25, 2019

adbridge removed the needs: review label Apr 25, 2019

NRF5x: rationalise sleep+critical #9358

NRF5x: rationalise sleep+critical #9358

Uh oh!

Conversation

kjbracey commented Jan 11, 2019

Description

Pull request type

Reviewers

Uh oh!

kjbracey commented Jan 11, 2019

Uh oh!

ciarmcom commented Jan 11, 2019

Uh oh!

marcuschangarm commented Jan 11, 2019

Uh oh!

Uh oh!

pan- Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

kjbracey Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

kjbracey Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

kjbracey Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

pan- Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

cmonr commented Jan 14, 2019

Uh oh!

0xc0170 commented Jan 28, 2019

Uh oh!

kjbracey commented Jan 28, 2019

Uh oh!

0xc0170 commented Feb 13, 2019

Uh oh!

TacoGrandeTX commented Feb 13, 2019

Uh oh!

kjbracey commented Feb 13, 2019

Uh oh!

cmonr commented Feb 20, 2019

Uh oh!

TacoGrandeTX commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

desmond-blue commented Feb 21, 2019

Uh oh!

cmonr commented Mar 19, 2019

Uh oh!

cmonr commented Mar 27, 2019

Uh oh!

mbed-ci commented Apr 17, 2019

Test run: FAILED

Uh oh!

adbridge commented Apr 25, 2019

Uh oh!

Uh oh!

kjbracey Jan 14, 2019 •

edited

Loading

TacoGrandeTX commented Feb 20, 2019 •

edited

Loading