Description
In the CDC Serial, 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.
However, when these methods are called with interrupts disabled (or from an ISR with higher priority than the USB 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). This is also the usecase that triggered this report: I was trying to print a message from the CPU fault handlers to let me know what was going on.
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 USB CDC code that can get interrupted by an ISR that accesses CDC Serial, 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 SMT32 CDC serial. One complication is that while on AVR there is a single "interrupts are disabled" flag, on STM32F4 (Cortex M4) you would have to check:
- Whether an exception is currently running, and if its priority is higher than the USB priority (Can be done using this method.
- Whether the
PRIMASKbit is set (which disables all interrupts) - Whether the minimum exception priority specified in
BASEPRIis low enough.
This might vary between different cortex cores, e.g. on M0 BASEPRI is not available, there is no VECTACTIVE (but the same value is in IPSR) and PRIMASK is called PM.
An implementation could look like (pseudocode):
while (wait_for_buffer_space) {
if (IPSR && GetPriority(IPSR) <= GetPriority(USB_IRQn) || (PRIMASK & PM) || BASEPRI <= GetPriority(USB_IRQn)
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.
A complication for implementing this is the layers of code that are involved in the CDC Serial code currently (I think there's at least two, probably three layers of USB code?), but maybe this could be added to the lowest layer and called from the upper layers?
One alternative implementation would be to detect this situation and temporarily raise the USB interrupt priority until the loop completes. However, this will not work when the PRIMASK 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.
The workaround I'm currently using is to lower the priority of the current ISR to less than the USB ISR priority, so the USB ISR can interrupt. In general, this is a bad idea, since that will also allow other interrupts to preempt.
I suspect that the hardware serial / UART code suffers from exactly the same problem and should probably be fixed in the same way, but have not tested this.
The same problem also happens with the Arduino SAMD core HardwareSerial, see arduino/ArduinoCore-samd#472 (mostly identical to this issue).