Skip to content

QSPI settings#72

Draft
x-ximena wants to merge 4 commits intomainfrom
qspi-flash-driver
Draft

QSPI settings#72
x-ximena wants to merge 4 commits intomainfrom
qspi-flash-driver

Conversation

@x-ximena
Copy link
Copy Markdown
Contributor

@x-ximena x-ximena commented Jan 22, 2026

Summary by CodeRabbit

  • New Features

    • Added support for MX25L25645G flash memory via QSPI interface on STM32H7 microcontrollers.
    • Implemented flash initialization and QPI mode configuration.
  • Chores

    • Updated header guard naming conventions.

@JasonBrave
Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 1, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 1, 2026

Walkthrough

These changes introduce a new MX25L25645G QSPI flash memory interface for STM32H7, including command definitions, hardware initialization, and QPI mode control. Additionally, header files are added for future QSPI flash abstraction and a minor header guard rename is made.

Changes

Cohort / File(s) Summary
Header Guard Maintenance
include/littlefs_sd_shim.h
Renamed header guard from ROCKETLIB_LITTLEFS_SHIM_H to ROCKETLIB_LITTLEFS_SD_SHIM_H with no functional changes.
Flash Interface Scaffolding
include/mx25l25645g.h, include/qspi_flash.h
Added empty header files with include guards, C++ compatibility macros, and necessary includes (stdbool.h, stdint.h, common.h) to support future flash abstraction layer development.
Flash Command Definitions
stm32h7/mx25l25645g_defs.h
Introduced command_t enum type defining 40+ opcodes for MX25L25645G flash operations, organized into sections: Read/Write Array Commands (3-byte and 4-byte address variants), Register/Setting Commands, ID/Security Commands, and Reset Commands.
QSPI Flash Implementation
stm32h7/mx25l25645g.c
Added complete QSPI initialization module including global QSPI handle, command templates (CMD_BASE for 1-line, CMD_BASE_QSPI for 4-line operations), MX_QUADSPI_Init function for HAL setup, QPI mode control functions (mx25l_enter_qpi, mx25l_exit_qpi), and main entry point that initializes HAL, GPIO, QSPI, and enters QPI mode.

Sequence Diagram

sequenceDiagram
    participant Main
    participant HAL_Init as HAL Initialization
    participant GPIO as GPIO Config
    participant QSPI as QSPI HAL Driver
    participant Flash as MX25L25645G

    Main->>HAL_Init: Initialize system
    HAL_Init-->>Main: HAL ready
    Main->>GPIO: Configure GPIO for QSPI pins
    GPIO-->>Main: GPIO configured
    Main->>QSPI: MX_QUADSPI_Init()
    QSPI->>QSPI: Apply QSPI config (baudrate, pins, etc.)
    QSPI-->>Main: QSPI initialized
    Main->>QSPI: Send CMD_BASE (single-line instruction)
    QSPI->>Flash: Issue EQIO command (0x38)
    Flash-->>QSPI: QPI mode enabled
    QSPI-->>Main: QPI mode active
    Main->>QSPI: Future 4-line ops via CMD_BASE_QSPI
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Through silicon so swift and wide,
Flash commands dance side by side!
From single lines to quadruple might,
The QSPI twirls in quantum light.
One hop, four hops, the memory sings,
As the rabbit's pawprint writes on wings! 🐇✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'QSPI settings' is overly vague and generic. It does not clearly convey the primary change, which is the introduction of a complete QSPI flash driver for the MX25L25645G chip with initialization, command definitions, and mode switching. Consider a more specific title that captures the main change, such as 'Add QSPI flash driver for MX25L25645G' or 'Implement MX25L25645G QSPI flash initialization and QPI mode'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch qspi-flash-driver

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
include/mx25l25645g.h (1)

1-2: Normalize header guard name to match the device identifier.

Line 1/Line 2 use ROCKETLIB_MX25L225645G_H (extra 2). This is harmless functionally, but typo-prone for future maintenance.

