Fix power button behavior with USB-C charging.#117
Open
marcoag wants to merge 2 commits intofossasia:masterfrom
Open
Fix power button behavior with USB-C charging.#117marcoag wants to merge 2 commits intofossasia:masterfrom
marcoag wants to merge 2 commits intofossasia:masterfrom
Conversation
Signed-off-by: Marco A. Gutierrez <marcogg@marcogg.com>
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts power button behavior and default hardware target for the USB-C board by limiting wake sources to KEY1, changing mode transitions out of charging (BOOT) mode, and enabling the USB-C build configuration by default. Sequence diagram for power button press while charging (BOOT to NORMAL)sequenceDiagram
actor User
participant PowerButton
participant Firmware
participant ModeController
User->>PowerButton: press
PowerButton->>Firmware: GPIO_interrupt
Firmware->>ModeController: change_mode()
alt mode_is_BOOT
ModeController->>ModeController: mode = NORMAL
ModeController-->>Firmware: return
else mode_is_not_BOOT
ModeController->>ModeController: NEXT_STATE(mode, 0, MODES_COUNT)
ModeController-->>Firmware: continue_to_mode_setup
end
Flow diagram for updated mode change logicflowchart TD
Start[change_mode_called] --> CheckBoot{mode == BOOT}
CheckBoot -->|Yes| SetNormal[Set mode to NORMAL]
SetNormal --> Return[Return without cycling modes]
CheckBoot -->|No| NextState["Advance mode using NEXT_STATE(mode, 0, MODES_COUNT)"]
NextState --> ModeSetup[Invoke corresponding mode_setup function]
ModeSetup --> End[Mode transition complete]
Flow diagram for poweroff wake configuration limited to KEY1flowchart TD
A[Enter poweroff] --> B[Configure GPIOA pins as floating input]
B --> C[Configure GPIOB pins as floating input]
C --> D[Configure KEY1_PIN as input pull-down]
D --> E[Enable rising edge interrupt on KEY1_PIN]
E --> F[Enable GPIO_A_IRQn in PFIC]
F --> G[Enable peripheral wake on GPIO]
G --> H[Device can wake only via KEY1_PIN]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
change_mode()special case formode == BOOTfeels a bit ad hoc; consider documenting the state machine behavior more clearly or centralizing the BOOT→NORMAL transition logic so future new modes don’t accidentally bypass this rule. - Disabling
CHARGE_STT_PINwake-up unconditionally inpoweroff()changes behavior for all hardware; if the issue is specific to the USB-C revision, consider#ifdef USBC_VERSIONor a similar hardware-specific guard so micro-USB boards can keep charge-based wake if desired. - The
Makefilecomment and flag name forUSBC_VERSIONare a bit confusing (comment says 'Comment below to build for micro USB version'); consider renaming the flag or rewording the comment to make the default and override behavior clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `change_mode()` special case for `mode == BOOT` feels a bit ad hoc; consider documenting the state machine behavior more clearly or centralizing the BOOT→NORMAL transition logic so future new modes don’t accidentally bypass this rule.
- Disabling `CHARGE_STT_PIN` wake-up unconditionally in `poweroff()` changes behavior for all hardware; if the issue is specific to the USB-C revision, consider `#ifdef USBC_VERSION` or a similar hardware-specific guard so micro-USB boards can keep charge-based wake if desired.
- The `Makefile` comment and flag name for `USBC_VERSION` are a bit confusing (comment says 'Comment below to build for micro USB version'); consider renaming the flag or rewording the comment to make the default and override behavior clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
5 tasks
Signed-off-by: Marco A. Gutierrez <marcogg@marcogg.com>
bessman
reviewed
Dec 30, 2025
Comment on lines
+83
to
90
| // Special case: BOOT mode exits directly to NORMAL (not part of cycle) | ||
| if (mode == BOOT) { | ||
| mode = NORMAL; | ||
| return; | ||
| } | ||
|
|
||
| // Standard mode cycling | ||
| NEXT_STATE(mode, 0, MODES_COUNT); |
Contributor
There was a problem hiding this comment.
I agree with the AI here; special-casing BOOT like this is inelegant. Set NORMAL to the minimum state in the NEXT_STATE call instead.
Suggested change
| // Special case: BOOT mode exits directly to NORMAL (not part of cycle) | |
| if (mode == BOOT) { | |
| mode = NORMAL; | |
| return; | |
| } | |
| // Standard mode cycling | |
| NEXT_STATE(mode, 0, MODES_COUNT); | |
| NEXT_STATE(mode, NORMAL, MODES_COUNT); |
Comment on lines
+17
to
+24
|
|
||
| #ifndef USBC_VERSION | ||
| // CHARGE_STT: Enable for micro-USB version | ||
| // Note: Disabled on USB-C version because it interferes with KEY1 wake-up | ||
| GPIOA_ModeCfg(CHARGE_STT_PIN, GPIO_ModeIN_PU); | ||
| GPIOA_ITModeCfg(CHARGE_STT_PIN, GPIO_ITMode_FallEdge); | ||
| #endif | ||
|
|
Contributor
There was a problem hiding this comment.
As discussed, since we don't know for sure there is a difference here between micro-USB and USB-C, let's not make this conditional.
Suggested change
| #ifndef USBC_VERSION | |
| // CHARGE_STT: Enable for micro-USB version | |
| // Note: Disabled on USB-C version because it interferes with KEY1 wake-up | |
| GPIOA_ModeCfg(CHARGE_STT_PIN, GPIO_ModeIN_PU); | |
| GPIOA_ITModeCfg(CHARGE_STT_PIN, GPIO_ITMode_FallEdge); | |
| #endif |
Open
|
Your changes remove the charging screen though. The interrupt is there to power on the board when a cable is plugged in, so it can display the charging animation correctly. Therefor your PR is just a workaround. |
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
3 participants
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.
Summary
Fixes power button issues on USB-C version when charging cable is connected.
This PR also makes USBC on by default as this is more widely used at this point than micro-usb.
Changes
poweroff(). This pin interferes with KEY1 wake-up when USB is connected.change_mode()to exit charging mode (BOOT) directly to NORMAL mode instead of cycling through all modes, preventing accidental power-off while charging.Testing
Related Issues
Fixes power button wake-up and prevents accidental shutdown during charging on USB-C hardware revision.
Summary by Sourcery
Adjust power management and build defaults to improve USB-C device behavior while charging.
Bug Fixes:
Build: