Position hold small improvements - #15366
Conversation
|
Do you want to test this code? You can flash it directly from the Betaflight App:
WARNING: It may be unstable. Use only for testing! |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPosition-hold XY state now uses vector-backed storage and revised sanity-distance handling in ChangesMultirotor position-hold and heading gain updates
Sequence Diagram(s)sequenceDiagram
participant positionControl
participant position estimator
participant nav subsystem
positionControl->>position estimator: read current XY position
position estimator-->>positionControl: XY position
positionControl->>nav subsystem: use target velocity when nav active
nav subsystem-->>positionControl: target velocity
positionControl->>positionControl: compute sanity distance and PID terms
positionControl-->>positionControl: set sticksActive and zero roll/pitch on failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/flight/imu.c (1)
459-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the healthy-mag suppression path in the IMU unit test.
This branch now changes
groundspeedGainwhensensors(SENSOR_MAG) && compassIsHealthy()is true, but the currentsrc/test/unit/flight_imu_unittest.ccstub only enablesSENSOR_ACCwhile still returningtrueforcompassIsHealthy(). That leavesMagSuppressionat1.0fin tests, so the new 0.1x path can regress unnoticed.🤖 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/main/flight/imu.c` around lines 459 - 465, The healthy-magnetometer suppression branch in the IMU logic is not covered by the unit test setup, so the new 0.1f path can regress unnoticed. Update the flight IMU test in src/test/unit/flight_imu_unittest.cc so it enables SENSOR_MAG in the sensor stub while compassIsHealthy() still returns true, and verify the behavior exercised through the IMU update logic that uses MagSuppression/groundspeedGain. Keep the existing ACC stub, but add a test case that explicitly hits the sensors(SENSOR_MAG) && compassIsHealthy() path.
🤖 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/main/flight/autopilot_multirotor.c`:
- Around line 452-456: The autopilot angle reset in autopilotMultirotor should
not depend on navActive, since manual stick input must always zero the outputs.
Update the condition around the autopilotAngle[AI_ROLL] and
autopilotAngle[AI_PITCH] assignments so any ap.sticksActive state forces both
angles to 0.0f before the rotation branch runs, keeping the fix localized in the
autopilotMultirotor control flow.
- Around line 318-320: The position-hold startup in resetPositionControl is
enabling held mode before the hold target is initialized, which leaves
positionControl and updatePositionHoldTarget working with stale or zero
targetPosition/posHoldStartPosition data. Initialize the hold target first by
calling the position-hold setup logic (the same initialization used by
initPositionHold and positionControl) before setting isPositionHeld true, so the
first held-mode cycle uses the current XY position instead of skipping target
initialization.
---
Nitpick comments:
In `@src/main/flight/imu.c`:
- Around line 459-465: The healthy-magnetometer suppression branch in the IMU
logic is not covered by the unit test setup, so the new 0.1f path can regress
unnoticed. Update the flight IMU test in src/test/unit/flight_imu_unittest.cc so
it enables SENSOR_MAG in the sensor stub while compassIsHealthy() still returns
true, and verify the behavior exercised through the IMU update logic that uses
MagSuppression/groundspeedGain. Keep the existing ACC stub, but add a test case
that explicitly hits the sensors(SENSOR_MAG) && compassIsHealthy() path.
🪄 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
Run ID: a7bc4c3d-4477-4626-a4d2-07f323284d3c
📒 Files selected for processing (2)
src/main/flight/autopilot_multirotor.csrc/main/flight/imu.c
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/unit/poshold_unittest.cc (1)
193-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-add assertions for the failure side effects.
positionControl()now returnsfalseonly after it also hands stick control back, levels roll/pitch, and writes the failure debug/status50insrc/main/flight/autopilot_multirotor.c:324-386. With onlyEXPECT_FALSE, regressions in those outputs would still pass even though they are part of this PR’s stated behavior.🤖 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/test/unit/poshold_unittest.cc` around lines 193 - 196, The poshold unit test only checks the boolean result from positionControl(), so regressions in the failure side effects can slip through. Update the test around positionControl() to also assert the expected failure behavior: stick control is handed back, roll/pitch are leveled, and the failure debug/status value 50 is written when the sanity check distance is exceeded.
🤖 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.
Nitpick comments:
In `@src/test/unit/poshold_unittest.cc`:
- Around line 193-196: The poshold unit test only checks the boolean result from
positionControl(), so regressions in the failure side effects can slip through.
Update the test around positionControl() to also assert the expected failure
behavior: stick control is handed back, roll/pitch are leveled, and the failure
debug/status value 50 is written when the sanity check distance is exceeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2000e552-7a41-4e8e-87f4-8d17c2af580c
📒 Files selected for processing (1)
src/test/unit/poshold_unittest.cc
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/unit/poshold_unittest.cc (1)
193-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep assertions for the new flyaway fail-safe side effects.
This change now appears to only check the
falsereturn, but the new contract also levels roll/pitch, re-enables manual sticks, and writes debug status50on sanity failure (src/main/flight/autopilot_multirotor.c, Lines 361-383). Please keep explicit assertions for those side effects here so this fail-safe path cannot regress silently.🤖 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/test/unit/poshold_unittest.cc` around lines 193 - 196, The positionControl() sanity-failure test currently only asserts the false return, but it should also verify the flyaway fail-safe side effects introduced in autopilot_multirotor behavior. Update the poshold_unittest case that manipulates testEstimate.position.x and calls positionControl() to explicitly assert roll/pitch are leveled, manual sticks are re-enabled, and the debug status is set to 50 when the sanity check fails, using the existing positionControl() test fixture and related state accessors.
🤖 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.
Nitpick comments:
In `@src/test/unit/poshold_unittest.cc`:
- Around line 193-196: The positionControl() sanity-failure test currently only
asserts the false return, but it should also verify the flyaway fail-safe side
effects introduced in autopilot_multirotor behavior. Update the poshold_unittest
case that manipulates testEstimate.position.x and calls positionControl() to
explicitly assert roll/pitch are leveled, manual sticks are re-enabled, and the
debug status is set to 50 when the sanity check fails, using the existing
positionControl() test fixture and related state accessors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b7948be-f2cb-407f-a7cb-458f404288ee
📒 Files selected for processing (2)
src/main/flight/autopilot_multirotor.csrc/test/unit/poshold_unittest.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/flight/autopilot_multirotor.c
* Don't adapt a mag heading to GPS COG unless there is very strong forward flight * Refactoring, improve sanity check distances and behaviour * fix unit test for sanity check to test to expect return false * lock sticks out of nav modes, minor refactoring * change unit tests * whitespace
This is a tidying up PR to follow #15361.
The main change is to block stick input from taking control during a Nav mode. Stick inputs in Nav Mode will be ignored. The Pilot must exit Nav mode to regain control. Previously, any time a pilot moved the sticks, the stick inuts would control the craft, while the Nav PIDs, iterms etc.remained active in the background.
Other minor functional changes:
Refactoring:
-use vectors and vector maths, not float arrays.
Position hold now works really well. Stops like a rock in just 1.2s from 8m/s existing velocity, using 45 deg pitch.Super clean and smooth motor requests. Amazing.
Todo:
Summary by CodeRabbit
Summary of Changes
Bug Fixes
Tests