Fix HAL_UART_ErrorCallback wedging the system when the Rx listener errors - #708
Conversation
|
Same as my other PR before this: I was using an LLM to help me make TouchPlaited a firmware for Simple Touch using the mpr121 sensor and after updating libDaisy this issue surfaced. Traced it back to this, however it might be purposefully implemented for another bug. |
|
So the lines you remove here were added to solve a problem (ironically, the frame errors you describe causing issues elsewhere in your case). The error callback should flag listening mode as off so that users can query Does the failure in your case persist if you, instead have an error callback: extern "C" void HAL_UART_ErrorCallback(UART_HandleTypeDef* huart)
{
auto* handle = MapInstanceToHandle(huart->Instance);
if (handle) {
handle->listener_mode_ = false;
}
UartHandler::Impl::DmaTransferFinished(huart, UartHandler::Result::ERR);
}If that works for you, please update. |
|
oh, actually I was just looking over my changes for a different project that happened to also touch this callback -- I can't remember exactly what required the change, but could have been a similar symptom that you experienced. That said, in the case below, the DmaTransferFinished wouldn't be called after a frame error while in listening mode while in yours it is called regardless. On that branch I have: extern "C" void HAL_UART_ErrorCallback(UART_HandleTypeDef* huart)
{
auto* handle = MapInstanceToHandle(huart->Instance);
if(handle->listener_mode_)
handle->listener_mode_ = false;
else
UartHandler::Impl::DmaTransferFinished(huart, UartHandler::Result::ERR);
}Still probably wise to a check on |
MapInstanceToHandle() returns NULL for any instance outside its list of 9 UART/USART peripherals, and f7c63ae writes through that pointer with no null check. The clearing of listener_mode_ also has to undo what DmaListenStart() armed. It enables the IDLE interrupt alongside setting the flag, and UART_IRQHandler acknowledges IDLE only inside its listener_mode_ branch -- ST's HAL does not touch the flag in HAL_UART_RECEPTION_STANDARD mode. So clearing the flag while IDLE stays enabled leaves the flag set with nobody left to clear it, and the UART interrupt re-asserts continuously. DmaListenStop() already disables both together; do the same here. Observed as a UART interrupt storm on hardware running TRS MIDI in continuously: floating-input framing noise starved I2C (OLED, touch) and USB while SAI-DMA audio kept running.
jonwaterschoot
changed the title
73083e8 to
9df11f1
Compare
|
Follow-up, and an answer to your question. One bit of context that turned out to matter more than I expected: I have two modded hardware builds of the Simple Touch. They are complete original kits, Daisy with mpr121 touch sensor, but modded:
I assumed the MIDI unit would be the one that could reproduce this. It's the other way round, and that ended up being most of the story. I ran the failing builds twice to confirm, and checked the decisive results on both units. I've also retitled the PR. I filed it as a null deref, and the testing below shows that isn't what it is — the null check is still worth having, but it isn't what fixes the freeze, and the old title might have sent you looking in the wrong place. The write-up below is Claude's. It guided the debugging and wrote the three candidate patches; I did the building, flashing and observing, so where it says "I", that's me reporting what I actually saw on hardware. The answerYour first snippet does not fix it. Your second one does, and so does a small variation on it that I'd like to propose. Five builds, all from the same commit, differing only in the body of
"Freezes" means: I2C dies (OLED stops updating, MPR121 touch pads stop responding) and USB stops enumerating, while SAI-DMA audio and the ADC keep running normally. I reproduced both the master failure and the first-snippet failure twice each. The repro needs no MIDI hardware at allIt's a floating USART1 Rx pin. My TRS MIDI unit never fails, even on unpatched master — the input optocoupler holds the line at a clean idle, so no framing errors are ever generated. My other unit has no MIDI circuit fitted, so D14 floats and picks up noise while the firmware starts the listener anyway. That one fails every time. So on a bare Seed: start a UART Rx listener, leave the Rx pin unconnected. Why it happensThe table above is measured. This part is inference from reading the code.
So clearing Re-entry alone isn't sufficient, though: your second snippet leaves IDLE enabled and is fine. What tips it over is re-entry plus the cost of What's pushedextern "C" void HAL_UART_ErrorCallback(UART_HandleTypeDef* huart)
{
auto* handle = MapInstanceToHandle(huart->Instance);
if(handle && handle->listener_mode_)
{
handle->listener_mode_ = false;
__HAL_UART_DISABLE_IT(huart, UART_IT_IDLE);
}
UartHandler::Impl::DmaTransferFinished(huart, UartHandler::Result::ERR);
}
I went with this over the skip- Afterwards I flashed this build onto the TRS MIDI unit with a keyboard plugged in: normal MIDI input works, and a glitch no longer wedges anything, since |
2 participants
MapInstanceToHandle() explicitly returns NULL for any instance not in its known list of 9 UART/USART peripherals. f7c63ae added an unconditional write through that pointer on every UART error (handle->listener_mode_ = false), with no null check.
Found on a board (TouchPlaited) that runs UART MIDI continuously alongside I2C (OLED, capacitive touch) and USB: a floating/idle MIDI input generating occasional UART framing noise caused the OLED to stop updating, touch pads to stop responding, and USB enumeration to fail entirely, while audio and switch/knob input kept working normally. Bisected the regression to this exact commit against a known-good older libDaisy snapshot; reverting just these two lines resolves all three symptoms on the affected hardware.
I haven't fully isolated the mechanism at the register level, it could be the null write itself for an unmapped instance, or the added per-interrupt cost under a high error rate starving other peripherals' interrupt servicing, but the fix is verified against the observed failure, and the missing null check is a real gap regardless of which it is.