From 126f972c34f25bffc07d0d1cb3e536921fec0979 Mon Sep 17 00:00:00 2001 From: Jim Mussared Date: Wed, 19 Aug 2020 12:55:38 +1000 Subject: [PATCH] extmod/nimble: Add timeout for HCI sync on startup. This allows `ble.active(1)` to fail correctly if the HCI controller is unavailable. It also avoids an infine loop in the NimBLE event handler where NimBLE doesn't correctly detect that the HCI controller is unavailable and keeps trying to reset. Furthermore, it fixes an issue where GATT service registrations were left allocated, which led to a bad realloc if the stack was activated multiple times. --- extmod/nimble/modbluetooth_nimble.c | 28 +++++++++++++++++++++------- extmod/nimble/nimble/nimble_npl_os.c | 15 ++++++++++++--- ports/unix/mpbthciport.c | 16 ++++++++++++++++ 3 files changed, 49 insertions(+), 10 deletions(-) diff --git a/extmod/nimble/modbluetooth_nimble.c b/extmod/nimble/modbluetooth_nimble.c index 4222f58fa8..9ca90b6024 100644 --- a/extmod/nimble/modbluetooth_nimble.c +++ b/extmod/nimble/modbluetooth_nimble.c @@ -52,6 +52,9 @@ STATIC uint8_t nimble_address_mode = BLE_OWN_ADDR_RANDOM; +#define NIMBLE_STARTUP_TIMEOUT 2000 +STATIC struct ble_npl_sem startup_sem; + // Any BLE_HS_xxx code not in this table will default to MP_EIO. STATIC int8_t ble_hs_err_to_errno_table[] = { [BLE_HS_EAGAIN] = MP_EAGAIN, @@ -206,6 +209,8 @@ STATIC void sync_cb(void) { ble_svc_gap_device_name_set(MICROPY_PY_BLUETOOTH_DEFAULT_GAP_NAME); mp_bluetooth_nimble_ble_state = MP_BLUETOOTH_NIMBLE_BLE_STATE_ACTIVE; + + ble_npl_sem_release(&startup_sem); } STATIC void gatts_register_cb(struct ble_gatt_register_ctxt *ctxt, void *arg) { @@ -355,6 +360,8 @@ int mp_bluetooth_init(void) { ble_hs_cfg.gatts_register_cb = gatts_register_cb; ble_hs_cfg.store_status_cb = ble_store_util_status_rr; + ble_npl_sem_init(&startup_sem, 0); + MP_STATE_PORT(bluetooth_nimble_root_pointers) = m_new0(mp_bluetooth_nimble_root_pointers_t, 1); mp_bluetooth_gatts_db_create(&MP_STATE_PORT(bluetooth_nimble_root_pointers)->gatts_db); @@ -370,21 +377,28 @@ int mp_bluetooth_init(void) { // Initialise NimBLE memory and data structures. nimble_port_init(); - // By default, just register the default gap/gatt service. - ble_svc_gap_init(); - ble_svc_gatt_init(); - // Make sure that the HCI UART and event handling task is running. mp_bluetooth_nimble_port_start(); // Static initialization is complete, can start processing events. mp_bluetooth_nimble_ble_state = MP_BLUETOOTH_NIMBLE_BLE_STATE_WAITING_FOR_SYNC; - // Wait for sync callback - while (mp_bluetooth_nimble_ble_state != MP_BLUETOOTH_NIMBLE_BLE_STATE_ACTIVE) { - MICROPY_EVENT_POLL_HOOK + ble_npl_sem_pend(&startup_sem, NIMBLE_STARTUP_TIMEOUT); + + if (mp_bluetooth_nimble_ble_state != MP_BLUETOOTH_NIMBLE_BLE_STATE_ACTIVE) { + mp_bluetooth_deinit(); + return MP_ETIMEDOUT; } + // By default, just register the default gap/gatt service. + ble_svc_gap_init(); + ble_svc_gatt_init(); + // The preceeding two calls allocate service definitions on the heap, + // then we must now call gatts_start to register those services + // and free the heap memory. + // Otherwise it will be realloc'ed on the next stack startup. + ble_gatts_start(); + DEBUG_printf("mp_bluetooth_init: ready\n"); return 0; diff --git a/extmod/nimble/nimble/nimble_npl_os.c b/extmod/nimble/nimble/nimble_npl_os.c index 46f7a0b291..2ec012940f 100644 --- a/extmod/nimble/nimble/nimble_npl_os.c +++ b/extmod/nimble/nimble/nimble_npl_os.c @@ -32,6 +32,7 @@ #include "extmod/nimble/hal/hal_uart.h" #include "extmod/modbluetooth.h" +#include "extmod/nimble/modbluetooth_nimble.h" #define DEBUG_OS_printf(...) // printf(__VA_ARGS__) #define DEBUG_MALLOC_printf(...) // printf(__VA_ARGS__) @@ -180,7 +181,8 @@ struct ble_npl_eventq *global_eventq = NULL; void mp_bluetooth_nimble_os_eventq_run_all(void) { for (struct ble_npl_eventq *evq = global_eventq; evq != NULL; evq = evq->nextq) { - while (evq->head != NULL) { + 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) { @@ -191,6 +193,13 @@ void mp_bluetooth_nimble_os_eventq_run_all(void) { 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). + break; + } } } } @@ -348,11 +357,11 @@ ble_npl_error_t ble_npl_sem_pend(struct ble_npl_sem *sem, ble_npl_time_t timeout } if (sem->count == 0) { - printf("NimBLE: HCI ACK timeout\n"); + DEBUG_SEM_printf("ble_npl_sem_pend: semaphore timeout\n"); return BLE_NPL_TIMEOUT; } - DEBUG_SEM_printf("got response in %u ms\n", (int)(mp_hal_ticks_ms() - t0)); + DEBUG_SEM_printf("ble_npl_sem_pend: acquired in %u ms\n", (int)(mp_hal_ticks_ms() - t0)); } sem->count -= 1; return BLE_NPL_OK; diff --git a/ports/unix/mpbthciport.c b/ports/unix/mpbthciport.c index dbfb5e0d0e..316a8831fe 100644 --- a/ports/unix/mpbthciport.c +++ b/ports/unix/mpbthciport.c @@ -105,6 +105,10 @@ STATIC int configure_uart(void) { // Apply immediately. if (tcsetattr(uart_fd, TCSANOW, &toptions) < 0) { DEBUG_printf("Couldn't set term attributes"); + + close(uart_fd); + uart_fd = -1; + return -1; } @@ -149,6 +153,10 @@ int mp_bluetooth_hci_uart_init(uint32_t port, uint32_t baudrate) { int mp_bluetooth_hci_uart_deinit(void) { DEBUG_printf("mp_bluetooth_hci_uart_deinit\n"); + if (uart_fd == -1) { + return 0; + } + // Wait for the poll loop to terminate when the state is set to OFF. pthread_join(hci_poll_thread_id, NULL); @@ -168,6 +176,10 @@ int mp_bluetooth_hci_uart_set_baudrate(uint32_t baudrate) { int mp_bluetooth_hci_uart_readchar(void) { // DEBUG_printf("mp_bluetooth_hci_uart_readchar\n"); + if (uart_fd == -1) { + return -1; + } + uint8_t c; ssize_t bytes_read = read(uart_fd, &c, 1); @@ -184,6 +196,10 @@ int mp_bluetooth_hci_uart_readchar(void) { int mp_bluetooth_hci_uart_write(const uint8_t *buf, size_t len) { // DEBUG_printf("mp_bluetooth_hci_uart_write\n"); + if (uart_fd == -1) { + return 0; + } + #if DEBUG_HCI_DUMP printf("[% 8ld] TX: %02x", mp_hal_ticks_ms(), buf[0]); for (size_t i = 1; i < len; ++i) {