Fixed bug with flushing

[chromhelm]

I tried to make a RS485 bridge and i use Serial.flush() to wait for the serial transmission is complete.
but it turned out that this never happened.
I used this sketch.
You need a oscilloscope, to measure pin 2 to verify it.

void setup()
{
Serial.begin(38400);
Serial1.begin(38400);
pinMode(2, OUTPUT);
}

void loop()
{
if(Serial.available())
{
digitalWrite(2, HIGH);
Serial1.write(Serial.read());
Serial1.flush();
digitalWrite(2, LOW);
}

if(Serial1.available())Serial.write(Serial1.read());
}

[facchinm]

Hi @chromhelm , I took a look at your sketch;
it seems that, shorting PIN 0 and 1 (Serial1 on Leonardo), the flush() works as expected and the text written in serial monitor is echoed back. Watching PIN 2 with a scope shows the correct behaviour.

Also, the _written variable you are setting to true inside flush() is only false between begin() and the first write(), so when your sketch executes the flush() the variable is already true.

Am I missing something?

[chromhelm]

Hi facchinm

I forgot to say, that this fails only in core version 1.6, in core version 1.0 it works fine

[facchinm]

I can't reproduce the issue but the fix is indeed correct.
The bug was introduced with commit 9ad14b2.
Thank you very much!

[matthijskooijman]

@chromhelm, good catch, thanks!

Might be even better to just change _written at the top of write() instead of in two places as now? Might save a few bytes of flash space.

[chromhelm]

Yes, i made a new pull request with your suggestion. @matthijskooijman
Saving same bytes in HardwareSerial::write #3254

[NicoHood]

This crashes my Arduino.Using a custom 1.6.6 build of a few days ago.

const int ledPin =  13;
bool ledState = LOW;
unsigned long previousMillis = 0;

void setup() {
  pinMode(ledPin, OUTPUT);
  Serial.begin(2000000);
}

void loop() {
  unsigned long currentMillis = millis();

  if (currentMillis - previousMillis >= 1000) {
    ledState = !ledState;

// 50 also works but crashes slower
    for (int i = 0; i < 250; i++) {
      Serial.write('a');
      Serial.flush();
      delay(6);
    }

    previousMillis = millis();
    digitalWrite(ledPin, ledState);
  }
}

[NicoHood]

A different (quick) test so see that the flush() function causes this problem:

// arduino uno, 64 byte serial tx rx buffers, default

const int dledPin =  13;
const int ledPin =  12;
bool ledState = LOW;
unsigned long previousMillis = 0;

void setup() {
  pinMode(ledPin, OUTPUT);
  pinMode(dledPin, OUTPUT);
  digitalWrite(dledPin, 0);
  Serial.begin(2000000);
}

void loop() {
  unsigned long currentMillis = millis();

  if ((currentMillis - previousMillis) >= 1000) {

    ledState = !ledState;

    digitalWrite(dledPin, 1);
    for (int i = 0; i < 250; i++) {

      Serial.write('a');
      digitalWrite(dledPin, 0);
      Serial.flush();
      digitalWrite(dledPin, 1);
      delay(6);
    }
    digitalWrite(dledPin, 1);

    previousMillis = millis();

    digitalWrite(ledPin, ledState);

  }
}

monitor with a 2nd arduino:

void setup() {
  // put your setup code here, to run once:
  Serial.begin(0);
  Serial1.begin(2000000);
}

void loop() {
  // put your main code here, to run repeatedly:
  if (Serial1.available()) {
    Serial1.read();
    static int c = 0;
    c++;
    Serial.println(c);
  }
}
#if SERIAL_RX_BUFFER_SIZE <200
#error
#endif

this count up to 73 for me. Could be because the receiver misses some bytes. I gave the receiver 256 bytes RX buffer so it is capable of all bytes. Its seems that the 73 is near the sending 64 bytes TX buffer. Edit: also tested with 16and 156 TX size, is always 73.

I used an arduino uno as sender and the 16u2 (with hoodloader) as receiver. You can use any other external serial device/arduino for this as well.

This while loop must cause the crash:
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp#L201-L208

Also the if misses brackets, it would be better to add them.

[NicoHood]

I know the next sketch is not that clear, but it show that the flush function stays in the while loop.

const int dledPin =  12;
const int ledPin =  13;
bool ledState = LOW;
unsigned long previousMillis = 0;

