Potential typo/bug in HardwareSerial::_tx_udr_empty_irq?

[ziriax]

I noticed the following code in HardwareSerial::_tx_udr_empty_irq:

ArduinoCore-avr/cores/arduino/HardwareSerial.cpp

Lines 103 to 107 in 855ea01

	#ifdef MPCM0
	*_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << MPCM0))) | (1 << TXC0);
	#else
	*_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << TXC0)));
	#endif

Shouldn't

*_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << TXC0)));

be

*_ucsra = ((*_ucsra) & ((1 << U2X0))) | (1 << TXC0);

just like the line in #ifdef MPCM0?

Because now it preserves the TXC0 bit instead of setting it?

[matthijskooijman]

(I've taken the liberty to edit your post to add a permalink to the code, so github renders it with a link and syntax highlighting)

Seems you are right indeed. However, I do suspect that this is not actually an issue in practice, since the TXC0 bit is cleared by writing a 1 to it, so if it was 1 it will be cleared, and if it was 0, there is no need to clear it. Still, probably good to fix the inconsistency.

For history, this line originally just wrote a 1 to the TXC0 bit:

fa8df58

Then it was changed to preserve the U2X0 and MPCM0 bits:

99c294c

Finally, the MPCM0 bit was made optional and this inconsistency was introduced:

6e23562

There is also a second instance of this inconsistency here:

ArduinoCore-avr/cores/arduino/HardwareSerial.cpp

Lines 243 to 247 in 855ea01

	#ifdef MPCM0
	*_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << MPCM0))) | (1 << TXC0);
	#else
	*_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << TXC0)));
	#endif

I can't commit on whether a PR for this would be merged, but I would be happy to review any PR :-)

[ziriax]

Little PR here: #603

I moved the (1 << TXC0) to the beginning of the expression to make it easier to spot the difference.

PS: It was Cursor that notified me of this discrepancy while I was adding code for non-blocking RS485 communication using HardwareSerial (which seems to work in my prototype); I have several MKS Servo 57C stepper drivers that I want to control with a MAX485 in parallel, together with many other components, and I couldn't find a way to do this with the standard HardwareSerial class, unless I added blocking delays. So I added a TX complete IRQ callback to allow setting the DE pin of MAX485 LOW immediately after the last bit is transmitted; timing here seems critical otherwise messages can get lost. I also added a method to try to write bytes in a non-blocking way. I guess these kind of changes are too specific to be accepted in a PR?

[matthijskooijman]

PS: It was Cursor that notified me of this discrepancy while I was adding code for non-blocking RS485 communication using HardwareSerial (which seems to work in my prototype); I have several MKS Servo 57C stepper drivers that I want to control with a MAX485 in parallel, together with many other components, and I couldn't find a way to do this with the standard HardwareSerial class, unless I added blocking delays. So I added a TX complete IRQ callback to allow setting the DE pin of MAX485 LOW immediately after the last bit is transmitted; timing here seems critical otherwise messages can get lost. I also added a method to try to write bytes in a non-blocking way. I guess these kind of changes are too specific to be accepted in a PR?

That is not up to me, but one of the challenges here is to get an API that stays simple but also consistent between different parts of the Arduino API, and there is, I think, not a clear approach for async APIs like this. Also, callbacks that run in an ISR are a bit tricky, especially for novice users, so until now I think mostly avoided.

[ziriax]

Yes, I totally get this. Actually, begin a former C++ video game developer (from C64 to PS3) I was advised by an experienced embedded engineer to switch to ESP32 or STM32, for more advanced fully event driven coding. So far I managed by forking the Arduino cores, lots of fun making a custom tennis/padel ball machine😉