SPI bus driver #128
Open
SPI bus driver #128
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Tejasgarg, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
bessman
reviewed
Oct 23, 2025
Reviewer's GuideThis PR introduces a basic SPI bus driver by adding low-level STM32H5 SPI code, exposing a high-level SPI_Handle API, configuring clocks and build files, and integrating an example usage in the application. Sequence diagram for SPI transmit and receive in main applicationsequenceDiagram
participant App as Application
participant SPI as SPI_Handle
participant LL as SPI_LL
participant HW as STM32 SPI Peripheral
App->>SPI: SPI_init(0)
SPI->>LL: SPI_LL_init(bus)
LL->>HW: Configure SPI peripheral
App->>SPI: SPI_transmit_receive(spi_handle, &tx_data, rx_data, size)
SPI->>LL: SPI_LL_transmit_receive(bus, tx_data, rx_data, size)
LL->>HW: HAL_SPI_TransmitReceive(...)
HW-->>LL: Data received
LL-->>SPI: Return received data
SPI-->>App: Return received data
Class diagram for new SPI bus driver APIclassDiagram
class SPI_Handle {
+SPI_Bus bus_id
+bool initialized
}
class SPI_Bus {
}
SPI_Handle -- SPI_Bus
SPI_Handle <|.. SPI_init
SPI_Handle <|.. SPI_deinit
SPI_Handle <|.. SPI_transmit
SPI_Handle <|.. SPI_receive
SPI_Handle <|.. SPI_transmit_receive
class SPI_init {
+SPI_Handle *SPI_init(size_t bus)
}
class SPI_deinit {
+void SPI_deinit(SPI_Handle *handle)
}
class SPI_transmit {
+void SPI_transmit(SPI_Handle *handle, uint8_t const *txbuf, size_t sz)
}
class SPI_receive {
+void SPI_receive(SPI_Handle *handle, uint8_t *rxbuf, size_t sz)
}
class SPI_transmit_receive {
+void SPI_transmit_receive(SPI_Handle *handle, uint8_t const *tx_data, uint8_t *rx_data, size_t size)
}
Class diagram for SPI low-level hardware interfaceclassDiagram
class SPIInstance {
+SPI_HandleTypeDef *hspi
+bool initialized
}
class SPI_Bus {
}
SPIInstance -- SPI_Bus
SPIInstance <|.. SPI_LL_init
SPIInstance <|.. SPI_LL_deinit
SPIInstance <|.. SPI_LL_transmit
SPIInstance <|.. SPI_LL_receive
SPIInstance <|.. SPI_LL_transmit_receive
class SPI_LL_init {
+void SPI_LL_init(SPI_Bus bus)
}
class SPI_LL_deinit {
+void SPI_LL_deinit(SPI_Bus bus)
}
class SPI_LL_transmit {
+void SPI_LL_transmit(SPI_Bus bus, uint8_t const *txdata, size_t size)
}
class SPI_LL_receive {
+void SPI_LL_receive(SPI_Bus bus, uint8_t *rxdata, size_t size)
}
class SPI_LL_transmit_receive {
+void SPI_LL_transmit_receive(SPI_Bus bus, uint8_t const *tx_data, uint8_t *rx_data, size_t size)
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In SPI_init you dereference g_spi_instances[bus_id] without checking for NULL (it starts out as nullptr), so the first init will crash—update the logic to check the pointer before deref and reset the slot on deinit.
- The bus enum and HAL_SPI_MspInit comment refer to SPI1, but the code enables/configures SPI3—align the instance mapping, names, and comments to the same peripheral to avoid confusion.
- It looks like src/system/bus/spi.c isn’t added to your CMakeLists.target.txt, so the new high-level SPI module won’t be built—please include it in the build configuration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In SPI_init you dereference g_spi_instances[bus_id] without checking for NULL (it starts out as nullptr), so the first init will crash—update the logic to check the pointer before deref and reset the slot on deinit.
- The bus enum and HAL_SPI_MspInit comment refer to SPI1, but the code enables/configures SPI3—align the instance mapping, names, and comments to the same peripheral to avoid confusion.
- It looks like src/system/bus/spi.c isn’t added to your CMakeLists.target.txt, so the new high-level SPI module won’t be built—please include it in the build configuration.
## Individual Comments
### Comment 1
<location> `src/system/bus/spi.c:25` </location>
<code_context>
+ bool initialized;
+};
+
+static SPI_Handle *g_spi_instances[SPI_BUS_COUNT] = { nullptr };
+
+/**
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential null pointer dereference in SPI_init.
g_spi_instances[bus_id] may be nullptr on first use, leading to a crash when accessing its members. Add a nullptr check before referencing g_spi_instances[bus_id]->initialized.
</issue_to_address>
### Comment 2
<location> `src/platform/h563xx/spi_ll.c:35` </location>
<code_context>
+} SPIInstance;
+
+/* HAL SPI handle */
+static SPI_HandleTypeDef g_hspi1 = { nullptr };
+
+/* SPI instance configuration */
</code_context>
<issue_to_address>
**issue (bug_risk):** Use of 'nullptr' in C source file.
Replace 'nullptr' with 'NULL' to ensure compatibility with C compilers.
</issue_to_address>
### Comment 3
<location> `src/platform/h563xx/spi_ll.c:53` </location>
<code_context>
+{
+ GPIO_InitTypeDef gpio_init = { 0 };
+
+ if (hspi->Instance == SPI3) {
+ /* SPI3 clock enable */
+ __HAL_RCC_SPI3_CLK_ENABLE();
</code_context>
<issue_to_address>
**nitpick (typo):** GPIO configuration comment refers to SPI1 instead of SPI3.
Please change the comment to 'SPI3 GPIO Configuration' to match the code.
```suggestion
/* SPI3 GPIO Configuration: PC10=SCK, PC11=MISO, PC12=MOSI */
```
</issue_to_address>
### Comment 4
<location> `src/application/main.c:22-19` </location>
<code_context>
}
+ // Initialize the SPI bus
+ SPI_Handle *spi_handle = SPI_init(0);
+ if (spi_handle == NULL) {
+ LOG_ERROR("Failed to initialize SPI bus");
+ }
+
// Main application loop
</code_context>
<issue_to_address>
**issue (bug_risk):** SPI bus initialization error is logged but not handled.
Please ensure the function exits or handles the error when SPI_init returns NULL to prevent invalid SPI operations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (!protocol_init()) { | ||
| LOG_ERROR("Failed to initialize protocol"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): SPI bus initialization error is logged but not handled.
Please ensure the function exits or handles the error when SPI_init returns NULL to prevent invalid SPI operations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a work in progress.
Currently, I have implemented the basic low-level driver for SPI bus, that implements two SPI buses (SPI1 and SPI2)
I will continue working on this to implement the complete working of the SPI bus module
Summary by Sourcery
Add basic SPI bus driver support by providing hardware abstraction, system-level APIs, platform configuration, and example usage in main
New Features:
Enhancements:
Build: