BHI260AP IMU and 2D Kalman filter#508
Conversation
… need to double-check Kalman filter implementation
…needs re-validatino
tkevinbest
left a comment
tkevinbest
left a comment
There was a problem hiding this comment.
Hi Katharine! Overall it looks good! I left a few questions in the code. Some other thoughts before we merge:
- Right now, it isn't passing the automated checks. Check out the contribution instructions page and it'll tell you how to set up a venv with uv and run all the checks to see what isn't passing.
- Is any of the driver code reused from another source or implementation? If so we need to credit the original. Same with the kalman filter.
- Thanks for making tutorial scripts! These should be paired with markdown docs that explain the tutorial. Can you add them for your new tutorials? Let me know if you have issues adding them.
- We need some unit tests for the new features! These can probably mirror those of the microstrain class, if there are any.
Otherwise looks good! I trust you've tested it and are currently using it haha.
…d reckoning update
|
@tkevinbest To address your other comments: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
I tried to address all of the test cases that were failing, but I don't know what to do about the last ones. If anyone could explain to me what those are I would appreciate it! |
|
@Katharine-Walters looks good! I've added the docs to the navigation system and tested them. I've also committed some initial copilot unit tests. I looked through them and they seem reasonable, but it would be good if you can scan them too. I've also fixed any remaining ruff issues that were failing. See this PR to bring in my changes: Katharine-Walters#1 |
docs: update imu and KF docs, fixes line refs
tkevinbest
left a comment
tkevinbest
left a comment
There was a problem hiding this comment.
Aside from the two remaining questions regarding the snippet in the docs and the __repr__, this looks good to go to me. @senthurayyappan or @ellliewilson have any thoughts before approving?
| """ | ||
| def __init__( | ||
| self, | ||
| tag: str = "KalmanFilter2D", |
There was a problem hiding this comment.
We can eliminate this right?
Or make it a repr response?
| tag: str = "KalmanFilter2D", | |
| tag: str = "KalmanFilter2D", | |
| ... | |
| self._tag = tag | |
| ... | |
| def __repr__(self): | |
| return self._tag |
| ## IMU Parameters | ||
|
|
||
| When initializing the BHI260AP, several important parameters can be configured: | ||
|
|
||
| ```python | ||
| --8<-- "tutorials/sensors/reading_bhi260ap_imu_data.py:14:23" | ||
| ``` |
There was a problem hiding this comment.
I dont' see what this snipped originally referenced in terms of line numbers. I got the rest updated
@tkevinbest The BHI260AP IMU is the one on the OSL electronics board; we are waiting on another shipment of them to do hardware tests for this PR, then it should be good to merge after :) |
|
@Katharine-Walters if you run |
|
@tkevinbest is there a way to run that without setting up the virtual environment? I tried doing that on my PC but Python environments make me crazy and I couldn't get it to work |
|
I typically use a codespace on github for pure code changes. Then the virtual environment works super easily |
|
Ah, that helps so much! |
|
Hey @ellliewilson and @tkevinbest, I'm merging this PR with main to address a git situation with the TMotor changes, which was mostly my blunder. Can we test @Katharine-Walters's |
4 participants
feat: added sensor class for BHI260AP IMU, added sensor class for IMU axis transformations, added filters utility with 2D Kalman filter implementation to estimate roll and pitch
Note 1: Added tutorial for reading from IMU and tutorial for running Kalman filter with axis transformation
Note 2: Kalman filter implementation tested against LordMicrostrain GX5