added changes to max7456.c to add setting speed and removing spi_mod_0 flag with defines from target.h file by DariaOrqa · Pull Request #11396 · iNavFlight/inav

DariaOrqa · 2026-03-04T09:22:33Z

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.

…_0 flag from target.h file

github-actions · 2026-03-04T09:22:44Z

Branch Targeting Suggestion

You've targeted the master branch with this PR. Please consider if a version branch might be more appropriate:

maintenance-9.x - If your change is backward-compatible and won't create compatibility issues between INAV firmware and Configurator 9.x versions. This will allow your PR to be included in the next 9.x release.
maintenance-10.x - If your change introduces compatibility requirements between firmware and configurator that would break 9.x compatibility. This is for PRs which will be included in INAV 10.x

If master is the correct target for this change, no action is needed.

This is an automated suggestion to help route contributions to the appropriate branch.

qodo-code-review · 2026-03-04T09:23:24Z

Review Summary by Qodo

Add configurable SPI speed and manual mode override for MAX7456

✨ Enhancement

Walkthroughs

Description

• 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

Diagram

flowchart 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 Changes

1. src/main/drivers/max7456.c ✨ Enhancement +28/-1

Add SPI speed and mode configuration functions

• Added max7456ApplyBusSpeed() function to support configurable SPI speed via MAX7456_SPI_SPEED
 define with fallback to BUS_SPEED_STANDARD
• Added max7456SpiModeOverride() function for STM32H7 targets to manually configure SPI mode when
 MAX7456_MANUAL_SPI_CONFIG is defined
• Replaced hardcoded busSetSpeed() call in max7456Init() with call to new
 max7456ApplyBusSpeed() function
• Both new functions are marked as static and called during initialization

src/main/drivers/max7456.c

qodo-code-review · 2026-03-04T09:23:25Z

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. ~~H7 SPI enabled at idle~~ ☑ 🐞 Bug ✓ Correctness

Description

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.

Code

src/main/drivers/max7456.c[R315-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

Evidence
The PR code explicitly sets SPE back to 1. But the STM32H7 SPI transfer routine sets transfer size,
then enables SPI, then disables SPI at end—indicating the expected idle state is disabled.
Re-enabling SPI early can make per-transfer setup run while already enabled.
src/main/drivers/max7456.c[315-319]
src/main/drivers/bus_spi_hal_ll.c[325-358]
src/main/drivers/bus_spi_hal_ll.c[279-285]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

2. Mode override forces MODE3 🐞 Bug ✓ Correctness

Description

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).

Code

src/main/drivers/max7456.c[R316-318]

+    maxSpiInstance->CFG2 &= ~(SPI_CFG2_CPHA | SPI_CFG2_CPOL);
+    maxSpiInstance->CFG2 |= (SPI_CFG2_CPHA | SPI_CFG2_CPOL);
+    maxSpiInstance->CR1 |= SPI_CR1_SPE;

Evidence
The bus framework defines DEVFLAGS_SPI_MODE_0 as CPOL=0/CPHA=0 and explicitly states that when it’s
unset, MODE3 is used. Common hardware registers MAX7456 with DEVFLAGS_SPI_MODE_0, but the new
override always sets CPOL+CPHA (MODE3), ignoring flags.
src/main/drivers/max7456.c[316-318]
src/main/drivers/bus.h[156-159]
src/main/target/common_hardware.c[411-413]
src/main/drivers/bus_busdev_spi.c[44-48]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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-&amp;gt;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

3. ~~Speed macro not validated~~ ☑ 🐞 Bug ⛯ Reliability

Description

MAX7456_SPI_SPEED is passed directly to busSetSpeed() as a busSpeed_e without validation. If a
target defines it as an out-of-range integer, spiBusSetSpeed() will index a fixed array by that
value, causing undefined behavior.

Code

src/main/drivers/max7456.c[R298-303]

+#if defined(MAX7456_SPI_SPEED)
+    busSetSpeed(state.dev, MAX7456_SPI_SPEED);
+#else
+    // Default safe speed for MAX7456
+    busSetSpeed(state.dev, BUS_SPEED_STANDARD);
+#endif

