Skip to content

Commit 708e3b6

Browse files
o-marshmallowo-marshmallow
committed
UART: RX interrupts are now properly restored after a flush
Added a unit test to make sure the expected behavior happens
1 parent 2e06f1f commit 708e3b6

File tree

2 files changed

+113
-14
lines changed

2 files changed

+113
-14
lines changed

‎components/driver/test/test_uart.c

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -359,3 +359,76 @@ TEST_CASE("uart tx with ringbuffer test", "[uart]")
359359
free(rd_data);
360360
free(wr_data);
361361
}
362+
363+
TEST_CASE("uart int state restored after flush", "[uart]")
364+
{
365+
/**
366+
* The first goal of this test is to make sure that when our RX FIFO is full,
367+
* we can continue receiving back data after flushing
368+
* For more details, check IDF-4374
369+
*/
370+
uart_config_t uart_config = {
371+
.baud_rate = 115200,
372+
.data_bits = UART_DATA_8_BITS,
373+
.parity = UART_PARITY_DISABLE,
374+
.stop_bits = UART_STOP_BITS_1,
375+
.flow_ctrl = UART_HW_FLOWCTRL_DISABLE,
376+
.source_clk = UART_SCLK_APB,
377+
};
378+
379+
const uart_port_t uart_echo = UART_NUM_1;
380+
const int uart_tx_signal = U1TXD_OUT_IDX;
381+
const int uart_tx = 4;
382+
const int uart_rx = 5;
383+
const int buf_size = 256;
384+
const int intr_alloc_flags = 0;
385+
386+
TEST_ESP_OK(uart_driver_install(uart_echo, buf_size * 2, 0, 0, NULL, intr_alloc_flags));
387+
TEST_ESP_OK(uart_param_config(uart_echo, &uart_config));
388+
TEST_ESP_OK(uart_set_pin(uart_echo, uart_tx, uart_rx, UART_PIN_NO_CHANGE, UART_PIN_NO_CHANGE));
389+
390+
/* Make sure UART2's RX signal is connected to TX pin
391+
* This creates a loop that lets us receive anything we send on the UART */
392+
esp_rom_gpio_connect_out_signal(uart_rx, uart_tx_signal, false, false);
393+
394+
uint8_t *data = (uint8_t *) malloc(buf_size);
395+
TEST_ASSERT_NOT_NULL(data);
396+
uart_write_bytes(uart_echo, (const char *) data, buf_size);
397+
398+
/* As we set up a loopback, we can read them back on RX */
399+
int len = uart_read_bytes(uart_echo, data, buf_size, 1000 / portTICK_RATE_MS);
400+
TEST_ASSERT_EQUAL(len, buf_size);
401+
402+
/* Fill the RX buffer, this should disable the RX interrupts */
403+
int written = uart_write_bytes(uart_echo, (const char *) data, buf_size);
404+
TEST_ASSERT_NOT_EQUAL(-1, written);
405+
written = uart_write_bytes(uart_echo, (const char *) data, buf_size);
406+
TEST_ASSERT_NOT_EQUAL(-1, written);
407+
written = uart_write_bytes(uart_echo, (const char *) data, buf_size);
408+
TEST_ASSERT_NOT_EQUAL(-1, written);
409+
410+
/* Flush the input buffer, RX interrupts should be re-enabled */
411+
uart_flush_input(uart_echo);
412+
written = uart_write_bytes(uart_echo, (const char *) data, buf_size);
413+
TEST_ASSERT_NOT_EQUAL(-1, written);
414+
len = uart_read_bytes(uart_echo, data, buf_size, 1000 / portTICK_RATE_MS);
415+
/* len equals buf_size bytes if interrupts were indeed re-enabled */
416+
TEST_ASSERT_EQUAL(len, buf_size);
417+
418+
/**
419+
* Second test, make sure that if we explicitly disable the RX interrupts,
420+
* they are NOT re-enabled after flushing
421+
* To do so, start by cleaning the RX FIFO, disable the RX interrupts,
422+
* flush again, send data to the UART and check that we haven't received
423+
* any of the bytes */
424+
uart_flush_input(uart_echo);
425+
uart_disable_rx_intr(uart_echo);
426+
uart_flush_input(uart_echo);
427+
written = uart_write_bytes(uart_echo, (const char *) data, buf_size);
428+
TEST_ASSERT_NOT_EQUAL(-1, written);
429+
len = uart_read_bytes(uart_echo, data, buf_size, 250 / portTICK_RATE_MS);
430+
TEST_ASSERT_EQUAL(len, 0);
431+
432+
TEST_ESP_OK(uart_driver_delete(uart_echo));
433+
free(data);
434+
}

