Interrupt lock missing in debug log functions

[HiFiPhile]

What happened ?

Interrupt lock need to be added to log functions, calling from ISR could enter deadlock mostly for serial logging. #2649

Options:

• Remove all logging from ISR.
• Defer ISR logging, should be difficult with va_args.
• Add interrupt lock, but macros need to be reworked since we don't know if they are called from device or host or typec stack in order to toggle interrupt accordingly.
• Add a simple re-entrancy flag to skip printf.

Edit: I must have bad memory, I met log racing while debugging dwc2 DMA with RTT but SEGGER_RTT_Write() is ISR-safe, it must happened inside libc printf().

How to reproduce ?

Run examples on ports who has log in ISR, like NRF5x.

[tlyu]

I haven't had trouble with deadlocks with RTT on nRF52, compared to the Arduino core's Serial1.

On the other hand, anything with Bluetooth (Bluefruit lib; haven't tried anything lower level yet) seems to cause trouble with segfaults during debugging, including RTT.

[HiFiPhile]

I haven't had trouble with deadlocks with RTT on nRF52

I think it depends on timing (luck), implementation side they don't have interrupt lock. https://github.com/NordicSemiconductor/nrfx/blob/4620e7ccfebc08dbd4c039bc7d530b62c4c68ca8/drivers/src/nrfx_uarte.c#L871

[ceedriic]

Options:

• Remove all logging from ISR.
• Defer ISR logging, should be difficult with va_args.

I think these two options are best.

In my own embedded code, if I really need to log in an IRQ, I use two custom functions that log either a single string or a string with a number, and these fonctions (1) don't use libc and (2) work "optimistically", (i.e. only if there is room in the output buffer). I think a general solution is difficult.

Also I don't think this is a real problem here for embedded systems, but in theory vfprintf() and friends can call malloc() and free() which is very bad practice in an IRQ....

If a tu_printf()-like function really wants to be called in an interrupt, then it would help if a different macro is defined (like tu_printf_irq()) and be overridable differently (CFG_TUSB_DEBUG_PRINTF vs CFG_TUSB_DEBUG_PRINTF_IRQ) so users can implement them in a different way. The context (irq or not) could be determined at runtime of course, but users could save code + IRQ latency by just defining CFG_TUSB_DEBUG_PRINTF_IRQ() a dummy statement.