Serial.flush() blocks forever

[jpmeijers]

Using the latest version of the SAMD core, doing a Serial.flush() will hang forever.

Problem can be reproduced using this sketch:

void setup() {
  SerialUSB.begin(115200);
  delay(2000);
  SerialUSB.println("Start");

  Serial.begin(9600);
  Serial.flush();

  SerialUSB.println("Setup end");
}

void loop() {
  SerialUSB.println("Loop");
  delay(1000);
}

Which will only print:

15:19:30.572 -> Start

Tracing the issue down takes me to Serial.flush(), which is defined in Uart.cpp. This calls sercom->flushUART(); in SERCOM.cpp.

The previous versions of this core had the following code:

void SERCOM::flushUART()
{
  // Skip checking transmission completion if data register is empty
  if(isDataRegisterEmptyUART())
    return;

  // Wait for transmission to complete
  while(!sercom->USART.INTFLAG.bit.TXC);
}

The current version has the if(isDataRegisterEmptyUART()) commented out:

ArduinoCore-samd/cores/arduino/SERCOM.cpp

Lines 115 to 116 in 0b60a79

	// if(isDataRegisterEmptyUART())
	// return;

I don't understand why this is commented out, as clearly the while(!sercom->USART.INTFLAG.bit.TXC); will never return if flush() is called when the TX buffer is already empty.

[facchinm]

See arduino-libraries/MKRWAN@0493918 . This PR #541 is the root cause, and it's not been analyzed well enough before merging. Sorry for that. Can we find a workaround to fix both the issues? @aentinger @cmaglie

[aentinger]

Revert #541? (Sounds cheap but as I see it clearly a bug was introduced).

[matthijskooijman]

I haven't looked closely, but I remember that on AVR, there was the problem that TXC is only set after all pending data was sent and cleared when new data was queued. This left the problem that on startup, when no data was written yet, no data was pending, but TXC was not set either, so just waiting for TXC in flush would wait indefinitely. From the SAMD datasheet, the TXC bit seems to behave the same here. I haven't looked close enough if this explains the problems in this issue, but I suspect it might.

On AVR, IIRC we solved this by simply keeping a variable that says whether or not data has been written at all and use that to bypass TXC if needed, I think the same approach would be valid here too.

[ghost]

I've made some changes that seems to have fixed this issue, but I'd appreciate if others could test. You can checkout the changes in the latest commit from my old PR #535 a744583 . It was a mistake to to push to that branch - sorry for the mess, but anyway the specific changes relating to this issue are clearly visible.

[coffeemachine]

@facchinm this bug is critical in my view. In my case it causes RS485 library to hang (sendBreak calls flush). Like @aentinger I'd suggest to revert PR #541 and make a release based on 1.8.11 with just this fixed. I'd address the solution (@acicuc) in a separate release. What do you think?

[ghost]

@coffeemachine was @matthijskooijman suggestion that I implemented in a744583 helpful in your case?

[facchinm]

@acicuc I pushed your patch (rebased) on #652 . Tested reflashing an ESP32 up to 1.5Mbaud, with flush() added here and there.

[WanaGo]

Was the fix (#652) for this rolled out for the Arduino Due (SAM) board library, or was it specific only to the SAMD ?
As the same issue happens on the Arduino Due.

[ghost]

@WanaGo, currently it doesn't look like it, but you may be able to implement a similar approach in the SAM core?