‎components/driver/uart.c

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ typedef struct {
103103
uint8_t *rx_head_ptr; /*!< pointer to the head of RX item*/
104104
uint8_t rx_data_buf[SOC_UART_FIFO_LEN]; /*!< Data buffer to stash FIFO data*/
105105
uint8_t rx_stash_len; /*!< stashed data length.(When using flow control, after reading out FIFO data, if we fail to push to buffer, we can just stash them.) */
106+
uint32_t rx_int_usr_mask; /*!< RX interrupt status. Valid at any time, regardless of RX buffer status. */
106107
uart_pat_rb_t rx_pattern_pos;
107108
int tx_buf_size; /*!< TX ring buffer size */
108109
bool tx_waiting_fifo; /*!< this flag indicates that some task is waiting for FIFO empty interrupt, used to send all data without any data buffer*/
@@ -346,16 +347,45 @@ esp_err_t uart_enable_intr_mask(uart_port_t uart_num, uint32_t enable_mask)
346347
{
347348
ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error");
348349
UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock));
350+
/* Keep track of the interrupt toggling. In fact, without such variable,
351+
* once the RX buffer is full and the RX interrupts disabled, it is
352+
* impossible what was the previous state (enabled/disabled) of these
353+
* interrupt masks. Thus, this will be very particularly handy when
354+
* emptying a filled RX buffer. */
355+
p_uart_obj[uart_num]->rx_int_usr_mask |= enable_mask;
349356
uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), enable_mask);
350357
uart_hal_ena_intr_mask(&(uart_context[uart_num].hal), enable_mask);
351358
UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock));
352359
return ESP_OK;
353360
}
354361

362+
/**
363+
* @brief Function re-enabling the given interrupts (mask) if and only if
364+
* they have not been disabled by the user.
365+
*
366+
* @param uart_num UART number to perform the operation on
367+
* @param enable_mask Interrupts (flags) to be re-enabled
368+
*
369+
* @return ESP_OK in success, ESP_FAIL if uart_num is incorrect
370+
*/
371+
static esp_err_t uart_reenable_intr_mask(uart_port_t uart_num, uint32_t enable_mask)
372+
{
373+
ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error");
374+
UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock));
375+
/* Mask will only contain the interrupt flags that needs to be re-enabled
376+
* AND which have NOT been explicitly disabled by the user. */
377+
uint32_t mask = p_uart_obj[uart_num]->rx_int_usr_mask & enable_mask;
378+
uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), mask);
379+
uart_hal_ena_intr_mask(&(uart_context[uart_num].hal), mask);
380+
UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock));
381+
return ESP_OK;
382+
}
383+
355384
esp_err_t uart_disable_intr_mask(uart_port_t uart_num, uint32_t disable_mask)
356385
{
357386
ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error");
358387
UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock));
388+
p_uart_obj[uart_num]->rx_int_usr_mask &= ~disable_mask;
359389
uart_hal_disable_intr_mask(&(uart_context[uart_num].hal), disable_mask);
360390
UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock));
361391
return ESP_OK;
@@ -1218,7 +1248,9 @@ static bool uart_check_buf_full(uart_port_t uart_num)
12181248
p_uart_obj[uart_num]->rx_buffered_len += p_uart_obj[uart_num]->rx_stash_len;
12191249
p_uart_obj[uart_num]->rx_buffer_full_flg = false;
12201250
UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock));
1221-
uart_enable_rx_intr(p_uart_obj[uart_num]->uart_num);
1251+
/* Only re-activate UART_INTR_RXFIFO_TOUT or UART_INTR_RXFIFO_FULL
1252+
* interrupts if they were NOT explicitly disabled by the user. */
1253+
uart_reenable_intr_mask(p_uart_obj[uart_num]->uart_num, UART_INTR_RXFIFO_TOUT | UART_INTR_RXFIFO_FULL);
12221254
return true;
12231255
}
12241256
}
@@ -1296,28 +1328,19 @@ esp_err_t uart_get_buffered_data_len(uart_port_t uart_num, size_t *size)
12961328

