fix: dynamically assign button pin to prevent false interrupts#10742
fix: dynamically assign button pin to prevent false interrupts#10742LN4CY wants to merge 2 commits into
Conversation
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
📝 WalkthroughWalkthroughButton GPIO selection now supports runtime device configuration with compile-time fallbacks across input initialization, sleep wake configuration, and idle-state wake event detection. ChangesButton GPIO handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/input/InputBroker.cpp`:
- Around line 170-178: Duplicate button GPIO resolution should be centralized in
an inline helper such as getResolvedButtonPin(), preserving the
USERPREFS_BUTTON_PIN, BUTTON_PIN, and config.device.button_gpio precedence. Add
the helper in the shared configuration or utility location, then replace the
duplicated blocks in src/input/InputBroker.cpp lines 170-178 and 329-337,
src/sleep.cpp lines 313-321, 440-448, and 485-493,
src/platform/esp32/main-esp32.cpp lines 355-363, and src/PowerFSM.cpp lines
180-188 with calls to it.
In `@src/platform/esp32/main-esp32.cpp`:
- Around line 367-369: Validate the resolved button GPIO before use, preferably
in the shared pin-resolution helper, using the platform’s GPIO validity check or
an equivalent bounds check. Apply this to the wake-mask shift in
main-esp32.cpp#L367-L369, the digitalRead path in PowerFSM.cpp#L189, and
OneButton instantiation in InputBroker.cpp#L348-L349; invalid pins must be
excluded so they cannot cause undefined shifts or phantom button events.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 32fa657f-7831-4f92-ba75-b84721911335
📒 Files selected for processing (4)
src/PowerFSM.cppsrc/input/InputBroker.cppsrc/platform/esp32/main-esp32.cppsrc/sleep.cpp
| uint32_t _btnPin = 0xFF; | ||
| #if defined(USERPREFS_BUTTON_PIN) | ||
| _btnPin = USERPREFS_BUTTON_PIN; | ||
| #elif defined(BUTTON_PIN) | ||
| _btnPin = BUTTON_PIN; | ||
| #endif | ||
| if (config.device.button_gpio != 0) { | ||
| _btnPin = config.device.button_gpio; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract duplicated button pin resolution logic into a helper function.
The exact block of logic that dynamically resolves the button GPIO from USERPREFS_BUTTON_PIN, BUTTON_PIN, and config.device.button_gpio is duplicated 7 times across the codebase. This violates the DRY principle and increases maintenance overhead.
Please extract this into a centralized inline helper function (e.g., getResolvedButtonPin() in configuration.h or utils.h) and replace these duplicate blocks with a single function call.
src/input/InputBroker.cpp#L170-L178: Replace the duplicated 9-line block with a call to the new helper function.src/input/InputBroker.cpp#L329-L337: Replace the duplicated block with the helper call.src/sleep.cpp#L313-L321: Replace the duplicated block with the helper call.src/sleep.cpp#L440-L448: Replace the duplicated block with the helper call.src/sleep.cpp#L485-L493: Replace the duplicated block with the helper call.src/platform/esp32/main-esp32.cpp#L355-L363: Replace the duplicated block with the helper call.src/PowerFSM.cpp#L180-L188: Replace the duplicated block with the helper call.
📍 Affects 4 files
src/input/InputBroker.cpp#L170-L178(this comment)src/input/InputBroker.cpp#L329-L337src/sleep.cpp#L313-L321src/sleep.cpp#L440-L448src/sleep.cpp#L485-L493src/platform/esp32/main-esp32.cpp#L355-L363src/PowerFSM.cpp#L180-L188
🤖 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 `@src/input/InputBroker.cpp` around lines 170 - 178, Duplicate button GPIO
resolution should be centralized in an inline helper such as
getResolvedButtonPin(), preserving the USERPREFS_BUTTON_PIN, BUTTON_PIN, and
config.device.button_gpio precedence. Add the helper in the shared configuration
or utility location, then replace the duplicated blocks in
src/input/InputBroker.cpp lines 170-178 and 329-337, src/sleep.cpp lines
313-321, 440-448, and 485-493, src/platform/esp32/main-esp32.cpp lines 355-363,
and src/PowerFSM.cpp lines 180-188 with calls to it.
| if (_btnPin != 0xFF) { | ||
| gpioMask |= (1ULL << _btnPin); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Missing validation for user-configured button GPIO leads to undefined behavior and phantom presses.
The dynamically resolved button pin (which can be overridden by the user via config.device.button_gpio protobuf payload) is not validated before use. If an invalid or out-of-bounds pin (e.g., >= 64) is provided, it triggers severe functional and safety failures across the system. The shared root cause is the lack of a bounds check (e.g., _btnPin < 64 or ESP-IDF's GPIO_IS_VALID_GPIO(_btnPin)) for the resolved pin.
src/platform/esp32/main-esp32.cpp#L367-L369: The operation1ULL << _btnPinresults in undefined behavior (shifting beyond the width of a 64-bit integer) if the pin is>= 64. This can corrupt the wake mask or crash the firmware upon entering deep sleep. Guard this with a validity check.src/PowerFSM.cpp#L189-L189:digitalReadon an invalid pin gracefully returns0(LOW), which evaluates!digitalReadpermanently totrue. This creates an infinite wake loop flooded with phantomEVENT_PRESStransitions. Guard the read with a validity check.src/input/InputBroker.cpp#L348-L349: Instantiating the thread with an invalid pin will causeOneButton's internaldigitalReadto constantly returnLOW, resulting in continuous phantom holds/clicks inside the thread. Guard the instantiation with a validity check.
If you consolidate the pin resolution into a single helper (as suggested separately), adding the validity check inside that helper will safely fix all these sites at once.
📍 Affects 3 files
src/platform/esp32/main-esp32.cpp#L367-L369(this comment)src/PowerFSM.cpp#L189-L189src/input/InputBroker.cpp#L348-L349
🤖 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 `@src/platform/esp32/main-esp32.cpp` around lines 367 - 369, Validate the
resolved button GPIO before use, preferably in the shared pin-resolution helper,
using the platform’s GPIO validity check or an equivalent bounds check. Apply
this to the wake-mask shift in main-esp32.cpp#L367-L369, the digitalRead path in
PowerFSM.cpp#L189, and OneButton instantiation in InputBroker.cpp#L348-L349;
invalid pins must be excluded so they cannot cause undefined shifts or phantom
button events.
1 participant
This PR resolves the false interrupt and random wake issues caused by floating button pins on variants where the button is not physically wired (such as
rak3401_1watt), while fully restoring button functionality when a GPIO is explicitly assigned via the Meshtastic configuration.Changes Made:
#ifdef BUTTON_PINguards inInputBroker.cpp,PowerFSM.cpp,sleep.cpp, andmain-esp32.cppin favor of#if HAS_BUTTON._btnPinusing the following priority:config.device.button_gpio(if explicitly assigned at runtime)USERPREFS_BUTTON_PIN(if defined in userprefs)BUTTON_PIN(if defined in the variant header)0xFF), the firmware skips configuring the button pull-ups, skips initializingButtonThread, and ignores digital reads during sleep, avoiding any false positives on floating pins.?? Attestations
Fixes #10574
Summary by CodeRabbit