Open
Conversation
poweroff() was called directly from button ISR context (via TMR3_IRQHandler), causing LowPower_Shutdown's __WFI to execute inside an interrupt handler. This is invalid and causes the CPU to hang. Fix: Set flag in ISR, handle poweroff in main loop. Add GPIOA_IRQHandler to clear wake interrupt flags.
Contributor
Reviewer's GuideDefers power-off handling from ISR context to the main loop and adds a dedicated GPIOA interrupt handler to safely clear wake-related GPIO interrupt flags, preventing CPU hangs when the power button is pressed multiple times. Sequence diagram for deferred poweroff handling from button interrupt to main loopsequenceDiagram
actor User
participant GPIOA as GPIOA_pin
participant ButtonISR as Button_ISR_change_mode
participant Main as main_loop
participant Power as power_module
User->>GPIOA: Press power button
GPIOA-->>ButtonISR: Trigger button interrupt
activate ButtonISR
ButtonISR->>ButtonISR: change_mode()
ButtonISR->>ButtonISR: Set pending_poweroff = 1
ButtonISR-->>GPIOA: Return from interrupt
deactivate ButtonISR
loop Main loop
Main->>Main: Check pending_poweroff
alt pending_poweroff == 1
Main->>Main: pending_poweroff = 0
Main->>Power: poweroff()
activate Power
Power-->>Main: Enter low power and execute __WFI safely
deactivate Power
else pending_poweroff == 0
Main->>Main: TMOS_SystemProcess()
end
end
Sequence diagram for GPIOA wake interrupt handlingsequenceDiagram
actor User
participant GPIOA as GPIOA_pins
participant GPIOA_ISR as GPIOA_IRQHandler
participant Power as LowPower_Shutdown
participant CPU as CPU_core
User->>GPIOA: Power button or charge state change
GPIOA-->>GPIOA_ISR: Trigger GPIOA interrupt
activate GPIOA_ISR
GPIOA_ISR->>GPIOA: GPIOA_ClearITFlagBit(KEY1_PIN | CHARGE_STT_PIN)
GPIOA_ISR-->>GPIOA: Return from interrupt
deactivate GPIOA_ISR
CPU->>Power: LowPower_Shutdown configured wake sources
Power->>CPU: Execute __WFI
CPU-->>Power: Wake when GPIOA interrupt occurs
Power-->>CPU: Resume normal execution in main loop
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since
pending_poweroffis written from ISR context and read/cleared in the main loop, consider making its type an explicitly sizedvolatile uint8_t(or similar) and keeping all writes as simple flag sets/clears to avoid any potential multi-byte access issues on this MCU. - Now that
POWER_OFFis handled viapending_poweroffinstead of the function pointer table, double-check that every code path that setsmode = POWER_OFFgoes throughchange_mode(); if not, you may want a single central place that translatesmode == POWER_OFFinto a deferred shutdown.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `pending_poweroff` is written from ISR context and read/cleared in the main loop, consider making its type an explicitly sized `volatile uint8_t` (or similar) and keeping all writes as simple flag sets/clears to avoid any potential multi-byte access issues on this MCU.
- Now that `POWER_OFF` is handled via `pending_poweroff` instead of the function pointer table, double-check that every code path that sets `mode = POWER_OFF` goes through `change_mode()`; if not, you may want a single central place that translates `mode == POWER_OFF` into a deferred shutdown.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
Fixes badge hang when pressing power button multiple times.
The issue was that poweroff() was called directly from button ISR context, causing __WFI to execute inside an interrupt handler which hangs the CPU.
Fix defers poweroff to main loop and adds GPIOA_IRQHandler for wake interrupt handling.
Summary by Sourcery
Defer badge power-off handling from interrupt context to the main loop and add a GPIO interrupt handler for wake-related interrupts.
Bug Fixes:
Enhancements: