arduino / arduino/ArduinoCore-samd

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

Open
#472 1 comment 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.