void setup() {
  pinMode(ledPin, OUTPUT);
  pinMode(dledPin, OUTPUT);
  pinMode(11, OUTPUT);
  digitalWrite(dledPin, 0);
  digitalWrite(ledPin, 0);
  digitalWrite(11, 0);
  delay(100);
  digitalWrite(dledPin, 1); //ch 5
  delay(100);
  digitalWrite(ledPin, 1); // ch4
  delay(100);
  digitalWrite(11, 1); //ch6
  delay(1000);
  digitalWrite(dledPin, 0);
  digitalWrite(ledPin, 0);
  digitalWrite(11, 0);

  Serial.begin(2000000);
  Serial.write('b');
}

void loop() {
  unsigned long currentMillis = millis();

  if ((currentMillis - previousMillis) >= 1000) {

    ledState = !ledState;

    digitalWrite(dledPin, 1);
    for (int i = 0; i < 250; i++) {

      Serial.write('a');
      PORTB |= (1 << 4);
      Serial.flush();
      PORTB &= ~(1 << 4);
      delay(6);
    }

    previousMillis = millis();

    digitalWrite(ledPin, ledState);

  }
}

Edit in hwserial.cpp:

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 (bit_is_set(*_ucsrb, UDRIE0) || bit_is_clear(*_ucsra, TXC0)) {
  PORTB |= (1<<3);
    if (bit_is_clear(SREG, SREG_I) && bit_is_set(*_ucsrb, UDRIE0))
    // Interrupts are globally disabled, but the DR empty
    // interrupt should be enabled, so poll the DR empty flag to
    // prevent deadlock
    if (bit_is_set(*_ucsra, UDRE0))
      _tx_udr_empty_irq();

      PORTB &= ~(1<<3);
  }
  // If we get here, nothing is queued anymore (DRIE is disabled) and
  // the hardware finished tranmission (TXC is set).
}

You can see that channel 5 keeps high (loop stuck in flush() function). After that Channel 6 keeps switching the state (while inside flush).

So there must be something wrong with the while loop inside flush().

Maybe this has something to do with this?
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp#L220-L224

There is a race condition somewhere in the code regarding _ucsrb. If you add cbi(*_ucsrb, UDRIE0); to the linked write function the bug is gone. Not sure if this is correct, and if this should be placed before or after the other calls. Maybe we need a cli() sei() here.

[NicoHood]

Also if the Serial TX buffer is >256 this needs to be inside a cli() sei():
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp#L243

Since I tested with 64 bytes this should be fine for the issue above, but can cause errors for bigger buffers.

---

I also tried

   while(bit_is_clear(*_ucsra, UDRE0)){
   PORTB |= (1<<2);
   PORTB &= ~(1<<2);
   }
  if (_tx_buffer_head == _tx_buffer_tail && bit_is_set(*_ucsra, UDRE0)) {
    *_udr = c;
    //cbi(*_ucsrb, UDRIE0);
    sbi(*_ucsra, TXC0);
    return 1;
  }
// with this (and the code above) the error doesnt occur.
// even though in both cases the code below should never be reached.
// the while is never executed.
// This means gcc accidently skips the if for some reason, sets the bit and flush wont return.
// one has to look at the asm I think.
// The while loop is never reached, thats the funny thing. gcc messes up something here.
/*
  while(1){
   PORTB |= (1<<1);
   PORTB &= ~(1<<1);
   }
*/

which waits until every byte is sent. an automatic flush you could say.
It seems that for some reason the UDRIE0 bit is still set somewhere and blocks flush. but since there is nothing to flush, it wont return. I dont know where this bit is set (maybe automatic after setting other bits) but this is the problem.

From the datasheet I cannot find a reason why this bit is set, it must

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 (bit_is_set(*_ucsrb, UDRIE0) || bit_is_clear(*_ucsra, TXC0)) {
  PORTB |= (1<<3);
    if (bit_is_clear(SREG, SREG_I) && bit_is_set(*_ucsrb, UDRIE0))
    // Interrupts are globally disabled, but the DR empty
    // interrupt should be enabled, so poll the DR empty flag to
    // prevent deadlock
    if (bit_is_set(*_ucsra, UDRE0))
      _tx_udr_empty_irq();

      PORTB &= ~(1<<3);

      if(_tx_buffer_head==_tx_buffer_tail)
      return;
  }
  // If we get here, nothing is queued anymore (DRIE is disabled) and
  // the hardware finished tranmission (TXC is set).
}

This works.