Skip to content

printf latency improvement#265

Open
IshDeshpa wants to merge 4 commits into
mainfrom
printf-holepunch
Open

printf latency improvement#265
IshDeshpa wants to merge 4 commits into
mainfrom
printf-holepunch

Conversation

@IshDeshpa
Copy link
Copy Markdown
Contributor

@IshDeshpa IshDeshpa commented Apr 14, 2026

  • DMA supported but not required
  • printf now goes through a special call where it is assumed that a buffer stays static in memory so that it doesn't have to be chunked up
  • Queue now supports both string data and pointers to static buffers

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements DMA support for UART transmissions and introduces a uart_send_buf function to facilitate zero-copy transfers of static buffers. The printf implementation has been refactored to use this new functionality, increasing the buffer pool size and utilizing binary semaphores for safe release from interrupts. Review feedback identified several compilation errors in the newly added test files where printf_init was called with insufficient arguments. Additionally, recommendations were made to add Doxygen documentation for the new UART API and to improve code clarity and type safety by using NULL for pointer arguments and removing redundant logic.

husart3->Init.Mode = UART_MODE_TX_RX;
husart3->Init.HwFlowCtl = UART_HWCONTROL_NONE;
husart3->Init.OverSampling = UART_OVERSAMPLING_16;
printf_init(husart3);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The call to printf_init is incorrect. It's missing the second argument for the DMA handle, which will cause a compilation error. If DMA is not used for this target, you should pass NULL.

    printf_init(husart3, NULL);

husart1->Init.HwFlowCtl = UART_HWCONTROL_NONE;
husart1->Init.OverSampling = UART_OVERSAMPLING_16;

printf_init(husart1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The call to printf_init is incorrect. It's missing the second argument for the DMA handle, which will cause a compilation error. If DMA is not used for this target, you should pass NULL.

    printf_init(husart1, NULL);

husart3->Init.Mode = UART_MODE_TX_RX;
husart3->Init.HwFlowCtl = UART_HWCONTROL_NONE;
husart3->Init.OverSampling = UART_OVERSAMPLING_16;
printf_init(husart3);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The call to printf_init is incorrect. It's missing the second argument for the DMA handle, which will cause a compilation error. If DMA is not used for this target, you should pass NULL.

    printf_init(husart3, NULL);

husart1->Init.HwFlowCtl = UART_HWCONTROL_NONE;
husart1->Init.OverSampling = UART_OVERSAMPLING_16;

printf_init(husart1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The call to printf_init is incorrect. It's missing the second argument for the DMA handle, which will cause a compilation error. If DMA is not used for this target, you should pass NULL.

    printf_init(husart1, NULL);

Comment thread bsp/Inc/UART.h
*/
uart_status_t uart_send(UART_HandleTypeDef* handle, const uint8_t* data, uint16_t length, TickType_t delay_ticks);

uart_status_t uart_send_buf(UART_HandleTypeDef* handle, const uint8_t* data, uint16_t length, SemaphoreHandle_t release_sem, TickType_t delay_ticks);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The new function uart_send_buf is missing a Doxygen comment explaining what it does, its parameters, and what it returns. Please add a comprehensive comment for better maintainability.

Suggested change
uart_status_t uart_send_buf(UART_HandleTypeDef* handle, const uint8_t* data, uint16_t length, SemaphoreHandle_t release_sem, TickType_t delay_ticks);
/**
* @brief Transmits a static buffer over UART without copying its contents.
* @details This function is designed for performance-critical sections where data is in a static or long-lived buffer.
* It queues a pointer to the data, avoiding a memcpy. A semaphore is given back by the UART driver upon transmission completion.
* The caller is expected to take this semaphore before calling this function to ensure the buffer is not modified during transmission.
* @param handle Pointer to the UART handle.
* @param data Pointer to the static buffer containing data to send.
* @param length Number of bytes to transmit.
* @param release_sem Semaphore to be released upon completion of the transmission.
* @param delay_ticks Ticks to wait if TX queue is full (0 = no wait, portMAX_DELAY = wait indefinitely).
* @return uart_status_t Returns UART_OK on success, UART_ERR on failure.
*/
uart_status_t uart_send_buf(UART_HandleTypeDef* handle, const uint8_t* data, uint16_t length, SemaphoreHandle_t release_sem, TickType_t delay_ticks);

Comment thread driver/Src/printf.c
Comment on lines +53 to +54
if(HAL_DMA_Init(hdma_uart_tx) != HAL_OK) return false;
else __HAL_LINKDMA(huart,hdmatx,*hdma_uart_tx);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The else statement here is unnecessary. Removing it improves code clarity, as __HAL_LINKDMA is only reached if HAL_DMA_Init succeeds.

        if(HAL_DMA_Init(hdma_uart_tx) != HAL_OK) {
            return false;
        }
        __HAL_LINKDMA(huart,hdmatx,*hdma_uart_tx);

Comment thread test/tests/printf_test.c
husart2->Init.HwFlowCtl = UART_HWCONTROL_NONE;
husart2->Init.OverSampling = UART_OVERSAMPLING_16;
printf_init(husart2);
printf_init(husart2, false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The second argument to printf_init should be a pointer. Using false works because it's treated as a null pointer constant, but for clarity and type safety, it's better to use NULL.

    printf_init(husart2, NULL);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant