Hellbender 0001 SDcard #865
Conversation
WalkthroughIntroduce QUADSPI external flash support and related macros for RP2350, set DEFAULT_BLACKBOX_DEVICE to SDCARD, enable USE_FLASH and QUADSPI device macros, and move SD-card CS wiring into a conditional that excludes it when USE_MAX7456 is enabled; add MAX7456 CS pin definition. Changes
Sequence Diagram(s)(Skipped — changes are configuration-level macros and conditional wiring; no runtime control-flow to diagram.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
configs/HELLBENDER_0001/config.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-08T22:26:29.906Z
Learnt from: haslinghuis
PR: betaflight/config#829
File: configs/HELLBENDER_0001/config.h:53-56
Timestamp: 2025-07-08T22:26:29.906Z
Learning: In Betaflight configurations, SDCARD_CS_PIN and FLASH_CS_PIN can share the same GPIO pin because Betaflight uses either flash or sdcard storage, not both simultaneously. This is a mutually exclusive design where only one storage method is active at a time, so shared CS pin assignments are intentional and do not cause conflicts.
Applied to files:
configs/HELLBENDER_0001/config.h
🔇 Additional comments (3)
configs/HELLBENDER_0001/config.h (3)
53-54: OSD/SD mutual exclusion: gating approach is correctKeeping USE_MAX7456 commented out by default and gating SD support elsewhere on !USE_MAX7456 is a clean way to ensure the two features remain mutually exclusive on SPI1.
69-72: LGTM: QSPI CS1 wiring and size macroThe RP2350 QMI CS1 GPIO and size declaration look correct for an 8 MiB external flash. Keep this in sync with the chip macro above.
73-79: LGTM: SD card enabled only when MAX7456 is not usedGating SD definitions on !USE_MAX7456 cleanly prevents SPI1 contention between OSD and SDCARD.
| #define DEFAULT_BLACKBOX_DEVICE BLACKBOX_DEVICE_SDCARD | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make DEFAULT_BLACKBOX_DEVICE conditional on OSD/SD availability
As written, if a user enables USE_MAX7456, SD card support is compiled out, yet the default blackbox device would still be SDCARD. That’s likely to produce a build-time config error or an unusable default. Define the default based on whether OSD disables SD (fallback to FLASH when SD is disabled).
Apply this diff to make the default adaptive:
-#define DEFAULT_BLACKBOX_DEVICE BLACKBOX_DEVICE_SDCARD
+#ifndef USE_MAX7456
+#define DEFAULT_BLACKBOX_DEVICE BLACKBOX_DEVICE_SDCARD
+#else
+#define DEFAULT_BLACKBOX_DEVICE BLACKBOX_DEVICE_FLASH
+#endifRun this repo-wide check to confirm which blackbox device constants are used across configs (and adjust the FLASH constant if your tree uses a different identifier):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Defaults used across configs (context shown):"
rg -n -C2 '^\s*#\s*define\s+DEFAULT_BLACKBOX_DEVICE\b'
echo
echo "All BLACKBOX_DEVICE_* identifiers referenced in configs (deduped):"
rg -nP 'BLACKBOX_DEVICE_[A-Z_]+' | gawk -F: '{print $3}' | sed -E 's/.*\b(BLACKBOX_DEVICE_[A-Z_]+)\b.*/\1/' | sort -uLength of output: 178056
🏁 Script executed:
#!/bin/bash
sed -n '1,80p' configs/HELLBENDER_0001/config.hLength of output: 2268
Wrap DEFAULT_BLACKBOX_DEVICE to respect USE_MAX7456
The board currently always defaults to BLACKBOX_DEVICE_SDCARD, but if USE_MAX7456 is enabled SD support is disabled, leading to a broken or unusable build. Change the default to fall back to flash when Max7456 OSD is in use:
configs/HELLBENDER_0001/config.h (around line 57)
-#define DEFAULT_BLACKBOX_DEVICE BLACKBOX_DEVICE_SDCARD
+#ifndef USE_MAX7456
+#define DEFAULT_BLACKBOX_DEVICE BLACKBOX_DEVICE_SDCARD
+#else
+#define DEFAULT_BLACKBOX_DEVICE BLACKBOX_DEVICE_FLASH
+#endif– Only BLACKBOX_DEVICE_SDCARD and BLACKBOX_DEVICE_FLASH exist in the tree, so this will cover both cases.
– Place the new conditional block immediately after the MAX7456_SPI_CS_PIN definition.
– Ensures a valid default regardless of OSD/SD compile-time flags.
🤖 Prompt for AI Agents
In configs/HELLBENDER_0001/config.h around lines 57-58, the
DEFAULT_BLACKBOX_DEVICE is unconditionally set to BLACKBOX_DEVICE_SDCARD which
breaks builds when USE_MAX7456 is enabled; replace that single define with a
conditional block placed immediately after the MAX7456_SPI_CS_PIN definition
that sets DEFAULT_BLACKBOX_DEVICE to BLACKBOX_DEVICE_FLASH when USE_MAX7456 is
defined and to BLACKBOX_DEVICE_SDCARD otherwise (only these two devices exist
in-tree), ensuring a valid default regardless of OSD/SD compile-time flags.
configs/HELLBENDER_0001/config.h
Outdated
| #define FLASH_CS_PIN PA0 | ||
| #define USE_FLASH_CHIP | ||
| #define USE_FLASH_W25Q128FV | ||
| #define USE_FLASHFS | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Flash chip identity/size mismatch and redundant CS pin under QUADSPI
Two issues to resolve:
- You enable QUADSPI below, so FLASH_CS_PIN (SPI CS) is not used on RP2350 QMI. Keeping it defined can confuse future maintenance or even collide with GPIO usage.
- The comment at Lines 69-72 states W25Q64 (8 MiB), but the macro enables W25Q128 (16 MiB). Align the chip macro with the actual fitted part, or adjust the size to 16 MiB.
Recommended change 1 (avoid stale SPI-CS definition when using QSPI). Pick one:
Option A — remove it:
-#define FLASH_CS_PIN PA0Option B — guard it so it only applies when not using QUADSPI:
-#define FLASH_CS_PIN PA0
+#ifndef USE_QUADSPI
+#define FLASH_CS_PIN PA0
+#endifRecommended change 2 (align capacity with the actual chip). Choose one coherent path:
Path 1 — If the board really has W25Q64 (8 MiB), switch the chip macro accordingly and keep 8 MiB:
-#define USE_FLASH_W25Q128FV
+#define USE_FLASH_W25Q64JVNote: Adjust the macro name to match what your Betaflight tree supports (JV/FV, etc.). See the verification script below.
Path 2 — If the board has 16 MiB fitted, keep W25Q128 and update the size at Lines 69-72:
-#define PICO_QSPI_CS1_SIZE_BYTES (8 * 1024 * 1024)
+#define PICO_QSPI_CS1_SIZE_BYTES (16 * 1024 * 1024)To confirm which macros and patterns are used in this repo (and avoid typos), run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check usage of SDCARD CS pin macro variants:"
rg -nP '\bSDCARD_(SPI_)?CS_PIN\b' -C2 || true
echo
echo "Check common flash chip macros referenced by configs:"
rg -nP 'USE_FLASH_W25Q(64|128)[A-Z]*' -C2 || trueLength of output: 96966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking for QUADSPI usage ==="
rg -nP '\bUSE_QUADSPI\b' -C2 configs/HELLBENDER_0001/config.h || true
echo
echo "=== Context: Lines 50–90 of config.h ==="
nl -ba configs/HELLBENDER_0001/config.h | sed -n '50,90p'Length of output: 614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Lines 60–80 of HELLBENDER_0001/config.h ==="
sed -n '60,80p' configs/HELLBENDER_0001/config.hLength of output: 790
Remove stale SPI‐CS definition & correct flash capacity
Two mandatory fixes needed in configs/HELLBENDER_0001/config.h:
• At the top (line 59), FLASH_CS_PIN is no longer used with QSPI. Remove it or wrap it so it only applies when SPI is in use:
-#define FLASH_CS_PIN PA0—or—
-#define FLASH_CS_PIN PA0
+#ifndef USE_QUADSPI
+#define FLASH_CS_PIN PA0
+#endif• The board’s comment indicates a Winbond W25Q64 (8 MiB) but the config enables W25Q128FV (16 MiB). Choose one path and apply both changes below:
- If the board is fitted with an 8 MiB W25Q64JV:
-#define USE_FLASH_W25Q128FV
+#define USE_FLASH_W25Q64JV(leave PICO_QSPI_CS1_SIZE_BYTES (8 * 1024 * 1024) as is)
- If it actually has a 16 MiB W25Q128FV:
-#define PICO_QSPI_CS1_SIZE_BYTES (8 * 1024 * 1024)
+#define PICO_QSPI_CS1_SIZE_BYTES (16 * 1024 * 1024)Confirm the correct macro name (USE_FLASH_W25Q64JV vs. what’s supported in the tree) and update accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In configs/HELLBENDER_0001/config.h around lines 59–63, remove the stale
FLASH_CS_PIN definition or wrap it with the SPI-only guard so it is only defined
when SPI mode is used (QSPI boards must not define an SPI CS pin); also
reconcile the flash part/capacity mismatch by choosing the correct chip macro
and size: if the board has an 8 MiB Winbond W25Q64, replace the W25Q128FV macro
with the supported W25Q64 macro (e.g. USE_FLASH_W25Q64JV) and keep
PICO_QSPI_CS1_SIZE_BYTES at 8 * 1024 * 1024; if the board actually has a 16 MiB
W25Q128FV, ensure the correct supported macro name is used and update
PICO_QSPI_CS1_SIZE_BYTES to 16 * 1024 * 1024; verify the macro name exists in
the tree and update both the chip macro and size consistently.
| // Enable QUADSPI on RP2350 (pins are fixed via QMI; platform code handles them) | ||
| #define USE_QUADSPI | ||
| #define USE_QUADSPI_DEVICE_1 | ||
| #define FLASH_QUADSPI_INSTANCE ((QUADSPI_TypeDef *)0x1) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
QUADSPI instance macro appears STM32-specific; likely invalid on RP2350
FLASH_QUADSPI_INSTANCE cast to QUADSPI_TypeDef is a HAL/STM32 idiom and does not apply to RP2350. RP2350 QMI is handled by the platform code with PICO_QSPI_* defines. Retain USE_QUADSPI, but drop the instance and device-id defines unless your Betaflight RP2350 branch explicitly uses them.
Apply this diff to remove the non-RP constructs:
// Enable QUADSPI on RP2350 (pins are fixed via QMI; platform code handles them)
#define USE_QUADSPI
-#define USE_QUADSPI_DEVICE_1
-#define FLASH_QUADSPI_INSTANCE ((QUADSPI_TypeDef *)0x1)Sanity-check whether any other configs use these macros (they likely won’t on RP targets):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for FLASH_QUADSPI_INSTANCE usage in configs:"
rg -n 'FLASH_QUADSPI_INSTANCE' -C2 || true
echo
echo "Search for USE_QUADSPI_DEVICE_* usage in configs:"
rg -nP 'USE_QUADSPI_DEVICE_\d+' -C2 || trueLength of output: 3000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for USE_QUADSPI_DEVICE_1 usage outside configs:"
rg -n 'USE_QUADSPI_DEVICE_1' --glob '!configs/**' -C2 || true
echo
echo "Searching for FLASH_QUADSPI_INSTANCE usage outside configs:"
rg -n 'FLASH_QUADSPI_INSTANCE' --glob '!configs/**' -C2 || true
echo
echo "Verifying conditional compilation around USE_QUADSPI in source files:"
rg -n '#\s*if.*USE_QUADSPI' --glob '!configs/**' -C2 || trueLength of output: 624
Remove STM32-specific QUADSPI macros in RP2350 config
RP2350’s QSPI is driven by QMI (PICO_QSPI_*) and doesn’t reference USE_QUADSPI_DEVICE_1 or FLASH_QUADSPI_INSTANCE. These defines only appear in configs and aren’t used elsewhere—drop them to avoid invalid HAL/STM32 casts.
• File: configs/HELLBENDER_0001/config.h Lines 66–67
Remove:
// Enable QUADSPI on RP2350 (pins are fixed via QMI; platform code handles them)
#define USE_QUADSPI
-#define USE_QUADSPI_DEVICE_1
-#define FLASH_QUADSPI_INSTANCE ((QUADSPI_TypeDef *)0x1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Enable QUADSPI on RP2350 (pins are fixed via QMI; platform code handles them) | |
| #define USE_QUADSPI | |
| #define USE_QUADSPI_DEVICE_1 | |
| #define FLASH_QUADSPI_INSTANCE ((QUADSPI_TypeDef *)0x1) | |
| // Enable QUADSPI on RP2350 (pins are fixed via QMI; platform code handles them) | |
| #define USE_QUADSPI |
🤖 Prompt for AI Agents
In configs/HELLBENDER_0001/config.h around lines 64–68, remove the two
STM32-specific QUADSPI macro lines (the #define USE_QUADSPI_DEVICE_1 and the
#define FLASH_QUADSPI_INSTANCE ...) and keep only the generic USE_QUADSPI
define; delete those lines so the file relies on the RP2350/QMI PICO_QSPI_*
configuration instead, and then rebuild to ensure no remaining references to
those removed macros exist.
|
@SteveCEvans just need to rebase and then I will merge. |
|
I’ll attend to the code rabbit review comments first |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
configs/HELLBENDER_0001/config.h (2)
57-57: Make DEFAULT_BLACKBOX_DEVICE follow the OSD/SD mutual exclusionIf a user enables USE_MAX7456, the SD-card block is compiled out but the default still points to SDCARD. Switch the default based on USE_MAX7456 to avoid an unusable/broken default.
Apply this diff:
-#define DEFAULT_BLACKBOX_DEVICE BLACKBOX_DEVICE_SDCARD +#ifndef USE_MAX7456 +#define DEFAULT_BLACKBOX_DEVICE BLACKBOX_DEVICE_SDCARD +#else +#define DEFAULT_BLACKBOX_DEVICE BLACKBOX_DEVICE_FLASH +#endifRun to confirm identifiers used across the tree:
#!/bin/bash set -euo pipefail echo "Defaults used across configs (context):" rg -n -C1 '^\s*#\s*define\s+DEFAULT_BLACKBOX_DEVICE\b' configs || true echo echo "BLACKBOX device identifiers referenced:" rg -nP 'BLACKBOX_DEVICE_[A-Z_]+' | gawk -F: '{print $3}' | sed -E 's/.*\b(BLACKBOX_DEVICE_[A-Z_]+)\b.*/\1/' | sort -u
66-67: Drop STM32-specific QUADSPI macros on RP2350These two lines are STM32/HAL idioms and not used by the RP2350/QMI path. Keeping them risks confusion and accidental use of an undefined QUADSPI_TypeDef.
Apply this diff:
#define USE_QUADSPI -#define USE_QUADSPI_DEVICE_1 -#define FLASH_QUADSPI_INSTANCE ((QUADSPI_TypeDef *)0x1)
🧹 Nitpick comments (1)
configs/HELLBENDER_0001/config.h (1)
53-53: Nit: remove or standardize commented feature togglesThe commented “#define USE_MAX7456” is fine for discoverability, but if the codebase avoids such comment-toggles elsewhere, consider removing it to reduce noise. No functional impact.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
configs/HELLBENDER_0001/config.h(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-03T04:33:57.480Z
Learnt from: ot0tot
PR: betaflight/config#0
File: :0-0
Timestamp: 2025-07-03T04:33:57.480Z
Learning: Always verify that the BOARD_NAME define in each config.h matches the directory name it resides in (e.g., configs/AXISFLYINGF7AIO/config.h must have #define BOARD_NAME AXISFLYINGF7AIO). This check is required for all Betaflight board configuration reviews.
Applied to files:
configs/HELLBENDER_0001/config.h
📚 Learning: 2025-07-08T22:26:29.906Z
Learnt from: haslinghuis
PR: betaflight/config#829
File: configs/HELLBENDER_0001/config.h:53-56
Timestamp: 2025-07-08T22:26:29.906Z
Learning: In Betaflight configurations, SDCARD_CS_PIN and FLASH_CS_PIN can share the same GPIO pin because Betaflight uses either flash or sdcard storage, not both simultaneously. This is a mutually exclusive design where only one storage method is active at a time, so shared CS pin assignments are intentional and do not cause conflicts.
Applied to files:
configs/HELLBENDER_0001/config.h
🔇 Additional comments (2)
configs/HELLBENDER_0001/config.h (2)
73-78: OSD/SD mutual exclusion block looks goodThe #ifndef USE_MAX7456 block cleanly prevents SD-card definitions when MAX7456 is enabled and vice versa. With the conditional DEFAULT_BLACKBOX_DEVICE change (Line 57) this will be robust.
59-63: Align flash macro with actual capacity and tidy CS definitionLines 61–71 of
configs/HELLBENDER_0001/config.hcurrently selectUSE_FLASH_W25Q128FV(16 MiB) while the size constant and comment refer to an 8 MiB chip. Please confirm which flash part is fitted on the HELLBENDER_0001 board and apply one of the options below:Option A — 8 MiB Winbond W25Q64JVXGIM (as the existing comment implies):
- #define USE_FLASH_W25Q128FV + #define USE_FLASH_W25Q64JV // Winbond W25Q64JVXGIM 8 MiB // RP2350 QMI CS1 wiring and external flash size (Winbond W25Q64JVXGIM 8 MiB) #define PICO_QSPI_CS1_GPIO 0 #define PICO_QSPI_CS1_SIZE_BYTES (8 * 1024 * 1024)Option B — 16 MiB Winbond W25Q128FV (to match the current macro):
#define USE_FLASH_W25Q128FV // Winbond W25Q128FV 16 MiB // RP2350 QMI CS1 wiring and external flash size (Winbond W25Q128FV 16 MiB) #define PICO_QSPI_CS1_GPIO 0 -#define PICO_QSPI_CS1_SIZE_BYTES (8 * 1024 * 1024) +#define PICO_QSPI_CS1_SIZE_BYTES (16 * 1024 * 1024)Optional cleanup — remove the stale, commented-out SPI CS define:
- //#define FLASH_CS_PIN PA0 // CS1 pin automatically used by the QSPI driverNote: sharing the SD-card CS pin and flash CS pin is intentional in Betaflight when only one storage backend is active at a time. No changes needed to the SD-card setup.
Please verify the actual flash chip on your board and choose the appropriate option above.
SteveCEvans
force-pushed
the
HELLBENDER_0001_SDcard
branch
from
d669aa0 to
19724bc
Compare
See betaflight/betaflight#14607
This PR adds the necessary definitions for both SD card and FLASH support but gates the SD card dependant on
USE_MAX7456being defined and the onboard OSD and SD card support are mutually exclusive.Summary by CodeRabbit