extmod/nimble: Make stm32 and unix NimBLE ports use synchronous events.

This changes stm32 from using PENDSV to run NimBLE to use the MicroPython
scheduler instead.  This allows Python BLE callbacks to be invoked directly
(and therefore synchronously) rather than via the ringbuffer.

The NimBLE UART HCI and event processing now happens in a scheduled task
every 128ms.  When RX IRQ idle events arrive, it will also schedule this
task to improve latency.

There is a similar change for the unix port where the background thread now
queues the scheduled task.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>

7 files changed, 128 insertions, 103 deletions

diff --git a/extmod/modbluetooth.h b/extmod/modbluetooth.h
index 6ed086d55..52e3446ff 100644
--- a/extmod/modbluetooth.h
+++ b/extmod/modbluetooth.h
@@ -50,6 +50,7 @@
 #endif
 
 // This is used to protect the ringbuffer.
+// A port may no-op this if MICROPY_PY_BLUETOOTH_USE_SYNC_EVENTS is enabled.
 #ifndef MICROPY_PY_BLUETOOTH_ENTER
 #define MICROPY_PY_BLUETOOTH_ENTER mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION();
 #define MICROPY_PY_BLUETOOTH_EXIT MICROPY_END_ATOMIC_SECTION(atomic_state);
diff --git a/extmod/nimble/hal/hal_uart.c b/extmod/nimble/hal/hal_uart.c
index c6d0850fe..230970b08 100644
--- a/extmod/nimble/hal/hal_uart.c
+++ b/extmod/nimble/hal/hal_uart.c
@@ -28,10 +28,13 @@
 #include "py/mphal.h"
 #include "nimble/ble.h"
 #include "extmod/nimble/hal/hal_uart.h"
+#include "extmod/nimble/nimble/nimble_npl_os.h"
 #include "extmod/mpbthci.h"
 
 #if MICROPY_PY_BLUETOOTH && MICROPY_BLUETOOTH_NIMBLE
 
+#define HCI_TRACE (0)
+
 static hal_uart_tx_cb_t hal_uart_tx_cb;
 static void *hal_uart_tx_arg;
 static hal_uart_rx_cb_t hal_uart_rx_cb;
@@ -62,10 +65,10 @@ void hal_uart_start_tx(uint32_t port) {
         mp_bluetooth_hci_cmd_buf[len++] = data;
     }
 
