Core debug hardening

[fpistm]

Fixes #1789
Clean up the debug uart functions
Core debug is now half duplex by default if not linked to another Serial instance.
Tested:

• Serial and debug on the same U(S)ART
• Serial and debug on different U(S)ART

[matthijskooijman]

I'm not sure I understand the cause of the problem properly. Is it that the old code would just try to start transmission with the hAL until success or a timeout, but did this with interrupts enabled, so the uart would (if Serial transmission was ongoing) would never change to READY state (because no ISR would run) and it would always fail?

If so, I can imagine that another fix could have been to simply remove the disable/enable IRQ lines? Though just waiting for idle and then trying to transmit once is of course a lot cleaner.

I do wonder if the disable/enable IRQ lines are still needed with the current code? AFAICS that once the uart is READY, the IRQ will be disabled anyway (not the complete IRQ, but all IE flags)? So the only thing that this would protect against is if the UART IRQ itself would start another transmission, which seems impossible (and if not, the code would still have a race condition to fix). Or is this intended to guard against something in the RX handling that makes the UART non-READY again? Speaking of which - I think that the current code might actually break RX by keeping the IRQ disabled, so that's one more reason to remove the IRQ disabling (though I guess since debug transmits one byte at a time, IRQs are never disabled long enough to really break reception).

To be clear, I'm talking about this section:

Arduino_Core_STM32/libraries/SrcWrapper/src/stm32/uart.c

Lines 746 to 752 in a10b175

	HAL_NVIC_DisableIRQ(serial_debug.irq);
	if (HAL_UART_Transmit(huart, data, size, TX_TIMEOUT) != HAL_OK) {
	size = 0;
	}
	HAL_NVIC_EnableIRQ(serial_debug.irq);

Note that disabling IRQs could still be useful to guard against a sketch that prints from an ISR, but that is not currently supported by HardwareSerial at all, I think (and also not by core_debug, since that would just timeout waiting for the uart to become READY).

One more thought: The use of a timeout waiting for READY also seems tricky - if called with interrupts disabled the UART never becomes READY so you always have to wait the full timeout (even though you could maybe already know that it would never happen - a bit tricky to know for sure maybe), but also the timeout might trigger when the Serial buffer is particularly full and takes a long time to empty (but at 1000ms timeout, that's probably only an issue on 300 or 600 baud or so, otherwise the HardwareSerial buffer is probably too small).

One more thought: This currently blocks until the entire HardwareSerial buffer is empty before transmitting (since if the current batch is completed, the TxComplete ISR calls into HardwareSerial which then immediately queues the next batch of pending data (if any)). It would maybe be nice if this would only wait for the current batch to complete, but I think this is essentially impossible, at least without changes to the HAL code (since the uart does not return to READY until the TxC ISR fires, and if that fires, it immediately queues up more).

Finally: I haven't tested the code yet. It looks like it would work at first glance, but I ran out of time for today. I'll test tomorrow :-)

[fpistm]

Hi @matthijskooijman

After more investigation I've made a new proposal with no IRQ disable 😉 this allows to kept RX.
It is closed t the first proposal, I've only made some more cleanup using obj directly.

[matthijskooijman]

Looking good. There is one worry I have still - if this code is called from inside an ISR (or another context where interrupts are disabled) and Serial has TX ongoing, I think this will result in an infinite loop, right? I know that logging/serial printing from inside an ISR is often considered unsupported and should not be done, but it is often very useful so if at all possible, we should support it (or at least not lock up - dropping output might be preferable then). For example AVR HardwareSerial does support it, and there's an issue about SAMD here (which also has some details on how it works on AVR).

[fpistm]

if this code is called from inside an ISR (or another context where interrupts are disabled) and Serial has TX ongoing, I think this will result in an infinite loop, right?

Right. Note that to note block I've added the CORE_DEBUG_TIMEOUT

I know that logging/serial printing from inside an ISR is often considered unsupported and should not be done,

exactly :)

but it is often very useful so if at all possible, we should support it (or at least not lock up - dropping output might be preferable then). For example AVR HardwareSerial does support it, and there's an issue about SAMD here (which also has some details on how it works on AVR).

Propbably, anyway, I think it should be managed as a separate issue/PR.

[matthijskooijman]

Propbably, anyway, I think it should be managed as a separate issue/PR.

Yeah, though the easy workaround could be to keep the timeout always active maybe? A proper fix would need more work to really flush the buffer instead of just doing a timeout, so that would definitely be for later.

I also think this might affect STM32LoRaWAN, since I think STM32CubeWL does some printing from inside ISRs too...

[fpistm]

I also think this might affect STM32LoRaWAN, since I think STM32CubeWL does some printing from inside ISRs too...

Bad news. Will check what I can do.