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 6 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
10 changes: 10 additions & 0 deletions cores/esp8266/uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,16 @@ size_t uart_rx_available(uart_t* uart)
if(uart == NULL || !uart->rx_enabled) {
return 0;
}
ETS_UART_INTR_DISABLE();
while((USS(uart->uart_nr) >> USRXC) & 0x7F){
Copy link
Member

Choose a reason for hiding this comment

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

With this modification, bytes which do not fit into rx_buffer will be dropped when calling uart_rx_available, which is a bit unexpected. Effectively, this reduces the total FIFO size from the (hardware FIFO size) + (software FIFO size) to just (software FIFO size), if uart_rx_available is called.

Shouldn't uart_rx_available meerly be checking the number of bytes in hardware FIFO and adding it to the size of rx_buffer? Actual copying can happen in uart_read_char function, if the rx_buffer is empty.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@igrr you are right. However, I suspect that the objective of the hw buffer is to accumulate what is coming in before it's handed off upwards (i.e.: driven by the rate of incoming bytes), while the software buffer's objective is to accumulate before it's handed off to the application (driven by the app's rate of service).
The issue described is that, if the interrupt is being triggered only when the FIFO is full, then it is essentially behaving like the overflow interrupt. In other words, the interrupt is triggered only when the hw FIFO is completely full, and the next byte would drop data. The following has to happen to avoid dropping bytes at hardware level:

  • interrupt trigger
  • jump to isr
  • execute isr code to xfer data from the fifo

If it happens that the fifo is full and the interrupts are off for the moment because of something else, then the next incoming byte(s) can be dropped.

Per the document reference above, the trigger is when the number of bytes in the FIFO is greater than the configured threshold. I understand from that reading that changing that threshold doesn't change the size of the FIFO. In other words, if the threshold is set to something lower than the full FIFO size of 0x7F, it will trigger the interrupt more often, but also allow buffer space for bytes coming for the latency between interrupt trigger and isr execution.
I think that explains @mribble 's original test that seemed to work:

I started trying things. I found if I reduce UCFFT from 127 to 7 in this line of uart.c:
USC1(uart->uart_nr) = (127 << UCFFT) | (0x02 << UCTOT) | (1 <<UCTOE );

If that 127 is the threshold, then it is currently set to the full FIFO size.

A question: what happens when the number of bytes that come in is smaller than the threshold? It seems to me that the isr isn't called, so do those bytes just sit in the hw FIFO?

Copy link
Member

Choose a reason for hiding this comment

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

In other words, if the threshold is set to something lower than the full FIFO size of 0x7F, it will trigger the interrupt more often, but also allow buffer space for bytes coming for the latency between interrupt trigger and isr execution.

This is correct!

A question: what happens when the number of bytes that come in is smaller than the threshold?

There is a timeout feature — set by UCTOE and UCTOT fields. When FIFO is not empty, and no new bytes arrive in given amount of time, an interrupt fires.

Copy link
Contributor Author

@mribble mribble Feb 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@devyte - There is also a timeout check called UCTOT. Our code sets that to 2. I couldn't find any document that tells me the units for that 2, but I think 2 milliseconds make sense so maybe that's it. That pdf says the timeout threshold is measured just for inactivity so if my ms guess is correct that means if you don't write anything to the uart then after 2ms that timeout occurs and the rx fifo would be drained by the isr.

@igrr You make a good point about the buffer overflowing. However, I don't think just checking the bytes of data in fifo is correct. Then if you were actually trying to read those bytes you'd get the wrong results because you'd be trying to read bytes that haven't been moved into the software buffer yet.

OPTION_1
I think the simplest fix would be to copy over however many bytes from the fifo that can fit into the software buffer and then return however many bytes are in the software buffer. This means the largest number of available bytes that could be returned is 256.

Here's an example. Let's say you have 250 bytes in the software buffer and 50 bytes in the hw fifo. In the available check you'd move 6 bytes from the fifo (now has 44 bytes) and the software buffer would have 256 bytes which is what we'd report back to the user.

OPTION_2
Another option would be to return the total number of bytes in the fifo and sw buffer. However, then in read and peek functions you'd need to add code to get from the fifo if there is data there.

I think I like option 2 better. @igrr if you agree I can code up Option 2 and have you review it. Here is a summary of how I'd implement it:

  • Return the sum of hardware fifo and sw buffer bytes in uart_rx_available()
  • In both uart_peek_char() and uart_read_char() I'd add that loop to copy all the bytes from the hardware fifo to the software buffer. But we would only do this if the software buffer is empty, which would mean we'd never overflow the software buffer and would also minimize the number of times we go into that loop.

Copy link
Member

Choose a reason for hiding this comment

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

I also like the 2nd option. I think the copying loop can be moved to a separate inline function, and called from ISR and read/peek functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm inclined to agree. Here is my take on what is needed:

  • uart_rx_available should be modified to return the sum of available bytes in the sw buffer + the number of available bytes in the hw FIFO. No transfer of bytes from FIFO to sw should be done.
  • uart_peek_char() and uart_read_char() should be modified as follows: if there are bytes in the FIFO, transfer as many bytes as can fit into the sw buffer. After that, proceed same as now.
  • reduce 127 to about 107. My reasoning: if 2ms time is taken as a guideline, which is a huge estimation for upper bound of interrupt latency, then about 20 bytes can come in at 115kbps considering 11bits/byte. So reducing the threshold by 20 bytes should give ample time to service the FIFO, and still not overflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks guys. I'll work on a change an upload it soon.

@devyte, we don't need to reduce the fifo size because the interrupt happens either when the fifo is full or when there is that timeout. So that code should work as it is now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mribble this is the sequence that I think is problematic:

  1. the FIFO gets full
  2. an interrupt is triggered
  3. there can be latency between the trigger and the calling of the uart isr (e.g.: something else is going on that has interrupts disabled)
  4. during this latency, more bytes come in, but the FIFO is full, so data is dropped
  5. the isr executes and bytes are transferred from the FIFO. Too late, data has already been dropped.

Per my understanding of the document that you referenced, reducing the number from 127 to 107 doesn't reduce the FIFO size. It only changes when the isr is called. In other words, the isr is called when the FIFO has 107 bytes instead of 127 bytes. With this reduction, the problem sequence becomes this:

  1. the FIFO fills to 107 bytes
  2. an interrupt is triggered
  3. there can be latency between the trigger and the calling of the uart isr (e.g.: something else is going on that has interrupts disabled)
  4. during this latency, more bytes come in, but the FIFO still has 20 bytes of space, so that's ok
  5. the isr executes and bytes are transferred from the FIFO. No data was dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@devyte - That's true of any of the stream hardware protocols (spi, i2c, uart) so you might want to consider how all those work too. In other systems I've worked with, you require IRS to both be short enough and infrequent enough that they don't cause the problem you described.

That said what you are saying seems like it would make things more robust in some case. I don't contribute much to this project though so I won't try to decide how this should be handled. @igrr is very involved so maybe he'll have an opinion. That said, this is a different feature than what I'm working on here so if you do that I think it should be done with a seperate push request.

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;
}
}
ETS_UART_INTR_ENABLE();
if(uart->rx_buffer->wpos < uart->rx_buffer->rpos) {
return (uart->rx_buffer->wpos + uart->rx_buffer->size) - uart->rx_buffer->rpos;
}
Expand Down