Javier/dma transfer #4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds QSPI and SPI simplex (unidirectional) transfer support to the DMA Transfer Driver, expanding beyond the existing full-duplex SPI implementation. The driver now supports transmit-only and receive-only operations for both SPI and QSPI peripherals.
Changes:
- Added SPI simplex transfer support (transmit-only and receive-only modes) alongside existing full-duplex mode
- Added QSPI peripheral support with transmit and receive DMA operations
- Updated documentation to include QSPI in the unified interface feature list and added usage examples for SPI simplex and QSPI transfers
- Fixed UART comment indentation for consistency
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| Drivers/Inc/DMATransfer.hpp | Added SPI simplex logic with separate transmit/receive paths, implemented QSPI support with transmit/receive operations, added debug error message for unsupported handle types, fixed UART comment indentation |
| Drivers/DMATransfer_README.md | Updated Features section to list QSPI_HandleTypeDef, added SPI simplex usage examples, added complete QSPI usage section with transmit/receive examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if constexpr (std::is_same_v<HandleType, SPI_HandleTypeDef>) { | ||
| // SPI Transfer (Full-Duplex) | ||
| return HAL_SPI_TransmitReceive_DMA(handle, txData, rxData, size); | ||
|
|
||
| // Transmit Only (Simplex) | ||
| if (txData != nullptr && rxData == nullptr){ | ||
| return HAL_SPI_Transmit_DMA(handle, txData, size); | ||
| } | ||
| // Receive Only (Simplex) | ||
| if (txData == nullptr && rxData != nullptr){ | ||
| return HAL_SPI_Receive_DMA(handle, rxData, size); | ||
| } | ||
| // Full Duplex | ||
| if (txData != nullptr && rxData != nullptr){ | ||
| return HAL_SPI_TransmitReceive_DMA(handle, txData, rxData, size); | ||
| } |
There was a problem hiding this comment.
The function does not handle the edge case where both txData and rxData are nullptr. In this scenario, the function will fall through all condition checks and return HAL_ERROR (line 99), but this invalid call pattern should be caught and potentially logged or asserted in debug builds before the handle type checks are performed.
Consider adding an early validation check after the cache management section (line 35) to handle this case explicitly, similar to how the debug message is printed for unsupported handle types.
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
There are unnecessary blank lines (lines 78-79) before the QSPI logic section. For consistency with the rest of the code, there should be only one blank line between sections (as seen between I2C and UART sections at line 67).
| } | ||
| #endif | ||
|
|
||
|
|
There was a problem hiding this comment.
There are unnecessary blank lines (lines 97-98) at the end of the function. For consistency with coding style, there should be at most one blank line before the return statement.
| if (txData != nullptr && rxData == nullptr){ | ||
| return HAL_SPI_Transmit_DMA(handle, txData, size); | ||
| } | ||
| // Receive Only (Simplex) | ||
| if (txData == nullptr && rxData != nullptr){ | ||
| return HAL_SPI_Receive_DMA(handle, rxData, size); | ||
| } | ||
| // Full Duplex | ||
| if (txData != nullptr && rxData != nullptr){ |
There was a problem hiding this comment.
Missing space after the closing parenthesis before the opening brace. The code style should have a space between ) and { for consistency. This occurs on lines 41, 45, and 49. The codebase appears to follow this convention based on other conditional statements in the file (e.g., lines 58, 63, 70, 74, 83, 87).
| #ifdef SOAR_DEBUG { | ||
| SOAR_PRINT("DMAControl::Transfer - Unsupported handle type\n"); | ||
| } | ||
| #endif | ||
|
|
||
|
|
There was a problem hiding this comment.
The #ifdef directive has incorrect syntax. The opening brace { should not be on the same line as #ifdef. The preprocessor directive should be on its own line, and the code block should follow on subsequent lines. Additionally, the closing brace } on line 95 should not be inside the #endif block.
The correct syntax should be:
#ifdef SOAR_DEBUG
SOAR_PRINT("DMAControl::Transfer - Unsupported handle type\n");
#endif
Without the braces, or if braces are needed for scoping:
#ifdef SOAR_DEBUG
{
SOAR_PRINT("DMAControl::Transfer - Unsupported handle type\n");
}
#endif
| #ifdef SOAR_DEBUG { | |
| SOAR_PRINT("DMAControl::Transfer - Unsupported handle type\n"); | |
| } | |
| #endif | |
| #ifdef SOAR_DEBUG | |
| SOAR_PRINT("DMAControl::Transfer - Unsupported handle type\n"); | |
| #endif |
| if (txData == nullptr && rxData != nullptr){ | ||
| return HAL_SPI_Receive_DMA(handle, rxData, size); | ||
| } | ||
| // Full Duplex | ||
| if (txData != nullptr && rxData != nullptr){ |
There was a problem hiding this comment.
The SPI logic uses sequential if statements instead of else if, which is inconsistent with the patterns used for I2C (lines 58-65) and UART (lines 70-76). While functionally equivalent in this case due to the early returns, using else if would be more consistent with the rest of the codebase, more efficient (avoiding unnecessary condition checks), and clearer in intent.
Consider changing lines 45 and 49 to use else if instead of if to match the established pattern in the I2C and UART sections.
| if (txData == nullptr && rxData != nullptr){ | |
| return HAL_SPI_Receive_DMA(handle, rxData, size); | |
| } | |
| // Full Duplex | |
| if (txData != nullptr && rxData != nullptr){ | |
| else if (txData == nullptr && rxData != nullptr){ | |
| return HAL_SPI_Receive_DMA(handle, rxData, size); | |
| } | |
| // Full Duplex | |
| else if (txData != nullptr && rxData != nullptr){ |
Added QSPI and SPI simplex transfers