fix(stm32f4): Hangs during flashing firmware#7415
Conversation
📝 WalkthroughWalkthroughFlash erase and program operations in the STM32 driver are wrapped with interrupt masking. ChangesFlash Operation Interrupt Masking
🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
richardclli
changed the title
richardclli
force-pushed
the
richardclli/fix-f4-flash-hangs
branch
from
3ff0440 to
b14c674
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
radio/src/targets/common/arm/stm32/flash_driver.cpp (1)
179-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame issue: restore previous interrupt state instead of unconditionally enabling.
Apply the same PRIMASK save/restore pattern here for consistency and correctness.
Proposed fix
int ret = 0; + uint32_t primask = __get_PRIMASK(); __disable_irq(); stm32_flash_unlock(); while (address < end_addr) { if (_FLASH_PROGRAM(address, p_data) != HAL_OK) { ret = -1; break; } address += sizeof(uint32_t) * FLASH_PROG_WORDS; p_data += FLASH_PROG_WORDS; } stm32_flash_lock(); - __enable_irq(); + __set_PRIMASK(primask); return ret;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/targets/common/arm/stm32/flash_driver.cpp` around lines 179 - 192, The block currently unconditionally calls __disable_irq() and later __enable_irq(); change it to save and restore the prior interrupt state using the PRIMASK pattern: capture the current PRIMASK (via __get_PRIMASK() or equivalent) before disabling, call __disable_irq(), perform stm32_flash_unlock(), the programming loop using _FLASH_PROGRAM, and stm32_flash_lock(), then restore the saved PRIMASK (via __set_PRIMASK(saved) or equivalent) instead of calling __enable_irq() directly so the interrupt state is preserved; update the surrounding code in flash_driver.cpp where __disable_irq(), stm32_flash_unlock(), stm32_flash_lock(), and __enable_irq() are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/targets/common/arm/stm32/flash_driver.cpp`:
- Around line 148-155: The code unconditionally calls __enable_irq() after using
__disable_irq(), which can enable interrupts that were previously disabled by
the caller; change the pattern to save and restore the caller's PRIMASK instead:
at function entry (around where __disable_irq() is currently called) read the
current PRIMASK via __get_PRIMASK(), then call __disable_irq(), perform
stm32_flash_unlock(), HAL_FLASHEx_Erase(...), stm32_flash_lock(), and finally
restore the original interrupt state by calling __set_PRIMASK(saved_primask)
instead of unconditionally calling __enable_irq(); update usage around
stm32_flash_unlock()/stm32_flash_lock()/HAL_FLASHEx_Erase to use this
save/restore PRIMASK pattern.
---
Outside diff comments:
In `@radio/src/targets/common/arm/stm32/flash_driver.cpp`:
- Around line 179-192: The block currently unconditionally calls __disable_irq()
and later __enable_irq(); change it to save and restore the prior interrupt
state using the PRIMASK pattern: capture the current PRIMASK (via
__get_PRIMASK() or equivalent) before disabling, call __disable_irq(), perform
stm32_flash_unlock(), the programming loop using _FLASH_PROGRAM, and
stm32_flash_lock(), then restore the saved PRIMASK (via __set_PRIMASK(saved) or
equivalent) instead of calling __enable_irq() directly so the interrupt state is
preserved; update the surrounding code in flash_driver.cpp where
__disable_irq(), stm32_flash_unlock(), stm32_flash_lock(), and __enable_irq()
are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d211f90-8d5f-4c97-b58a-b508b0fe27d7
📒 Files selected for processing (1)
radio/src/targets/common/arm/stm32/flash_driver.cpp
| __disable_irq(); | ||
| stm32_flash_unlock(); | ||
| if (HAL_FLASHEx_Erase(&eraseInit, §or_errors) != HAL_OK) { | ||
| ret = -1; | ||
| } | ||
|
|
||
| stm32_flash_lock(); | ||
| __enable_irq(); |
There was a problem hiding this comment.
Unconditional __enable_irq() may unexpectedly alter caller's interrupt state.
If interrupts were already disabled before entering this function, __enable_irq() will enable them unexpectedly. Use PRIMASK save/restore pattern to preserve the original interrupt state.
Proposed fix
int ret = 0;
uint32_t sector_errors = 0;
+ uint32_t primask = __get_PRIMASK();
__disable_irq();
stm32_flash_unlock();
if (HAL_FLASHEx_Erase(&eraseInit, §or_errors) != HAL_OK) {
ret = -1;
}
stm32_flash_lock();
- __enable_irq();
+ __set_PRIMASK(primask);
return ret;📝 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.
| __disable_irq(); | |
| stm32_flash_unlock(); | |
| if (HAL_FLASHEx_Erase(&eraseInit, §or_errors) != HAL_OK) { | |
| ret = -1; | |
| } | |
| stm32_flash_lock(); | |
| __enable_irq(); | |
| uint32_t primask = __get_PRIMASK(); | |
| __disable_irq(); | |
| stm32_flash_unlock(); | |
| if (HAL_FLASHEx_Erase(&eraseInit, §or_errors) != HAL_OK) { | |
| ret = -1; | |
| } | |
| stm32_flash_lock(); | |
| __set_PRIMASK(primask); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@radio/src/targets/common/arm/stm32/flash_driver.cpp` around lines 148 - 155,
The code unconditionally calls __enable_irq() after using __disable_irq(), which
can enable interrupts that were previously disabled by the caller; change the
pattern to save and restore the caller's PRIMASK instead: at function entry
(around where __disable_irq() is currently called) read the current PRIMASK via
__get_PRIMASK(), then call __disable_irq(), perform stm32_flash_unlock(),
HAL_FLASHEx_Erase(...), stm32_flash_lock(), and finally restore the original
interrupt state by calling __set_PRIMASK(saved_primask) instead of
unconditionally calling __enable_irq(); update usage around
stm32_flash_unlock()/stm32_flash_lock()/HAL_FLASHEx_Erase to use this
save/restore PRIMASK pattern.
|
Tested to flash > 10 times using my PL18U, no more hangs. However, the probability of seeing hangs during flashing is quite low. Maybe need more to test to confirm if it is gone. As the flash operation can still works properly. No harm to merge anyway. |
Probably quite a few more... I think we were counting something like 1 in 30 flashes... very strange gremlin |
pfeerick
added
backport/2.11
2 participants
Trying to fix the problem by chance the flashing will stop in the middle. Using opencode + deepseek v4 to discover the fix
Summary by CodeRabbit