feat(core): faster and more robust BLE communication

core/embed/ble_firmware/int_comm.c

@@ -15,6 +15,7 @@
 #define SPI_SS_PIN 31
 
 static uint8_t m_uart_rx_data[BLE_NUS_MAX_DATA_LEN];
+static uint8_t m_spi_tx_data[BLE_PACKET_SIZE];
 static bool m_uart_rx_data_ready_internal = false;
 static uint16_t *m_p_conn_handle = NULL;
 
@@ -22,9 +23,9 @@ BLE_NUS_DEF(m_nus,
             NRF_SDH_BLE_TOTAL_LINK_COUNT); /**< BLE NUS service instance. */
 
 static const nrf_drv_spi_t spi =
-    NRF_DRV_SPI_INSTANCE(SPI_INSTANCE); /**< SPI instance. */
+    NRF_DRV_SPI_INSTANCE(SPI_INSTANCE);    /**< SPI instance. */
-static volatile bool spi_xfer_done; /**< Flag used to indicate that SPI instance
+static volatile bool spi_xfer_done = true; /**< Flag used to indicate that SPI
-                                       completed the transfer. */
+                                       instance completed the transfer. */
 
 /**
  * @brief SPI user event handler.
@@ -200,12 +201,17 @@ void nus_data_handler(ble_nus_evt_t *p_evt) {
     NRF_LOG_HEXDUMP_DEBUG(p_evt->params.rx_data.p_data,
                           p_evt->params.rx_data.length);
 
-    if (p_evt->params.rx_data.length != 64) {
+    if (p_evt->params.rx_data.length != BLE_PACKET_SIZE) {
       return;
     }
 
-    nrf_drv_spi_transfer(&spi, p_evt->params.rx_data.p_data,
+    while (!spi_xfer_done)
-                         p_evt->params.rx_data.length, NULL, 0);
+      ;
+    spi_xfer_done = false;
+
+    memcpy(m_spi_tx_data, p_evt->params.rx_data.p_data, BLE_PACKET_SIZE);
+
+    nrf_drv_spi_transfer(&spi, m_spi_tx_data, BLE_PACKET_SIZE, NULL, 0);
   }
 }
 /**@snippet [Handling the data received over BLE] */

core/embed/ble_firmware/main.c

@@ -110,15 +110,15 @@
   18000 /**< The advertising duration (180 seconds) in units of 10 \
            milliseconds. */
 
