HardwareSerial hangs in flush() with flow control enabled and no device attached

[fronders]

Describe the bug
I'm using Sparkfun's SARA-R5 library - an LTE modem using UART connection.
The init sequence includes autobaud selection - it cycles through different UART baudrates and reinitiailizes the UART like this (original code here):

void SARA_R5::beginSerial(unsigned long baud)
{
  delay(100);
  if (_hardSerial != nullptr)
  {
    _hardSerial->end();
    _hardSerial->begin(baud);
  }
...
  delay(100);
}

Then it sends ATE0 and waits for OK response on selected baudrate - classic stuff.

The problem comes up when HardwareSerial has hw flow control enabled and there's no device attached (i.e. CTS not asserted). The end() function gets stuck forever because of flush() waiting for the ringbuffer to become empty (line 499).

Arduino_Core_STM32/cores/arduino/HardwareSerial.cpp

Lines 490 to 504 in 0c69f37

	void HardwareSerial::flush()
	{
	// If we have never written a byte, no need to flush. This special
	// case is needed since there is no way to force the TXC (transmit
	// complete) bit to 1 during initialization
	if (!_written) {
	return;
	}
	while ((_serial.tx_head != _serial.tx_tail)) {
	// nop, the interrupt handler will free up space for us
	}
	// If we get here, nothing is queued anymore (DRIE is disabled) and
	// the hardware finished transmission (TXC is set).
	}

But because hw flow control enabled and CTS not asserted (i.e. SARA-R5 not connected) the bytes pending in the ringbuffer never get transmitted (remember it tries to send ATE0) so the whole while loop just hangs forever.

I've made a "dirty" fix that removes the problem in this specific case - if CTS is enabled and no CTS asserted (UART CTS flag is not set) then simply reset ringbuffer:

void HardwareSerial::flush()
{
  // If we have never written a byte, no need to flush. This special
  // case is needed since there is no way to force the TXC (transmit
  // complete) bit to 1 during initialization
  if (!_written) {
    return;
  }

  // If we have CTS flow control enabled and CTS is not asserted, no need to flush - just reset tx buffer.
  if (_serial.pin_cts != NC && !__HAL_UART_GET_FLAG(&_serial.handle, UART_FLAG_CTS)) {
    _serial.tx_head = 0;
    _serial.tx_tail = 0;
    return;
  }

  while ((_serial.tx_head != _serial.tx_tail)) {
    // nop, the interrupt handler will free up space for us
  }
  // If we get here, nothing is queued anymore (DRIE is disabled) and
  // the hardware finished transmission (TXC is set).
}

But this generally opens a broader issue some of HardwareSerial code has infinite loops relying purely on interrupts and no timeout exit: flush() and write(). Kinda similar to what #1834 brings up

Desktop (please complete the following information):

• OS: Windows 10 x64
• Arduino IDE version: 2.2.1
• STM32 core version: 2.6.0
• Tools menu settings if not the default: -g3, -Os, newlib nano + printf, USB CDC (supersede U(S)ART). USART Enabled (generic 'Serial), own board variant
• Upload method: SWD

Board (please complete the following information):
Custom PCB using STM32L496RGT6 MCU connected via UART with CTS\RTS to a u-blox SARA-R5 modem

[fpistm]

Hi @fronders

Thanks for this detailed issue.

Hard to define in which case we do not need to flush. For me if CTS is not asserted this simply means the device is not ready to receive.

  // If we have CTS flow control enabled and CTS is not asserted, no need to flush - just reset tx buffer.
  if (_serial.pin_cts != NC && !__HAL_UART_GET_FLAG(&_serial.handle, UART_FLAG_CTS)) {
    _serial.tx_head = 0;
    _serial.tx_tail = 0;
    return;
  }

This would need to add a default timeout when the end() called flush(). In that case, no more break.
Would it be acceptable for you?

[fronders]

Hi @fpistm

Yeah, I agree this was a "quick hack" - we're losing data in case device wasn't ready to receive, but in this edge-case we were de-initializing UART by calling end() anyway. I think just having a timeout for end() call to flush() regardless of CTS state should be sufficient

[fpistm]

Hi @fronders
I've made a PR to avoid this behavior.
Let me know if it is ok for you.

[fronders]

@fpistm Looks good, but I'll be able to test only late tonight. I'll report back once I have it

UPD: do you think we also might need smth similar here? :)

Arduino_Core_STM32/cores/arduino/HardwareSerial.cpp

Lines 521 to 525 in 0c69f37

	// If the output buffer is full, there's nothing for it other than to
	// wait for the interrupt handler to free space
	while (!availableForWrite()) {
	// nop, the interrupt handler will free up space for us
	}

[fpistm]

UPD: do you think we also might need smth similar here? :)

Arduino_Core_STM32/cores/arduino/HardwareSerial.cpp

Lines 521 to 525 in 0c69f37

	// If the output buffer is full, there's nothing for it other than to
	// wait for the interrupt handler to free space
	while (!availableForWrite()) {
	// nop, the interrupt handler will free up space for us
	}

I don't think, we follow the Arduino API reference:
https://www.arduino.cc/reference/en/language/functions/communication/serial/write/

As stated at the end:

Notes and Warnings
As of Arduino IDE 1.0, serial transmission is asynchronous. If there is enough empty space in the transmit buffer, Serial.write() will return before any characters are transmitted over serial. If the transmit buffer is full then Serial.write() will block until there is enough space in the buffer. To avoid blocking calls to Serial.write(), you can first check the amount of free space in the transmit buffer using availableForWrite().

[fronders]

@fpistm yeap that PR fix works a charm!

yes, I agree, but shouldn't there be a reasonable time we give ringbuffer to get out?
Example: in same case of hw flow control enabled and no device attached that write() would hang forever if I try to send smth larger than the ringbuffer size (64 bytes) or do a sequence of write() calls exceeding 64 bytes total

[fpistm]

No. Up to you to check if you can send as stated in the api.
You can also increase the buffer size.