NRFCordioHCIDriver: remove idle_hook (and RTOS dependency)

[LDong-Arm]

Summary of changes

In PR #8876 when adding Cordio support for nRF52* targets, we attempted to workaround sleep latency issues by not entering sleep if Cordio's timer (WsfTimer) is due to expire. This was done by overriding the RTOS idle hook.

However, the condition to bypass sleeps never seems to get satisfied:

	nextExpiration = WsfTimerNextExpiration(&timerRunning);
	if(timerRunning && nextExpiration > 0)
	{
		// Make sure we hae enough time to go to sleep
		if( nextExpiration < 1 /* 10 ms per tick which is long enough to got to sleep */ )
		{
			// Bail
			return;
		}
	}

where nextExpiration, an unsigned integer, cannot be both > 0 and < 1. This means the hook may not have made any difference.

Over the past year or so, BLE on nRF52* targets have generally worked fine. So this PR removes the hook to avoid dependency on RTOS. Investigation and a proper fix for the sleep latency issues will be future work.

Impact of changes

Migration actions required

Documentation

We will update https://os.mbed.com/docs/mbed-os/v6.0-preview/bare-metal/index.html to not exclude TARGET_NORDIC_CORDIO from bare metal support.

---

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

The BLE_SM example works as expected on NRF52840_DK, with both the full profile and the bare metal profile.

---

Reviewers

@evedon @ARMmbed/mbed-os-pan

---

[ciarmcom]

@LDong-Arm, thank you for your changes.
@evedon @ARMmbed/mbed-os-pan @ARMmbed/mbed-os-maintainers please review.

[evedon]

Good work @LDong-Arm it does not seem that the idle_hook was effective!

[donatieng]

Good catch @LDong-Arm! One thing I'd like to understand is whether we know what the latency is for going to sleep/waking up on these platforms? Maybe this needs to be fixed (change to >=0) and make sure this is also handled properly in bare metal mode.

[kjbracey]

Attaching an RTOS idle hook is far from ideal - there's a bunch of logic including deep sleep latency compensation (for RTOS wakeups) in the default idle hook, and you'd be losing that, rather than adding to it. (It would be nice to have an API to add to that, rather than replace).

In fact, the entire "tickless" implementation is in the idle hook - you're losing that and going back to ticking mode...

If there's a separate timing layer in Cordio, that needs similar consideration, and it's not visible to Mbed OS, then it should deal with it inside its hal_sleep and/or hal_deepsleep call. They can return immediately rather than sleeping, or choose to shallow sleep instead of deep.

That would also handle it in bare metal mode.

[donatieng]

Please hold your fire @0xc0170.

@kjbracey-arm agreed that the right place for these checks (if required) should be in hal_sleep()/hal_deepsleep().

@LDong-Arm before moving this forward I'd like to understand if any such logic is required, hence understanding the latency implications on this platform and Cordio's timing requirements. Happy to have an offline sync if needed.

[LDong-Arm]

Can we simply lock deep sleep when an asynchronous operation (e.g. reset or send a command to the BLE controller) starts, and unlock deep sleep when the operation finishes (i.e. when the controller replies with an ack) or times out?

[0xc0170]

I'll leave the Ci to complete, will be in review until approved

[LDong-Arm]

Cypress's transport driver does a whole bunch of deep sleep locks/unlocks, for instance:

https://github.com/ARMmbed/mbed-os/blob/master/features/FEATURE_BLE/targets/TARGET_Cypress/COMPONENT_CYW43XXX/CyH4TransportDriver.cpp

[kjbracey]

Can we simply lock deep sleep when an asynchronous operation (e.g. reset or send a command to the BLE controller) starts, and unlock deep sleep when the operation finishes (i.e. when the controller replies with an ack) or times out?

That would be the conventional thing to do, if it is deep sleep latency or inability to be woken from deep sleep at all by your IRQ source that's the problem, and you're fine to wake from shallow sleep.

In many cases this is handled automatically by attach type calls like SerialBase::attach, but if this is a driver that has a custom IRQ source not handled by the HAL, then it would be up to you to do that.

Note that my earlier "inside hal_sleep" suggestion was on the assumption that there must be some reason you were trying to do this "under the hood" without the OS noticing, and it was a better way of doing that. If this is actually an Mbed OS driver, then Mbed OS lock/unlock calls are better.

[donatieng]

I'm not worried about the host stack and HCI link but rather the Link Layer which has potentially tight timing requirements. I think we should ping PacketCraft to understand the impact (I think timing-critical tasks use a HW timer but would prefer them to confirm).

[mbed-ci]

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

[donatieng]

After offline discussion -
Clearly this code is not doing anything so no harm removing it! A task will be created to capture the work needed to fix this properly.

[evedon]

@adbridge Please update label as this should be ready to merge.

[yogeshk19]

@donatieng @LDong-Arm how does this change effect existing Nordic targets. Since the idle_hook has been removed in mbed-os-6.0.0, is this going to be invoked elsewhere to call the power management API's or do we have to implement the idle_hook in our custom firmware to call the sleep management API?

Thanks,
Yogesh

[LDong-Arm]

Hi @yogeshk19, the default idle hook of Mbed OS is used, just as most other targets. As @kjbracey-arm said, it's preferred not to override the default one which contains the logic for tickless.

So, there should be no need for applications to change anything because of this.