Add flushReceiveBuffer() to HardwareSerial

[RobTillaart]

In Serial communication sometimes one wants to flush all data in the receive buffer at once. E.g. because data has become invalid. - https://forum.arduino.cc/index.php?topic=530650 -

The code to do this very fast in HardwareSerial is straightforward,

void HardwareSerial::flushReceiveBuffer()
{
  _rx_buffer_head = _rx_buffer_tail;
}

[PaulStoffregen]

Processing has this function, named "clear".

https://processing.org/reference/libraries/serial/Serial_clear_.html

Dunno if following Processing's conventions is still a thing?

[RobTillaart]

Good point, question
The Processing Serial.clear(), does that clear the RX buffer or also TX buffer? (no P expert)

If we follow the P convention I propose to add the buffer name so clearReceiveBuffer() to make it explicit what is done.

[RobTillaart]

(1) makes sense, however nowhere in the current HardwareSerial (AVR) I see a guard like you mention.

(2) would this be solved by assigning the tail iso the head? a new incoming char would move head 1 position, so at most one char is in the buffer now

{
_rx_buffer_tail = _rx_buffer_head;  
}

I will check the discussion (later)

[johnholman]

Rob - my earlier message was about flushing the transmit buffer, but later realised that function is already there and you are talking about flushing the receive buffer. So I deleted the message - sorry about the confusion.

A guard might be needed here to prevent the rx_complete_irq function moving the head pointer after read of the tail pointer but before write of the head pointer in your function. I'm not sure whether this would actually cause a problem, but it does need consideration.

If so, something like
ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
_rx_buffer_head = _rx_buffer_tail;
}
would be safer.

You are right that there are no such guards presently, but there are in the upcoming version in the separate AVR core repository (#6855) to fix a couple of race conditions in the current code.

[RobTillaart]

Thanks John, (read the discussion)

if we do not use ATOMIC and use

_rx_buffer_head = _rx_buffer_tail;

Three cases - assuming single byte indices

• if the rx_complete_irq comes before the read of tail -> no problem
• if the rx_complete_irq comes after the read of tail and before write of head, -> head will be overwritten and the new char will effectively be "flushed" too
• if the rx_complete_irq comes after the write of head, -> there is just a new char after the flush (happens all the time.

in case of multi byte indices ATOMIC is needed (no doubt about that)

ATOMIC could be added conditionally depending on #if (SERIAL_RX_BUFFER_SIZE>256). but as I don't know how much clock cycles are added by the ATOMIC that might be overkill. Also from maintenance point of view.

So the state of the proposal is now:

void HardwareSerial::clearReceiveBuffer()
{
  ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
    _rx_buffer_head = _rx_buffer_tail;
  }
}

[johnholman]

Rob - your analysis suggests to me that the race condition is harmless in the default case of 256 byte buffers, so protection isn't needed here.

To support larger buffers the function could be

void HardwareSerial::clearReceiveBuffer()
{
TX_BUFFER_ATOMIC {
_rx_buffer_head = _rx_buffer_tail;
}
}

The TX_BUFFER_ATOMIC macro (defined in the new version of HardwareSerial.cpp) adds a guard
only for buffers > 256 bytes, so adds no overhead to code space or execution time in the default case and I think keeps the code more readable than an explicit #if each time a conditional guard is needed.

Note that there are other unfixed race conditions that can still cause problems for large buffers, but it could be argued that new code at least shouldn't add new ones.

[johnholman]

Drat - I did it again, confusing transmit and receive buffers. But an RX_BUFFER_ATOMIC macro
similar to the TX_BUFFER_ATOMIC macro that applies a guard only when the receive buffer is greater than 256 bytes would be suitable. That macro doesn't exist yet though, but could also be useful if and when the other large buffer problems are fixed.

So now I'd suggest

// macro to guard critical sections when needed for large RX buffer sizes
#if (SERIAL_RX_BUFFER_SIZE>256)
#define RX_BUFFER_ATOMIC ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
#else
#define RX_BUFFER_ATOMIC
#endif

void HardwareSerial::clearReceiveBuffer()
{
RX_BUFFER_ATOMIC {
_rx_buffer_head = _rx_buffer_tail;
}
}

[RobTillaart]

There are other places in HWSerial where RX_BUFFER_ATOMIC should be used too.
I recall from my head flush() as that does also assign to _rx_buffer_tail / head .

[johnholman]

There are indeed. Those are addressed in PR arduino/Arduino#5 in the AVR core repository (which also defines the RX_BUFFER_ATOMIC macro suggested above). That PR hasn't received much interest - perhaps because there would still be race problems for the transmit case that it doesn't address and because large buffers is a corner case that is not fully supported (e.g. you have to edit HardwareSerial.h to implement them).

[RobTillaart]

All bugs are corner case :)

[RobTillaart]

Open for too long => not relevant anymore for me

[OParczyk]

It recently became relevant to me though.
I've got a line that suffers from rare bursts of noise, but I can predict very well, when actual data will arrive. While data is being sent, the noise is no issue.
I'd like to be able to flush the receive buffer before then, to minimize chances I'll interpret previously received noise as data.

Currently I'm emptying the buffer beforehand by reading until there's nothing available anymore, but that takes a considerable amount of time.

[RobTillaart]

@OParczyk et al?
Should it be reopened?

[OParczyk]

I, for one, would prefer it to be.
Thanks for taking a look again!