fix bug # add AT32-F435 sram config - #11774
Open
sunyanmeng963 wants to merge 1 commit into
Open
Conversation
Contributor
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Branch Targeting SuggestionYou've targeted the
If This is an automated suggestion to help route contributions to the appropriate branch. |
PR Summary by QodoFix AT32F435 SRAM sizing by initializing flash USD EOPB0 config
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Flash size macro missing
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+32
to
+36
| #if 256 < TARGET_FLASH_SIZE | ||
| #define USD_EOPB0_SRAM_CONFIG_MASK 0x7 | ||
| #else | ||
| #define USD_EOPB0_SRAM_CONFIG_MASK 0x3 | ||
| #endif |
There was a problem hiding this comment.
1. Flash size macro missing 🐞 Bug ≡ Correctness
system_at32f43x.c now branches on TARGET_FLASH_SIZE, but AT32 builds define MCU_FLASH_SIZE in CMake and other AT32 targets do not define TARGET_FLASH_SIZE; when undefined it evaluates as 0 in #if expressions, leaving get_sram_config() with no return path and causing undefined behavior (or -Werror build failure).
Agent Prompt
### Issue description
`system_at32f43x.c` uses `TARGET_FLASH_SIZE` in preprocessor conditionals, but the AT32 build system provides `MCU_FLASH_SIZE` (CMake) and most AT32 targets do not define `TARGET_FLASH_SIZE`. When undefined, `TARGET_FLASH_SIZE` is treated as `0` in `#if`, so the `get_sram_config()` switch compiles with no cases and no return, leading to undefined behavior (and often failing the build under `-Werror=return-type`).
### Issue Context
- AT32 CMake toolchain defines `MCU_FLASH_SIZE`, not `TARGET_FLASH_SIZE`.
- Only one AT32 target header was updated to define `TARGET_FLASH_SIZE`, leaving other AT32 targets inconsistent.
### Fix Focus Areas
- src/main/drivers/system_at32f43x.c[31-80]
- cmake/at32f4.cmake[80-101]
- src/main/target/BLUEBERRYF435WING/target.h[18-46]
### Suggested fix
- Replace `TARGET_FLASH_SIZE` usage with `MCU_FLASH_SIZE` (preferred), or add a single global fallback in a common header:
- `#ifndef TARGET_FLASH_SIZE` / `#define TARGET_FLASH_SIZE MCU_FLASH_SIZE`.
- Remove the per-target `#define TARGET_FLASH_SIZE ...` if it becomes redundant, to avoid divergence.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+88
to
+93
| if (((USD->eopb0) & USD_EOPB0_SRAM_CONFIG_MASK) != sram_cfg) { | ||
| flash_unlock(); | ||
| flash_user_system_data_erase(); | ||
| flash_eopb0_config(sram_cfg); | ||
| systemReset(); | ||
| } |
There was a problem hiding this comment.
2. Flash ops unchecked 🐞 Bug ☼ Reliability
init_sram_config() erases/programs USD and immediately resets without checking flash_user_system_data_erase()/flash_eopb0_config() return values; if the operations fail, the board can reboot into the same mismatch and get stuck in a persistent reset loop.
Agent Prompt
### Issue description
`init_sram_config()` ignores the return statuses of `flash_user_system_data_erase()` and `flash_eopb0_config()`, then calls `systemReset()` unconditionally. If erase/program fails, the SRAM-config mismatch will still be present on next boot and the device can repeatedly erase/reset.
### Issue Context
The vendor flash driver APIs return `flash_status_type` specifically to allow callers to detect failures/timeouts. Additionally, the vendor driver documents that USD erase changes `eopb0` to `0xFF` (which can affect SRAM sizing), so resetting without confirming successful re-programming is risky.
### Fix Focus Areas
- src/main/drivers/system_at32f43x.c[84-94]
- lib/main/AT32F43x/Drivers/AT32F43x_StdPeriph_Driver/inc/at32f435_437_flash.h[671-690]
- lib/main/AT32F43x/Drivers/AT32F43x_StdPeriph_Driver/src/at32f435_437_flash.c[508-599]
### Suggested fix
- Capture and validate return values:
- If erase fails: abort the update and avoid reset (enter a safe failure mode, or keep running with current config).
- If programming fails after a successful erase: avoid reset-loop; signal failure and stop.
- Only call `systemReset()` after confirming successful programming.
- Consider re-locking flash (and/or USD) if execution continues after failure.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+89
to
+92
| flash_unlock(); | ||
| flash_user_system_data_erase(); | ||
| flash_eopb0_config(sram_cfg); | ||
| systemReset(); |
There was a problem hiding this comment.
3. Usd erase wipes settings 🐞 Bug ≡ Correctness
init_sram_config() uses flash_user_system_data_erase() (documented to erase all USD except FAP) but then only restores eopb0, so any existing USD fields (SSB/DATA/EPP/QSPIKEY/etc.) are lost whenever SRAM config differs.
Agent Prompt
### Issue description
The new SRAM configuration path erases the entire User System Data (USD) area but reprograms only `eopb0`. Per the vendor driver, this erase clears all USD fields except the FAP byte, so board-specific/provisioned values in other USD fields can be destroyed.
### Issue Context
`usd_type` contains multiple fields beyond `eopb0` (`ssb`, `data0/1`, `epp*`, `qspikey[]`, etc.). If any of these are set (factory provisioning, security settings, boot behavior), erasing USD during firmware boot can cause permanent behavior changes.
### Fix Focus Areas
- src/main/drivers/system_at32f43x.c[84-94]
- lib/main/AT32F43x/Drivers/AT32F43x_StdPeriph_Driver/src/at32f435_437_flash.c[508-558]
- lib/main/AT32F43x/Drivers/AT32F43x_StdPeriph_Driver/inc/at32f435_437_flash.h[631-652]
### Suggested fix
- Avoid a full USD erase if the platform supports updating `eopb0` without erasing the whole USD block.
- If erase is required, first snapshot all USD fields that must be preserved (read `USD->...`), perform the erase, then reprogram **all** preserved fields plus the desired `eopb0` value.
- At minimum, explicitly set any required USD fields to known-safe defaults after erase, rather than leaving them erased.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
No description provided.