Stop accelerometer thread when double-tap/wake-on-motion disabled at runtime#11025
Open
thebentern wants to merge 1 commit into
Open
Stop accelerometer thread when double-tap/wake-on-motion disabled at runtime#11025thebentern wants to merge 1 commit into
thebentern wants to merge 1 commit into
Conversation
…runtime double_tap_as_button_press and wake_on_tap_or_motion are applied live: the OFF->ON edge calls accelerometerThread->start(), but there was no ON->OFF branch, so turning either flag off left the sensor thread running (polling I2C, drawing power) until reboot. Worse, because enabled stayed true, a later OFF->ON edge was a no-op (the enabled==false guard blocked re-start), leaving the feature un-restartable without a reboot. Add the symmetric ON->OFF branch in both handlers. When a flag goes true->off and the other consumer of the shared thread is also off, call accelerometerThread->disable() (stops runOnce polling and clears enabled so a later re-enable can start() again). Each branch checks the other flag first so disabling one feature never stops the sensor while the other still needs it.
Contributor
📝 WalkthroughWalkthrough
ChangesAccelerometer configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/AdminModule.cpp (1)
791-803: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winNull-pointer dereference on devices without an accelerometer.
accelerometerThreadis only allocated if compatible hardware is detected at runtime. If an admin configuration update toggling these settings is received on a node lacking an accelerometer, dereferencingaccelerometerThread->enabledwill cause a null-pointer crash. Both the newdisable()paths and the adjacent existingstart()paths need to be guarded against a null pointer.
src/modules/AdminModule.cpp#L791-L803: Add a null check before accessingaccelerometerThreadfor the device configuration toggle.src/modules/AdminModule.cpp#L902-L913: Add a null check before accessingaccelerometerThreadfor the display configuration toggle.🛡️ Proposed fixes
For
src/modules/AdminModule.cpp#L791-L803:- if (config.device.double_tap_as_button_press == false && c.payload_variant.device.double_tap_as_button_press == true && - accelerometerThread->enabled == false) { + if (config.device.double_tap_as_button_press == false && c.payload_variant.device.double_tap_as_button_press == true && + accelerometerThread && accelerometerThread->enabled == false) { config.device.double_tap_as_button_press = c.payload_variant.device.double_tap_as_button_press; accelerometerThread->enabled = true; accelerometerThread->start(); } // Turning double-tap off: stop the accelerometer thread, but only if wake-on-motion doesn't still need it. // disable() clears enabled so a later re-enable can restart it. - else if (config.device.double_tap_as_button_press == true && - c.payload_variant.device.double_tap_as_button_press == false && config.display.wake_on_tap_or_motion == false && - accelerometerThread->enabled == true) { + else if (config.device.double_tap_as_button_press == true && + c.payload_variant.device.double_tap_as_button_press == false && config.display.wake_on_tap_or_motion == false && + accelerometerThread && accelerometerThread->enabled == true) { accelerometerThread->disable(); }For
src/modules/AdminModule.cpp#L902-L913:- if (config.display.wake_on_tap_or_motion == false && c.payload_variant.display.wake_on_tap_or_motion == true && - accelerometerThread->enabled == false) { + if (config.display.wake_on_tap_or_motion == false && c.payload_variant.display.wake_on_tap_or_motion == true && + accelerometerThread && accelerometerThread->enabled == false) { config.display.wake_on_tap_or_motion = c.payload_variant.display.wake_on_tap_or_motion; accelerometerThread->enabled = true; accelerometerThread->start(); } // Turning wake-on-motion off: stop the accelerometer thread, but only if double-tap doesn't still need it. // disable() clears enabled so a later re-enable can restart it. - else if (config.display.wake_on_tap_or_motion == true && c.payload_variant.display.wake_on_tap_or_motion == false && - config.device.double_tap_as_button_press == false && accelerometerThread->enabled == true) { + else if (config.display.wake_on_tap_or_motion == true && c.payload_variant.display.wake_on_tap_or_motion == false && + config.device.double_tap_as_button_press == false && accelerometerThread && accelerometerThread->enabled == true) { accelerometerThread->disable(); }🤖 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/modules/AdminModule.cpp` around lines 791 - 803, Guard every accelerometerThread access in the device configuration toggle at src/modules/AdminModule.cpp lines 791-803 and the display configuration toggle at src/modules/AdminModule.cpp lines 902-913 with a null check, including enabled checks, start(), and disable(). Preserve the existing toggle conditions while ensuring updates on devices without an accelerometer do not dereference the null pointer.
🤖 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.
Outside diff comments:
In `@src/modules/AdminModule.cpp`:
- Around line 791-803: Guard every accelerometerThread access in the device
configuration toggle at src/modules/AdminModule.cpp lines 791-803 and the
display configuration toggle at src/modules/AdminModule.cpp lines 902-913 with a
null check, including enabled checks, start(), and disable(). Preserve the
existing toggle conditions while ensuring updates on devices without an
accelerometer do not dereference the null pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e4bc40e5-52c8-4c59-acb2-135433be5a7c
📒 Files selected for processing (1)
src/modules/AdminModule.cpp
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (29)
Build artifacts expire on 2026-08-15. Updated for |
Collaborator
|
Does this break the compass, particularly on devices that are 6+3 axis? |
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
2 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.
Problem
double_tap_as_button_press(device config) andwake_on_tap_or_motion(display config) are applied live, without a reboot — therequiresRebootequality gates inhandleSetConfigdeliberately exclude these two fields. On the OFF→ON edge, each handler callsaccelerometerThread->start()and setsenabled = true.But there was no ON→OFF branch anywhere (
grep 'accelerometerThread->'shows onlycalibrate,enabled = true, andstart()). So:enabledstayed stucktrue, a later OFF→ON edge became a no-op (theenabled == falseguard blocked the re-start()), leaving the feature un-restartable without a reboot.Fix
Add the symmetric ON→OFF branch to both handlers. When a flag transitions true→false and the other consumer of the shared thread is also off, call
accelerometerThread->disable():OSThread::disable()setsenabled = falseandsetInterval(INT32_MAX), haltingrunOnce()(no more I2C polling) without deleting the sensor. Sinceinit()is idempotent, a laterstart()cleanly resumes — no leak, fully restartable.Chose the live
disable()(symmetric with the existing livestart()) over adding these fields to therequiresRebootgate, since the flags are intentionally applied live — a reboot gate would clash with the existing live-start.Transition matrix (D = double-tap, W = wake-on-motion; only one config variant changes per
setConfig):start()disable()(off, enabled cleared)start()disable()This is a cooperative
OSThreadscheduled onmainControllerin the same loop context assetConfig, sodisable()is safe with no concurrency concern.Testing
rak4631(nRF52840) target, where the accelerometer block is active (the native/portduino test build#if-excludes this region).AdminModule.cppcompiled cleanly.Summary by CodeRabbit