Refactor/spi #554
Refactor/spi #554
Conversation
… funcion y reorganizar un poco
…nically consteval), and a few more constexprs
…les de los modulos al start
…a estan siendo utilizadas
…ia hacer el start
…pp, fixed irqn function, and minor changes to inscribe_dma for legibility
…Alternate functions
…a cual se puede repetir). cambios en sus constructores y nuevo metodo (setDMAHandle) para poder pasar el handle desde el periferico
There was a problem hiding this comment.
Pull request overview
This PR refactors the SPI driver implementation to integrate with a new compile-time infrastructure. The refactor removes the protocol layer that was mixed with the base driver and introduces a cleaner separation of concerns using the new DMA infrastructure.
Changes:
- Introduces new SPI2.hpp/cpp with compile-time configuration and separate master/slave APIs
- Adds new DMA2.hpp/cpp with compile-time domain-based architecture
- Comments out old SPI/DMA callback implementations in legacy code (FMAC, I2C, ADC, old SPI)
- Updates ST-LIB.hpp to integrate new SPIDomain and DMA_Domain
- Enables USE_FULL_ASSERT and changes library type from STATIC to OBJECT in build configuration
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| Inc/HALAL/Models/SPI/SPI2.hpp | New SPI domain with compile-time configuration, pin validation, and separate master/slave wrapper APIs |
| Src/HALAL/Models/SPI/SPI2.cpp | Implementation of IRQ handlers, HAL callbacks, and prescaler calculation for new SPI |
| Inc/HALAL/Models/SPI/SPIConfig.hpp | New comprehensive SPI configuration types with translation to HAL constants |
| Inc/HALAL/Models/DMA/DMA2.hpp | New DMA domain with compile-time configuration and peripheral-specific initialization |
| Src/HALAL/Models/DMA/DMA2.cpp | DMA IRQ handlers for all 16 streams |
| Inc/ST-LIB.hpp | Integration of new SPIDomain and DMA_Domain into build context |
| Src/HALAL/Services/Communication/SPI/SPI.cpp | Old SPI callbacks commented out for future removal |
| Src/HALAL/Services/FMAC/FMAC.cpp | DMA inscribe call commented out pending migration |
| Src/HALAL/Services/Communication/I2C/I2C.cpp | DMA inscribe call commented out pending migration |
| Src/HALAL/Services/ADC/ADC.cpp | DMA-related code removed, error message updated |
| Inc/HALAL/Services/FMAC/FMAC.hpp | DMA stream fields removed from FMACInstance struct |
| Inc/HALAL/Services/Communication/SPI/SPI.hpp | DMA stream fields removed from SPI Instance |
| Inc/HALAL/Services/Communication/I2C/I2C.hpp | DMA stream fields removed from I2C Instance |
| Inc/HALAL/Services/ADC/ADC.hpp | DMA stream removed from ADC InitData |
| Inc/HALAL/Models/Pin.hpp | Updated PA15 alternate function mask |
| Inc/HALAL/HALAL.hpp | Switched from old DMA.hpp to DMA2.hpp, added SPI2.hpp |
| Src/HALAL/HALAL.cpp | Commented out old DMA::start() call |
| Src/HALAL/Models/DMA/DMA.cpp.tmp | Added Spanish comment about coexistence with new DMA |
| Inc/HALAL/Models/DMA/DMA.hpp | Minor formatting fix |
| Inc/stm32h7xx_hal_conf_template.h | Enabled USE_FULL_ASSERT for debugging |
| CMakeLists.txt | Changed library type from STATIC to OBJECT |
Comments suppressed due to low confidence (1)
Src/HALAL/Models/DMA/DMA.cpp.tmp:10
- This comment is written in Spanish and contains informal language. Comments in the codebase should be in English for consistency and international collaboration. The comment reads: "So that this code can coexist with the new dma, it has to be forced a little. I (Jorge not pm), believe that the new DMA can work for all peripherals. In case it is not like that, I will change this so it works (it will only be a moment)". This temporary comment should either be removed or properly translated and formalized.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| !compare_pin(sck_pin, PA7)) { | ||
| compile_error("Invalid SCK pin for SPI6"); | ||
| } | ||
| if (!compare_pin(miso_pin, PB4) && | ||
| !compare_pin(miso_pin, PG12) && | ||
| !compare_pin(miso_pin, PA6)) { | ||
| compile_error("Invalid MISO pin for SPI6"); | ||
| } | ||
| if (!compare_pin(mosi_pin, PB5) && | ||
| !compare_pin(mosi_pin, PG14) && | ||
| !compare_pin(mosi_pin, PA7)) { |
There was a problem hiding this comment.
The same pin PA7 is listed as valid for both SCK and MOSI on SPI6. This is likely an error as a single pin cannot serve two different SPI functions simultaneously. According to the STM32H7 datasheet, PA7 is typically an SPI1 MOSI pin, not an SPI6 pin. Please verify the correct pin mappings for SPI6 and update the validation accordingly.
| * HAL drivers code | ||
| */ | ||
| /* #define USE_FULL_ASSERT 1U */ | ||
| #define USE_FULL_ASSERT 1 |
There was a problem hiding this comment.
USE_FULL_ASSERT has been enabled (changed from commented out to defined as 1). This enables parameter assertion checks in the HAL drivers, which adds runtime overhead and code size. While this is useful for debugging, it should typically be disabled in release builds for optimal performance. Consider using conditional compilation based on build type (e.g., only enable in Debug builds) rather than enabling it unconditionally.
| SPI_HandleTypeDef hspi; | ||
| SPI_TypeDef* instance; | ||
|
|
||
| volatile bool* operation_flag; |
There was a problem hiding this comment.
The operation_flag pointer in the Instance struct is not explicitly initialized. When Instance objects are created (as part of the instances array in Init), this pointer will have an indeterminate value. Although the callback checks for nullptr before dereferencing (line 101 in SPI2.cpp), relying on uninitialized memory to happen to be nullptr is undefined behavior. The pointer should be explicitly initialized to nullptr when the Instance is constructed to ensure predictable behavior.
| volatile bool* operation_flag; | |
| volatile bool* operation_flag = nullptr; |
| prescaler *= 2; // Prescaler doubles each step (it must be a power of 2) | ||
|
|
||
| if (prescaler > 256) { | ||
| ErrorHandler("Cannot achieve desired baudrate, speed is too low"); |
There was a problem hiding this comment.
The error message "Cannot achieve desired baudrate, speed is too low" is misleading. The condition that triggers this error is when the prescaler exceeds 256, which actually means the requested baudrate is too LOW (i.e., the source frequency cannot be divided down enough to achieve such a slow speed). The message should clarify that either the requested baudrate is too low for the given source frequency, or provide specific values to help with debugging (e.g., "Cannot achieve baudrate %u Hz with source frequency %u Hz").
| ErrorHandler("Cannot achieve desired baudrate, speed is too low"); | |
| ErrorHandler("Cannot achieve requested baudrate: requested baudrate is too low for source frequency"); |
| uint32_t buffer_length = peripheral->init_data.channels.size(); | ||
| if (HAL_ADC_Start_DMA(peripheral->handle, (uint32_t*) peripheral->dma_data_buffer, buffer_length) != HAL_OK) { | ||
| ErrorHandler("DMA - %d - of ADC - %d - did not start correctly", peripheral->init_data.dma_stream, id); | ||
| ErrorHandler("DMA - %d - of ADC - %d - did not start correctly", id); |
There was a problem hiding this comment.
The error message format is incomplete. It expects a format specifier for a DMA stream parameter (the first %d), but only one argument 'id' is provided. This will cause undefined behavior when the error is triggered. The format string should either be updated to match the number of arguments, or the missing DMA stream identifier should be passed as a second argument.
| ErrorHandler("DMA - %d - of ADC - %d - did not start correctly", id); | |
| ErrorHandler("DMA of ADC instance %d did not start correctly", id); |
| // // void HAL_SPI_TxCpltCallback(SPI_HandleTypeDef* hspi) {} | ||
|
|
||
| // // void HAL_SPI_RxCpltCallback(SPI_HandleTypeDef* hspi) {} | ||
|
|
||
| // // void HAL_SPI_ErrorCallback(SPI_HandleTypeDef* hspi) { | ||
| // // if ((hspi->ErrorCode & HAL_SPI_ERROR_UDR) != 0) { | ||
| // // SPI::spi_recover(SPI::registered_spi_by_handler[hspi], hspi); | ||
| // // } else { | ||
| // // ErrorHandler("SPI error number %u", hspi->ErrorCode); | ||
| // // } | ||
| // // } | ||
|
|
There was a problem hiding this comment.
These lines contain double-commented code (// //). This appears to be an inconsistency where some code has been commented out twice. Either remove these lines entirely or use single-line comments consistently. The extra layer of commenting makes the code harder to read and suggests unclear intention about whether this code should be preserved or deleted.
| // // void HAL_SPI_TxCpltCallback(SPI_HandleTypeDef* hspi) {} | |
| // // void HAL_SPI_RxCpltCallback(SPI_HandleTypeDef* hspi) {} | |
| // // void HAL_SPI_ErrorCallback(SPI_HandleTypeDef* hspi) { | |
| // // if ((hspi->ErrorCode & HAL_SPI_ERROR_UDR) != 0) { | |
| // // SPI::spi_recover(SPI::registered_spi_by_handler[hspi], hspi); | |
| // // } else { | |
| // // ErrorHandler("SPI error number %u", hspi->ErrorCode); | |
| // // } | |
| // // } |
|
|
||
| # ============================ | ||
| # Librería STLIB_LIBRARY | ||
| # ============================ |
There was a problem hiding this comment.
The library type was changed from STATIC to OBJECT without explanation. OBJECT libraries behave differently than STATIC libraries - they don't create an archive file and their object files are directly linked into targets that use them. This change can affect link-time optimization, symbol visibility, and how the library is consumed by other targets. Please document why this change was made and verify it doesn't break existing build configurations or downstream projects that depend on this library.
| # ============================ | |
| # ============================ | |
| # NOTE / NOTA: | |
| # This target is intentionally an OBJECT library instead of STATIC. | |
| # Historically this library was built as STATIC, but it was changed to | |
| # OBJECT so that its object files can be directly reused by multiple | |
| # firmware / executable targets while still being selected via generator | |
| # expressions (CMAKE_CROSSCOMPILING, USE_ETHERNET, etc.). | |
| # | |
| # Compatibility: | |
| # - Downstream projects must link against the CMake target `${STLIB_LIBRARY}` | |
| # (e.g. `target_link_libraries(<app> PRIVATE ${STLIB_LIBRARY})`) and must | |
| # not rely on a `.a` or `.lib` archive file name. | |
| # - Existing build configurations that referenced the CMake target itself | |
| # continue to work: they still see the same sources and compile options, | |
| # only the internal representation changed from a STATIC archive to an | |
| # OBJECT library. | |
| # - No public interface of the library target has been changed by this | |
| # switch; only the way CMake materializes it at link time is different. | |
| # | |
| # If a downstream project requires a real archive file, it should create its | |
| # own STATIC or SHARED target that links against `${STLIB_LIBRARY}` or wraps | |
| # its object files with `$<TARGET_OBJECTS:${STLIB_LIBRARY}>`. |
| auto error_code = HAL_SPI_Transmit(&spi_instance.hspi, (uint8_t*)data.data(), data.size_bytes() / frame_size, 10); | ||
| return check_error_code(error_code); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Sends a trivially copyable data type over SPI in blocking mode. | ||
| */ | ||
| template <typename T> | ||
| bool send(const T& data) requires std::is_trivially_copyable_v<T> { | ||
| if (sizeof(T) % frame_size != 0) { | ||
| ErrorHandler("SPI data type size (%d) not aligned to frame size (%d)", sizeof(T), frame_size); | ||
| return false; | ||
| } | ||
| auto error_code = HAL_SPI_Transmit(&spi_instance.hspi, reinterpret_cast<const uint8_t*>(&data), sizeof(T) / frame_size, 10); | ||
| return check_error_code(error_code); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Receives data over SPI in blocking mode. | ||
| */ | ||
| template <typename E, size_t S> | ||
| bool receive(span<E, S> data) { | ||
| if (data.size_bytes() % frame_size != 0) { | ||
| ErrorHandler("SPI data size (%d) not aligned to frame size (%d)", data.size_bytes(), frame_size); | ||
| return false; | ||
| } | ||
| auto error_code = HAL_SPI_Receive(&spi_instance.hspi, (uint8_t*)data.data(), data.size_bytes() / frame_size, 10); | ||
| return check_error_code(error_code); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Receives a trivially copyable data type over SPI in blocking mode. | ||
| */ | ||
| template <typename T> | ||
| bool receive(T& data) requires std::is_trivially_copyable_v<T> { | ||
| if (sizeof(T) % frame_size != 0) { | ||
| ErrorHandler("SPI data type size (%d) not aligned to frame size (%d)", sizeof(T), frame_size); | ||
| return false; | ||
| } | ||
| auto error_code = HAL_SPI_Receive(&spi_instance.hspi, reinterpret_cast<uint8_t*>(&data), sizeof(T) / frame_size, 10); | ||
| return check_error_code(error_code); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Sends and receives data over SPI in blocking mode. | ||
| */ | ||
| template <typename E1, size_t S1, typename E2, size_t S2> | ||
| bool transceive(span<E1, S1> tx_data, span<E2, S2> rx_data) { | ||
| size_t size = std::min(tx_data.size_bytes(), rx_data.size_bytes()); | ||
| if (size % frame_size != 0) { | ||
| ErrorHandler("SPI transaction size (%d) not aligned to frame size (%d)", size, frame_size); | ||
| return false; | ||
| } | ||
| auto error_code = HAL_SPI_TransmitReceive(&spi_instance.hspi, (uint8_t*)tx_data.data(), (uint8_t*)rx_data.data(), size / frame_size, 10); | ||
| return check_error_code(error_code); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Sends and receives a trivially copyable data type over SPI in blocking mode. | ||
| */ | ||
| template <typename E, size_t S, typename T> | ||
| bool transceive(span<E, S> tx_data, T& rx_data) requires std::is_trivially_copyable_v<T> { | ||
| size_t size = std::min(tx_data.size_bytes(), sizeof(T)); | ||
| if (size % frame_size != 0) { | ||
| ErrorHandler("SPI transaction size (%d) not aligned to frame size (%d)", size, frame_size); | ||
| return false; | ||
| } | ||
| auto error_code = HAL_SPI_TransmitReceive(&spi_instance.hspi, (uint8_t*)tx_data.data(), reinterpret_cast<uint8_t*>(&rx_data), size / frame_size, 10); | ||
| return check_error_code(error_code); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Sends and receives a trivially copyable data type over SPI in blocking mode. | ||
| */ | ||
| template <typename T, typename E, size_t S> | ||
| bool transceive(const T& tx_data, span<E, S> rx_data) requires std::is_trivially_copyable_v<T> { | ||
| size_t size = std::min(sizeof(T), rx_data.size_bytes()); | ||
| if (size % frame_size != 0) { | ||
| ErrorHandler("SPI transaction size (%d) not aligned to frame size (%d)", size, frame_size); | ||
| return false; | ||
| } | ||
| auto error_code = HAL_SPI_TransmitReceive(&spi_instance.hspi, reinterpret_cast<const uint8_t*>(&tx_data), (uint8_t*)rx_data.data(), size / frame_size, 10); | ||
| return check_error_code(error_code); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Sends and receives a trivially copyable data type over SPI in blocking mode. | ||
| */ | ||
| template <typename T1, typename T2> | ||
| bool transceive(const T1& tx_data, T2& rx_data) requires (std::is_trivially_copyable_v<T1> && std::is_trivially_copyable_v<T2>) { | ||
| size_t size = std::min(sizeof(T1), sizeof(T2)); | ||
| if (size % frame_size != 0) { | ||
| ErrorHandler("SPI transaction size (%d) not aligned to frame size (%d)", size, frame_size); | ||
| return false; | ||
| } | ||
| auto error_code = HAL_SPI_TransmitReceive(&spi_instance.hspi, reinterpret_cast<const uint8_t*>(&tx_data), reinterpret_cast<uint8_t*>(&rx_data), size / frame_size, 10); |
There was a problem hiding this comment.
All blocking SPI operations use a hardcoded timeout value of 10 milliseconds. This magic number should be either: (1) defined as a named constant to make it clear what it represents and allow for easy modification, or (2) made configurable per-device to accommodate different use cases. Some SPI operations may require longer timeouts depending on the slave device and data size, while others may need shorter timeouts.
| if (!compare_pin(sck_pin, PB3) && | ||
| !compare_pin(sck_pin, PG11) && | ||
| !compare_pin(sck_pin, PA5)) { | ||
| compile_error("Invalid SCK pin for SPI1"); | ||
| } | ||
| if (!compare_pin(miso_pin, PB4) && | ||
| !compare_pin(miso_pin, PG9) && | ||
| !compare_pin(miso_pin, PA6)) { | ||
| compile_error("Invalid MISO pin for SPI1"); | ||
| } | ||
| if (!compare_pin(mosi_pin, PB5) && | ||
| !compare_pin(mosi_pin, PD7) && | ||
| !compare_pin(mosi_pin, PA7)) { | ||
| compile_error("Invalid MOSI pin for SPI1"); | ||
| } | ||
| break; | ||
|
|
||
| case SPIPeripheral::spi2: | ||
| if (!compare_pin(sck_pin, PD3) && | ||
| !compare_pin(sck_pin, PA12) && | ||
| !compare_pin(sck_pin, PA9) && | ||
| !compare_pin(sck_pin, PB13) && | ||
| !compare_pin(sck_pin, PB10)) { | ||
| compile_error("Invalid SCK pin for SPI2"); | ||
| } | ||
| if (!compare_pin(miso_pin, PC2) && | ||
| !compare_pin(miso_pin, PB14)) { | ||
| compile_error("Invalid MISO pin for SPI2"); | ||
| } | ||
| if (!compare_pin(mosi_pin, PC3) && | ||
| !compare_pin(mosi_pin, PC1) && | ||
| !compare_pin(mosi_pin, PB15)) { | ||
| compile_error("Invalid MOSI pin for SPI2"); | ||
| } | ||
| break; | ||
|
|
||
| case SPIPeripheral::spi3: | ||
| if (!compare_pin(sck_pin, PB3) && | ||
| !compare_pin(sck_pin, PC10)) { | ||
| compile_error("Invalid SCK pin for SPI3"); | ||
| } | ||
| if (!compare_pin(miso_pin, PB4) && | ||
| !compare_pin(miso_pin, PC11)) { | ||
| compile_error("Invalid MISO pin for SPI3"); | ||
| } | ||
| if (!compare_pin(mosi_pin, PB5) && | ||
| !compare_pin(mosi_pin, PD6) && | ||
| !compare_pin(mosi_pin, PC12) && | ||
| !compare_pin(mosi_pin, PB2)) { | ||
| compile_error("Invalid MOSI pin for SPI3"); | ||
| } | ||
| break; | ||
|
|
||
| case SPIPeripheral::spi4: | ||
| if (!compare_pin(sck_pin, PE2) && | ||
| !compare_pin(sck_pin, PE12)) { | ||
| compile_error("Invalid SCK pin for SPI4"); | ||
| } | ||
| if (!compare_pin(miso_pin, PE5) && | ||
| !compare_pin(miso_pin, PE13)) { | ||
| compile_error("Invalid MISO pin for SPI4"); | ||
| } | ||
| if (!compare_pin(mosi_pin, PE6) && | ||
| !compare_pin(mosi_pin, PE14)) { | ||
| compile_error("Invalid MOSI pin for SPI4"); | ||
| } | ||
| break; | ||
|
|
||
| case SPIPeripheral::spi5: | ||
| if (!compare_pin(sck_pin, PF7)) { | ||
| compile_error("Invalid SCK pin for SPI5"); | ||
| } | ||
| if (!compare_pin(miso_pin, PF8)) { | ||
| compile_error("Invalid MISO pin for SPI5"); | ||
| } | ||
| if (!compare_pin(mosi_pin, PF9) && | ||
| !compare_pin(mosi_pin, PF11)) { | ||
| compile_error("Invalid MOSI pin for SPI5"); | ||
| } | ||
| break; | ||
|
|
||
| case SPIPeripheral::spi6: | ||
| if (!compare_pin(sck_pin, PB3) && | ||
| !compare_pin(sck_pin, PG13) && | ||
| !compare_pin(sck_pin, PC10) && | ||
| !compare_pin(sck_pin, PA7)) { | ||
| compile_error("Invalid SCK pin for SPI6"); | ||
| } | ||
| if (!compare_pin(miso_pin, PB4) && | ||
| !compare_pin(miso_pin, PG12) && | ||
| !compare_pin(miso_pin, PA6)) { | ||
| compile_error("Invalid MISO pin for SPI6"); | ||
| } | ||
| if (!compare_pin(mosi_pin, PB5) && | ||
| !compare_pin(mosi_pin, PG14) && | ||
| !compare_pin(mosi_pin, PA7)) { | ||
| compile_error("Invalid MOSI pin for SPI6"); | ||
| } |
There was a problem hiding this comment.
Pin PC10 is listed as valid for both SPI3 (line 304) and SPI6 (line 350) SCK pins. Additionally, PB3 is listed for SPI1, SPI3, and SPI6 SCK, and PB4 is listed for SPI1, SPI3, and SPI6 MISO. While it's possible for pins to support multiple peripherals through alternate functions, this should be verified against the STM32H7 datasheet. If these are indeed valid, users need to be careful not to configure multiple SPI peripherals with overlapping pins simultaneously.
| prescaler *= 2; // Prescaler doubles each step (it must be a power of 2) | ||
|
|
||
| if (prescaler > 256) { | ||
| ErrorHandler("Cannot achieve desired baudrate, speed is too low"); |
There was a problem hiding this comment.
After calling ErrorHandler when prescaler exceeds 256, the function continues execution and attempts to call get_prescaler_flag with an invalid prescaler value (>256). The function should either return a default prescaler value or use a more severe error handling mechanism that doesn't return. Consider adding 'return SPI_BAUDRATEPRESCALER_256;' after the ErrorHandler call to provide a fallback behavior.
| ErrorHandler("Cannot achieve desired baudrate, speed is too low"); | |
| ErrorHandler("Cannot achieve desired baudrate, speed is too low"); | |
| return SPI_BAUDRATEPRESCALER_256; |
Refactor SPI to the new compile infrastructure.
It is a refactor of the base driver, it discards all the logic that was built on top of the driver and mixed with it in the previous implementation.
Uses the new DMA (will update the code a little when it's final version comes arround).
Allows any desired spi config you may need.
If you want a protocol on top of it, you have to implement it (it is out of the scope for this pr, I'm planning on doing a general protocol class that's a mixin and can use any driver, format and operations). But it is pretty easy to use with packets (will add a better explanation in the wiki later):
It uses a different api for master and slave to make clear the difference in behaviour (master instantly start the operation, while slave prepares it but must wait for the master)