Nanostack RF driver callbacks not ISR-safe

[stevew817]

@SeppoTakalo @sg-

Now that RTX5 is going to get switched on, and mutexes are no longer disabled, I encountered the following:

mbed-os/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_interrupt.c

Line 30 in f31ea01

osMutexAcquire(critical_mutex_id, osWaitForever);

In its documentation, Nanostack defines the enter_ & exit_critical functions as turning on and off global interrupts. This works when calling the stack from an ISR context, such as a radio receive callback in an RF SoC.
However, now that there is a mutex in the way, this no longer works. Nowhere in the documentation specifies that radio drivers should be thread- and mutex safe. And since nanostack already has its own eventOS, why require it?

[kjbracey]

mutexes are no longer disabled

Not sure what you mean by that. They've always been enabled. But there have been some new asserts added - perhaps you're now being told off for trying to claim a mutex from interrupt. Loud failure rather than silent failure and unreliability, is the idea.

Nowhere in the documentation specifies that radio drivers should be thread- and mutex safe.

The documentation may leave a little to be desired - it should be covered here, but don't think it is: https://docs.mbed.com/docs/arm-ipv66lowpan-stack/en/latest/16_API_porting/#how-nanostack-runs-inside-mbed-os

What platform_enter_critical does is platform+port dependent - in mbed OS 5, as you note, it's a mutex.

Nanostack's eventOS assumes one foreground context (its event loop) and one background context which can interrupt the event loop, but which can be blocked by platform_enter_critical. Certain Nanostack APIs (like Phy RX callbacks) can be called from the background context, which ever it is.

In mbed OS 5, the background context was chosen to be a thread, rather than interrupts, with platform_enter_critical being a mutex. You must therefore call Nanostack from thread context, never from IRQs.

As mbed OS 5 was brought up, existing mbed OS 3 drivers were converted to defer Nanostack callbacks to thread from interrupt. Example here: PelionIoT/atmel-rf-driver#31 (That retains the ability to worth both ways by flagging on HAVE_RTOS).

The reason this was done was to improve interrupt latency - we had a real problem with radio drivers such as the Atmel driver disabling system interrupts too long - it would hold platform_enter_critical for a complete 802.15.4 frame transfer. This was enough to cause serial port overrun when platform_enter_critical disabled IRQs.

As the RTOS was now present, we decided to use it - that driver and others now use RealTime priority threads to do all the work they previously did directly in interrupt. The latency cost for the driver is minimal, and the overall system performance and stability is improved.

There is a remaining issue with multiple RealTime threads being created by multple components to do this deferral, wasting memory for stacks.

I have some pending work to address that issue here: PelionIoT/atmel-rf-driver#55

[SeppoTakalo]

We will take this into our sprint and update the documentation.

[SeppoTakalo]

This information is appended into the driver documentation and will be released with the new handbook (Still a work in progress https://github.com/ARMmbed/Handbook/blob/new_engine/docs/tutorials/mesh/driver_api.md#worker-thread-for-mbed-os)