Stop accelerometer thread when double-tap/wake-on-motion disabled at runtime#11025
Merged
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesAccelerometer configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant AdminModule
participant AccelerometerThread
participant MotionSensor
AdminModule->>AccelerometerThread: Reconcile device or display feature transition
AccelerometerThread->>MotionSensor: Query providesHeading()
MotionSensor-->>AccelerometerThread: Return heading capability
AccelerometerThread-->>AdminModule: Enable, disable, or retain thread state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
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-16. Updated for |
Collaborator
|
Does this break the compass, particularly on devices that are 6+3 axis? |
Member
Not any more |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/motion/MotionSensor.h`:
- Around line 45-49: Shorten the comment above providesHeading() to no more than
two lines, retaining only the essential capability and runtime-thread behavior
details while removing redundant examples and obvious explanation. Leave the
providesHeading() implementation unchanged.
🪄 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: 91be94b0-d4a7-48d3-87a7-d106b16fb469
📒 Files selected for processing (6)
src/modules/AdminModule.cppsrc/motion/AccelerometerThread.hsrc/motion/BMM150Sensor.hsrc/motion/BMX160Sensor.hsrc/motion/ICM20948Sensor.hsrc/motion/MotionSensor.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/AdminModule.cpp
The device and display config handlers had mirror-image blocks reconciling the shared accelerometer thread. Extract reconcileAccelerometerThread(wasOn, nowOn, otherFeatureOn) so the null guard, edge logic, compass (providesHeading) guard, and rationale live in one place; each call site is now a single call. Behavior is unchanged. Also drops the redundant per-field assignment that the whole-struct `config.device = ...` / `config.display = ...` overwrites anyway.
thebentern
force-pushed
the
claude/compassionate-tesla-de5f79
branch
from
July 18, 2026 11:20
038841d to
44a9247
Compare
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.
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