[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...
|
|
||
| // Create DTE with appropriate terminal type | ||
| #ifdef CONFIG_EXAMPLE_MODEM_UART_USE_DMA | ||
| auto uart_dte = create_uart_dma_dte(&dte_config); |
There was a problem hiding this comment.
Why create a separated function? Isn't the use_dma field not enough to differentiate the call?
|
|
||
| bool UartDmaTerminal::uhci_tx_done_callback(uhci_controller_handle_t uhci_ctrl, const uhci_tx_done_event_data_t *edata, void *user_ctx) | ||
| { | ||
| // TX completion can be handled here if needed |
There was a problem hiding this comment.
Do user have way to override this callback? And is the comment a left over or we miss the implementation?
4 participants
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.