[modem]: Add support for UARTs DMA mode #895
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| .uart_port = uart.port, | ||
| .tx_trans_queue_depth = 2, | ||
| .max_receive_internal_mem = dma_buffer_size, | ||
| .max_transmit_size = dma_buffer_size, |
Check warning
Code scanning / clang-tidy
ISO C++ requires field designators to be specified in declaration order; field 'max_receive_internal_mem' will be initialized after field 'max_transmit_size' [clang-diagnostic-reorder-init-list] Warning
| // Protect shared state access | ||
| if (xSemaphoreTake(rx_mutex, pdMS_TO_TICKS(10)) == pdTRUE) { | ||
| size_t current_received_size = received_size; | ||
| bool current_rx_complete = rx_complete; |
Check warning
Code scanning / clang-tidy
Value stored to 'current_rx_complete' during its initialization is never read [clang-analyzer-deadcode.DeadStores] Warning
| // Protect shared state access | ||
| if (xSemaphoreTake(rx_mutex, pdMS_TO_TICKS(10)) == pdTRUE) { | ||
| size_t current_received_size = received_size; | ||
| bool current_rx_complete = rx_complete; |
Check warning
Code scanning / clang-tidy
unused variable 'current_rx_complete' [clang-diagnostic-unused-variable] Warning
| if (rx_mutex) { | ||
| vSemaphoreDelete(rx_mutex); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Uninitialized Mutex Leads to Undefined Behavior
The rx_mutex member isn't initialized in the UartDmaTerminal constructor. When DMA is disabled, this leaves rx_mutex with an undefined value, causing the destructor's if (rx_mutex) check and vSemaphoreDelete call to operate on an uninitialized pointer, which can lead to undefined behavior.
| // Notify the task that data is available | ||
| xSemaphoreGiveFromISR(terminal->rx_semaphore, &xHigherPriorityTaskWoken); | ||
|
|
||
| return xHigherPriorityTaskWoken; // Return whether a higher priority task was woken |
There was a problem hiding this comment.
Bug: ISR Mutex Conflict and Return Type Mismatch
The uhci_rx_event_callback runs in an ISR, but it uses rx_mutex (a regular mutex) with ISR-safe semaphore functions, which can lead to undefined behavior. There's also a type mismatch where the function returns a BaseType_t (xHigherPriorityTaskWoken) while declared to return bool.
| xSemaphoreGive(rx_mutex); | ||
| return copy_len; | ||
| } | ||
| return 0; // Timeout or error |
There was a problem hiding this comment.
Bug: DMA Buffer Corruption During Concurrent Access
A race condition exists where the DMA rx_buffer is accessed by the read() function or on_read callback while the DMA hardware may be concurrently writing to it. The rx_mutex protects software state but doesn't prevent the DMA hardware from corrupting the buffer during software operations like memmove or on_read processing, leading to data corruption or inconsistent reads.
Additional Locations (1)
| src/esp_modem_term_uart.cpp | ||
| src/esp_modem_netif.cpp) | ||
| set(dependencies driver esp_event esp_netif) | ||
| set(dependencies driver esp_event esp_netif esp_driver_uart) |
There was a problem hiding this comment.
@david-cermak Is there any ETA for this change?
Currently it breaks our builds with esp-idf/master. (We build esp_modem_usb_dte in our USB CI, which in turn pulls in esp_modem.) https://github.com/espressif/esp-usb/actions/runs/18191462194/job/51787270030?pr=276#step:4:1917
We can temporarily disable the build job if next esp_modem release needs more time
There was a problem hiding this comment.
Also maybe...?
| set(dependencies driver esp_event esp_netif esp_driver_uart) | |
| set(dependencies esp_event esp_netif esp_driver_uart) |
There was a problem hiding this comment.
We can temporarily disable the build job if next esp_modem release needs more time
Please disable the build for now, got some breaking changes on master already, so I'll have to branch off...
WIP, needs testing
Note
Introduces UART DMA (UHCI) terminal/DTE with config flags, updates API and build, and enables optional DMA in the modem_console example.
src/esp_modem_uart_dma.cppwith public APIinclude/esp_modem_uart_dma.h.create_uart_dma_dteinesp_modem_api.hppand implement insrc/esp_modem_api_target.cpp.esp_modem_uart_term_configwithuse_dmaanddma_buffer_size(defaults set inESP_MODEM_DTE_DEFAULT_CONFIG).src/esp_modem_uart_dma.cppinCMakeLists.txtand add dependency onesp_driver_uart.use_dmaanddma_buffer_size; create DTE withcreate_uart_dma_dtewhen enabled; add logs.sdkconfig.defaults.Written by Cursor Bugbot for commit 3b0b7dd. This will update automatically on new commits. Configure here.