Evidence

busSpeed_e is defined as a small enum (0..4). spiBusSetSpeed maps this enum to a clock via an array
indexed by speed with no general bounds check (only optional BUS_SPI_SPEED_MAX). Passing an
invalid MAX7456_SPI_SPEED value can therefore index out of bounds.

src/main/drivers/max7456.c[298-303]
src/main/drivers/bus.h[50-56]
src/main/drivers/bus_busdev_spi.c[62-78]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`MAX7456_SPI_SPEED` is used as a `busSpeed_e` but not validated. An out-of-range macro value can cause out-of-bounds indexing in `spiBusSetSpeed()`.
## Issue Context
- `busSpeed_e` valid values are 0..4.
- `spiBusSetSpeed()` maps via `spiClock[speed]`.
## Fix Focus Areas
- src/main/drivers/max7456.c[296-304]
- src/main/drivers/bus_busdev_spi.c[62-78]
- src/main/drivers/bus.h[50-56]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ⓘ The new review experience is currently in Beta. Learn more

src/main/drivers/max7456.c

qodo-code-review · 2026-03-04T09:27:55Z

src/main/drivers/max7456.c

+    maxSpiInstance->CFG2 &= ~(SPI_CFG2_CPHA | SPI_CFG2_CPOL);
+    maxSpiInstance->CFG2 |= (SPI_CFG2_CPHA | SPI_CFG2_CPOL);
+    maxSpiInstance->CR1 |= SPI_CR1_SPE;


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

sensei-hacker · 2026-03-04T15:48:13Z

Thanks for submitting this!
The suggestions from the Qodo bot are sometimes accurate, sometimes not. Please check them and see what you think.

You mentioned "specific STM32H7-based targets" -- any that are currently publicly available? Or is this new hardware you're working on?

DariaOrqa · 2026-03-05T13:13:30Z

Thanks for the review!
Regarding the hardware: these are two new H7-based targets we are finalizing for a PR soon.

I have updated the code to address the 2 Qodo bot's points: SPI state and validation.

Regarding the Mode 3 override:
In common_hardware.c, the MAX7456 is currently hardcoded to register with DEVFLAGS_SPI_MODE_0. However, our specific H7 hardware requires Mode 3 to function correctly.
I chose to implement this as a driver rather than modifying common_hardware.c because that way it would not affect all existing targets.

Without this override, the OSD simply does not initialize on these H7 targets because the bus remains stuck in the default Mode 0 configuration.

DariaOrqa added 2 commits March 4, 2026 09:38

added changes to max7456.c to add setting speed and removing spi_mode…

3b183b3

…_0 flag from target.h file

changed spacing and added static to function

ada5eb9

qodo-code-review bot reviewed Mar 4, 2026

View reviewed changes

added bounds check for speed and fixed spi re-enable issue

a294302

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added changes to max7456.c to add setting speed and removing spi_mod_0 flag with defines from target.h file#11396

added changes to max7456.c to add setting speed and removing spi_mod_0 flag with defines from target.h file#11396
DariaOrqa wants to merge 3 commits intoiNavFlight:masterfrom
orqafpv:max7456-add-custom-defines

DariaOrqa commented Mar 4, 2026

Uh oh!

github-actions bot commented Mar 4, 2026

Uh oh!

qodo-code-review bot commented Mar 4, 2026

Uh oh!

qodo-code-review bot commented Mar 4, 2026 •

edited

Loading

Uh oh!

Uh oh!

qodo-code-review bot Mar 4, 2026

Uh oh!

sensei-hacker commented Mar 4, 2026

Uh oh!

DariaOrqa commented Mar 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

DariaOrqa commented Mar 4, 2026

Uh oh!

github-actions bot commented Mar 4, 2026

Branch Targeting Suggestion

Uh oh!

qodo-code-review bot commented Mar 4, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review bot commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

qodo-code-review bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

sensei-hacker commented Mar 4, 2026

Uh oh!

DariaOrqa commented Mar 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qodo-code-review bot commented Mar 4, 2026 •

edited

Loading