Serial1.flush() could hang processor if called inside a higher priority interrupt

[KurtE]

I was playing around with some different stuff, and ran into an interesting issue.

And I am trying to decide if it is worth fixing.

Here is some quick typed in code (probably has errors) that will likely hang a teensy)

I

ntervalTimer mytimer;
void timer_function();

void setup() {
    Serial1.begin(115200);
    mytimer.priority(32);
    mytimer.begin(&timer_function, 250000);
}
void loop()
{
}

void timer_function() {
    Serial1.print("print from interval timer");
    Serial1.flush();  // this will likely hang

}

The HardwareSerial write9 function is protected from this hang, in the case that the Serial1
output buffer is filled and waiting for room to become available. In that case the write9 function, will
detect that it is running at a lower priority and then if the hardware port shows that the TDRE flag is set,
in which case it will stuff the data out into the DATA register.

But the HardwareSerial::flush()
simply:

void HardwareSerial::flush(void)
{
	while (transmitting_) yield(); // wait
}

And since the transmitting_ is cleared in the ISR which is at a lower priority it will not get called.

Also in this case it will call yield, in the write9 case it does not call yield if the current running code is running at a
priority >= 256. Which I am assuming is the main code ....


I guess the question is, is this worth fixing. Or is this simply a case of: doctor doctor, it hurts when I do ...

Kurt

[KurtE]

Side comments to self:
Part of the write9 code that currently runs, I have wondered if it still fails at times:

	while (tx_buffer_tail_ == head) {
		int priority = nvic_execution_priority();
		if (priority <= hardware->irq_priority) {
			if ((port->STAT & LPUART_STAT_TDRE)) {
				uint32_t tail = tx_buffer_tail_;
				if (++tail >= tx_buffer_total_size_) tail = 0;
				if (tail < tx_buffer_size_) {
					n = tx_buffer_[tail];
				} else {
					n = tx_buffer_storage_[tail-tx_buffer_size_];
				}
				port->DATA  = n;
				tx_buffer_tail_ = tail;
			}
		} else if (priority >= 256) 
		{
			yield(); // wait
		} 
	}

In particular the comparing the priority to the default priority for the Serial port:
if (priority <= hardware->irq_priority) {

I am wondering if some sketches may choose to change the Hardware serial ports ISR priority. If I were wanting to lower the priority for a specific sketch I would more likely simply call NVIC_SetPriority on the hardware ISR, then go and edit the core sources to lower it, as changing the core changes it for all sketches that you might build for.

So wondering if the code could simply be something like:

	bool tdre_was_set_before = false;
	while (tx_buffer_tail_ == head) {
		int priority = nvic_execution_priority();
		if ((port->STAT & LPUART_STAT_TDRE)) {
			if (tdre_was_set_before) {
				uint32_t tail = tx_buffer_tail_;
				if (++tail >= tx_buffer_total_size_) tail = 0;
				if (tail < tx_buffer_size_) {
					n = tx_buffer_[tail];
				} else {
					n = tx_buffer_storage_[tail-tx_buffer_size_];
				}
				port->DATA  = n;
				tx_buffer_tail_ = tail;
			} else {
				tdre_was_set_before = true;
			}
		} else if (priority >= 256) 
		{
			yield(); // wait
		} 
	}

That is if we see TDRE set twice still with the same tx_buffer_tail, then obviously the hardware serial ISR was not called...

But maybe I am missing something?

[defragster]

justifies general rule: don't print in an ISR

Though doesn't justify a stuck CPU if there was a good reason ... debug or other ... for wanting that done there.

Since it is in the more rare .flush() used when seems 'critical', so would make sense to have it accounted for - though it will give confusing results where the .flush() fails to be able to complete