-    #if 0
-    printf("[% 8d] BTUTX: %02x", mp_hal_ticks_ms(), hci_cmd_buf[0]);
-    for (int i = 1; i < len; ++i) {
-        printf(":%02x", hci_cmd_buf[i]);
+    #if HCI_TRACE
+    printf("< [% 8d] %02x", mp_hal_ticks_ms(), mp_bluetooth_hci_cmd_buf[0]);
+    for (size_t i = 1; i < len; ++i) {
+        printf(":%02x", mp_bluetooth_hci_cmd_buf[i]);
     }
     printf("\n");
     #endif
@@ -77,13 +80,21 @@ int hal_uart_close(uint32_t port) {
     return 0; // success
 }
 
-void mp_bluetooth_nimble_hci_uart_process(void) {
+void mp_bluetooth_nimble_hci_uart_process(bool run_events) {
     bool host_wake = mp_bluetooth_hci_controller_woken();
 
     int chr;
     while ((chr = mp_bluetooth_hci_uart_readchar()) >= 0) {
-        // printf("UART RX: %02x\n", data);
+        #if HCI_TRACE
+        printf("> %02x (%d)\n", chr);
+        #endif
         hal_uart_rx_cb(hal_uart_rx_arg, chr);
+
+        // Incoming data may result in events being enqueued. If we're in
+        // scheduler context then we can run those events immediately.
+        if (run_events) {
+            mp_bluetooth_nimble_os_eventq_run_all();
+        }
     }
 
     if (host_wake) {
diff --git a/extmod/nimble/hal/hal_uart.h b/extmod/nimble/hal/hal_uart.h
index 1ff1c1743..647e8ab47 100644
--- a/extmod/nimble/hal/hal_uart.h
+++ b/extmod/nimble/hal/hal_uart.h
@@ -43,6 +43,6 @@ void hal_uart_start_tx(uint32_t port);
 int hal_uart_close(uint32_t port);
 
 // --- Called by the MicroPython port when UART data is available -------------
-void mp_bluetooth_nimble_hci_uart_process(void);
+void mp_bluetooth_nimble_hci_uart_process(bool run_events);
 
 #endif // MICROPY_INCLUDED_EXTMOD_NIMBLE_HAL_HAL_UART_H
diff --git a/extmod/nimble/modbluetooth_nimble.c b/extmod/nimble/modbluetooth_nimble.c
index 7ee6ae867..c961aee32 100644
--- a/extmod/nimble/modbluetooth_nimble.c
+++ b/extmod/nimble/modbluetooth_nimble.c
@@ -382,6 +382,7 @@ int mp_bluetooth_init(void) {
     mp_bluetooth_nimble_ble_state = MP_BLUETOOTH_NIMBLE_BLE_STATE_WAITING_FOR_SYNC;
 
     // Initialise NimBLE memory and data structures.
+    DEBUG_printf("mp_bluetooth_init: nimble_port_init\n");
     nimble_port_init();
 
     // Make sure that the HCI UART and event handling task is running.
@@ -402,6 +403,8 @@ int mp_bluetooth_init(void) {
         return MP_ETIMEDOUT;
     }
 
+    DEBUG_printf("mp_bluetooth_init: starting services\n");
+
     // By default, just register the default gap/gatt service.
     ble_svc_gap_init();
     ble_svc_gatt_init();
@@ -417,7 +420,7 @@ int mp_bluetooth_init(void) {
 }
 
 void mp_bluetooth_deinit(void) {
-    DEBUG_printf("mp_bluetooth_deinit\n");
+    DEBUG_printf("mp_bluetooth_deinit %d\n", mp_bluetooth_nimble_ble_state);
     if (mp_bluetooth_nimble_ble_state == MP_BLUETOOTH_NIMBLE_BLE_STATE_OFF) {
         return;
     }
diff --git a/extmod/nimble/nimble.mk b/extmod/nimble/nimble.mk
index fbd031b3e..00a244d6e 100644
--- a/extmod/nimble/nimble.mk
+++ b/extmod/nimble/nimble.mk
@@ -17,6 +17,13 @@ CFLAGS_MOD += -DMICROPY_BLUETOOTH_NIMBLE_BINDINGS_ONLY=$(MICROPY_BLUETOOTH_NIMBL
 
 ifeq ($(MICROPY_BLUETOOTH_NIMBLE_BINDINGS_ONLY),0)
 
+# On all ports where we provide the full implementation (i.e. not just
+# bindings like on ESP32), then we don't need to use the ringbuffer. In this
+# case, all NimBLE events are run by the MicroPython scheduler. On Unix, the
+# scheduler is also responsible for polling the UART, whereas on STM32 the
+# UART is also polled by the RX IRQ.
+CFLAGS_MOD += -DMICROPY_PY_BLUETOOTH_USE_SYNC_EVENTS=1
+
 NIMBLE_LIB_DIR = lib/mynewt-nimble
 
 LIB_SRC_C += $(addprefix $(NIMBLE_LIB_DIR)/, \
diff --git a/extmod/nimble/nimble/nimble_npl_os.c b/extmod/nimble/nimble/nimble_npl_os.c
index 2ec012940..b68957fab 100644
--- a/extmod/nimble/nimble/nimble_npl_os.c
+++ b/extmod/nimble/nimble/nimble_npl_os.c
@@ -179,63 +179,100 @@ int nimble_sprintf(char *str, const char *fmt, ...) {
 
 struct ble_npl_eventq *global_eventq = NULL;
 
+// This must not be called recursively or concurrently with the UART handler.
 void mp_bluetooth_nimble_os_eventq_run_all(void) {
-    for (struct ble_npl_eventq *evq = global_eventq; evq != NULL; evq = evq->nextq) {
-        int n = 0;
-        while (evq->head != NULL && mp_bluetooth_nimble_ble_state > MP_BLUETOOTH_NIMBLE_BLE_STATE_OFF) {
-            struct ble_npl_event *ev = evq->head;
-            evq->head = ev->next;
-            if (ev->next) {
-                ev->next->prev = NULL;
-                ev->next = NULL;
-            }
-            ev->prev = NULL;
-            DEBUG_EVENT_printf("event_run(%p)\n", ev);
-            ev->fn(ev);
-            DEBUG_EVENT_printf("event_run(%p) done\n", ev);
-
-            if (++n > 3) {
-                // Limit to running 3 tasks per queue.
-                // Some tasks (such as reset) can enqueue themselves
-                // making this an infinite loop (while in PENDSV).
+    if (mp_bluetooth_nimble_ble_state == MP_BLUETOOTH_NIMBLE_BLE_STATE_OFF) {
+        return;
+    }
+
+    // Keep running while there are pending events.
+    while (true) {
+        struct ble_npl_event *ev = NULL;
+
+        os_sr_t sr;
+        OS_ENTER_CRITICAL(sr);
+        // Search all queues for an event.
+        for (struct ble_npl_eventq *evq = global_eventq; evq != NULL; evq = evq->nextq) {
+            ev = evq->head;
+            if (ev) {
+                // Remove this event from the queue.
+                evq->head = ev->next;
+                if (ev->next) {
+                    ev->next->prev = NULL;
+                    ev->next = NULL;
+                }
+                ev->prev = NULL;
+
+                ev->pending = false;
+
+                // Stop searching and execute this event.
                 break;
             }
         }
+        OS_EXIT_CRITICAL(sr);
+
+        if (!ev) {
+            break;
+        }
+
+        // Run the event handler.
+        DEBUG_EVENT_printf("event_run(%p)\n", ev);
+        ev->fn(ev);
+        DEBUG_EVENT_printf("event_run(%p) done\n", ev);
+
+        if (ev->pending) {
+            // If this event has been re-enqueued while it was running, then
+            // stop running further events. This prevents an infinite loop
+            // where the reset event re-enqueues itself on HCI timeout.
+            break;
+        }
     }
 }
 
 void ble_npl_eventq_init(struct ble_npl_eventq *evq) {
     DEBUG_EVENT_printf("ble_npl_eventq_init(%p)\n", evq);
+    os_sr_t sr;
+    OS_ENTER_CRITICAL(sr);
     evq->head = NULL;
     struct ble_npl_eventq **evq2;
     for (evq2 = &global_eventq; *evq2 != NULL; evq2 = &(*evq2)->nextq) {
     }
     *evq2 = evq;
     evq->nextq = NULL;
+    OS_EXIT_CRITICAL(sr);
 }
 
 void ble_npl_eventq_put(struct ble_npl_eventq *evq, struct ble_npl_event *ev) {
     DEBUG_EVENT_printf("ble_npl_eventq_put(%p, %p (%p, %p))\n", evq, ev, ev->fn, ev->arg);
+    os_sr_t sr;
+    OS_ENTER_CRITICAL(sr);
     ev->next = NULL;
+    ev->pending = true;
     if (evq->head == NULL) {
+        // Empty list, make this the first item.
         evq->head = ev;
         ev->prev = NULL;
     } else {
-        struct ble_npl_event *ev2 = evq->head;
+        // Find the tail of this list.
+        struct ble_npl_event *tail = evq->head;
         while (true) {
-            if (ev2 == ev) {
+            if (tail == ev) {
                 DEBUG_EVENT_printf("  --> already in queue\n");
-                return;
+                // Already in the list (e.g. a fragmented ACL will enqueue an
+                // event to process it for each fragment).
+                break;
             }
-            if (ev2->next == NULL) {
+            if (tail->next == NULL) {
+                // Found the end of the list, add this event as the tail.
+                tail->next = ev;
+                ev->prev = tail;
                 break;
             }
-            DEBUG_EVENT_printf("  --> %p\n", ev2->next);
-            ev2 = ev2->next;
+            DEBUG_EVENT_printf("  --> %p\n", tail->next);
+            tail = tail->next;
         }
-        ev2->next = ev;
-        ev->prev = ev2;
     }
+    OS_EXIT_CRITICAL(sr);
 }
 
 void ble_npl_event_init(struct ble_npl_event *ev, ble_npl_event_fn *fn, void *arg) {
@@ -243,6 +280,7 @@ void ble_npl_event_init(struct ble_npl_event *ev, ble_npl_event_fn *fn, void *ar
     ev->fn = fn;
     ev->arg = arg;
     ev->next = NULL;
+    ev->pending = false;
 }
 
 void *ble_npl_event_get_arg(struct ble_npl_event *ev) {
@@ -258,44 +296,17 @@ void ble_npl_event_set_arg(struct ble_npl_event *ev, void *arg) {
 /******************************************************************************/
 // MUTEX
 
-// This is what MICROPY_BEGIN_ATOMIC_SECTION returns on Unix (i.e. we don't
-// need to preserve the atomic state to unlock).
-#define ATOMIC_STATE_MUTEX_NOT_HELD 0xffffffff
-
 ble_npl_error_t ble_npl_mutex_init(struct ble_npl_mutex *mu) {
     DEBUG_MUTEX_printf("ble_npl_mutex_init(%p)\n", mu);
     mu->locked = 0;
-    mu->atomic_state = ATOMIC_STATE_MUTEX_NOT_HELD;
     return BLE_NPL_OK;
 }
 
 ble_npl_error_t ble_npl_mutex_pend(struct ble_npl_mutex *mu, ble_npl_time_t timeout) {
-    DEBUG_MUTEX_printf("ble_npl_mutex_pend(%p, %u) locked=%u irq=%d\n", mu, (uint)timeout, (uint)mu->locked);
+    DEBUG_MUTEX_printf("ble_npl_mutex_pend(%p, %u) locked=%u\n", mu, (uint)timeout, (uint)mu->locked);
 
-    // This is a recursive mutex which we implement on top of the IRQ priority
-    // scheme. Unfortunately we have a single piece of global storage, where
-    // enter/exit critical needs an "atomic state".
-
-    // There are two different acquirers, either running in a VM thread (i.e.
-    // a direct Python call into NimBLE), or in the NimBLE task (i.e. polling
-    // or UART RX).
-
-    // On STM32 the NimBLE task runs in PENDSV, so cannot be interrupted by a VM thread.
-    // Therefore we only need to ensure that a VM thread that acquires a currently-unlocked mutex
-    // now raises the priority (thus preventing context switches to other VM threads and
-    // the PENDSV irq). If the mutex is already locked, then it must have been acquired
-    // by us.
-
-    // On Unix, the critical section is completely recursive and doesn't require us to manage
-    // state so we just acquire and release every time.
-
-    // TODO: The "volatile" on locked/atomic_state isn't enough to protect against memory re-ordering.
-
-    // First acquirer of this mutex always enters the critical section, unless
-    // we're on Unix where it happens every time.
-    if (mu->atomic_state == ATOMIC_STATE_MUTEX_NOT_HELD) {
-        mu->atomic_state = mp_bluetooth_nimble_hci_uart_enter_critical();
-    }
+    // All NimBLE code is executed by the scheduler (and is therefore
+    // implicitly mutexed) so this mutex implementation is a no-op.
 
     ++mu->locked;
 
@@ -303,17 +314,11 @@ ble_npl_error_t ble_npl_mutex_pend(struct ble_npl_mutex *mu, ble_npl_time_t time
 }
 
 ble_npl_error_t ble_npl_mutex_release(struct ble_npl_mutex *mu) {
-    DEBUG_MUTEX_printf("ble_npl_mutex_release(%p) locked=%u irq=%d\n", mu, (uint)mu->locked);
+    DEBUG_MUTEX_printf("ble_npl_mutex_release(%p) locked=%u\n", mu, (uint)mu->locked);
     assert(mu->locked > 0);
 
     --mu->locked;
 
-    // Only exit the critical section for the final release, unless we're on Unix.
-    if (mu->locked == 0 || mu->atomic_state == ATOMIC_STATE_MUTEX_NOT_HELD) {
-        mp_bluetooth_nimble_hci_uart_exit_critical(mu->atomic_state);
-        mu->atomic_state = ATOMIC_STATE_MUTEX_NOT_HELD;
-    }
-
     return BLE_NPL_OK;
 }
 
@@ -329,30 +334,19 @@ ble_npl_error_t ble_npl_sem_init(struct ble_npl_sem *sem, uint16_t tokens) {
 ble_npl_error_t ble_npl_sem_pend(struct ble_npl_sem *sem, ble_npl_time_t timeout) {
     DEBUG_SEM_printf("ble_npl_sem_pend(%p, %u) count=%u\n", sem, (uint)timeout, (uint)sem->count);
 
-    // This is called by NimBLE to synchronously wait for an HCI ACK. The
-    // corresponding ble_npl_sem_release is called directly by the UART rx
-    // handler (i.e. hal_uart_rx_cb in extmod/nimble/hal/hal_uart.c).
-
-    // So this implementation just polls the UART until either the semaphore
-    // is released, or the timeout occurs.
+    // This is only called by NimBLE in ble_hs_hci_cmd_tx to synchronously
+    // wait for an HCI ACK. The corresponding ble_npl_sem_release is called
+    // directly by the UART rx handler (i.e. hal_uart_rx_cb in
+    // extmod/nimble/hal/hal_uart.c). So this loop needs to run only the HCI
+    // UART processing but not run any events.
 
     if (sem->count == 0) {
         uint32_t t0 = mp_hal_ticks_ms();
         while (sem->count == 0 && mp_hal_ticks_ms() - t0 < timeout) {
-            // This can be called either from code running in NimBLE's "task"
-            // (i.e. PENDSV) or directly by application code, so for the
-            // latter case, prevent the "task" from running while we poll the
-            // UART directly.
-            MICROPY_PY_BLUETOOTH_ENTER
-            mp_bluetooth_nimble_hci_uart_process();
-            MICROPY_PY_BLUETOOTH_EXIT
-
             if (sem->count != 0) {
                 break;
             }
 
-            // Because we're polling, might as well wait for a UART IRQ indicating
-            // more data available.
             mp_bluetooth_nimble_hci_uart_wfi();
         }
 
@@ -384,6 +378,8 @@ uint16_t ble_npl_sem_get_count(struct ble_npl_sem *sem) {
 static struct ble_npl_callout *global_callout = NULL;
 
 void mp_bluetooth_nimble_os_callout_process(void) {
+    os_sr_t sr;
+    OS_ENTER_CRITICAL(sr);
     uint32_t tnow = mp_hal_ticks_ms();
     for (struct ble_npl_callout *c = global_callout; c != NULL; c = c->nextc) {
         if (!c->active) {
@@ -393,17 +389,24 @@ void mp_bluetooth_nimble_os_callout_process(void) {
             DEBUG_CALLOUT_printf("callout_run(%p) tnow=%u ticks=%u evq=%p\n", c, (uint)tnow, (uint)c->ticks, c->evq);
             c->active = false;
             if (c->evq) {
+                // Enqueue this callout for execution in the event queue.
                 ble_npl_eventq_put(c->evq, &c->ev);
             } else {
+                // Execute this callout directly.
+                OS_EXIT_CRITICAL(sr);
                 c->ev.fn(&c->ev);
+                OS_ENTER_CRITICAL(sr);
             }
             DEBUG_CALLOUT_printf("callout_run(%p) done\n", c);
         }
     }
+    OS_EXIT_CRITICAL(sr);
 }
 
 void ble_npl_callout_init(struct ble_npl_callout *c, struct ble_npl_eventq *evq, ble_npl_event_fn *ev_cb, void *ev_arg) {
     DEBUG_CALLOUT_printf("ble_npl_callout_init(%p, %p, %p, %p)\n", c, evq, ev_cb, ev_arg);
+    os_sr_t sr;
+    OS_ENTER_CRITICAL(sr);
     c->active = false;
     c->ticks = 0;
     c->evq = evq;
@@ -413,17 +416,22 @@ void ble_npl_callout_init(struct ble_npl_callout *c, struct ble_npl_eventq *evq,
     for (c2 = &global_callout; *c2 != NULL; c2 = &(*c2)->nextc) {
         if (c == *c2) {
             // callout already in linked list so don't link it in again
+            OS_EXIT_CRITICAL(sr);
             return;
         }
     }
     *c2 = c;
     c->nextc = NULL;
+    OS_EXIT_CRITICAL(sr);
 }
 
 ble_npl_error_t ble_npl_callout_reset(struct ble_npl_callout *c, ble_npl_time_t ticks) {
     DEBUG_CALLOUT_printf("ble_npl_callout_reset(%p, %u) tnow=%u\n", c, (uint)ticks, (uint)mp_hal_ticks_ms());
+    os_sr_t sr;
+    OS_ENTER_CRITICAL(sr);
     c->active = true;
     c->ticks = ble_npl_time_get() + ticks;
+    OS_EXIT_CRITICAL(sr);
     return BLE_NPL_OK;
 }
 
@@ -493,23 +501,20 @@ void ble_npl_time_delay(ble_npl_time_t ticks) {
 // CRITICAL
 
 // This is used anywhere NimBLE modifies global data structures.
-// We need to protect between:
-//  - A MicroPython VM thread.
-//  - The NimBLE "task" (e.g. PENDSV on STM32, pthread on Unix).
-// On STM32, by disabling PENDSV, we ensure that either:
-//  - If we're in the NimBLE task, we're exclusive anyway.
-//  - If we're in a VM thread, we can't be interrupted by the NimBLE task, or switched to another thread.
-// On Unix, there's a global mutex.
 
-// TODO: Both ports currently use MICROPY_PY_BLUETOOTH_ENTER in their implementation,
-// maybe this doesn't need to be port-specific?
+// Currently all NimBLE code is invoked by the scheduler so there should be no
+// races, so on STM32 MICROPY_PY_BLUETOOTH_ENTER/MICROPY_PY_BLUETOOTH_EXIT are
+// no-ops. However, in the future we may wish to make HCI UART processing
+// happen asynchronously (e.g. on RX IRQ), so the port can implement these
+// macros accordingly.
 
 uint32_t ble_npl_hw_enter_critical(void) {
     DEBUG_CRIT_printf("ble_npl_hw_enter_critical()\n");
-    return mp_bluetooth_nimble_hci_uart_enter_critical();
+    MICROPY_PY_BLUETOOTH_ENTER
+    return atomic_state;
 }
 
-void ble_npl_hw_exit_critical(uint32_t ctx) {
-    DEBUG_CRIT_printf("ble_npl_hw_exit_critical(%u)\n", (uint)ctx);
-    mp_bluetooth_nimble_hci_uart_exit_critical(ctx);
+void ble_npl_hw_exit_critical(uint32_t atomic_state) {
+    MICROPY_PY_BLUETOOTH_EXIT
+    DEBUG_CRIT_printf("ble_npl_hw_exit_critical(%u)\n", (uint)atomic_state);
 }
diff --git a/extmod/nimble/nimble/nimble_npl_os.h b/extmod/nimble/nimble/nimble_npl_os.h
index 3ef07aa9c..bfabe56e8 100644
--- a/extmod/nimble/nimble/nimble_npl_os.h
+++ b/extmod/nimble/nimble/nimble_npl_os.h
@@ -42,6 +42,7 @@ typedef int32_t ble_npl_stime_t;
 struct ble_npl_event {
     ble_npl_event_fn *fn;
     void *arg;
+    bool pending;
     struct ble_npl_event *prev;
     struct ble_npl_event *next;
 };
@@ -61,7 +62,6 @@ struct ble_npl_callout {
 
 struct ble_npl_mutex {
     volatile uint8_t locked;
-    volatile uint32_t atomic_state;
 };
 
 struct ble_npl_sem {
@@ -76,7 +76,5 @@ void mp_bluetooth_nimble_os_callout_process(void);
 // --- Must be provided by the MicroPython port -------------------------------
 
 void mp_bluetooth_nimble_hci_uart_wfi(void);
-uint32_t mp_bluetooth_nimble_hci_uart_enter_critical(void);
-void mp_bluetooth_nimble_hci_uart_exit_critical(uint32_t atomic_state);
 
 #endif // MICROPY_INCLUDED_STM32_NIMBLE_NIMBLE_NPL_OS_H