-
Notifications
You must be signed in to change notification settings - Fork 7
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
ladislas/feature/replace mahony with new fusion #1287
ladislas/feature/replace mahony with new fusion #1287
Conversation
ladislas
commented
Feb 6, 2023
•
edited
Loading
edited
- 🚚 (spikes): Rename accel_gyro to sensors_imu_lsm6dsox
- 🔥 (IMUKit): Remove Mahony implementation
Version comparison
|
File comparision analysis report🔖 Info
Click to show memory sections
📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
File comparision analysis report🔖 Info
Click to show memory sections
📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
Codecov Report
@@ Coverage Diff @@
## develop #1287 +/- ##
===========================================
+ Coverage 98.70% 98.73% +0.02%
===========================================
Files 147 145 -2
Lines 3789 3715 -74
===========================================
- Hits 3740 3668 -72
+ Misses 49 47 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
689821d
to
e386be6
Compare
0e5866e
to
c282404
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal comments
// TODO(@ladislas): to implement | ||
// ? Reseting the algorithm might not be the best answer as it takes a few second to stabilize | ||
// ? Because the readings are very stable, it would be easier to take the current yaw | ||
// ? and start counting from there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be implemented in a follow up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c6a0495
to
63007fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LTGM
Still have a question about testing. IMUKit unit tests are very light and functional tests are not updated yet. Is fusion inertial calibration which prevent to write generic functional tests or you plan to do some ?
63007fb
to
1d394b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate with
- lk_imu_kit spike
The whole integration looks good to me. I have some suggestions:
- Complete a unit test
- Fix failing test in CI for clang-format
- Add some "documentation"
Also found a potential bug
1d394b4
to
cb305b1
Compare
cb305b1
to
a735c11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good for me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Kudos, SonarCloud Quality Gate passed! |