12971329
esp_err_t uart_flush(uart_port_t uart_num) __attribute__((alias("uart_flush_input")));
12981330

1299-
static esp_err_t uart_disable_intr_mask_and_return_prev(uart_port_t uart_num, uint32_t disable_mask, uint32_t *prev_mask)
1300-
{
1301-
ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error");
1302-
UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock));
1303-
*prev_mask = uart_hal_get_intr_ena_status(&uart_context[uart_num].hal) & disable_mask;
1304-
uart_hal_disable_intr_mask(&(uart_context[uart_num].hal), disable_mask);
1305-
UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock));
1306-
return ESP_OK;
1307-
}
1308-
13091331
esp_err_t uart_flush_input(uart_port_t uart_num)
13101332
{
13111333
ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error");
13121334
ESP_RETURN_ON_FALSE((p_uart_obj[uart_num]), ESP_FAIL, UART_TAG, "uart driver error");
13131335
uart_obj_t *p_uart = p_uart_obj[uart_num];
13141336
uint8_t *data;
13151337
size_t size;
1316-
uint32_t prev_mask;
13171338

13181339
//rx sem protect the ring buffer read related functions
13191340
xSemaphoreTake(p_uart->rx_mux, (portTickType)portMAX_DELAY);
1320-
uart_disable_intr_mask_and_return_prev(uart_num, UART_INTR_RXFIFO_FULL | UART_INTR_RXFIFO_TOUT, &prev_mask);
1341+
UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock));
1342+
uart_hal_disable_intr_mask(&(uart_context[uart_num].hal), UART_INTR_RXFIFO_FULL | UART_INTR_RXFIFO_TOUT);
1343+
UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock));
13211344
while (true) {
13221345
if (p_uart->rx_head_ptr) {
13231346
vRingbufferReturnItem(p_uart->rx_ring_buf, p_uart->rx_head_ptr);
@@ -1365,7 +1388,9 @@ esp_err_t uart_flush_input(uart_port_t uart_num)
13651388
p_uart->rx_cur_remain = 0;
13661389
p_uart->rx_head_ptr = NULL;
13671390
uart_hal_rxfifo_rst(&(uart_context[uart_num].hal));
1368-
uart_enable_intr_mask(uart_num, prev_mask);
1391+
/* Only re-enable UART_INTR_RXFIFO_TOUT or UART_INTR_RXFIFO_FULL if they
1392+
* were explicitly enabled by the user. */
1393+
uart_reenable_intr_mask(uart_num, UART_INTR_RXFIFO_TOUT | UART_INTR_RXFIFO_FULL);
13691394
xSemaphoreGive(p_uart->rx_mux);
13701395
return ESP_OK;
13711396
}
@@ -1544,6 +1569,7 @@ esp_err_t uart_driver_install(uart_port_t uart_num, int rx_buffer_size, int tx_b
15441569
p_uart_obj[uart_num]->tx_waiting_fifo = false;
15451570
p_uart_obj[uart_num]->rx_ptr = NULL;
15461571
p_uart_obj[uart_num]->rx_cur_remain = 0;
1572+
p_uart_obj[uart_num]->rx_int_usr_mask = UART_INTR_RXFIFO_FULL | UART_INTR_RXFIFO_TOUT;
15471573
p_uart_obj[uart_num]->rx_head_ptr = NULL;
15481574
p_uart_obj[uart_num]->tx_buf_size = tx_buffer_size;
15491575
p_uart_obj[uart_num]->uart_select_notif_callback = NULL;

0 commit comments

Comments
 (0)