Skip to content

Commit 279062e

Browse files
Damian-Nordicrlubos
Damian-Nordic
authored and
rlubos
committed
nrf_rpc: address UART transport review comments
1. Capitalize enum constants. 2. Use Kconfig instead of hard-coded constants. 3. Use more intuitive variable naming. 4. Replace semaphore with mutex for critical section. 5. Move RX work queue stack to the nrf_rpc_uart struct. Signed-off-by: Damian Krolik <[email protected]>
1 parent 441982e commit 279062e

File tree

3 files changed

+128
-103
lines changed

3 files changed

+128
-103
lines changed

‎include/nrf_rpc/nrf_rpc_uart.h‎

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -24,41 +24,6 @@ extern "C" {
2424
* @{
2525
*/
2626

27-
#define NRF_RPC_MAX_FRAME_SIZE 1536
28-
#define NRF_RPC_RX_RINGBUF_SIZE 2048
29-
30-
typedef enum {
31-
hdlc_state_unsync,
32-
hdlc_state_frame_start,
33-
hdlc_state_frame_found,
34-
hdlc_state_escape,
35-
} hdlc_state_t;
36-
37-
struct nrf_rpc_uart {
38-
const struct device *uart;
39-
nrf_rpc_tr_receive_handler_t receive_callback;
40-
void *receive_ctx;
41-
const struct nrf_rpc_tr *transport;
42-
43-
/* RX buffer populated by UART ISR */
44-
uint8_t rx_buffer[NRF_RPC_RX_RINGBUF_SIZE];
45-
struct ring_buf rx_ringbuf;
46-
47-
/* ISR function callback worker */
48-
struct k_work cb_work;
49-
50-
/* HDLC frame parsing state */
51-
hdlc_state_t hdlc_state;
52-
uint8_t frame_buffer[NRF_RPC_MAX_FRAME_SIZE];
53-
size_t frame_len;
54-
55-
/* UART send semaphore */
56-
struct k_sem uart_tx_sem;
57-
58-
/* Workqueue for rx*/
59-
struct k_work_q rx_workq;
60-
};
61-
6227
/**
6328
* @brief Returns nRF RPC UART transport object name for the given devicetree node.
6429
*

‎subsys/nrf_rpc/Kconfig‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ config _NRF_RPC_DUMMY_SELECT
1616
select THREAD_CUSTOM_DATA
1717

1818
choice NRF_RPC_TRANSPORT
19-
prompt "NRF RPC transport choice"
19+
prompt "NRF RPC transport"
2020
default NRF_RPC_IPC_SERVICE
2121

2222
config NRF_RPC_IPC_SERVICE
@@ -25,13 +25,15 @@ config NRF_RPC_IPC_SERVICE
2525
select MBOX
2626
select EVENTS
2727
help
28-
If enabled, selects the IPC Service as a transport layer for nRF RPC.
28+
If enabled, selects the IPC Service as a transport layer for the nRF RPC.
2929

3030
config NRF_RPC_UART_TRANSPORT
31-
bool "UART transport for nRF RPC"
31+
bool "nRF RPC over UART"
3232
select UART_NRFX
3333
select RING_BUFFER
3434
select CRC
35+
help
36+
If enabled, selects the UART as a transport layer for the nRF RPC.
3537

3638
endchoice
3739

@@ -56,6 +58,34 @@ config NRF_RPC_IPC_SERVICE_BIND_TIMEOUT_MS
5658

5759
endif # NRF_RPC_IPC_SERVICE
5860

61+
62+
menu "nRF RPC over UART configuration"
63+
depends on NRF_RPC_UART_TRANSPORT
64+
65+
config NRF_RPC_UART_MAX_PACKET_SIZE
66+
int "Maximum packet size"
67+
default 1536
68+
help
69+
Defines the maximum size of an nRF RPC packet that can be sent or received
70+
using the UART transport.
71+
72+
config NRF_RPC_UART_RX_RINGBUF_SIZE
73+
int "RX ring buffer size"
74+
default 2048
75+
help
76+
Defines the size of the ring buffer used to relay received bytes between
77+
the UART interrupt service routine and the UART transport RX worker thread.
78+
79+
config NRF_RPC_UART_RX_THREAD_STACK_SIZE
80+
int "RX thread stack size"
81+
default 4096
82+
help
83+
Defines the stack size of the UART transport RX worker thread. The worker
84+
thread is responsible for consuming data received over the UART, and
85+
passing decoded nRF RPC packets to the nRF RPC core.
86+
87+
endmenu # "nRF RPC over UART configuration"
88+
5989
config NRF_RPC_CBOR
6090
bool
6191
select ZCBOR

‎subsys/nrf_rpc/nrf_rpc_uart.c‎

Lines changed: 95 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -4,54 +4,83 @@
44
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
55
*/
66

7-
#include <stdbool.h>
87
#include <nrf_rpc.h>
98
#include <nrf_rpc_tr.h>
109
#include <nrf_rpc/nrf_rpc_uart.h>
1110
#include <nrf_rpc_errno.h>
12-
#include <sys/_stdint.h>
11+
1312
#include <zephyr/drivers/uart.h>
13+
#include <zephyr/logging/log.h>
1414
#include <zephyr/sys/util.h>
1515
#include <zephyr/sys/byteorder.h>
1616
#include <zephyr/sys/crc.h>
1717

18-
#include <zephyr/logging/log.h>
19-
2018
LOG_MODULE_REGISTER(nrf_rpc_uart, CONFIG_NRF_RPC_TR_LOG_LEVEL);
2119

2220
enum {
23-
/* TODO: check if we need handle other special values like XON or XOFF */
24-
hdlc_char_escape = 0x7d,
25-
hdlc_char_delimiter = 0x7e,
21+
HDLC_CHAR_ESCAPE = 0x7d,
22+
HDLC_CHAR_DELIMITER = 0x7e,
23+
};
24+
25+
typedef enum {
26+
HDLC_STATE_UNSYNC,
27+
HDLC_STATE_FRAME_START,
28+
HDLC_STATE_FRAME_FOUND,
29+
HDLC_STATE_ESCAPE,
30+
} hdlc_state_t;
31+
32+
struct nrf_rpc_uart {
33+
const struct device *uart;
34+
nrf_rpc_tr_receive_handler_t receive_callback;
35+
void *receive_ctx;
36+
const struct nrf_rpc_tr *transport;
37+
38+
/* RX ring buffer populated by UART ISR */
39+
uint8_t rx_buffer[CONFIG_NRF_RPC_UART_RX_RINGBUF_SIZE];
40+
struct ring_buf rx_ringbuf;
41+
42+
/* RX work to consume and decode bytes from RX ring buffer */
43+
struct k_work rx_work;
44+
struct k_work_q rx_workq;
45+
46+
K_KERNEL_STACK_MEMBER(rx_workq_stack, CONFIG_NRF_RPC_UART_RX_THREAD_STACK_SIZE);
47+
48+
/* HDLC frame parsing state */
49+
hdlc_state_t hdlc_state;
50+
uint8_t rx_packet[CONFIG_NRF_RPC_UART_MAX_PACKET_SIZE];
51+
size_t rx_packet_len;
52+
53+
/* TX lock */
54+
struct k_mutex tx_lock;
2655
};
2756

2857
#define CRC_SIZE sizeof(uint16_t)
2958

30-
static int hdlc_input_byte(struct nrf_rpc_uart *uart_tr, uint8_t byte)
59+
static int hdlc_decode_byte(struct nrf_rpc_uart *uart_tr, uint8_t byte)
3160
{
3261
/* TODO: handle frame buffer overflow */
3362

34-
if (uart_tr->hdlc_state == hdlc_state_escape) {
35-
uart_tr->frame_buffer[uart_tr->frame_len++] = byte ^ 0x20;
36-
uart_tr->hdlc_state = hdlc_state_frame_start;
37-
} else if (byte == hdlc_char_escape) {
38-
uart_tr->hdlc_state = hdlc_state_escape;
39-
} else if (byte == hdlc_char_delimiter) {
40-
uart_tr->hdlc_state = (uart_tr->hdlc_state == hdlc_state_frame_start)
41-
? hdlc_state_frame_found
42-
: hdlc_state_unsync;
63+
if (uart_tr->hdlc_state == HDLC_STATE_ESCAPE) {
64+
uart_tr->rx_packet[uart_tr->rx_packet_len++] = byte ^ 0x20;
65+
uart_tr->hdlc_state = HDLC_STATE_FRAME_START;
66+
} else if (byte == HDLC_CHAR_ESCAPE) {
67+
uart_tr->hdlc_state = HDLC_STATE_ESCAPE;
68+
} else if (byte == HDLC_CHAR_DELIMITER) {
69+
uart_tr->hdlc_state = (uart_tr->hdlc_state == HDLC_STATE_FRAME_START)
70+
? HDLC_STATE_FRAME_FOUND
71+
: HDLC_STATE_UNSYNC;
4372
} else {
44-
uart_tr->frame_buffer[uart_tr->frame_len++] = byte;
45-
uart_tr->hdlc_state = hdlc_state_frame_start;
73+
uart_tr->rx_packet[uart_tr->rx_packet_len++] = byte;
74+
uart_tr->hdlc_state = HDLC_STATE_FRAME_START;
4675
}
4776

48-
if (uart_tr->hdlc_state == hdlc_state_frame_found) {
49-
LOG_HEXDUMP_DBG(uart_tr->frame_buffer, uart_tr->frame_len, "Received frame");
50-
if (uart_tr->frame_len > CRC_SIZE) {
51-
uart_tr->frame_len -= CRC_SIZE;
77+
if (uart_tr->hdlc_state == HDLC_STATE_FRAME_FOUND) {
78+
LOG_HEXDUMP_DBG(uart_tr->rx_packet, uart_tr->rx_packet_len, "Received frame");
79+
if (uart_tr->rx_packet_len > CRC_SIZE) {
80+
uart_tr->rx_packet_len -= CRC_SIZE;
5281
} else {
53-
uart_tr->frame_len = 0;
54-
uart_tr->hdlc_state = hdlc_state_unsync;
82+
uart_tr->rx_packet_len = 0;
83+
uart_tr->hdlc_state = HDLC_STATE_UNSYNC;
5584
}
5685
}
5786

@@ -60,7 +89,7 @@ static int hdlc_input_byte(struct nrf_rpc_uart *uart_tr, uint8_t byte)
6089

6190
static void work_handler(struct k_work *work)
6291
{
63-
struct nrf_rpc_uart *uart_tr = CONTAINER_OF(work, struct nrf_rpc_uart, cb_work);
92+
struct nrf_rpc_uart *uart_tr = CONTAINER_OF(work, struct nrf_rpc_uart, rx_work);
6493
const struct nrf_rpc_tr *transport = uart_tr->transport;
6594
uint8_t *data;
6695
size_t len;
@@ -69,30 +98,32 @@ static void work_handler(struct k_work *work)
6998
uint16_t crc_calculated = 0;
7099

71100
while (!ring_buf_is_empty(&uart_tr->rx_ringbuf)) {
72-
len = ring_buf_get_claim(&uart_tr->rx_ringbuf, &data, NRF_RPC_MAX_FRAME_SIZE);
101+
len = ring_buf_get_claim(&uart_tr->rx_ringbuf, &data,
102+
CONFIG_NRF_RPC_UART_MAX_PACKET_SIZE);
73103
for (size_t i = 0; i < len; i++) {
74-
if (hdlc_input_byte(uart_tr, data[i]) == 0) {
75-
76-
if (uart_tr->hdlc_state == hdlc_state_frame_found) {
77-
crc_received = sys_get_le16(uart_tr->frame_buffer +
78-
uart_tr->frame_len);
79-
crc_calculated = crc16_ccitt(0xffff, uart_tr->frame_buffer,
80-
uart_tr->frame_len);
81-
if (crc_calculated != crc_received) {
82-
LOG_ERR("Invalid CRC, calculated %x received %x",
83-
crc_calculated, crc_received);
84-
uart_tr->frame_len = 0;
85-
uart_tr->hdlc_state = hdlc_state_unsync;
86-
return;
87-
}
88-
89-
uart_tr->receive_callback(transport, uart_tr->frame_buffer,
90-
uart_tr->frame_len,
91-
uart_tr->receive_ctx);
92-
uart_tr->frame_len = 0;
93-
uart_tr->hdlc_state = hdlc_state_unsync;
94-
}
104+
if (hdlc_decode_byte(uart_tr, data[i]) != 0) {
105+
continue;
106+
}
107+
108+
if (uart_tr->hdlc_state != HDLC_STATE_FRAME_FOUND) {
109+
continue;
110+
}
111+
112+
crc_received = sys_get_le16(uart_tr->rx_packet + uart_tr->rx_packet_len);
113+
crc_calculated =
114+
crc16_ccitt(0xffff, uart_tr->rx_packet, uart_tr->rx_packet_len);
115+
if (crc_calculated != crc_received) {
116+
LOG_ERR("Invalid CRC, calculated %x received %x", crc_calculated,
117+
crc_received);
118+
uart_tr->rx_packet_len = 0;
119+
uart_tr->hdlc_state = HDLC_STATE_UNSYNC;
120+
return;
95121
}
122+
123+
uart_tr->receive_callback(transport, uart_tr->rx_packet,
124+
uart_tr->rx_packet_len, uart_tr->receive_ctx);
125+
uart_tr->rx_packet_len = 0;
126+
uart_tr->hdlc_state = HDLC_STATE_UNSYNC;
96127
}
97128

98129
ret = ring_buf_get_finish(&uart_tr->rx_ringbuf, len);
@@ -133,13 +164,10 @@ static void serial_cb(const struct device *uart, void *user_data)
133164
}
134165

135166
if (new_data) {
136-
k_work_submit_to_queue(&uart_tr->rx_workq, &uart_tr->cb_work);
167+
k_work_submit_to_queue(&uart_tr->rx_workq, &uart_tr->rx_work);
137168
}
138169
}
139170

140-
/* TODO: for moving it to uart_tr.*/
141-
K_THREAD_STACK_DEFINE(uart_tr_workq_stack_area, 4096);
142-
143171
static int init(const struct nrf_rpc_tr *transport, nrf_rpc_tr_receive_handler_t receive_cb,
144172
void *context)
145173
{
@@ -180,27 +208,26 @@ static int init(const struct nrf_rpc_tr *transport, nrf_rpc_tr_receive_handler_t
180208
return 0;
181209
}
182210

183-
k_sem_init(&uart_tr->uart_tx_sem, 1, 1);
211+
k_mutex_init(&uart_tr->tx_lock);
184212

185213
k_work_queue_init(&uart_tr->rx_workq);
186-
k_work_queue_start(&uart_tr->rx_workq, uart_tr_workq_stack_area,
187-
K_THREAD_STACK_SIZEOF(uart_tr_workq_stack_area), 0, NULL);
214+
k_work_queue_start(&uart_tr->rx_workq, uart_tr->rx_workq_stack,
215+
K_THREAD_STACK_SIZEOF(uart_tr->rx_workq_stack), K_PRIO_PREEMPT(0), NULL);
188216

189-
k_work_init(&uart_tr->cb_work, work_handler);
217+
k_work_init(&uart_tr->rx_work, work_handler);
190218
ring_buf_init(&uart_tr->rx_ringbuf, sizeof(uart_tr->rx_buffer), uart_tr->rx_buffer);
191219

192-
uart_tr->hdlc_state = hdlc_state_unsync;
193-
uart_tr->frame_len = 0;
220+
uart_tr->hdlc_state = HDLC_STATE_UNSYNC;
221+
uart_tr->rx_packet_len = 0;
194222
uart_irq_rx_enable(uart_tr->uart);
195223

196224
return 0;
197225
}
198226

199227
static void send_byte(const struct device *dev, uint8_t byte)
200228
{
201-
202-
if (byte == hdlc_char_delimiter || byte == hdlc_char_escape) {
203-
uart_poll_out(dev, hdlc_char_escape);
229+
if (byte == HDLC_CHAR_DELIMITER || byte == HDLC_CHAR_ESCAPE) {
230+
uart_poll_out(dev, HDLC_CHAR_ESCAPE);
204231
byte ^= 0x20;
205232
}
206233

@@ -214,9 +241,9 @@ static int send(const struct nrf_rpc_tr *transport, const uint8_t *data, size_t
214241

215242
LOG_HEXDUMP_DBG(data, length, "Sending frame");
216243

217-
k_sem_take(&uart_tr->uart_tx_sem, K_FOREVER);
244+
k_mutex_lock(&uart_tr->tx_lock, K_FOREVER);
218245

219-
uart_poll_out(uart_tr->uart, hdlc_char_delimiter);
246+
uart_poll_out(uart_tr->uart, HDLC_CHAR_DELIMITER);
220247

221248
for (size_t i = 0; i < length; i++) {
222249
send_byte(uart_tr->uart, data[i]);
@@ -226,10 +253,11 @@ static int send(const struct nrf_rpc_tr *transport, const uint8_t *data, size_t
226253
send_byte(uart_tr->uart, crc[0]);
227254
send_byte(uart_tr->uart, crc[1]);
228255

229-
uart_poll_out(uart_tr->uart, hdlc_char_delimiter);
256+
uart_poll_out(uart_tr->uart, HDLC_CHAR_DELIMITER);
230257

231258
k_free((void *)data);
232-
k_sem_give(&uart_tr->uart_tx_sem);
259+
260+
k_mutex_unlock(&uart_tr->tx_lock);
233261

234262
return 0;
235263
}
@@ -255,6 +283,8 @@ static void *tx_buf_alloc(const struct nrf_rpc_tr *transport, size_t *size)
255283

256284
static void tx_buf_free(const struct nrf_rpc_tr *transport, void *buf)
257285
{
286+
ARG_UNUSED(transport);
287+
258288
k_free(buf);
259289
}
260290

0 commit comments

Comments
 (0)