drivers: serial: uart_async_rx: Improvements in uart_async_rx module

drivers/serial/uart_async_rx.c

@@ -54,13 +54,14 @@ void uart_async_rx_on_rdy(struct uart_async_rx *rx_data, uint8_t *buffer, size_t
 
 static void buf_reset(struct uart_async_rx_buf *buf)
 {
-	buf->rd_idx = 0;
 	buf->wr_idx = 0;
 	buf->completed = 0;
 }
+
 static void usr_rx_buf_release(struct uart_async_rx *rx_data, struct uart_async_rx_buf *buf)
 {
 	buf_reset(buf);
+	rx_data->rd_idx = 0;
 	rx_data->rd_buf_idx = inc(rx_data, rx_data->rd_buf_idx);
 	atomic_inc(&rx_data->free_buf_cnt);
 	__ASSERT_NO_MSG(rx_data->free_buf_cnt <= rx_data->config->buf_cnt);
@@ -75,7 +76,6 @@ void uart_async_rx_on_buf_rel(struct uart_async_rx *rx_data, uint8_t *buffer)
 		(struct uart_async_rx_buf *)(buffer - offsetof(struct uart_async_rx_buf, buffer));
 
 	rx_buf->completed = 1;
-	rx_data->wr_buf_idx = inc(rx_data, rx_data->wr_buf_idx);
 }
 
 size_t uart_async_rx_data_claim(struct uart_async_rx *rx_data, uint8_t **data, size_t length)
@@ -89,33 +89,43 @@ size_t uart_async_rx_data_claim(struct uart_async_rx *rx_data, uint8_t **data, s
 
 	do {
 		buf = get_buf(rx_data, rx_data->rd_buf_idx);
-		if ((buf->rd_idx == buf->wr_idx) && (buf->completed == 1)) {
+		/* Even though buffer is released in consume phase it is possible that
+		 * it is required here as well (e.g. was not completed previously).
+		 */
+		if ((buf->completed == 1) && (rx_data->rd_idx == buf->wr_idx)) {
 			usr_rx_buf_release(rx_data, buf);
 		} else {
 			break;
 		}
 	} while (1);
 
-	*data = &buf->buffer[buf->rd_idx];
-	rem = buf->wr_idx - buf->rd_idx;
+	*data = &buf->buffer[rx_data->rd_idx];
+	rem = buf->wr_idx - rx_data->rd_idx;
 
 	return MIN(length, rem);
 }
 
-void uart_async_rx_data_consume(struct uart_async_rx *rx_data, size_t length)
+bool uart_async_rx_data_consume(struct uart_async_rx *rx_data, size_t length)
 {
 	struct uart_async_rx_buf *buf = get_buf(rx_data, rx_data->rd_buf_idx);
 
-	buf->rd_idx += length;
+	rx_data->rd_idx += length;
+	/* Attempt to release the buffer if it is completed and all data is consumed. */
+	if ((buf->completed == 1) && (rx_data->rd_idx == buf->wr_idx)) {
+		usr_rx_buf_release(rx_data, buf);
+	}
 
 	atomic_sub(&rx_data->pending_bytes, length);
 
-	__ASSERT_NO_MSG(buf->rd_idx <= buf->wr_idx);
+	__ASSERT_NO_MSG(rx_data->rd_idx <= buf->wr_idx);
+
+	return rx_data->free_buf_cnt > 0;
 }
 
 void uart_async_rx_reset(struct uart_async_rx *rx_data)
 {
 	rx_data->free_buf_cnt = rx_data->config->buf_cnt;
+	rx_data->rd_idx = 0;
 	for (uint8_t i = 0; i < rx_data->config->buf_cnt; i++) {
 		buf_reset(get_buf(rx_data, i));
 	}
@@ -128,6 +138,10 @@ int uart_async_rx_init(struct uart_async_rx *rx_data,
 	memset(rx_data, 0, sizeof(*rx_data));
 	rx_data->config = config;
 	rx_data->buf_len = (config->length / config->buf_cnt) - UART_ASYNC_RX_BUF_OVERHEAD;
+
+	if (rx_data->buf_len >= BIT(7)) {
+		return -EINVAL;
+	}
 	uart_async_rx_reset(rx_data);
 
 	return 0;

drivers/serial/uart_async_to_irq.c

@@ -195,24 +195,23 @@ int z_uart_async_to_irq_fifo_read(const struct device *dev,
 	}
 
 	memcpy(buf, claim_buf, claim_len);
-	uart_async_rx_data_consume(async_rx, claim_len);
+	bool buf_available = uart_async_rx_data_consume(async_rx, claim_len);
 
-	if (data->rx.pending_buf_req) {
+	if (data->rx.pending_buf_req && buf_available) {
 		buf = uart_async_rx_buf_req(async_rx);
-		if (buf) {
-			int err;
-			size_t rx_len = uart_async_rx_get_buf_len(async_rx);
+		__ASSERT_NO_MSG(buf != NULL);
+		int err;
+		size_t rx_len = uart_async_rx_get_buf_len(async_rx);
 
-			atomic_dec(&data->rx.pending_buf_req);
-			err = config->api->rx_buf_rsp(dev, buf, rx_len);
+		atomic_dec(&data->rx.pending_buf_req);
+		err = config->api->rx_buf_rsp(dev, buf, rx_len);
+		if (err < 0) {
+			if (err == -EACCES) {
+				data->rx.pending_buf_req = 0;
+				err = rx_enable(dev, data, buf, rx_len);
+			}
 			if (err < 0) {
-				if (err == -EACCES) {
-					data->rx.pending_buf_req = 0;
-					err = rx_enable(dev, data, buf, rx_len);
-				}
-				if (err < 0) {
-					return err;
-				}
+				return err;
 			}
 		}
 	}

include/zephyr/drivers/serial/uart_async_rx.h

@@ -23,15 +23,10 @@ struct uart_async_rx_buf {
 	/* Write index which is incremented whenever new data is reported to be
 	 * received to that buffer.
 	 */
-	uint8_t wr_idx;
-
-	/* Read index which is incremented whenever data is consumed from the buffer.
-	 * Read index cannot be higher than the write index.
-	 */
-	uint8_t rd_idx;
+	uint8_t wr_idx:7;
 
 	/* Set to one if buffer is released by the driver. */
-	uint8_t completed;
+	uint8_t completed:1;
 
 	/* Location which is passed to the UART driver. */
 	uint8_t buffer[];
@@ -54,11 +49,13 @@ struct uart_async_rx {
 	/* Index of the next buffer to be provided to the driver. */
 	uint8_t drv_buf_idx;
 
-	/* Current buffer to which data is written. */
-	uint8_t wr_buf_idx;
-
 	/* Current buffer from which data is being consumed. */
 	uint8_t rd_buf_idx;
+
+	/* Current read index in the buffer from which data is being consumed.
+	 * Read index which is incremented whenever data is consumed from the buffer.
+	 */
+	uint8_t rd_idx;
 };
 
 /** @brief UART asynchronous RX helper configuration structure. */
@@ -166,8 +163,11 @@ size_t uart_async_rx_data_claim(struct uart_async_rx *async_rx, uint8_t **data,
  *
  * @param async_rx Pointer to the helper instance.
  * @param length Amount of data to consume. It must be less or equal than amount of claimed data.
+ *
+ * @retval true If there are free buffers in the pool after data got consumed.
+ * @retval false If there are no free buffers.
  */
-void uart_async_rx_data_consume(struct uart_async_rx *async_rx, size_t length);
+bool uart_async_rx_data_consume(struct uart_async_rx *async_rx, size_t length);
 
 #ifdef __cplusplus
 }

subsys/shell/backends/shell_uart.c

@@ -454,26 +454,24 @@ static int async_read(struct shell_uart_async *sh_uart,
 
 	memcpy(data, buf, blen);
 #endif
-	uart_async_rx_data_consume(async_rx, sh_cnt);
+	bool buf_available = uart_async_rx_data_consume(async_rx, sh_cnt);
 	*cnt = sh_cnt;
 
-	if (sh_uart->pending_rx_req) {
+	if (sh_uart->pending_rx_req && buf_available) {
 		uint8_t *buf = uart_async_rx_buf_req(async_rx);
+		size_t len = uart_async_rx_get_buf_len(async_rx);
+		int err;
 
-		if (buf) {
-			int err;
-			size_t len = uart_async_rx_get_buf_len(async_rx);
-
-			atomic_dec(&sh_uart->pending_rx_req);
-			err = uart_rx_buf_rsp(sh_uart->common.dev, buf, len);
-			/* If it is too late and RX is disabled then re-enable it. */
-			if (err < 0) {
-				if (err == -EACCES) {
-					sh_uart->pending_rx_req = 0;
-					err = rx_enable(sh_uart->common.dev, buf, len);
-				} else {
-					return err;
-				}
+		__ASSERT_NO_MSG(buf != NULL);
+		atomic_dec(&sh_uart->pending_rx_req);
+		err = uart_rx_buf_rsp(sh_uart->common.dev, buf, len);
+		/* If it is too late and RX is disabled then re-enable it. */
+		if (err < 0) {
+			if (err == -EACCES) {
+				sh_uart->pending_rx_req = 0;
+				err = rx_enable(sh_uart->common.dev, buf, len);
+			} else {
+				return err;
 			}
 		}
 	}

tests/drivers/uart/uart_async_rx/src/main.c

@@ -41,6 +41,7 @@ ZTEST(uart_async_rx, test_rx)
 	uint8_t *claim_buf;
 	uint8_t *aloc_buf;
 	struct uart_async_rx async_rx;
+	bool buf_available;
 	const struct uart_async_rx_config config = {
 		.buffer = buf,
 		.length = sizeof(buf),
@@ -87,7 +88,8 @@ ZTEST(uart_async_rx, test_rx)
 	zassert_true(mem_check(claim_buf, 0, aloc_len - 2));
 
 	/* Consume first 2 bytes. */
-	uart_async_rx_data_consume(&async_rx, 2);
+	buf_available = uart_async_rx_data_consume(&async_rx, 2);
+	zassert_true(buf_available);
 
 	/* Now claim will return buffer taking into account that first 2 bytes are
 	 * consumed.
@@ -98,7 +100,8 @@ ZTEST(uart_async_rx, test_rx)
 	zassert_true(mem_check(claim_buf, 2, aloc_len - 4));
 
 	/* Consume rest of data. Get indication that it was end of the buffer. */
-	uart_async_rx_data_consume(&async_rx, aloc_len - 4);
+	buf_available = uart_async_rx_data_consume(&async_rx, aloc_len - 4);
+	zassert_true(buf_available);
 }
 
 ZTEST(uart_async_rx, test_rx_late_consume)
@@ -134,7 +137,7 @@ ZTEST(uart_async_rx, test_rx_late_consume)
 		zassert_equal(claim_len, 1);
 		zassert_equal(claim_buf[0], (uint8_t)i);
 
-		uart_async_rx_data_consume(&async_rx, 1);
+		(void)uart_async_rx_data_consume(&async_rx, 1);
 	}
 
 	claim_len = uart_async_rx_data_claim(&async_rx, &claim_buf, 100);
@@ -217,13 +220,13 @@ static bool consumer(void *user_data, uint32_t cnt, bool last, int prio)
 			test_data->exp_consume++;
 		}
 
-		uart_async_rx_data_consume(async_rx, len);
+		bool buf_released = uart_async_rx_data_consume(async_rx, len);
 
-		if (test_data->pending_req) {
+		if (buf_released && test_data->pending_req) {
 			buf = uart_async_rx_buf_req(async_rx);
-			if (buf) {
-				atomic_dec(&test_data->pending_req);
-			}
+			zassert_true(buf != NULL);
+
+			atomic_dec(&test_data->pending_req);
 			k_spinlock_key_t key = k_spin_lock(&test_data->lock);
 
 			if (test_data->curr_buf == NULL) {