Alt hold with accel support - #232
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds AHRS and complementary filter primitives, rewires fusion and altitude/barometer processing, changes accelerometer state access to a seqlock wrapper, adds filter-delay reporting to the CLI, updates configuration, adds AHRS tests, and applies formatting changes. ChangesAHRS Fusion and Sensor Pipeline Rework
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/Espfc/src/ModelConfig.h (1)
47-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClamp legacy fusion modes on load Raw
fusion.modevalues are still accepted, so old configs can exceedFUSION_MAXand fall through the default branch inFusion::update(). Clamp or remap out-of-range values during load/parsing.🤖 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 `@lib/Espfc/src/ModelConfig.h` around lines 47 - 53, Clamp or remap legacy raw fusion mode values when loading/parsing ModelConfig so they cannot exceed FUSION_MAX and later fall into the default path in Fusion::update(). Update the config deserialization/loading logic that populates fusion.mode in ModelConfig.h to validate the enum value and coerce any out-of-range or legacy value to a supported FusionMode before it is used.
🧹 Nitpick comments (2)
lib/Espfc/src/Utils/Filter.cpp (1)
498-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated notch/BPF Q-approximation formula.
The Q estimate
(cutoff * fc) / ((fc - cutoff) * (fc + cutoff))reimplements exactly the same formula already provided byFilter::getNotchQApprox. Extracting a shared free function (or makinggetNotchQApproxa static/free helper reusable here) would avoid the two formulas drifting apart if one is tuned later.♻️ Suggested consolidation
- const float q = std::clamp((cutoff * fc) / ((fc - cutoff) * (fc + cutoff)), 0.0f, 100.0f); + const float q = std::clamp(Filter::getNotchQApprox(fc, cutoff), 0.0f, 100.0f);(requires
getNotchQApproxto bestatic, or extracting the formula into a free function shared by both.)🤖 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 `@lib/Espfc/src/Utils/Filter.cpp` around lines 498 - 508, The Q approximation in the FILTER_NOTCH/FILTER_NOTCH_DF1/FILTER_BPF branch duplicates the formula already defined in Filter::getNotchQApprox, so consolidate it to avoid divergence. Refactor the shared Q calculation into a reusable helper by either making getNotchQApprox a static method or extracting a free function, then call that helper from this switch branch instead of inlining the formula.lib/Espfc/src/Sensor/BaroSensor.hpp (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_altitudeFilterlooks unused.begin()no longer initializes it andupdateAltitude()doesn’t reference it anymore, so remove the member if nothing else still depends on it.🤖 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 `@lib/Espfc/src/Sensor/BaroSensor.hpp` at line 36, The BaroSensor class still declares `_altitudeFilter`, but it is no longer initialized in `begin()` or used in `updateAltitude()`. Remove the unused `_altitudeFilter` member from `Sensor::BaroSensor` and verify there are no remaining references to it elsewhere in the class or related implementation.
🤖 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 `@lib/AHRS/src/Rtqf.hpp`:
- Around line 98-110: Clamp rotationError.w before calling acos in Rtqf’s
rotation interpolation logic, and avoid normalizing rotationVector when the
rotation error is effectively identity. Update the rotation-power calculation
path in the same block so that if the error magnitude is near zero you skip
rotationVector.normalize() and directly use the identity rotation, preventing
NaNs from being written into _quaternion.
In `@lib/Espfc/src/Connect/Cli.cpp`:
- Around line 1282-1299: The IMU delay calculation in Cli::printFilters uses a
hard-coded PT1 term based on aRate / 3 that does not correspond to any actual
filter stage. Update the imuDelay logic to derive the added delay from the real
IMU/accelerometer configuration used in Fusion::begin() and
AccelSensor::begin()/filter(), or otherwise make it explicit that this is only
an estimate. Keep the existing gyroDelay, accelDelay, and qDelay reporting
unchanged, but replace the synthetic contribution so the CLI status reflects the
true configured pipeline.
- Line 1317: The use of std::size in Cli.cpp relies on a transitive include, so
make the dependency explicit by adding the appropriate header in the translation
unit. Update the includes in the file containing the armingDisableNamesLength
initialization so std::size is provided directly, and keep the existing use in
the code that computes armingDisableNamesLength unchanged.
In `@lib/Espfc/src/Control/Fusion.cpp`:
- Around line 68-74: Normalize the filtered quaternion returned by
filterQuaternion in Fusion::update before using it for a.getRotated and
computing _model.state.attitude.cosTheta, since the biquad-filtered quaternion
can drift from unit length and VectorBase::getRotated assumes a normalized
rotation. Keep the existing filterQuaternion flow in Fusion.cpp, but renormalize
fq immediately after filtering so the accel world transform and cosine-theta
calculation stay correct.
In `@lib/Espfc/src/Sensor/BaroSensor.cpp`:
- Around line 129-132: Seed BaroState::altitudePrev before the first vario
calculation so BaroSensor::updateAltitude() never reads an indeterminate
previous altitude on startup. Initialize altitudePrev in BaroSensor::begin() or
reset the full baro state there before calling _varioFilter.update(), and keep
the initialization aligned with the existing altitude/altitudeGround setup.
---
Outside diff comments:
In `@lib/Espfc/src/ModelConfig.h`:
- Around line 47-53: Clamp or remap legacy raw fusion mode values when
loading/parsing ModelConfig so they cannot exceed FUSION_MAX and later fall into
the default path in Fusion::update(). Update the config deserialization/loading
logic that populates fusion.mode in ModelConfig.h to validate the enum value and
coerce any out-of-range or legacy value to a supported FusionMode before it is
used.
---
Nitpick comments:
In `@lib/Espfc/src/Sensor/BaroSensor.hpp`:
- Line 36: The BaroSensor class still declares `_altitudeFilter`, but it is no
longer initialized in `begin()` or used in `updateAltitude()`. Remove the unused
`_altitudeFilter` member from `Sensor::BaroSensor` and verify there are no
remaining references to it elsewhere in the class or related implementation.
In `@lib/Espfc/src/Utils/Filter.cpp`:
- Around line 498-508: The Q approximation in the
FILTER_NOTCH/FILTER_NOTCH_DF1/FILTER_BPF branch duplicates the formula already
defined in Filter::getNotchQApprox, so consolidate it to avoid divergence.
Refactor the shared Q calculation into a reusable helper by either making
getNotchQApprox a static method or extracting a free function, then call that
helper from this switch branch instead of inlining the formula.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e1b13788-f945-4b3a-9f2e-71c6da192345
📒 Files selected for processing (22)
.clang-formatbin/bbread.pylib/AHRS/src/Complementary.hlib/AHRS/src/Rtqf.hpplib/AHRS/src/helper_3dmath.hlib/Espfc/src/Blackbox/Blackbox.cpplib/Espfc/src/Connect/Cli.cpplib/Espfc/src/Connect/MspProcessor.cpplib/Espfc/src/Control/Altitude.hpplib/Espfc/src/Control/Fusion.cpplib/Espfc/src/Control/Fusion.hlib/Espfc/src/ModelConfig.hlib/Espfc/src/ModelState.hlib/Espfc/src/Sensor/AccelSensor.cpplib/Espfc/src/Sensor/AccelSensor.hlib/Espfc/src/Sensor/BaroSensor.cpplib/Espfc/src/Sensor/BaroSensor.hpplib/Espfc/src/Sensor/GyroSensor.cpplib/Espfc/src/Utils/Filter.cpplib/Espfc/src/Utils/Filter.hlib/Espfc/src/Utils/Math.hpplib/Espfc/src/Utils/SeqLockWrapper.hpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/AHRS/src/Rtqf.hpp (1)
51-57: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
f-suffixed math calls on the fast path.cos/sin/atan2here, andacos/sin/cosinapplyRtqf, go through double-precision math;atan2fis already used below, so keep this code in float math withcosf/sinf/atan2f/acosf.🤖 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 `@lib/AHRS/src/Rtqf.hpp` around lines 51 - 57, Replace the unsuffixed cos, sin, and atan2 calls in the shown pose calculation with cosf, sinf, and atan2f; also update the corresponding acos, sin, and cos calls in applyRtqf to acosf, sinf, and cosf, preserving the existing float-based calculations.
🤖 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 `@lib/AHRS/src/Rtqf.hpp`:
- Around line 84-123: Normalize the predicted quaternion immediately after the
four first-order integration assignments in the AHRS update method, before
calculating the rotation delta via q.getConjugate() * _poseQ. This ensures the
skip path when mag <= 1e-9f stores a unit quaternion and keeps getQuaternion
stable.
---
Nitpick comments:
In `@lib/AHRS/src/Rtqf.hpp`:
- Around line 51-57: Replace the unsuffixed cos, sin, and atan2 calls in the
shown pose calculation with cosf, sinf, and atan2f; also update the
corresponding acos, sin, and cos calls in applyRtqf to acosf, sinf, and cosf,
preserving the existing float-based calculations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 72681f4b-3f74-40d9-aa04-9c0febfc41b3
📒 Files selected for processing (1)
lib/AHRS/src/Rtqf.hpp
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Merged
Merged
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
1 participant
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 by CodeRabbit
New Features
statuswith a new “filters” section showing estimated filter delays; addedfusion_use_magandpid_althold_baro_tau.Bug Fixes
Tests