Fix Hardwareserial sending a byte twice

[NicoHood]

https://github.com/arduino/Arduino/issues/3745

This also solves a flush() lockup.

[matthijskooijman]

This code still has the sbi() for clearing TXC0, which is incorrect in theory (though we haven't properly deduced if it actually causes problems in practice too, and in theory making it atomic also prevents issues).

Also, perhaps you could use ATOMIC_BLOCK? That makes things more readable and should be used for new code IMHO.

[NicoHood]

Added, also applied for the one left cli() that was there before.

[matthijskooijman]

Looking at this PR, it actually seems like it applies good fixes. See https://github.com/arduino/Arduino/issues/3745#issuecomment-331627824 for a summary of the related problems.

This PR makes the following changes:

1. Replace cli()/SREG with ATOMIC_BLOCK
2. Makes _tx_buffer_head == _tx_buffer_tail in write() atomic.
3. Makes UDR write and TXC clear atomic.
4. Makes _tx_buffer_head and UDRIE writes atomic.

All of which I think are useful and relevant. However, I think this PR could still be improved:

• By putting the ATOMIC_BLOCK commit first, so the rest can just use ATOMIC_BLOCK from the start.
• By, instead of using ATOMIC_BLOCK directly for the tx buffer head/tail accesses, introduce a TX_BUFFER_ATOMIC macro or similar. When the TX buffer is <= 256 bytes, this macro would just be empty, otherwise it would resolve to ATOMIC_BLOCK(ATOMIC_RESTORESTATE). Doing so removes the need for the #if (SERIAL_TX_BUFFER_SIZE>256)` checks all over the code and improves readability.
• By fixing the way TXC0 is set (e.g. do not use sbi(), see issue 1 described at https://github.com/arduino/Arduino/issues/3745#issuecomment-331627824).
• By splitting 9aa902e into two separate commits, and describing the problem in a bit more detail in the commit message.

@NicoHood, would you be interested in making these changes?

[NicoHood]

@matthijskooijman I've granted arduino maintainers access to my patch branch via github. I currently have no time to do and test the changes. Feel free to modify or do a new PR, the important stuff is already there (how to fix the issue).

[johnholman]

Are either of you working on this? If not I'm happy to make a new PR based on @NicoHood's work and incorporating changes suggested by @matthijskooijman.

@matthijskooijman I've defined the TX_BUFFER_ATOMIC macro in HardwareSerial.h, but would you prefer it in HardwareSerial.cpp? Also is the include of <util/atomic.h> needed for ATOMIC_BLOCK better kept local in HardwareSerial.h or in Arduino.h to support use elsewhere?

[matthijskooijman]

@johnholman, I'm not finding the time for this, so I'm not working on this. If you would, that would be great.

As for the TX_BUFFER_ATOMIC macro, if it's only used inside the .cpp file, I'd define it in the .cpp file, to prevent polluting the global scope of other source files. The same goes for the util/atomic.h include.

[johnholman]

@matthijskooijman, @NicoHood I've created pull request #6855 for this.

[matthijskooijman]

I'm closing this one in favor of #6855.