diff --git a/drivers/serial/uart_async_rx.c b/drivers/serial/uart_async_rx.c index 68e17691c3ef..7c51a6cd2519 100644 --- a/drivers/serial/uart_async_rx.c +++ b/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; diff --git a/drivers/serial/uart_async_to_irq.c b/drivers/serial/uart_async_to_irq.c index 209e8d4f2053..46614a3eec93 100644 --- a/drivers/serial/uart_async_to_irq.c +++ b/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; } } } diff --git a/include/zephyr/drivers/serial/uart_async_rx.h b/include/zephyr/drivers/serial/uart_async_rx.h index 646a2befc946..4df46f36c51c 100644 --- a/include/zephyr/drivers/serial/uart_async_rx.h +++ b/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 } diff --git a/subsys/shell/backends/shell_uart.c b/subsys/shell/backends/shell_uart.c index e11eebc7850d..e80317664b97 100644 --- a/subsys/shell/backends/shell_uart.c +++ b/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; } } } diff --git a/tests/drivers/uart/uart_async_rx/src/main.c b/tests/drivers/uart/uart_async_rx/src/main.c index d95c6a690661..624c023bb632 100644 --- a/tests/drivers/uart/uart_async_rx/src/main.c +++ b/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) {