Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly deal with sensor rotation in AHRS #1762

Merged
merged 16 commits into from
Jul 27, 2024
Merged

Correctly deal with sensor rotation in AHRS #1762

merged 16 commits into from
Jul 27, 2024

Conversation

dellaert
Copy link
Member

@dellaert dellaert commented Jun 9, 2024

Sensor displacement rotation was not handled correctly in the AHRS factors. I spent a day tracking it down and fixed it:

  • Significantly cleaned up testAHRSFactor.cpp, now uses FactorTesting.h macros
  • To try and debug, deprecated two methods in PreintegratedRotation with hard-to-understand Jacobians
  • Instead, added a function object with sane Jacobians, and fixed bug in Jacobians (missing chain rule).
  • End-result is still not exactly how I'd like it ("insane" Jacobian in biascorrectedDeltaRij) but fixes bug

Tested:

  • add testPreintegratedRotation.cpp, all tests pass
  • Added new end-to-end integration test in testAHRSFactor.cpp, inspired by testImuFactor.cpp
  • added a test in testManifoldPreintegration.cpp to establish equivalence
  • added tests on deprecated methods, work, but deleted them in commit 1ac286f

@dellaert dellaert added bugfix Fixes an issue or bug cleanup Help clean up old/obsolete aspects of GTSAM labels Jun 9, 2024
@dellaert dellaert requested a review from andre-michelin June 9, 2024 21:54
@dellaert dellaert changed the title Fix sensor displacement in AHRS Fix sensor ~displacement~ rotation in AHRS Jun 9, 2024
@dellaert dellaert changed the title Fix sensor ~displacement~ rotation in AHRS Correctly deal with sensor rotation in AHRS Jun 9, 2024
gtsam/navigation/PreintegratedRotation.h Outdated Show resolved Hide resolved
gtsam/navigation/PreintegratedRotation.h Outdated Show resolved Hide resolved
gtsam/navigation/PreintegratedRotation.h Show resolved Hide resolved
gtsam/navigation/PreintegratedRotation.h Show resolved Hide resolved
@dellaert
Copy link
Member Author

@andre-michelin Please review and confirm these changes work in our code?

@dellaert
Copy link
Member Author

Thanks @varunagrawal , I addressed two of your comments in my local repo but will wait for Andre's comments to push, so we have to do CI only once more before merging.

@dellaert dellaert merged commit c6bd3f8 into develop Jul 27, 2024
31 checks passed
@dellaert dellaert deleted the fix/ahrs branch September 9, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes an issue or bug cleanup Help clean up old/obsolete aspects of GTSAM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants