esp32/machine_uart: Correctly manage UART queue and event task. #17135
Conversation
If the driver was reinitialised while there was already an event task running the queue that task is trying to receive from would be deleted, causing it to try to take a lock that no longer existed and deadlocking the CPU. This change ensures the task is always shut down before recreating the queue and recreates the task afterwards. It also removes the task when the UART is deinitialized (which was previously missing), adds a check that no event task can be started when no queue exists, and adds a check to prevent reinitialising the UART driver unnecessarily. Signed-off-by: Daniël van de Giessen <daniel@dvdgiessen.nl>
| @@ -59,6 +59,7 @@ | |||
| #define UART_IRQ_BREAK (1 << UART_BREAK) | |||
| #define MP_UART_ALLOWED_FLAGS (UART_IRQ_RX | UART_IRQ_RXIDLE | UART_IRQ_BREAK) | |||
| #define RXIDLE_TIMER_MIN (5000) // 500 us | |||
| #define UART_QUEUE_SIZE (3) | |||
There was a problem hiding this comment.
This number 3 is the exact same magic number that was previously used when IRQ RXIDLE support was introduced in #14041. I only moved it to a define since it's now used in two places. I don't know why it's 3 and didn't see any mention in the previous MR about that, but decided to not change it since it seems to work fine.
| mp_raise_ValueError(MP_ERROR_TEXT("UART does not support IRQs")); | ||
| } | ||
| #endif | ||
| if (self->uart_queue == NULL && args[MP_IRQ_ARG_INIT_handler].u_obj != mp_const_none) { |
There was a problem hiding this comment.
This does allow setting a None IRQ handler, which is to prevent cleanup code from raising errors needlessly.
For example if used with PPP when closing the PPP connection the cleanup will happen asynchronously triggered by a callback from the lwIP TCP/IP thread, which would normally unset the IRQ it previously configured on it's UART object. If the UART object was already cleaned up as well (deinited or GC collected) that cleanup callback would encounter a ValueError and subsequently crash because the lwIP thread can't handle a non-linear return. (The fact PPP/lwIP breaks like that is a separate issue this PR is not addressing.)
Summary
If the driver was reinitialised while there was already an event task running the queue that task is trying to receive from would be deleted, causing it to try to take a lock that no longer existed and deadlocking the CPU.
This change ensures the task is always shut down before recreating the queue and recreates the task afterwards.
It also:
Testing
Tested on three custom boards (1x original ESP32, 2x ESP32S3) to talk to three modem chips (1x Sequans, 2x different SIMCOM) all using the UART IRQ's to trigger the PPP.poll() method.