added changes to max7456.c to add setting speed and removing spi_mod_0 flag with defines from target.h file#11396
Open
DariaOrqa wants to merge 2 commits intoiNavFlight:masterfrom
Open
Conversation
Branch Targeting SuggestionYou've targeted the
If This is an automated suggestion to help route contributions to the appropriate branch. |
Contributor
Review Summary by QodoAdd configurable SPI speed and manual mode override for MAX7456
WalkthroughsDescription• Add configurable SPI speed support via MAX7456_SPI_SPEED define • Implement manual SPI mode configuration for STM32H7 targets • Replace hardcoded BUS_SPEED_STANDARD with dynamic speed setting • Support custom hardware configurations without modifying common_hardware Diagramflowchart LR
A["max7456Init"] --> B["max7456ApplyBusSpeed"]
A --> C["max7456SpiModeOverride"]
B --> D["MAX7456_SPI_SPEED define"]
B --> E["BUS_SPEED_STANDARD fallback"]
C --> F["STM32H7 + MAX7456_MANUAL_SPI_CONFIG"]
C --> G["SPI CR1/CFG2 register config"]
File Changes1. src/main/drivers/max7456.c
|
Contributor
Code Review by Qodo
1. H7 SPI enabled at idle
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+315
to
+319
| maxSpiInstance->CR1 &= ~SPI_CR1_SPE; | ||
| maxSpiInstance->CFG2 &= ~(SPI_CFG2_CPHA | SPI_CFG2_CPOL); | ||
| maxSpiInstance->CFG2 |= (SPI_CFG2_CPHA | SPI_CFG2_CPOL); | ||
| maxSpiInstance->CR1 |= SPI_CR1_SPE; | ||
| #endif |
Contributor
There was a problem hiding this comment.
1. H7 spi enabled at idle 🐞 Bug ✓ Correctness
max7456SpiModeOverride() disables SPI, tweaks mode bits, then re-enables SPI on STM32H7. The STM32H7 SPI LL implementation enables/disables SPI per-transfer and sets transfer size before enabling; leaving SPI enabled can violate those assumptions and lead to misconfigured or stuck transfers.
Agent Prompt
## Issue description
On STM32H7, `max7456SpiModeOverride()` re-enables the SPI peripheral (sets `SPE=1`). The existing STM32H7 SPI transfer implementation expects SPI to be disabled when idle and enables/disables it per transfer, setting transfer size before enabling. Leaving SPI enabled can break the expected sequencing.
## Issue Context
- The H7 LL SPI transfer path (`spiTransfer`) does `SetTransferSize()` then `Enable()` and later `Disable()`.
- The PR code currently ends by setting `SPE` back to 1.
## Fix Focus Areas
- src/main/drivers/max7456.c[306-320]
- src/main/drivers/max7456.c[412-417]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+316
to
+318
| maxSpiInstance->CFG2 &= ~(SPI_CFG2_CPHA | SPI_CFG2_CPOL); | ||
| maxSpiInstance->CFG2 |= (SPI_CFG2_CPHA | SPI_CFG2_CPOL); | ||
| maxSpiInstance->CR1 |= SPI_CR1_SPE; |
Contributor
There was a problem hiding this comment.
2. Mode override forces mode3 🐞 Bug ✓ Correctness
The manual override hard-sets CPOL+CPHA (MODE3) regardless of the bus-device configuration. This can silently override targets that register MAX7456 with DEVFLAGS_SPI_MODE_0 (MODE0), or be redundant when DEVFLAGS_SPI_MODE_0 is omitted (since the bus layer already defaults to MODE3).
Agent Prompt
## Issue description
`max7456SpiModeOverride()` currently forces CPOL+CPHA (SPI MODE3) unconditionally. This can conflict with the bus-device configuration where MAX7456 is commonly registered as MODE0 (`DEVFLAGS_SPI_MODE_0`). It can also be redundant if `DEVFLAGS_SPI_MODE_0` is omitted because the bus layer already defaults to MODE3.
## Issue Context
- `DEVFLAGS_SPI_MODE_0` explicitly maps to MODE0; when unset, MODE3 is used by default.
- The override ignores `state.dev->flags` and always writes MODE3 bits.
## Fix Focus Areas
- src/main/drivers/max7456.c[306-320]
- src/main/drivers/bus.h[152-160]
- src/main/drivers/bus_busdev_spi.c[44-48]
- src/main/target/common_hardware.c[411-413]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
|
Thanks for submitting this! You mentioned "specific STM32H7-based targets" -- any that are currently publicly available? Or is this new hardware you're working on? |
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
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 PR introduces the ability to override default SPI parameters for the MAX7456 OSD chip. These changes are necessary to support specific STM32H7-based targets and custom hardware configurations where the standard DEVFLAGS_SPI_MODE_0 or BUS_SPEED_STANDARD are insufficient or incompatible with the hardware routing.