Skip to content

Fix for #4565 (rx fifo length), protect access to rx_buffer #4568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Mar 28, 2018
Merged

Fix for #4565 (rx fifo length), protect access to rx_buffer #4568

Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9247bd0
Fix for #4565 (rx fifo length), protect access to rx_buffer
devyte Mar 26, 2018
7f0e690
Merge branch 'master' into uart_int
devyte Mar 26, 2018
982663d
Fix typo
devyte Mar 26, 2018
9f2af3f
reworked to separate safe from unsafe functions, factorized some code…
devyte Mar 27, 2018
ec358a9
additional rework for uart_rx_fifo_available()
devyte Mar 27, 2018
e40f5a8
Merge branch 'master' into uart_int
devyte Mar 27, 2018
0b6ea38
swapped unsafe function definition order
devyte Mar 27, 2018
ebe5f4d
Merge branch 'uart_int' of https://github.com/devyte/Arduino into uar…
devyte Mar 27, 2018
912536b
Merge branch 'master' into uart_int
devyte Mar 27, 2018
225e570
Remove static for overrun string
devyte Mar 27, 2018
c0470bb
Merge branch 'uart_int' of https://github.com/devyte/Arduino into uar…
devyte Mar 27, 2018
ae40588
Merge branch 'master' into uart_int
devyte Mar 27, 2018
c610910
Some shorthand for perf and readability
devyte Mar 28, 2018
6fa79ac
Merge branch 'uart_int' of https://github.com/devyte/Arduino into uar…
devyte Mar 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions cores/esp8266/uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ inline size_t uart_rx_buffer_available(uart_t* uart) {
}

inline size_t uart_rx_fifo_available(uart_t* uart) {
return (USS(uart->uart_nr) >> USRXC) & 0x7F;
return (USS(uart->uart_nr) >> USRXC) & 0xFF;
}

const char overrun_str [] ICACHE_RODATA_ATTR STORE_ATTR = "uart input full!\r\n";
Expand Down Expand Up @@ -140,15 +140,17 @@ int uart_peek_char(uart_t* uart)
if(uart == NULL || !uart->rx_enabled) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't comment right next to uart_rx_copy_fifo_to_buffer(). For more clarity, I guess it could be renamed to uart_rx_copy_fifo_to_buffer_unprotected() too.

return -1;
}
ETS_UART_INTR_DISABLE(); //access to rx_buffer can be interrupted by the isr (similar to a critical section), so disable interrupts here
if (!uart_rx_available(uart)) {
ETS_UART_INTR_ENABLE();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uart_rx_available() also conflictly calls INTR_DISABLE+ENABLE. Maybe adding a new static uart_rx_available_unprotected() would help (L82 too)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this needs rework.

return -1;
}
if (uart_rx_buffer_available(uart) == 0) {
ETS_UART_INTR_DISABLE();
uart_rx_copy_fifo_to_buffer(uart);
ETS_UART_INTR_ENABLE();
}
return uart->rx_buffer->buffer[uart->rx_buffer->rpos];
int ret = uart->rx_buffer->buffer[uart->rx_buffer->rpos];
ETS_UART_INTR_ENABLE();
return ret;
}

int uart_read_char(uart_t* uart)
Expand All @@ -158,7 +160,9 @@ int uart_read_char(uart_t* uart)
}
int data = uart_peek_char(uart);
if(data != -1) {
ETS_UART_INTR_DISABLE();
uart->rx_buffer->rpos = (uart->rx_buffer->rpos + 1) % uart->rx_buffer->size;
ETS_UART_INTR_ENABLE();
}
return data;
}
Expand All @@ -168,7 +172,11 @@ size_t uart_rx_available(uart_t* uart)
if(uart == NULL || !uart->rx_enabled) {
return 0;
}
return uart_rx_buffer_available(uart) + uart_rx_fifo_available(uart);
ETS_UART_INTR_DISABLE();
int uartrxbufferavailable = uart_rx_buffer_available(uart);
ETS_UART_INTR_ENABLE();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, for more clarity, what about renaming uart_rx_buffer_available to uart_rx_buffer_available_unprotected ?


return uartrxbufferavailable + uart_rx_fifo_available(uart);
}


Expand All @@ -186,7 +194,7 @@ void ICACHE_RAM_ATTR uart_isr(void * arg)
USIC(uart->uart_nr) = USIS(uart->uart_nr);
}

void uart_start_isr(uart_t* uart)
static void uart_start_isr(uart_t* uart)
{
if(uart == NULL || !uart->rx_enabled) {
return;
Expand All @@ -202,7 +210,7 @@ void uart_start_isr(uart_t* uart)
ETS_UART_INTR_ENABLE();
}

void uart_stop_isr(uart_t* uart)
static void uart_stop_isr(uart_t* uart)
{
if(uart == NULL || !uart->rx_enabled) {
return;
Expand Down Expand Up @@ -383,6 +391,8 @@ void uart_uninit(uart_t* uart)
return;
}

uart_stop_isr(uart);

switch(uart->rx_pin) {
case 3:
pinMode(3, INPUT);
Expand Down