arduino / arduino/ArduinoCore-samd
HardwareSerial/Uart write and flush lock up when interrupts are disabled
- Dominant language
- C
- Stars
- 502
- Forks
- 740
- PR merge metrics
- No merged PRs in 30d
Description
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](https://github.com/arduino/ArduinoCore-avr/blob/c8d6aef6d9331af1ec8dea6f78b4e43174cdb55d/cores/arduino/HardwareSerial.cpp#L205-L223)). 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](https://community.arm.com/developer/ip-products/processors/f/cortex-m-forum/5387/get-current-active-interrupt-priority), 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.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the SAMD Uart (HardwareSerial) write() and flush() paths described in the issue, then compare their behavior with cores/arduino/HardwareSerial.cpp in the linked AVR implementation. Investigate the IPSR, interrupt priority, and PRIMASK conditions before deciding how the UART ISR should be serviced while interrupts cannot run. Done means write() and flush() no longer block indefinitely in those conditions, with USB CDC considered separately.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- embedded-iot
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100