-#define MIN_CONN_INTERVAL                                                    \
+#define MIN_CONN_INTERVAL                                                     \
-  MSEC_TO_UNITS(                                                             \
+  MSEC_TO_UNITS(                                                              \
-      20, UNIT_1_25_MS) /**< Minimum acceptable connection interval (20 ms), \
+      7.5, UNIT_1_25_MS) /**< Minimum acceptable connection interval (20 ms), \
                            Connection interval uses 1.25 ms units. */
-#define MAX_CONN_INTERVAL                                                    \
+#define MAX_CONN_INTERVAL                                                     \
-  MSEC_TO_UNITS(                                                             \
+  MSEC_TO_UNITS(                                                              \
-      75, UNIT_1_25_MS) /**< Maximum acceptable connection interval (75 ms), \
+      7.5, UNIT_1_25_MS) /**< Maximum acceptable connection interval (75 ms), \
                            Connection interval uses 1.25 ms units. */
-#define SLAVE_LATENCY 0 /**< Slave latency. */
+#define SLAVE_LATENCY 0  /**< Slave latency. */
 #define CONN_SUP_TIMEOUT                                                     \
   MSEC_TO_UNITS(4000,                                                        \
                 UNIT_10_MS) /**< Connection supervisory timeout (4 seconds), \

core/embed/extmod/modtrezorio/modtrezorio-poll.h

@@ -132,7 +132,7 @@ STATIC mp_obj_t mod_trezorio_poll(mp_obj_t ifaces, mp_obj_t list_ref,
           return mp_const_true;
         }
       } else if (iface == BLE_EVENTS_IFACE) {
-        ble_int_comm_poll();
+        ble_event_poll();
         uint8_t connected = ble_connected();
         if (connected != ble_connected_previously) {
           ble_connected_previously = connected;
@@ -206,7 +206,7 @@ STATIC mp_obj_t mod_trezorio_poll(mp_obj_t ifaces, mp_obj_t list_ref,
         }
       } else if (iface == BLE_IFACE_EXT) {
         if (mode == POLL_READ) {
-          uint8_t buf[64] = {0};
+          uint8_t buf[BLE_PACKET_SIZE] = {0};
           int len = ble_ext_comm_receive(buf, sizeof(buf));
           if (len > 0) {
             ret->items[0] = MP_OBJ_NEW_SMALL_INT(i);

core/embed/trezorhal/ble/comm.c

@@ -26,24 +26,31 @@
 #include "int_comm_defs.h"
 #include "state.h"
 
-#define SPI_PACKET_SIZE 64
+#define SPI_QUEUE_SIZE 10
-#define SPI_QUEUE_SIZE 4
+#define UART_PACKET_SIZE 64
 
 static UART_HandleTypeDef urt;
-static uint8_t last_init_byte = 0;
 
 static SPI_HandleTypeDef spi = {0};
 static DMA_HandleTypeDef spi_dma = {0};
 
 typedef struct {
-  uint8_t buffer[SPI_PACKET_SIZE];
+  uint8_t buffer[BLE_PACKET_SIZE];
   bool used;
   bool ready;
 } spi_buffer_t;
 
 spi_buffer_t spi_queue[SPI_QUEUE_SIZE];
 static int head = 0, tail = 0;
-static bool overrun = 1;
+static bool overrun = false;
+volatile uint16_t overrun_count = 0;
+volatile uint16_t msg_cntr = 0;
+volatile uint16_t first_overrun_at = 0;
+
+static uint8_t int_comm_buffer[UART_PACKET_SIZE];
+static uint16_t int_comm_msg_len = 0;
+static uint8_t int_event_buffer[UART_PACKET_SIZE];
+static uint16_t int_event_msg_len = 0;
 
 void ble_comm_init(void) {
   GPIO_InitTypeDef GPIO_InitStructure;
@@ -101,7 +108,10 @@ void ble_comm_init(void) {
 
   set_initialized(false);
 
-  HAL_SPI_Receive_DMA(&spi, spi_queue[0].buffer, 64);
+  HAL_SPI_Receive_DMA(&spi, spi_queue[0].buffer, BLE_PACKET_SIZE);
+  spi_queue[0].used = true;
+
+  tail = 0;
 }
 
 void ble_comm_send(uint8_t *data, uint32_t len) {
@@ -168,22 +178,13 @@ void flush_line(void) {
   }
 }
 
-uint32_t ble_int_comm_poll(void) {
+void ble_uart_receive(void) {
-  uint8_t data[64] = {0};
   if (urt.Instance->SR & USART_SR_RXNE) {
     uint8_t init_byte = 0;
 
-    if (last_init_byte != 0) {
+    HAL_UART_Receive(&urt, &init_byte, 1, 1);
-      if (last_init_byte == INTERNAL_EVENT) {
-        init_byte = last_init_byte;
-      } else {
-        return 0;
-      }
-    } else {
-      HAL_UART_Receive(&urt, &init_byte, 1, 1);
-    }
 
-    if (init_byte == INTERNAL_EVENT) {
+    if (init_byte == INTERNAL_EVENT || init_byte == INTERNAL_MESSAGE) {
       uint8_t len_hi = 0;
       uint8_t len_lo = 0;
       HAL_UART_Receive(&urt, &len_hi, 1, 1);
@@ -191,125 +192,111 @@ uint32_t ble_int_comm_poll(void) {
 
       uint16_t act_len = (len_hi << 8) | len_lo;
 
-      if (act_len > sizeof(data) + OVERHEAD_SIZE) {
+      if (act_len > UART_PACKET_SIZE + OVERHEAD_SIZE) {
-        last_init_byte = 0;
         flush_line();
-        return 0;
+        return;
+      }
+
+      uint8_t *data = NULL;
+      uint16_t *len = NULL;
+      if (init_byte == INTERNAL_EVENT) {
+        data = int_event_buffer;
+        len = &int_event_msg_len;
+      } else if (init_byte == INTERNAL_MESSAGE) {
+        data = int_comm_buffer;
+        len = &int_comm_msg_len;
+      } else {
+        memset(data, 0, UART_PACKET_SIZE);
+        *len = 0;
+        flush_line();
+        return;
       }
 
       HAL_StatusTypeDef result =
           HAL_UART_Receive(&urt, data, act_len - OVERHEAD_SIZE, 5);
 
       if (result != HAL_OK) {
-        last_init_byte = 0;
+        memset(data, 0, UART_PACKET_SIZE);
+        *len = 0;
         flush_line();
-        return 0;
+        return;
       }
 
       uint8_t eom = 0;
       HAL_UART_Receive(&urt, &eom, 1, 1);
 
       if (eom == EOM) {
-        process_poll(data, act_len - OVERHEAD_SIZE);
+        *len = act_len - OVERHEAD_SIZE;
-        last_init_byte = 0;
+      } else {
-        return act_len - OVERHEAD_SIZE;
+        memset(data, 0, UART_PACKET_SIZE);
+        *len = 0;
+        flush_line();
       }
-      return 0;
-
-    } else if (init_byte == INTERNAL_MESSAGE || init_byte == EXTERNAL_MESSAGE) {
-      last_init_byte = init_byte;
     } else {
       flush_line();
     }
-    return 0;
+  }
+  //
+}
+
+void ble_event_poll() {
+  ble_uart_receive();
+
+  if (int_event_msg_len > 0) {
+    process_poll(int_event_buffer, int_event_msg_len);
+    memset(int_event_buffer, 0, UART_PACKET_SIZE);
+    int_event_msg_len = 0;
   }
 
   if (!ble_initialized()) {
     uint8_t cmd = INTERNAL_CMD_SEND_STATE;
     ble_int_comm_send(&cmd, sizeof(cmd), INTERNAL_EVENT);
   }
-
-  return 0;
 }
 
 uint32_t ble_int_comm_receive(uint8_t *data, uint32_t len) {
-  if (urt.Instance->SR & USART_SR_RXNE) {
+  ble_uart_receive();
-    uint8_t init_byte = 0;
+  if (int_comm_msg_len > 0) {
-
+    memcpy(data, int_comm_buffer,
-    if (last_init_byte != 0) {
+           int_comm_msg_len > len ? len : int_comm_msg_len);
-      if (last_init_byte == INTERNAL_MESSAGE) {
+    memset(int_comm_buffer, 0, UART_PACKET_SIZE);
-        init_byte = last_init_byte;
+    uint32_t res = int_comm_msg_len;
-      } else {
+    int_comm_msg_len = 0;
-        return 0;
+    return res;
-      }
-
-    } else {
-      HAL_UART_Receive(&urt, &init_byte, 1, 1);
-    }
-
-    if (init_byte == INTERNAL_MESSAGE) {
-      uint8_t len_hi = 0;
-      uint8_t len_lo = 0;
-      HAL_UART_Receive(&urt, &len_hi, 1, 1);
-      HAL_UART_Receive(&urt, &len_lo, 1, 1);
-
-      uint16_t act_len = (len_hi << 8) | len_lo;
-
-      if (act_len > len + OVERHEAD_SIZE) {
-        last_init_byte = 0;
-        flush_line();
-        return 0;
-      }
-
-      HAL_StatusTypeDef result =
-          HAL_UART_Receive(&urt, data, act_len - OVERHEAD_SIZE, 5);
-
-      if (result != HAL_OK) {
-        last_init_byte = 0;
-        flush_line();
-        return 0;
-      }
-
-      uint8_t eom = 0;
-      HAL_UART_Receive(&urt, &eom, 1, 1);
-
-      if (eom == EOM) {
-        last_init_byte = 0;
-        return act_len - OVERHEAD_SIZE;
-      }
-      return 0;
-
-    } else if (init_byte == INTERNAL_EVENT) {
-      last_init_byte = init_byte;
-    } else {
-      flush_line();
-      return 0;
-    }
   }
-
   return 0;
 }
 
 bool start_spi_dma(void) {
-  if (spi_queue[tail].used || spi_queue[tail].ready) {
+  int tmp_tail = (tail + 1) % SPI_QUEUE_SIZE;
+  if (spi_queue[tmp_tail].used || spi_queue[tmp_tail].ready) {
     overrun = true;
+    overrun_count++;
+    if (first_overrun_at == 0) {
+      first_overrun_at = msg_cntr;
+    }
     return false;
   }
-  spi_queue[tail].used = true;
+  spi_queue[tmp_tail].used = true;
-  HAL_SPI_Receive_DMA(&spi, spi_queue[tail].buffer, SPI_PACKET_SIZE);
+  HAL_SPI_Receive_DMA(&spi, spi_queue[tmp_tail].buffer, BLE_PACKET_SIZE);
+
+  tail = tmp_tail;
   return true;
 }
 
 void HAL_SPI_RxCpltCallback(SPI_HandleTypeDef *hspi) {
   spi_queue[tail].ready = true;
-  tail = (tail + 1) % SPI_QUEUE_SIZE;
+  msg_cntr++;
   start_spi_dma();
 }
 
+#include "supervise.h"
+
 uint32_t ble_ext_comm_receive(uint8_t *data, uint32_t len) {
+  svc_disableIRQ(DMA2_Stream0_IRQn);
   if (spi_queue[head].ready) {
     uint8_t *buffer = (uint8_t *)spi_queue[head].buffer;
-    memcpy(data, buffer, len > SPI_PACKET_SIZE ? SPI_PACKET_SIZE : len);
+    memcpy(data, buffer, len > BLE_PACKET_SIZE ? BLE_PACKET_SIZE : len);
 
     spi_queue[head].used = false;
     spi_queue[head].ready = false;
@@ -320,8 +307,26 @@ uint32_t ble_ext_comm_receive(uint8_t *data, uint32_t len) {
       overrun = false;
     }
 
-    return len > SPI_PACKET_SIZE ? SPI_PACKET_SIZE : len;
+    if (data[0] != '?') {
+      // bad packet, restart the DMA
+      HAL_SPI_Abort(&spi);
+
+      memset(spi_queue, 0, sizeof(spi_queue));
+      head = 0;
+      tail = 0;
+      overrun = false;
+      HAL_SPI_Receive_DMA(&spi, spi_queue[0].buffer, BLE_PACKET_SIZE);
+      spi_queue[0].used = true;
+      // todo return error?
+      svc_enableIRQ(DMA2_Stream0_IRQn);
+
+      return 0;
+    }
+
+    svc_enableIRQ(DMA2_Stream0_IRQn);
+    return len > BLE_PACKET_SIZE ? BLE_PACKET_SIZE : len;
   }
 
+  svc_enableIRQ(DMA2_Stream0_IRQn);
   return 0;
 }

core/embed/trezorhal/ble/comm.h

@@ -14,6 +14,6 @@ void ble_int_comm_send(uint8_t *data, uint32_t len, uint8_t message_type);
 uint32_t ble_int_comm_receive(uint8_t *data, uint32_t len);
 uint32_t ble_ext_comm_receive(uint8_t *data, uint32_t len);
 
-uint32_t ble_int_comm_poll(void);
+void ble_event_poll(void);
 
 #endif

core/embed/trezorhal/ble/int_comm_defs.h

@@ -2,6 +2,8 @@
 #ifndef __INT_COMM_DEFS__
 #define __INT_COMM_DEFS__
 
+#define BLE_PACKET_SIZE (244)
+
 #define COMM_HEADER_SIZE (3)
 #define COMM_FOOTER_SIZE (1)
 #define OVERHEAD_SIZE (COMM_HEADER_SIZE + COMM_FOOTER_SIZE)

python/src/trezorlib/tealblue.py

@@ -322,10 +322,14 @@ class Characteristic:
     def read(self):
         return bytes(self._char.ReadValue({}))
 
-    def write(self, value):
+    def write(self, value, command=True):
         start = time.time()
         try:
-            self._char.WriteValue(value, {})
+            if command:
+                self._char.WriteValue(value, {"type": "command"})
+            else:
+                self._char.WriteValue(value, {"type": "request"})
+
         except dbus.DBusException as e:
             if (
                 e.get_dbus_name() == "org.bluez.Error.Failed"

python/src/trezorlib/transport/ble.py

@@ -68,7 +68,7 @@ class BleTransport(ProtocolBasedTransport):
                 self.ble_device = d
                 break
 
-        super().__init__(protocol=ProtocolV1(self))
+        super().__init__(protocol=ProtocolV1(self, replen=244))
 
     def get_path(self) -> str:
         return "{}:{}".format(self.PATH_PREFIX, self.device)

python/src/trezorlib/transport/protocol.py

@@ -75,8 +75,9 @@ class Protocol:
     its messages.
     """
 
-    def __init__(self, handle: Handle) -> None:
+    def __init__(self, handle: Handle, replen=REPLEN) -> None:
         self.handle = handle
+        self.replen = replen
         self.session_counter = 0
 
     # XXX we might be able to remove this now that TrezorClient does session handling
@@ -133,10 +134,10 @@ class ProtocolV1(Protocol):
 
         while buffer:
             # Report ID, data padded to 63 bytes
-            chunk = b"?" + buffer[: REPLEN - 1]
+            chunk = b"?" + buffer[: self.replen - 1]
-            chunk = chunk.ljust(REPLEN, b"\x00")
+            chunk = chunk.ljust(self.replen, b"\x00")
             self.handle.write_chunk(chunk)
-            buffer = buffer[63:]
+            buffer = buffer[self.replen - 1 :]
 
     def read(self) -> MessagePayload:
         buffer = bytearray()