HardwareSerial/Uart write and flush lock up when interrupts are disabled

[matthijskooijman]

In Uart (HardwareSerial), when the write() method is called and the buffer is full, it blocks until buffer room becomes available. When flush() is called, it blocks until the buffer is empty (and the last byte is also transmitted).

However, when these methods are called with interrupts disabled (or from an ISR with higher priority than the UART interrupt), the ISR that makes room in the buffer will never run, so this waiting blocks indefinitely, locking up the system.

Of course, one can say that serial printing from an ISR or with interrupts disabled is not a good idea, but it often makes sense for debugging (or e.g. for printing an error on a failed assertion).

On AVR, this is fixed by detecting that interrupts are disabled, and if so, manually checking the interrupt flag and run the ISR manually when the interrupt flag is set (code). This makes sure that printing and flushing with interrupts disabled works as expected (there is still the related issue of Uart code that can get interrupted by an ISR that accesses the UART, leading to inconsistent state, but that is a separate issue and far less likely to occur in practice, so not so problematic for debugging).

It would be good to implement the same thing in the SAMD core Uart. One complication is that while on AVR there is a single "interrupts are disabled" flag, on SAMD you would have to check:

• Whether an exception is currently running, and if its priority is higher than the UART priority (Can be done using this method, noting that the M0+ has no VECTACTIVE, but does have the IPSR register that provides the needed information).
• Whether the PM bit in the PRIMASK register is set (which disables all interrupts)

See also http://infocenter.arm.com/help/topic/com.arm.doc.dui0662b/DUI0662B_cortex_m0p_r0p1_dgug.pdf

An implementation could look like (pseudocode):

while (wait_for_buffer_space) {
  if (IPSR && GetPriority(IPSR) <= GetPriority(irqn) || (PRIMASK & PM)
	run_isr();
}

This might also need to clear the pending bit if that is not automatic. Also, the if might be good to put into a utility function, so it can be used in other places too.

One alternative implementation would be to detect this situation and temporarily raise the Uart interrupt priority until the loop completes. However, this will not work when the PM bit is set, when the current interrupt already has priority 0 (or less, for e.g. NMI or hardfault), so this probably is not a viable option.

A completely different behaviour would be to detect that the ISR cannot run and drop bytes instead of waiting, but that significantly changes the behaviour of these functions (and this deviates from the AVR behaviour), so I don't think that's a good idea.

I suspect that the USB CDC code suffers from exactly the same problem and should probably be fixed in the same way, but have not tested this.

[matthijskooijman]

I think the same problem (and probably the same solution) applies to the delay() function as well (this is not fixed on AVR yet, though).