Also applies to: 12-12

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/mx25l25645g.h` around lines 1 - 2, The header guard macro is
misspelled with an extra "2" (ROCKETLIB_MX25L225645G_H); update both the
`#ifndef/`#define and the matching `#endif` comment to use the normalized device
identifier macro ROCKETLIB_MX25L25645G_H so the guard consistently matches the
MX25L25645G symbol (update occurrences of ROCKETLIB_MX25L225645G_H to
ROCKETLIB_MX25L25645G_H, including the trailing `#endif` comment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@include/littlefs_sd_shim.h`:
- Line 4: The header include/littlefs_sd_shim.h is platform-specific (references
STM32H7 types like SD_HandleTypeDef and includes "lfs.h") and should be moved
next to its implementation; relocate littlefs_sd_shim.h from include/ into the
platform directory (stm32h7/) alongside stm32h7/littlefs_sd_shim.c, update any
internal `#include` paths or project headers referencing littlefs_sd_shim.h to the
new location, and ensure build/CI include paths are adjusted so the file is
discovered from stm32h7/ (preserving the same header guard and symbols used by
littlefs_sd_shim.c).

In `@include/qspi_flash.h`:
- Line 7: Remove the unused `#include` "common.h" from include/qspi_flash.h to
satisfy lint; locate the include directive in the header (the line with `#include`
"common.h") and delete it, or if any symbols from common.h are actually required
by declarations in qspi_flash.h (e.g., types/macros used by functions like
qspi_init, qspi_read, qspi_write), replace the include with the minimal forward
declarations or include the specific header that provides those symbols instead
of common.h.
- Around line 1-2: The include guard identifiers are invalid (contain
dots/space); replace the malformed macros ROCKETLIB_QSPI_FLASH.H and
ROCKETLIB_QSPI_FLASH .H with a valid identifier (for example
ROCKETLIB_QSPI_FLASH_H), updating the `#ifndef` and `#define` lines and the
corresponding `#endif` comment (the symbol referenced at the end of the header) so
they all match the new valid macro name.

In `@stm32h7/mx25l25645g.c`:
- Around line 112-128: This driver defines a second program entry point `main()`
which must be removed; delete the `main()` function and instead expose/init
functions from this module (keep `mx25l_enter_qpi()`, `MX_QUADSPI_Init()`,
`MX_GPIO_Init()`, etc.) so the application/test `main()` can call them; ensure
any initialization sequence (HAL_Init/SystemClock_Config) is either removed or
wrapped in a clearly named function (e.g., `mx25l_driver_init()`) that the
application calls, and update or add header declarations for the exported init
functions so no `main` symbol remains in this driver.
- Around line 122-124: mx25l_enter_qpi()'s return value must be checked and
handled like other HAL calls; update the code that currently calls
mx25l_enter_qpi() to capture its HAL_StatusTypeDef result and if it is not
HAL_OK, call the same error path used elsewhere (e.g., Error_Handler() or the
existing flash init failure handling) or return the error to the caller;
specifically modify the call to mx25l_enter_qpi() and add a HAL_OK check and
appropriate error handling to avoid continuing initialization when QPI entry
fails.
- Around line 27-41: The CMD_BASE QSPI_CommandTypeDef initializer uses
semicolons between fields which is invalid C; replace each ';' between
designated initializer entries with commas and ensure the struct initializer
uses comma-separated fields (and include a trailing comma before the closing
brace) so the CMD_BASE definition and any similar command templates compile
correctly.

---

Nitpick comments:
In `@include/mx25l25645g.h`:
- Around line 1-2: The header guard macro is misspelled with an extra "2"
(ROCKETLIB_MX25L225645G_H); update both the `#ifndef/`#define and the matching
`#endif` comment to use the normalized device identifier macro
ROCKETLIB_MX25L25645G_H so the guard consistently matches the MX25L25645G symbol
(update occurrences of ROCKETLIB_MX25L225645G_H to ROCKETLIB_MX25L25645G_H,
including the trailing `#endif` comment).

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a2e80d1 and 01e5aca.

📒 Files selected for processing (5)
  • include/littlefs_sd_shim.h
  • include/mx25l25645g.h
  • include/qspi_flash.h
  • stm32h7/mx25l25645g.c
  • stm32h7/mx25l25645g_defs.h

#ifndef ROCKETLIB_LITTLEFS_SD_SHIM_H
#define ROCKETLIB_LITTLEFS_SD_SHIM_H

#include "lfs.h"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Locate littlefs headers and current include patterns.
echo "=== Searching for lfs.h files ==="
fd -a 'lfs.h'

echo -e "\n=== Searching for include patterns related to lfs ==="
rg -nP '#\s*include\s*[<"]lfs\.h[>"]|#\s*include\s*[<"].*lfs.*[>"]' -t c -t h --max-count 20

Repository: waterloo-rocketry/rocketlib

Length of output: 251


🏁 Script executed:

#!/bin/bash
echo "=== Checking for build configuration files ==="
ls -la | grep -E "CMakeLists|Makefile|package.json|\.gitmodules|\.git"

echo -e "\n=== Checking .gitmodules for submodules ==="
cat .gitmodules 2>/dev/null || echo "No .gitmodules found"

echo -e "\n=== Searching for littlefs references in build/config files ==="
rg "littlefs|lfs\.h" -t cmake -t make -t json --max-count 20

echo -e "\n=== Directory structure overview ==="
find . -maxdepth 2 -type d | grep -v "\.git" | sort

Repository: waterloo-rocketry/rocketlib

Length of output: 526


🏁 Script executed:

#!/bin/bash
echo "=== GitHub Actions workflow files ==="
find .github -type f 2>/dev/null | head -20

echo -e "\n=== Checking workflow content ==="
for f in $(find .github -name "*.yml" -o -name "*.yaml" 2>/dev/null); do
  echo "--- $f ---"
  cat "$f"
done

echo -e "\n=== Checking for build configs in subdirectories ==="
find . -maxdepth 2 -name "CMakeLists.txt" -o -name "Makefile" -o -name "*.mk" 2>/dev/null | head -20

echo -e "\n=== Checking for include paths in stm32h7 ==="
ls -la stm32h7/ 2>/dev/null | head -20

Repository: waterloo-rocketry/rocketlib

Length of output: 4723


🏁 Script executed:

#!/bin/bash
echo "=== README or documentation files ==="
find . -maxdepth 2 -iname "README*" -o -iname "*.md" | head -10

echo -e "\n=== Checking git log for littlefs mentions (last 10 commits) ==="
git log --oneline -20 | head -10

echo -e "\n=== Checking if littlefs is mentioned in any config/setup file ==="
rg "littlefs|lfs" -t md -t txt -t rst --max-count 20

echo -e "\n=== Checking include/littlefs_sd_shim.h full content ==="
cat include/littlefs_sd_shim.h

Repository: waterloo-rocketry/rocketlib

Length of output: 789


Move header to platform-specific directory to fix CI lint failure.

The #include "lfs.h" fails clang-tidy because this header is platform-specific (uses STM32H7 types like SD_HandleTypeDef) but resides in include/ which gets checked with only -Iinclude flag, without access to the littlefs dependency. Move include/littlefs_sd_shim.h to stm32h7/ alongside its implementation in stm32h7/littlefs_sd_shim.c.

🧰 Tools
🪛 GitHub Actions: C Lint Check

[error] 4-4: 'lfs.h' file not found

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/littlefs_sd_shim.h` at line 4, The header include/littlefs_sd_shim.h
is platform-specific (references STM32H7 types like SD_HandleTypeDef and
includes "lfs.h") and should be moved next to its implementation; relocate
littlefs_sd_shim.h from include/ into the platform directory (stm32h7/)
alongside stm32h7/littlefs_sd_shim.c, update any internal `#include` paths or
project headers referencing littlefs_sd_shim.h to the new location, and ensure
build/CI include paths are adjusted so the file is discovered from stm32h7/
(preserving the same header guard and symbols used by littlefs_sd_shim.c).

Comment on lines +1 to +2
#ifndef ROCKETLIB_QSPI_FLASH.H
#define ROCKETLIB_QSPI_FLASH .H
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n include/qspi_flash.h

Repository: waterloo-rocketry/rocketlib

Length of output: 421


🏁 Script executed:

# Check if any symbols from common.h are used in the file
rg -n 'ROCKETLIB_QSPI_FLASH' include/qspi_flash.h

Repository: waterloo-rocketry/rocketlib

Length of output: 176


🏁 Script executed:

# Verify C preprocessor identifier syntax rules
grep -n '.' include/qspi_flash.h | head -5

Repository: waterloo-rocketry/rocketlib

Length of output: 204


🏁 Script executed:

# Verify C preprocessor standard - identifiers must match pattern [a-zA-Z_][a-zA-Z0-9_]*
# The actual error these lines would cause
python3 << 'EOF'
import re

# C preprocessor identifier pattern (from C standard)
valid_identifier = r'^[a-zA-Z_][a-zA-Z0-9_]*$'

test_cases = [
    "ROCKETLIB_QSPI_FLASH.H",     # Line 1 ifndef token
    "ROCKETLIB_QSPI_FLASH",        # Line 2 define token
    ".H",                          # Line 2 second token
    "ROCKETLIB_QSPI_FLASH_H",      # Proposed fix
]

for token in test_cases:
    is_valid = bool(re.match(valid_identifier, token))
    status = "✓ VALID" if is_valid else "✗ INVALID"
    print(f"{status:12} : {token}")
EOF

Repository: waterloo-rocketry/rocketlib

Length of output: 200


Fix invalid include guard macros—build blocker.

Preprocessor identifiers must contain only alphanumeric characters and underscores. Lines 1–2 contain invalid dots and whitespace that prevent compilation.

🔧 Proposed fix
-#ifndef ROCKETLIB_QSPI_FLASH.H
-#define ROCKETLIB_QSPI_FLASH .H
+#ifndef ROCKETLIB_QSPI_FLASH_H
+#define ROCKETLIB_QSPI_FLASH_H
@@
-#endif /* ROCKETLIB_QSPI_FLASH.H */
+#endif /* ROCKETLIB_QSPI_FLASH_H */

Also applies to: 17–17

🧰 Tools
🪛 GitHub Actions: C Lint Check

[error] 1-1: extra tokens at end of #ifndef directive

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/qspi_flash.h` around lines 1 - 2, The include guard identifiers are
invalid (contain dots/space); replace the malformed macros
ROCKETLIB_QSPI_FLASH.H and ROCKETLIB_QSPI_FLASH .H with a valid identifier (for
example ROCKETLIB_QSPI_FLASH_H), updating the `#ifndef` and `#define` lines and the
corresponding `#endif` comment (the symbol referenced at the end of the header) so
they all match the new valid macro name.

#include <stdbool.h>
#include <stdint.h>

#include "common.h"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Remove unused common.h include to satisfy lint.

Line 7 is flagged as unused in CI. If no symbols are consumed from common.h, remove it for now.

🔧 Proposed fix
-#include "common.h"
📝 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.

Suggested change
#include "common.h"
🧰 Tools
🪛 GitHub Actions: C Lint Check

[error] 7-7: Included header 'common.h' is not used directly

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/qspi_flash.h` at line 7, Remove the unused `#include` "common.h" from
include/qspi_flash.h to satisfy lint; locate the include directive in the header
(the line with `#include` "common.h") and delete it, or if any symbols from
common.h are actually required by declarations in qspi_flash.h (e.g.,
types/macros used by functions like qspi_init, qspi_read, qspi_write), replace
the include with the minimal forward declarations or include the specific header
that provides those symbols instead of common.h.

Comment on lines +27 to +41
const QSPI_CommandTypeDef CMD_BASE = {.Instruction = 0x0;
// .Address = 0x014;
// .AlternateBytes = 0x0;
// .AlternateBytesSize = QSPI_ALTERNATE_BYTES_8_BITS;
.DummyCycles = 0;
.InstructionMode = QSPI_INSTRUCTION_1_LINE;
.AddressMode = QSPI_ADDRESS_NONE;
.AlternateByteMode = QSPI_ALTERNATE_BYTES_NONE;
.DataMode = QSPI_DATA_NONE;
// .NbData = 0x0;
.DdrMode = QSPI_DDR_MODE_DISABLE;
// .DdrHoldHalfCycle = QSPI_DDR_HHC_ANALOG_DELAY;
.SIOOMode = QSPI_SIOO_INST_EVERY_CMD;
}
;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n stm32h7/mx25l25645g.c | head -70

Repository: waterloo-rocketry/rocketlib

Length of output: 2527


🏁 Script executed:

# Check if there's a build system or makefile
fd -type f '(Makefile|CMakeLists\.txt|build\.sh)' . --max-results 5

Repository: waterloo-rocketry/rocketlib

Length of output: 241


🏁 Script executed:

fd -type f -name 'Makefile' . && \
fd -type f -name 'CMakeLists.txt' . && \
fd -type f -name 'build.sh' .

Repository: waterloo-rocketry/rocketlib

Length of output: 241


🏁 Script executed:

find . -maxdepth 2 -name 'Makefile' -o -name 'CMakeLists.txt' -o -name 'build.sh' 2>/dev/null

Repository: waterloo-rocketry/rocketlib

Length of output: 87


🏁 Script executed:

# Check the repository structure
ls -la stm32h7/

Repository: waterloo-rocketry/rocketlib

Length of output: 386


🏁 Script executed:

# Verify the complete context of the initializers
cat -n stm32h7/mx25l25645g.c | sed -n '25,65p'

Repository: waterloo-rocketry/rocketlib

Length of output: 1626


Fix designated-initializer syntax in command templates (compilation blocker).

Lines 27 and 44 start designated initializers but use ; between fields. In C, initializer fields must be comma-separated.

🔧 Proposed fix
-const QSPI_CommandTypeDef CMD_BASE = {.Instruction = 0x0;
+const QSPI_CommandTypeDef CMD_BASE = {
+	.Instruction = 0x0,
 // .Address = 0x014;
 // .AlternateBytes = 0x0;
 // .AlternateBytesSize = QSPI_ALTERNATE_BYTES_8_BITS;
-.DummyCycles = 0;
-.InstructionMode = QSPI_INSTRUCTION_1_LINE;
-.AddressMode = QSPI_ADDRESS_NONE;
-.AlternateByteMode = QSPI_ALTERNATE_BYTES_NONE;
-.DataMode = QSPI_DATA_NONE;
+	.DummyCycles = 0,
+	.InstructionMode = QSPI_INSTRUCTION_1_LINE,
+	.AddressMode = QSPI_ADDRESS_NONE,
+	.AlternateByteMode = QSPI_ALTERNATE_BYTES_NONE,
+	.DataMode = QSPI_DATA_NONE,
 // .NbData = 0x0;
-.DdrMode = QSPI_DDR_MODE_DISABLE;
+	.DdrMode = QSPI_DDR_MODE_DISABLE,
 // .DdrHoldHalfCycle = QSPI_DDR_HHC_ANALOG_DELAY;
-.SIOOMode = QSPI_SIOO_INST_EVERY_CMD;
-}
-;
+	.SIOOMode = QSPI_SIOO_INST_EVERY_CMD,
+};

-QSPI_CommandTypeDef CMD_BASE_QSPI = {.Instruction = 0x0;
+QSPI_CommandTypeDef CMD_BASE_QSPI = {
+	.Instruction = 0x0,
 // .Address = 0x014;
 // .AlternateBytes = 0x0;
 // .AddressSize = QSPI_ADDRESS_8_BITS;
 .AlternateBytesSize =
-	QSPI_ALTERNATE_BYTES_8_BITS; // may need to change to QSPI_ADDRESS_24_BITS or 32
-.DummyCycles = 0; // was 8
-.InstructionMode = QSPI_INSTRUCTION_4_LINES;
-.AddressMode = QSPI_ADDRESS_NONE;
-.AlternateByteMode = QSPI_ALTERNATE_BYTES_NONE;
-.DataMode = QSPI_DATA_NONE;
-.NbData = 0;
-.DdrMode = QSPI_DDR_MODE_DISABLE;
-.DdrHoldHalfCycle = QSPI_DDR_HHC_ANALOG_DELAY;
-.SIOOMode = QSPI_SIOO_INST_EVERY_CMD;
-}
-;
+	QSPI_ALTERNATE_BYTES_8_BITS, // may need to change to QSPI_ADDRESS_24_BITS or 32
+	.DummyCycles = 0, // was 8
+	.InstructionMode = QSPI_INSTRUCTION_4_LINES,
+	.AddressMode = QSPI_ADDRESS_NONE,
+	.AlternateByteMode = QSPI_ALTERNATE_BYTES_NONE,
+	.DataMode = QSPI_DATA_NONE,
+	.NbData = 0,
+	.DdrMode = QSPI_DDR_MODE_DISABLE,
+	.DdrHoldHalfCycle = QSPI_DDR_HHC_ANALOG_DELAY,
+	.SIOOMode = QSPI_SIOO_INST_EVERY_CMD,
+};
🧰 Tools
🪛 Cppcheck (2.19.0)

[error] 31-31: syntax error

(syntaxError)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@stm32h7/mx25l25645g.c` around lines 27 - 41, The CMD_BASE QSPI_CommandTypeDef
initializer uses semicolons between fields which is invalid C; replace each ';'
between designated initializer entries with commas and ensure the struct
initializer uses comma-separated fields (and include a trailing comma before the
closing brace) so the CMD_BASE definition and any similar command templates
compile correctly.

Comment on lines +112 to +128
int main(void) {
HAL_Init();
SystemClock_Config();

// Initialize the QSPI MSP
MX_GPIO_Init();

// Initialize the QSPI mode with 1 instruction line, CMD_BASE
MX_QUADSPI_Init();

// Enable QPI mode on flash
mx25l_enter_qpi();

// From now on instruction becomes 4 lines since qspi is enabled, QSPI_CMD_BASE

while (1) {}
} No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

rg -nP --type=c --type=cpp '\bint\s+main\s*\('

Repository: waterloo-rocketry/rocketlib

Length of output: 171


🏁 Script executed:

# Check the repository structure and file organization
git ls-files | head -30

Repository: waterloo-rocketry/rocketlib

Length of output: 696


🏁 Script executed:

# Look at the context around stm32h7/mx25l25645g.c to understand what this file is
fd -type f -name "*.c" -o -name "*.h" | grep -E "(stm32h7|mx25l)" | head -20

Repository: waterloo-rocketry/rocketlib

Length of output: 241


🏁 Script executed:

head -50 stm32h7/mx25l25645g.c

Repository: waterloo-rocketry/rocketlib

Length of output: 1479


🏁 Script executed:

cat include/mx25l25645g.h

Repository: waterloo-rocketry/rocketlib

Length of output: 241


Remove main() from this driver module.

Line 112 introduces a second main() symbol; a duplicate exists at tests/rockettest.cpp:9. Driver modules must not define entry points—they should export functions to be called from an application's main(). This will cause a linker error.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@stm32h7/mx25l25645g.c` around lines 112 - 128, This driver defines a second
program entry point `main()` which must be removed; delete the `main()` function
and instead expose/init functions from this module (keep `mx25l_enter_qpi()`,
`MX_QUADSPI_Init()`, `MX_GPIO_Init()`, etc.) so the application/test `main()`
can call them; ensure any initialization sequence (HAL_Init/SystemClock_Config)
is either removed or wrapped in a clearly named function (e.g.,
`mx25l_driver_init()`) that the application calls, and update or add header
declarations for the exported init functions so no `main` symbol remains in this
driver.

Comment on lines +122 to +124
// Enable QPI mode on flash
mx25l_enter_qpi();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the exact code at lines 122-124
cat -n stm32h7/mx25l25645g.c | sed -n '120,130p'

Repository: waterloo-rocketry/rocketlib

Length of output: 302


🏁 Script executed:

# Search for mx25l_enter_qpi function definition to understand its return type
rg -n 'mx25l_enter_qpi' stm32h7/mx25l25645g.c -A 5

Repository: waterloo-rocketry/rocketlib

Length of output: 443


🏁 Script executed:

# Check for HAL_OK usage in the file to understand the error handling pattern
rg -n 'HAL_OK' stm32h7/mx25l25645g.c

Repository: waterloo-rocketry/rocketlib

Length of output: 113


🏁 Script executed:

# Look for Error_Handler usage in the file
rg -n 'Error_Handler' stm32h7/mx25l25645g.c -B 2 -A 1

Repository: waterloo-rocketry/rocketlib

Length of output: 173


Check return status of mx25l_enter_qpi() before proceeding.

Line 123 ignores the return value of mx25l_enter_qpi(). The function returns HAL_StatusTypeDef, and if it fails, the flash may not enter QPI mode correctly while initialization continues into the infinite loop. This is inconsistent with the error handling pattern used elsewhere in the file (e.g., line 21).

🔧 Proposed fix
 	// Enable QPI mode on flash
-	mx25l_enter_qpi();
+	if (mx25l_enter_qpi() != HAL_OK) {
+		Error_Handler();
+	}
📝 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.

Suggested change
// Enable QPI mode on flash
mx25l_enter_qpi();
// Enable QPI mode on flash
if (mx25l_enter_qpi() != HAL_OK) {
Error_Handler();
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@stm32h7/mx25l25645g.c` around lines 122 - 124, mx25l_enter_qpi()'s return
value must be checked and handled like other HAL calls; update the code that
currently calls mx25l_enter_qpi() to capture its HAL_StatusTypeDef result and if
it is not HAL_OK, call the same error path used elsewhere (e.g., Error_Handler()
or the existing flash init failure handling) or return the error to the caller;
specifically modify the call to mx25l_enter_qpi() and add a HAL_OK check and
appropriate error handling to avoid continuing initialization when QPI entry
fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants