From 0c1096e5a37c04be78f2e3c09a91ac348bd7b02e Mon Sep 17 00:00:00 2001 From: Damian Krolik Date: Tue, 18 Mar 2025 10:47:13 +0100 Subject: [PATCH 1/3] nrf_rpc_uart: improve hdlc decoding 1. Skip incoming bytes while in the unsync state. 2. Handle receive buffer overflow. 3. Prepare hdlc_decode_byte() for multiple simultaneous decoding states. Signed-off-by: Damian Krolik --- subsys/nrf_rpc/nrf_rpc_uart.c | 117 ++++++++++++++++++++-------------- 1 file changed, 69 insertions(+), 48 deletions(-) diff --git a/subsys/nrf_rpc/nrf_rpc_uart.c b/subsys/nrf_rpc/nrf_rpc_uart.c index 8c7cd545a67c..300b73e22012 100644 --- a/subsys/nrf_rpc/nrf_rpc_uart.c +++ b/subsys/nrf_rpc/nrf_rpc_uart.c @@ -33,12 +33,24 @@ struct trx_flips { uint16_t last_rx_crc; }; -typedef enum { +enum hdlc_state { + /* Ignore incoming bytes until the delimiter is found. */ HDLC_STATE_UNSYNC, - HDLC_STATE_FRAME_START, + /* Append incoming bytes to the output buffer. */ + HDLC_STATE_FRAME, + /* Found the delimeter when the output buffer was non-empty. */ HDLC_STATE_FRAME_FOUND, + /* Found the escape byte. Append the following byte XORed with 0x20 to the output buffer. */ HDLC_STATE_ESCAPE, -} hdlc_state_t; +}; + +struct hdlc_decode_ctx { + enum hdlc_state state; + /* The number of bytes of the current packet that have been decoded so far. */ + uint16_t len; + /* The capacity of the buffer to store a decoded packet. */ + uint16_t capacity; +}; struct nrf_rpc_uart { const struct device *uart; @@ -56,10 +68,9 @@ struct nrf_rpc_uart { K_KERNEL_STACK_MEMBER(rx_workq_stack, CONFIG_NRF_RPC_UART_RX_THREAD_STACK_SIZE); - /* HDLC frame parsing state */ - hdlc_state_t hdlc_state; - uint8_t rx_packet[CONFIG_NRF_RPC_UART_MAX_PACKET_SIZE]; - size_t rx_packet_len; + /* HDLC packet decoding state */ + struct hdlc_decode_ctx rx_pkt_ctx; + uint8_t rx_pkt[CONFIG_NRF_RPC_UART_MAX_PACKET_SIZE]; /* Ack waiting semaphore */ struct k_sem ack_sem; @@ -91,12 +102,12 @@ static void send_byte(const struct device *dev, uint8_t byte); static void ack_rx(struct nrf_rpc_uart *uart_tr) { - if (!IS_ENABLED(CONFIG_NRF_RPC_UART_RELIABLE) || uart_tr->rx_packet_len != CRC_SIZE) { - log_hexdump_dbg(uart_tr->rx_packet, uart_tr->rx_packet_len, ">>> RX invalid frame"); + if (!IS_ENABLED(CONFIG_NRF_RPC_UART_RELIABLE) || uart_tr->rx_pkt_ctx.len != CRC_SIZE) { + log_hexdump_dbg(uart_tr->rx_pkt, uart_tr->rx_pkt_ctx.len, ">>> RX invalid frame"); return; } - uint16_t rx_ack = sys_get_le16(uart_tr->rx_packet); + uint16_t rx_ack = sys_get_le16(uart_tr->rx_pkt); LOG_DBG(">>> RX ack %04x", rx_ack); @@ -176,39 +187,48 @@ static bool crc_compare(uint16_t rx_crc, uint16_t calc_crc) return rx_crc == calc_crc; } -static void hdlc_decode_byte(struct nrf_rpc_uart *uart_tr, uint8_t byte) +static void hdlc_decode_byte(struct hdlc_decode_ctx *ctx, uint8_t *out, uint8_t in) { - /* TODO: handle frame buffer overflow */ - - if (uart_tr->hdlc_state == HDLC_STATE_ESCAPE) { - uart_tr->rx_packet[uart_tr->rx_packet_len++] = byte ^ 0x20; - uart_tr->hdlc_state = HDLC_STATE_FRAME_START; - } else if (byte == HDLC_CHAR_ESCAPE) { - uart_tr->hdlc_state = HDLC_STATE_ESCAPE; - } else if (byte == HDLC_CHAR_DELIMITER) { - uart_tr->hdlc_state = (uart_tr->hdlc_state == HDLC_STATE_FRAME_START) - ? HDLC_STATE_FRAME_FOUND - : HDLC_STATE_UNSYNC; - } else { - uart_tr->rx_packet[uart_tr->rx_packet_len++] = byte; - uart_tr->hdlc_state = HDLC_STATE_FRAME_START; + switch (ctx->state) { + case HDLC_STATE_UNSYNC: + if (in == HDLC_CHAR_DELIMITER) { + ctx->len = 0; + ctx->state = HDLC_STATE_FRAME; + } + return; + case HDLC_STATE_FRAME_FOUND: + ctx->len = 0; + ctx->state = HDLC_STATE_FRAME; + __fallthrough; + case HDLC_STATE_FRAME: + if (in == HDLC_CHAR_DELIMITER) { + if (ctx->len > 0) { + ctx->state = HDLC_STATE_FRAME_FOUND; + } + return; + } else if (in == HDLC_CHAR_ESCAPE) { + ctx->state = HDLC_STATE_ESCAPE; + return; + } + break; + case HDLC_STATE_ESCAPE: + in ^= 0x20; + ctx->state = HDLC_STATE_FRAME; + break; } - if (uart_tr->hdlc_state == HDLC_STATE_FRAME_FOUND) { - if (uart_tr->rx_packet_len > CRC_SIZE) { - uart_tr->rx_packet_len -= CRC_SIZE; - } else { - ack_rx(uart_tr); - uart_tr->rx_packet_len = 0; - uart_tr->hdlc_state = HDLC_STATE_UNSYNC; - } + if (ctx->len >= ctx->capacity) { + /* Ignore too long frame */ + ctx->state = HDLC_STATE_UNSYNC; + return; } + + out[ctx->len++] = in; } static void work_handler(struct k_work *work) { struct nrf_rpc_uart *uart_tr = CONTAINER_OF(work, struct nrf_rpc_uart, rx_work); - const struct nrf_rpc_tr *transport = uart_tr->transport; uint8_t *data; size_t len; int ret; @@ -219,24 +239,28 @@ static void work_handler(struct k_work *work) len = ring_buf_get_claim(&uart_tr->rx_ringbuf, &data, CONFIG_NRF_RPC_UART_MAX_PACKET_SIZE); for (size_t i = 0; i < len; i++) { - hdlc_decode_byte(uart_tr, data[i]); + hdlc_decode_byte(&uart_tr->rx_pkt_ctx, uart_tr->rx_pkt, data[i]); - if (uart_tr->hdlc_state != HDLC_STATE_FRAME_FOUND) { + if (uart_tr->rx_pkt_ctx.state != HDLC_STATE_FRAME_FOUND) { continue; } - crc_received = sys_get_le16(uart_tr->rx_packet + uart_tr->rx_packet_len); + if (uart_tr->rx_pkt_ctx.len <= CRC_SIZE) { + ack_rx(uart_tr); + continue; + } + + uart_tr->rx_pkt_ctx.len -= CRC_SIZE; + crc_received = sys_get_le16(uart_tr->rx_pkt + uart_tr->rx_pkt_ctx.len); crc_calculated = - crc16_ccitt(0xffff, uart_tr->rx_packet, uart_tr->rx_packet_len); + crc16_ccitt(0xffff, uart_tr->rx_pkt, uart_tr->rx_pkt_ctx.len); - log_hexdump_dbg(uart_tr->rx_packet, uart_tr->rx_packet_len, + log_hexdump_dbg(uart_tr->rx_pkt, uart_tr->rx_pkt_ctx.len, ">>> RX packet %04x", crc_received); if (!crc_compare(crc_received, crc_calculated)) { LOG_ERR("Invalid packet CRC: calculated %04x but received %04x", crc_calculated, crc_received); - uart_tr->rx_packet_len = 0; - uart_tr->hdlc_state = HDLC_STATE_UNSYNC; continue; } @@ -245,13 +269,10 @@ static void work_handler(struct k_work *work) if (rx_flip_check(uart_tr, crc_received)) { LOG_WRN("Duplicate packet %04x", crc_received); } else { - uart_tr->receive_callback(transport, uart_tr->rx_packet, - uart_tr->rx_packet_len, + uart_tr->receive_callback(uart_tr->transport, uart_tr->rx_pkt, + uart_tr->rx_pkt_ctx.len, uart_tr->receive_ctx); } - - uart_tr->rx_packet_len = 0; - uart_tr->hdlc_state = HDLC_STATE_UNSYNC; } ret = ring_buf_get_finish(&uart_tr->rx_ringbuf, len); @@ -350,8 +371,8 @@ static int init(const struct nrf_rpc_tr *transport, nrf_rpc_tr_receive_handler_t k_work_init(&uart_tr->rx_work, work_handler); ring_buf_init(&uart_tr->rx_ringbuf, sizeof(uart_tr->rx_buffer), uart_tr->rx_buffer); - uart_tr->hdlc_state = HDLC_STATE_UNSYNC; - uart_tr->rx_packet_len = 0; + uart_tr->rx_pkt_ctx.state = HDLC_STATE_UNSYNC; + uart_tr->rx_pkt_ctx.capacity = sizeof(uart_tr->rx_pkt); uart_irq_rx_enable(uart_tr->uart); nrf_rpc_uart_initialized_hook(uart_tr->uart); From d386ff3398f0fd3f2c6028e8d8abf5c234270551 Mon Sep 17 00:00:00 2001 From: Damian Krolik Date: Tue, 18 Mar 2025 11:11:07 +0100 Subject: [PATCH 2/3] nrf_rpc_uart: decode ACKs in ISR Currently, decoding both normal packets and ACKs happens in the receive thread. This imposes certain limitations, for example the receive thread shall not send any packets as this would introduce a chicken-egg problem: the thread is waiting for an ACK but the ACK can only be decoded by the current thread. More importantly, this can also introduce spurious ACK timeouts and retransissions because the receive thread may be blocked when trying to push a packet to the thead pool. Move the ACK decoding to ISR to overcome the mentioned issues. Signed-off-by: Damian Krolik --- subsys/nrf_rpc/nrf_rpc_uart.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/subsys/nrf_rpc/nrf_rpc_uart.c b/subsys/nrf_rpc/nrf_rpc_uart.c index 300b73e22012..41e2e427dc6f 100644 --- a/subsys/nrf_rpc/nrf_rpc_uart.c +++ b/subsys/nrf_rpc/nrf_rpc_uart.c @@ -17,6 +17,8 @@ LOG_MODULE_REGISTER(nrf_rpc_uart, CONFIG_NRF_RPC_TR_LOG_LEVEL); +#define CRC_SIZE sizeof(uint16_t) + enum { HDLC_CHAR_ESCAPE = 0x7d, HDLC_CHAR_DELIMITER = 0x7e, @@ -68,6 +70,10 @@ struct nrf_rpc_uart { K_KERNEL_STACK_MEMBER(rx_workq_stack, CONFIG_NRF_RPC_UART_RX_THREAD_STACK_SIZE); + /* HDLC ack decoding state */ + struct hdlc_decode_ctx rx_ack_ctx; + uint8_t rx_ack[CRC_SIZE]; + /* HDLC packet decoding state */ struct hdlc_decode_ctx rx_pkt_ctx; uint8_t rx_pkt[CONFIG_NRF_RPC_UART_MAX_PACKET_SIZE]; @@ -96,18 +102,16 @@ static void log_hexdump_dbg(const uint8_t *data, size_t length, const char *fmt, } } -#define CRC_SIZE sizeof(uint16_t) - static void send_byte(const struct device *dev, uint8_t byte); static void ack_rx(struct nrf_rpc_uart *uart_tr) { - if (!IS_ENABLED(CONFIG_NRF_RPC_UART_RELIABLE) || uart_tr->rx_pkt_ctx.len != CRC_SIZE) { - log_hexdump_dbg(uart_tr->rx_pkt, uart_tr->rx_pkt_ctx.len, ">>> RX invalid frame"); + if (!IS_ENABLED(CONFIG_NRF_RPC_UART_RELIABLE) || uart_tr->rx_ack_ctx.len != CRC_SIZE) { + log_hexdump_dbg(uart_tr->rx_ack, uart_tr->rx_ack_ctx.len, ">>> RX invalid frame"); return; } - uint16_t rx_ack = sys_get_le16(uart_tr->rx_pkt); + uint16_t rx_ack = sys_get_le16(uart_tr->rx_ack); LOG_DBG(">>> RX ack %04x", rx_ack); @@ -117,7 +121,6 @@ static void ack_rx(struct nrf_rpc_uart *uart_tr) } k_sem_give(&uart_tr->ack_sem); - k_yield(); } static void ack_tx(struct nrf_rpc_uart *uart_tr, uint16_t ack_pld) @@ -245,8 +248,8 @@ static void work_handler(struct k_work *work) continue; } + /* ACKs are already handled in ISR, so process only normal packets here */ if (uart_tr->rx_pkt_ctx.len <= CRC_SIZE) { - ack_rx(uart_tr); continue; } @@ -282,6 +285,17 @@ static void work_handler(struct k_work *work) } } +static void decode_ack(struct nrf_rpc_uart *inst, const uint8_t *in, size_t len) +{ + for (size_t i = 0; i < len; i++) { + hdlc_decode_byte(&inst->rx_ack_ctx, inst->rx_ack, in[i]); + + if (inst->rx_ack_ctx.state == HDLC_STATE_FRAME_FOUND) { + ack_rx(inst); + } + } +} + static void serial_cb(const struct device *uart, void *user_data) { struct nrf_rpc_uart *uart_tr = user_data; @@ -294,6 +308,7 @@ static void serial_cb(const struct device *uart, void *user_data) uart_tr->rx_ringbuf.size); if (rx_len > 0) { rx_len = uart_fifo_read(uart, rx_buffer, rx_len); + decode_ack(uart_tr, rx_buffer, rx_len); int err = ring_buf_put_finish(&uart_tr->rx_ringbuf, rx_len); (void)err; /*silence the compiler*/ __ASSERT_NO_MSG(err == 0); @@ -373,6 +388,8 @@ static int init(const struct nrf_rpc_tr *transport, nrf_rpc_tr_receive_handler_t uart_tr->rx_pkt_ctx.state = HDLC_STATE_UNSYNC; uart_tr->rx_pkt_ctx.capacity = sizeof(uart_tr->rx_pkt); + uart_tr->rx_ack_ctx.state = HDLC_STATE_UNSYNC; + uart_tr->rx_ack_ctx.capacity = sizeof(uart_tr->rx_ack); uart_irq_rx_enable(uart_tr->uart); nrf_rpc_uart_initialized_hook(uart_tr->uart); From 05596daab4a38685c7dd3d23b3bd2e6bc0c1fe08 Mon Sep 17 00:00:00 2001 From: Damian Krolik Date: Tue, 18 Mar 2025 12:25:42 +0100 Subject: [PATCH 3/3] manifest: pull nrf_rpc initialization fix Fix one missed scenario in which transport_initialized flag may be false because of the thread running nrf_rpc_init() being switched out by the scheduler before setting this flag. Signed-off-by: Damian Krolik --- west.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/west.yml b/west.yml index 382d5ca762da..5321f53a8265 100644 --- a/west.yml +++ b/west.yml @@ -144,7 +144,7 @@ manifest: - name: nrfxlib repo-path: sdk-nrfxlib path: nrfxlib - revision: 2251371286d4aeeb35cdd87531cc14162158b014 + revision: 77461844f6812642c43b69e0ab448b830f03caec - name: trusted-firmware-m repo-path: sdk-trusted-firmware-m path: modules/tee/tf-m/trusted-firmware-m