-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Fixing BMI088 IMU connected via I2C #23141
base: main
Are you sure you want to change the base?
Conversation
@mrpollo - here is a log file - rover makes a couple turns in manual mode. I have MPU9250/SPI as https://review.px4.io/plot_app?log=e4e0e80c-3d80-490c-ae9b-821a76a0f3a0 |
Just FYI: I have a version of bmi088_i2c driver that works for my rovers well. It is cleaned off all the FIFO code and I revamped some register settings for clarity. I use low frequencies for low pass filters too, so that all vibrations are filtered out. I also added additional EMA filter to smooth the data even further in controlled manner. https://github.com/slgrobotics/PX4-Autopilot/tree/main/src/drivers/imu/bosch/bmi088_i2c |
@@ -908,11 +913,29 @@ bool BMI088_Accelerometer::NormalRead(const hrt_abstime ×tamp_sample) | |||
const int16_t accel_y = combine(RATE_Y_MSB, RATE_Y_LSB); | |||
const int16_t accel_z = combine(RATE_Z_MSB, RATE_Z_LSB); | |||
|
|||
if (accel_x == INT16_MIN) { | |||
PX4_WARN("accel_x == INT16_MIN"); |
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.
Generally don't print messages in drivers, they can run at high rate, spam the console, and even hurt realtime performance.
If you want to track anything more like this I'd suggest adding perf counters.
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.
Agreed. This was a temporary measure to see if CrazyFlie test (if ever happening) would produce any warnings. BTW, this happens at low rate randomly and doesn't overwhelm the console.
PX4_WARN("accel_z == INT16_MIN"); | ||
} | ||
|
||
if (accel_x == INT16_MIN || accel_y == INT16_MIN || accel_z == INT16_MIN) { |
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.
Is this described in the datasheet? What about INT16_MAX?
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.
INT16_MIN is binary "all ones" and seems to be a fluke in FIFO reading process. I didn't see anything about it in the datasheet, only some mentioning in forums. Direct reads from registers (opposed to FIFO) don't seem to produce those.
@slgrobotics can you try the BMI088_I2C fixes in this stale branch? #19632 I had the BMI088 on I2C working on a different system (not crazyflie) and was able to continue using the FIFO. I2C had to be at 400 kHz. |
I'm always interested in additional filtering, but we try to implement it downstream of the driver for a few reasons.
|
@dagar -
|
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: |
Solved Problem
In my testing of Bosch Sensortec BMI088 IMU data logged to an ulg file had strange spikes and was not good for EKF either.
Fixes #23111
Solution
It looks like there's a problem reading FIFO - at least for I2C connected devices.
Reading data directly from device registers (for both accelerometer and gyroscope) - using NormalRead() functions seem to solve this issue.
Test coverage
I use a Raspberry Pi 4 and SeeedStudio BMI088 breakboard to test this code, as this is what I have on my two rovers.
More testing on Bitcraze CrazyFlie 2.1 hardware should be done, as this is the only PX4-supported platform that uses I2C for this IMU. All others connect BMI088 via SPI and therefore use different driver.
Notes:
I believe that the code for SPI-connected devices has an error when incoming values are compared with INT16_MIN. If any of X, Y or Z values are equal to INT16_MIN - whole sample must be discarded.
This is described in BMI088 connected via I2C does not work right[Bug] #23111
There's a lot of unused FIFO-related code in bmi088_i2c driver. It can be removed as we progress with this PR.