Skip to content

Rx fifo latency fix #4328

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 17 commits into from
Mar 8, 2018
Merged

Rx fifo latency fix #4328

Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2400e74
Add a new resetmethod_menu_all macro to give the choice of all the re…
mribble Jan 25, 2018
8fe2727
Change reset menu from all to extra. This gets rid of duplicated code
mribble Jan 26, 2018
666de32
Merge with upstream
mribble Feb 8, 2018
0cb9617
Fix boards.txt
mribble Feb 8, 2018
a4e1b91
Flush the rx fifo when checking available bytes in fifo. This gives …
mribble Feb 8, 2018
0e04ea3
Merge branch 'master' into rx-fifo-latency-fix
devyte Feb 8, 2018
42d4cb3
Merge branch 'master' of https://github.com/esp8266/Arduino
mribble Feb 8, 2018
f23c4b0
Merge branch 'master' of https://github.com/mribble/Arduino-1 into rx…
mribble Feb 8, 2018
aa16f80
Merge branch 'rx-fifo-latency-fix' of https://github.com/mribble/Ardu…
mribble Feb 8, 2018
d1d17d2
When rx_avaiable is checked return rx_buffer plus rx_fifo. Then duri…
mribble Feb 8, 2018
f4a62c0
Clean up early out case.
mribble Feb 9, 2018
7d4f7e0
Merge branch 'master' into rx-fifo-latency-fix
mribble Feb 14, 2018
0d3d43f
Merge branch 'master' of https://github.com/esp8266/Arduino into rx-f…
mribble Feb 14, 2018
001be65
Merge branch 'rx-fifo-latency-fix' of https://github.com/mribble/Ardu…
mribble Feb 14, 2018
e97a9fa
Merge branch 'master' of https://github.com/esp8266/Arduino into rx-f…
mribble Feb 14, 2018
c527a76
Set the rx full fifo ISR to trigger a little sooner. This makes the …
mribble Feb 14, 2018
ff7983f
Merge branch 'master' into rx-fifo-latency-fix
devyte Mar 8, 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
46 changes: 34 additions & 12 deletions cores/esp8266/uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,33 @@ size_t uart_resize_rx_buffer(uart_t* uart, size_t new_size)
return uart->rx_buffer->size;
}

inline size_t uart_rx_buffer_available(uart_t* uart) {
if(uart->rx_buffer->wpos < uart->rx_buffer->rpos) {
return (uart->rx_buffer->wpos + uart->rx_buffer->size) - uart->rx_buffer->rpos;
}
return uart->rx_buffer->wpos - uart->rx_buffer->rpos;
}

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

// Copy all the rx fifo bytes that fit into the rx buffer
inline void uart_rx_copy_fifo_to_buffer(uart_t* uart) {
while(uart_rx_fifo_available(uart)){
size_t nextPos = (uart->rx_buffer->wpos + 1) % uart->rx_buffer->size;
if(nextPos != uart->rx_buffer->rpos) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't use inverted logic. Instead, use direct logic to detect an early return or break, and leave the 3 lines of code free-standing after that.
E.g.:
if(nextPos == rpos)
return;
uint8_t data = ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the code would look like this:
'
inline void uart_rx_copy_fifo_to_buffer(uart_t* uart) {
while(uart_rx_fifo_available(uart)){
size_t nextPos = (uart->rx_buffer->wpos + 1) % uart->rx_buffer->size;
if(nextPos == uart->rx_buffer->rpos) {
// Stop copying if rx buffer is full
break;
}
uint8_t data = USF(uart->uart_nr);
uart->rx_buffer->buffer[uart->rx_buffer->wpos] = data;
uart->rx_buffer->wpos = nextPos;
}
}
'

I agree that code looks better, but I do have a concern. The line that initizes data with a value from the fifo also drains the fifo by 1 byte. So that line of code really can't be executed in the case we want to execute the break. I'm not sure if the compiler is allowed to rearrange that line to be before the if. If it can do that with any optimization level then we'd have a bug. So while it isn't quite as clean I'd suggest putting that code in an else (I can keep the current order though so the break is first). However, if you are sure this is legal then I can make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind. It's late and I was overthinking this. Of course a compile can't reorder this if it might affect results. I'll test this change with my test case and if it passes will push the change.

uint8_t data = USF(uart->uart_nr);
uart->rx_buffer->buffer[uart->rx_buffer->wpos] = data;
uart->rx_buffer->wpos = nextPos;
}
else {
// Stop copying if rx buffer is full
break;
}
}
}

int uart_peek_char(uart_t* uart)
{
if(uart == NULL || !uart->rx_enabled) {
Expand All @@ -99,6 +126,11 @@ int uart_peek_char(uart_t* uart)
if (!uart_rx_available(uart)) {
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];
}

Expand All @@ -119,10 +151,7 @@ size_t uart_rx_available(uart_t* uart)
if(uart == NULL || !uart->rx_enabled) {
return 0;
}
if(uart->rx_buffer->wpos < uart->rx_buffer->rpos) {
return (uart->rx_buffer->wpos + uart->rx_buffer->size) - uart->rx_buffer->rpos;
}
return uart->rx_buffer->wpos - uart->rx_buffer->rpos;
return uart_rx_buffer_available(uart) + uart_rx_fifo_available(uart);
}


Expand All @@ -135,14 +164,7 @@ void ICACHE_RAM_ATTR uart_isr(void * arg)
return;
}
if(USIS(uart->uart_nr) & ((1 << UIFF) | (1 << UITO))){
while((USS(uart->uart_nr) >> USRXC) & 0x7F){
uint8_t data = USF(uart->uart_nr);
size_t nextPos = (uart->rx_buffer->wpos + 1) % uart->rx_buffer->size;
if(nextPos != uart->rx_buffer->rpos) {
uart->rx_buffer->buffer[uart->rx_buffer->wpos] = data;
uart->rx_buffer->wpos = nextPos;
}
}
uart_rx_copy_fifo_to_buffer(uart);
}
USIC(uart->uart_nr) = USIS(uart->uart_nr);
}
Expand Down