From 02bc42a5f0bc4a8fbb29a8bb68227c36ad18a046 Mon Sep 17 00:00:00 2001 From: tychovrahe Date: Sun, 19 Mar 2023 12:37:06 +0100 Subject: [PATCH] feat(core): faster and more robust BLE communication --- core/embed/ble_firmware/int_comm.c | 18 +- core/embed/ble_firmware/main.c | 14 +- .../extmod/modtrezorio/modtrezorio-poll.h | 4 +- core/embed/trezorhal/ble/comm.c | 189 +++++++++--------- core/embed/trezorhal/ble/comm.h | 2 +- core/embed/trezorhal/ble/int_comm_defs.h | 2 + python/src/trezorlib/tealblue.py | 8 +- python/src/trezorlib/transport/ble.py | 2 +- python/src/trezorlib/transport/protocol.py | 9 +- 9 files changed, 133 insertions(+), 115 deletions(-) diff --git a/core/embed/ble_firmware/int_comm.c b/core/embed/ble_firmware/int_comm.c index 7904e71b2f..6cf06e1bc1 100644 --- a/core/embed/ble_firmware/int_comm.c +++ b/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. */ -static volatile bool spi_xfer_done; /**< Flag used to indicate that SPI instance - completed the transfer. */ + NRF_DRV_SPI_INSTANCE(SPI_INSTANCE); /**< SPI instance. */ +static volatile bool spi_xfer_done = true; /**< Flag used to indicate that SPI + 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, - p_evt->params.rx_data.length, NULL, 0); + while (!spi_xfer_done) + ; + 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] */ diff --git a/core/embed/ble_firmware/main.c b/core/embed/ble_firmware/main.c index 0973b2d727..713323abc4 100644 --- a/core/embed/ble_firmware/main.c +++ b/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 \ - MSEC_TO_UNITS( \ - 20, UNIT_1_25_MS) /**< Minimum acceptable connection interval (20 ms), \ +#define MIN_CONN_INTERVAL \ + MSEC_TO_UNITS( \ + 7.5, UNIT_1_25_MS) /**< Minimum acceptable connection interval (20 ms), \ Connection interval uses 1.25 ms units. */ -#define MAX_CONN_INTERVAL \ - MSEC_TO_UNITS( \ - 75, UNIT_1_25_MS) /**< Maximum acceptable connection interval (75 ms), \ +#define MAX_CONN_INTERVAL \ + MSEC_TO_UNITS( \ + 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), \ diff --git a/core/embed/extmod/modtrezorio/modtrezorio-poll.h b/core/embed/extmod/modtrezorio/modtrezorio-poll.h index c8d94663d6..30ad20f5f9 100644 --- a/core/embed/extmod/modtrezorio/modtrezorio-poll.h +++ b/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); diff --git a/core/embed/trezorhal/ble/comm.c b/core/embed/trezorhal/ble/comm.c index 864a74deaa..48c1ab4ce8 100644 --- a/core/embed/trezorhal/ble/comm.c +++ b/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 4 +#define SPI_QUEUE_SIZE 10 +#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) { - uint8_t data[64] = {0}; +void ble_uart_receive(void) { if (urt.Instance->SR & USART_SR_RXNE) { uint8_t init_byte = 0; - if (last_init_byte != 0) { - if (last_init_byte == INTERNAL_EVENT) { - init_byte = last_init_byte; - } else { - return 0; - } - } else { - HAL_UART_Receive(&urt, &init_byte, 1, 1); - } + 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) { - last_init_byte = 0; + if (act_len > UART_PACKET_SIZE + OVERHEAD_SIZE) { 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); - last_init_byte = 0; - return act_len - OVERHEAD_SIZE; + *len = act_len - OVERHEAD_SIZE; + } else { + 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) { - uint8_t init_byte = 0; - - if (last_init_byte != 0) { - if (last_init_byte == INTERNAL_MESSAGE) { - init_byte = last_init_byte; - } else { - return 0; - } - - } 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; - } + ble_uart_receive(); + if (int_comm_msg_len > 0) { + memcpy(data, int_comm_buffer, + int_comm_msg_len > len ? len : int_comm_msg_len); + memset(int_comm_buffer, 0, UART_PACKET_SIZE); + uint32_t res = int_comm_msg_len; + int_comm_msg_len = 0; + return res; } - 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; - HAL_SPI_Receive_DMA(&spi, spi_queue[tail].buffer, SPI_PACKET_SIZE); + spi_queue[tmp_tail].used = true; + 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; } diff --git a/core/embed/trezorhal/ble/comm.h b/core/embed/trezorhal/ble/comm.h index 19a6325cc0..3d7561d617 100644 --- a/core/embed/trezorhal/ble/comm.h +++ b/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 diff --git a/core/embed/trezorhal/ble/int_comm_defs.h b/core/embed/trezorhal/ble/int_comm_defs.h index 0758cf8920..64f74da18f 100644 --- a/core/embed/trezorhal/ble/int_comm_defs.h +++ b/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) diff --git a/python/src/trezorlib/tealblue.py b/python/src/trezorlib/tealblue.py index 38aa73687e..f869507d9f 100755 --- a/python/src/trezorlib/tealblue.py +++ b/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" diff --git a/python/src/trezorlib/transport/ble.py b/python/src/trezorlib/transport/ble.py index feebebfb4a..55cdb289da 100644 --- a/python/src/trezorlib/transport/ble.py +++ b/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) diff --git a/python/src/trezorlib/transport/protocol.py b/python/src/trezorlib/transport/protocol.py index 9069650f5b..4e94334513 100644 --- a/python/src/trezorlib/transport/protocol.py +++ b/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 = chunk.ljust(REPLEN, b"\x00") + chunk = b"?" + buffer[: self.replen - 1] + 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()