mgmt: smp: shell: use net_buf API for storing UART SMP fragments

So far there was a simple char array used as buffer, with additional
variable representing number of bytes already written into it. After
full frame was written, a boolean flag was simply set to notify thread
about being ready to be processed. There was however no mechanism
implemented to prevent new incoming bytes from overwriting such buffer
before (or during) being processed.

Use net_buf to store temporary frame. Define dedicated net_buf_pool,
from which such buffer will be allocated and freed after being
processed. This will prevent from reusing the same buffer before having
it fully processed (and returning once again to available buffer pool)
in shell thread.

Define also fifo that will store buffers that are ready to be
processed. This will be the mechanism for notifying thread about new
UART SMP fragments.

net_buf pool and k_fifo are used on purpose, keeping in mind their
additional overhead (mostly in RAM/ROM usage). This makes the code ready
for increasing number of buffers if needed. In this commit however we
stick with only 1 buffer, to keep minimal changes in processing flow.

Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>

include/mgmt/mcumgr/smp_shell.h

@@ -17,12 +17,14 @@
 extern "C" {
 #endif
 
+#define SMP_SHELL_RX_BUF_SIZE	127
+
 /** @brief Data used by SMP shell */
 struct smp_shell_data {
-	char mcumgr_buff[128];
+	struct net_buf_pool *buf_pool;
-	bool cmd_rdy;
+	struct k_fifo buf_ready;
+	struct net_buf *buf;
 	atomic_t esc_state;
-	uint32_t cur;
 };
 
 /**

subsys/mgmt/mcumgr/smp_shell.c

@@ -98,23 +98,26 @@ size_t smp_shell_rx_bytes(struct smp_shell_data *data, const uint8_t *bytes,
 
 		if (mcumgr_state == SMP_SHELL_MCUMGR_STATE_NONE) {
 			break;
+		} else if (mcumgr_state == SMP_SHELL_MCUMGR_STATE_HEADER &&
+			   !data->buf) {
+			data->buf = net_buf_alloc(data->buf_pool, K_NO_WAIT);
 		}
 
-		if (data->cur < sizeof(data->mcumgr_buff) - 1) {
+		if (data->buf && net_buf_tailroom(data->buf) > 0) {
-			data->mcumgr_buff[data->cur] = byte;
+			net_buf_add_u8(data->buf, byte);
-			++data->cur;
 		}
 
 		/* Newline in payload means complete frame */
 		if (mcumgr_state == SMP_SHELL_MCUMGR_STATE_PAYLOAD &&
 		    byte == '\n') {
-			data->mcumgr_buff[data->cur] = '\0';
+			if (data->buf) {
-			data->cmd_rdy = true;
+				net_buf_put(&data->buf_ready, data->buf);
+				data->buf = NULL;
+			}
 			atomic_clear_bit(&data->esc_state, ESC_MCUMGR_PKT_1);
 			atomic_clear_bit(&data->esc_state, ESC_MCUMGR_PKT_2);
 			atomic_clear_bit(&data->esc_state, ESC_MCUMGR_FRAG_1);
 			atomic_clear_bit(&data->esc_state, ESC_MCUMGR_FRAG_2);
-			data->cur = 0U;
 		}
 
 		++consumed;
@@ -125,21 +128,22 @@ size_t smp_shell_rx_bytes(struct smp_shell_data *data, const uint8_t *bytes,
 
 void smp_shell_process(struct smp_shell_data *data)
 {
-	if (data->cmd_rdy) {
+	struct net_buf *buf;
-		data->cmd_rdy = false;
+	struct net_buf *nb;
-		struct net_buf *nb;
-		int line_len;
 
-		/* Strip the trailing newline. */
+	buf = net_buf_get(&data->buf_ready, K_NO_WAIT);
-		line_len = strlen(data->mcumgr_buff) - 1;
+	if (!buf) {
-
+		return;
-		nb = mcumgr_serial_process_frag(&smp_shell_rx_ctxt,
-						data->mcumgr_buff,
-						line_len);
-		if (nb != NULL) {
-			zephyr_smp_rx_req(&smp_shell_transport, nb);
-		}
 	}
+
+	nb = mcumgr_serial_process_frag(&smp_shell_rx_ctxt,
+					buf->data,
+					buf->len);
+	if (nb != NULL) {
+		zephyr_smp_rx_req(&smp_shell_transport, nb);
+	}
+
+	net_buf_unref(buf);
 }
 
 static uint16_t smp_shell_get_mtu(const struct net_buf *nb)

subsys/shell/shell_uart.c

@@ -8,6 +8,7 @@
 #include <drivers/uart.h>
 #include <init.h>
 #include <logging/log.h>
+#include <net/buf.h>
 
 #define LOG_MODULE_NAME shell_uart
 LOG_MODULE_REGISTER(shell_uart);
@@ -18,6 +19,11 @@ LOG_MODULE_REGISTER(shell_uart);
 #define RX_POLL_PERIOD K_NO_WAIT
 #endif
 
+#ifdef CONFIG_MCUMGR_SMP_SHELL
+NET_BUF_POOL_DEFINE(smp_shell_rx_pool, 1, SMP_SHELL_RX_BUF_SIZE,
+		    0, NULL);
+#endif /* CONFIG_MCUMGR_SMP_SHELL */
+
 SHELL_UART_DEFINE(shell_transport_uart,
 		  CONFIG_SHELL_BACKEND_SERIAL_TX_RING_BUFFER_SIZE,
 		  CONFIG_SHELL_BACKEND_SERIAL_RX_RING_BUFFER_SIZE);
@@ -169,6 +175,11 @@ static int init(const struct shell_transport *transport,
 	sh_uart->ctrl_blk->handler = evt_handler;
 	sh_uart->ctrl_blk->context = context;
 
+#ifdef CONFIG_MCUMGR_SMP_SHELL
+	sh_uart->ctrl_blk->smp.buf_pool = &smp_shell_rx_pool;
+	k_fifo_init(&sh_uart->ctrl_blk->smp.buf_ready);
+#endif
+
 	if (IS_ENABLED(CONFIG_SHELL_BACKEND_SERIAL_INTERRUPT_DRIVEN)) {
 		uart_irq_init(sh_uart);
 	} else {