shell: shell_uart: add ring_buffers and interrupt support

Improved RX path to use ring buffer for incoming data instead of single
byte buffer. Improved TX path to use ring buffer. Added support for
asynchronous UART API (interrupts).

Signed-off-by: Krzysztof Chruscinski <[email protected]>

Author: nordic-krch

include/shell/shell_uart.h

@@ -8,24 +8,65 @@
 #define SHELL_UART_H__
 
 #include <shell/shell.h>
+#include <ring_buffer.h>
+#include <atomic.h>
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
 extern const struct shell_transport_api shell_uart_transport_api;
 
-struct shell_uart {
+/** @brief Shell UART transport instance control block (RW data). */
+struct shell_uart_ctrl_blk {
 	struct device *dev;
 	shell_transport_handler_t handler;
-	struct k_timer timer;
 	void *context;
-	u8_t rx[1];
-	size_t rx_cnt;
+	atomic_t tx_busy;
+	bool blocking;
+};
+
+#ifdef CONFIG_UART_INTERRUPT_DRIVEN
+#define UART_SHELL_TX_RINGBUF_DECLARE(_name, _size) \
+	RING_BUF_DECLARE(_name##_tx_ringbuf, _size)
+
+#define UART_SHELL_TX_BUF_DECLARE(_name) \
+	u8_t _name##_txbuf[SHELL_UART_TX_BUF_SIZE]
+
+#define UART_SHELL_RX_TIMER_DECLARE(_name) /* Empty */
+
+#define UART_SHELL_TX_RINGBUF_PTR(_name) (&_name##_tx_ringbuf)
+
+#define UART_SHELL_RX_TIMER_PTR(_name) NULL
+
+#else /* CONFIG_UART_INTERRUPT_DRIVEN */
+#define UART_SHELL_TX_RINGBUF_DECLARE(_name, _size) /* Empty */
+#define UART_SHELL_TX_BUF_DECLARE(_name) /* Empty */
+#define UART_SHELL_RX_TIMER_DECLARE(_name) static struct k_timer _name##_timer
+#define UART_SHELL_TX_RINGBUF_PTR(_name) NULL
+#define UART_SHELL_RX_TIMER_PTR(_name) (&_name##_timer)
+#endif /* CONFIG_UART_INTERRUPT_DRIVEN */
+
+/** @brief Shell UART transport instance structure. */
+struct shell_uart {
+	struct shell_uart_ctrl_blk *ctrl_blk;
+	struct k_timer *timer;
+	struct ring_buf *tx_ringbuf;
+	struct ring_buf *rx_ringbuf;
 };
 
-#define SHELL_UART_DEFINE(_name)					\
-	static struct shell_uart _name##_shell_uart;			\
+/** @brief Macro for creating shell UART transport instance. */
+#define SHELL_UART_DEFINE(_name, _tx_ringbuf_size, _rx_ringbuf_size)	\
+	static struct shell_uart_ctrl_blk _name##_ctrl_blk;		\
+	UART_SHELL_RX_TIMER_DECLARE(_name);				\
+	UART_SHELL_TX_RINGBUF_DECLARE(_name, _tx_ringbuf_size);		\
+	RING_BUF_DECLARE(_name##_rx_ringbuf, _rx_ringbuf_size);		\
+	static const struct shell_uart _name##_shell_uart = {		\
+		.ctrl_blk = &_name##_ctrl_blk,				\
+		.timer = UART_SHELL_RX_TIMER_PTR(_name),		\
+		.tx_ringbuf = UART_SHELL_TX_RINGBUF_PTR(_name),		\
+		.rx_ringbuf = &_name##_rx_ringbuf,			\
+	};								\
 	struct shell_transport _name = {				\
 		.api = &shell_uart_transport_api,			\
 		.ctx = (struct shell_uart *)&_name##_shell_uart		\

samples/net/echo_server/overlay-netusb.conf

@@ -9,3 +9,6 @@ CONFIG_USB_DEVICE_NETWORK_ECM=y
 CONFIG_USB_DRIVER_LOG_LEVEL_INF=y
 CONFIG_USB_DEVICE_LOG_LEVEL_INF=y
 CONFIG_INIT_STACKS=n
+
+# Disable shell built-in commands to reduce ROM footprint
+CONFIG_SHELL_CMDS=n

subsys/shell/Kconfig.backends

@@ -18,9 +18,46 @@ config SHELL_BACKEND_SERIAL
 	bool "Enable serial backends."
 	default y
 	select SERIAL
+	select RING_BUFFER
 	help
 	  Enable serial backends.
 
+# Internal config to enable UART interrupts if supported.
+config SHELL_BACKEND_SERIAL_FORCE_INTERRUPTS
+	bool
+	default y
+	depends on SERIAL_SUPPORT_INTERRUPT
+	imply UART_INTERRUPT_DRIVEN
+
+if SHELL_BACKEND_SERIAL
+
+config SHELL_BACKEND_SERIAL_TX_RING_BUFFER_SIZE
+	int "Set TX ring buffer size"
+	default 8
+	depends on UART_INTERRUPT_DRIVEN
+	help
+	  If UART is utilizing DMA transfers then increasing ring buffer size
+	  increases transfers length and reduces number of interrupts.
+
+config SHELL_BACKEND_SERIAL_RX_RING_BUFFER_SIZE
+	int "Set RX ring buffer size"
+	default 64
+	help
+	  RX ring buffer size impacts accepted latency of handling incoming
+	  bytes by shell. If shell input is coming from the keyboard then it is
+	  usually enough if ring buffer is few bytes (more than one due to
+	  escape sequences). However, if bulk data is transferred it may be
+	  required to increase it.
+
+config SHELL_BACKEND_SERIAL_RX_POLL_PERIOD
+	int "RX polling period (in milliseconds)"
+	default 10
+	depends on !UART_INTERRUPT_DRIVEN
+	help
+	  Determines how often UART is polled for RX byte.
+
+endif #SHELL_BACKEND_SERIAL
+
 config SHELL_BACKEND_RTT
 	bool "Enable RTT backend."
 	select RTT_CONSOLE

subsys/shell/shell_uart.c

@@ -7,37 +7,131 @@
 #include <shell/shell_uart.h>
 #include <uart.h>
 #include <init.h>
+#include <logging/log.h>
 
-SHELL_UART_DEFINE(shell_transport_uart);
+#define LOG_MODULE_NAME shell_uart
+LOG_MODULE_REGISTER(shell_uart);
+
+#ifdef CONFIG_SHELL_BACKEND_SERIAL_RX_POLL_PERIOD
+#define RX_POLL_PERIOD CONFIG_SHELL_BACKEND_SERIAL_RX_POLL_PERIOD
+#else
+#define RX_POLL_PERIOD 0
+#endif
+
+SHELL_UART_DEFINE(shell_transport_uart,
+		  CONFIG_SHELL_BACKEND_SERIAL_TX_RING_BUFFER_SIZE,
+		  CONFIG_SHELL_BACKEND_SERIAL_RX_RING_BUFFER_SIZE);
 SHELL_DEFINE(uart_shell, "uart:~$ ", &shell_transport_uart, 10,
 	     SHELL_FLAG_OLF_CRLF);
 
-static void timer_handler(struct k_timer *timer)
+#ifdef CONFIG_UART_INTERRUPT_DRIVEN
+static void uart_rx_handle(const struct shell_uart *sh_uart)
 {
-	struct shell_uart *sh_uart =
-			CONTAINER_OF(timer, struct shell_uart, timer);
+	u8_t *data;
+	u32_t len;
+	u32_t rd_len;
+	bool new_data = false;
+
+	do {
+		len = ring_buf_put_claim(sh_uart->rx_ringbuf, &data,
+					 sh_uart->rx_ringbuf->size);
+		rd_len = uart_fifo_read(sh_uart->ctrl_blk->dev, data, len);
+
+		if (rd_len) {
+			new_data = true;
+		}
+
+		ring_buf_put_finish(sh_uart->rx_ringbuf, rd_len);
+	} while (rd_len && (rd_len == len));
+
+	if (new_data) {
+		sh_uart->ctrl_blk->handler(SHELL_TRANSPORT_EVT_RX_RDY,
+					   sh_uart->ctrl_blk->context);
+	}
+}
 
-	if (uart_poll_in(sh_uart->dev, sh_uart->rx) == 0) {
-		sh_uart->rx_cnt = 1;
-		sh_uart->handler(SHELL_TRANSPORT_EVT_RX_RDY, sh_uart->context);
+static void uart_tx_handle(const struct shell_uart *sh_uart)
+{
+	u32_t len;
+	u8_t *data;
+	int err;
+	struct device *dev = sh_uart->ctrl_blk->dev;
+
+	len = ring_buf_get_claim(sh_uart->tx_ringbuf, &data,
+				 sh_uart->tx_ringbuf->size);
+	if (len) {
+		len = uart_fifo_fill(dev, data, len);
+		err = ring_buf_get_finish(sh_uart->tx_ringbuf, len);
+		__ASSERT_NO_MSG(err == 0);
+	} else {
+		uart_irq_tx_disable(dev);
+		sh_uart->ctrl_blk->tx_busy = 0;
 	}
+
+	sh_uart->ctrl_blk->handler(SHELL_TRANSPORT_EVT_TX_RDY,
+				   sh_uart->ctrl_blk->context);
 }
 
+static void uart_callback(void *user_data)
+{
+	const struct shell_uart *sh_uart = (struct shell_uart *)user_data;
+	struct device *dev = sh_uart->ctrl_blk->dev;
+
+	uart_irq_update(dev);
+
+	if (uart_irq_rx_ready(dev)) {
+		uart_rx_handle(sh_uart);
+	}
+
+	if (uart_irq_tx_ready(dev)) {
+		uart_tx_handle(sh_uart);
+	}
+}
+#endif /* CONFIG_UART_INTERRUPT_DRIVEN */
+
+static void uart_irq_init(const struct shell_uart *sh_uart)
+{
+#ifdef CONFIG_UART_INTERRUPT_DRIVEN
+	struct device *dev = sh_uart->ctrl_blk->dev;
+
+	uart_irq_callback_user_data_set(dev, uart_callback, (void *)sh_uart);
+	uart_irq_rx_enable(dev);
+#endif
+}
+
+static void timer_handler(struct k_timer *timer)
+{
+	u8_t c;
+	const struct shell_uart *sh_uart = k_timer_user_data_get(timer);
+
+	while (uart_poll_in(sh_uart->ctrl_blk->dev, &c) == 0) {
+		if (ring_buf_put(sh_uart->rx_ringbuf, &c, 1) == 0) {
+			/* ring buffer full. */
+			LOG_WRN("RX ring buffer full.");
+		}
+		sh_uart->ctrl_blk->handler(SHELL_TRANSPORT_EVT_RX_RDY,
+					   sh_uart->ctrl_blk->context);
+	}
+}
 
 static int init(const struct shell_transport *transport,
 		const void *config,
 		shell_transport_handler_t evt_handler,
 		void *context)
 {
-	struct shell_uart *sh_uart = (struct shell_uart *)transport->ctx;
-
-	sh_uart->dev = device_get_binding(CONFIG_UART_CONSOLE_ON_DEV_NAME);
-	sh_uart->handler = evt_handler;
-	sh_uart->context = context;
+	const struct shell_uart *sh_uart = (struct shell_uart *)transport->ctx;
 
-	k_timer_init(&sh_uart->timer, timer_handler, NULL);
+	sh_uart->ctrl_blk->dev = (struct device *)config;
+	sh_uart->ctrl_blk->handler = evt_handler;
+	sh_uart->ctrl_blk->context = context;
 
-	k_timer_start(&sh_uart->timer, 20, 20);
+	if (IS_ENABLED(CONFIG_UART_INTERRUPT_DRIVEN)) {
+		uart_irq_init(sh_uart);
+	} else {
+		k_timer_init(sh_uart->timer, timer_handler, NULL);
+		k_timer_user_data_set(sh_uart->timer, (void *)sh_uart);
+		k_timer_start(sh_uart->timer, RX_POLL_PERIOD, RX_POLL_PERIOD);
+	}
 
 	return 0;
 }
@@ -49,22 +143,54 @@ static int uninit(const struct shell_transport *transport)
 
 static int enable(const struct shell_transport *transport, bool blocking)
 {
+	const struct shell_uart *sh_uart = (struct shell_uart *)transport->ctx;
+
+	sh_uart->ctrl_blk->blocking = blocking;
+
+	if (blocking) {
+		if (!IS_ENABLED(CONFIG_UART_INTERRUPT_DRIVEN)) {
+			k_timer_stop(sh_uart->timer);
+		}
+#ifdef CONFIG_UART_INTERRUPT_DRIVEN
+		uart_irq_rx_disable(sh_uart->ctrl_blk->dev);
+		uart_irq_tx_disable(sh_uart->ctrl_blk->dev);
+#endif
+	}
+
 	return 0;
 }
 
+static void irq_write(const struct shell_uart *sh_uart, const void *data,
+		     size_t length, size_t *cnt)
+{
+	*cnt = ring_buf_put(sh_uart->tx_ringbuf, data, length);
+
+	if (atomic_set(&sh_uart->ctrl_blk->tx_busy, 1) == 0) {
+#ifdef CONFIG_UART_INTERRUPT_DRIVEN
+		uart_irq_tx_enable(sh_uart->ctrl_blk->dev);
+#endif
+	}
+}
+
 static int write(const struct shell_transport *transport,
 		 const void *data, size_t length, size_t *cnt)
 {
-	struct shell_uart *sh_uart = (struct shell_uart *)transport->ctx;
+	const struct shell_uart *sh_uart = (struct shell_uart *)transport->ctx;
 	const u8_t *data8 = (const u8_t *)data;
 
-	for (size_t i = 0; i < length; i++) {
-		uart_poll_out(sh_uart->dev, data8[i]);
-	}
+	if (IS_ENABLED(CONFIG_UART_INTERRUPT_DRIVEN) &&
+		!sh_uart->ctrl_blk->blocking) {
+		irq_write(sh_uart, data, length, cnt);
+	} else {
+		for (size_t i = 0; i < length; i++) {
+			uart_poll_out(sh_uart->ctrl_blk->dev, data8[i]);
+		}
 
-	*cnt = length;
+		*cnt = length;
 
-	sh_uart->handler(SHELL_TRANSPORT_EVT_TX_RDY, sh_uart->context);
+		sh_uart->ctrl_blk->handler(SHELL_TRANSPORT_EVT_TX_RDY,
+					   sh_uart->ctrl_blk->context);
+	}
 
 	return 0;
 }
@@ -73,17 +199,8 @@ static int read(const struct shell_transport *transport,
 		void *data, size_t length, size_t *cnt)
 {
 	struct shell_uart *sh_uart = (struct shell_uart *)transport->ctx;
-	u32_t key;
 
-	key = irq_lock();
-	if (sh_uart->rx_cnt) {
-		memcpy(data, sh_uart->rx, 1);
-		sh_uart->rx_cnt = 0;
-		*cnt = 1;
-	} else {
-		*cnt = 0;
-	}
-	irq_unlock(key);
+	*cnt = ring_buf_get(sh_uart->rx_ringbuf, data, length);
 
 	return 0;
 }
@@ -99,7 +216,9 @@ const struct shell_transport_api shell_uart_transport_api = {
 static int enable_shell_uart(struct device *arg)
 {
 	ARG_UNUSED(arg);
-	shell_init(&uart_shell, NULL, true, true, LOG_LEVEL_INF);
+	struct device *dev =
+			device_get_binding(CONFIG_UART_CONSOLE_ON_DEV_NAME);
+	shell_init(&uart_shell, dev, true, true, LOG_LEVEL_INF);
 	return 0;
 }
 SYS_INIT(enable_shell_uart, POST_KERNEL, 0);