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

PX4Accelerometer/PX4Gyroscope: integrated data use simple rotate_3f to avoid unnecessary floating point operations #14752

Merged
merged 1 commit into from
Apr 26, 2020

Conversation

dagar
Copy link
Member

@dagar dagar commented Apr 24, 2020

Possible fix for (part of) #14735.

Screenshot from 2020-04-24 10-02-20

Related history: #11713

@dagar dagar added the bug label Apr 24, 2020
@dagar
Copy link
Member Author

dagar commented Apr 24, 2020

Screenshot from 2020-04-24 11-55-48

@dagar
Copy link
Member Author

dagar commented Apr 24, 2020

It's a bit hard to see because the raw sensor data was captured at a low rate, but if we filter both the raw data and integrated data of the same axis side-by-side you can see the difference.

Screenshot from 2020-04-24 12-02-40

This is not present in the y or z axis.

Screenshot from 2020-04-24 12-04-15

Screenshot from 2020-04-24 12-05-48

@dagar dagar requested a review from mhkabir April 24, 2020 16:17
@dagar dagar added this to the Release v1.11.0 milestone Apr 24, 2020
@dagar dagar added the Drivers 🔧 Sensors, Actuators, etc label Apr 24, 2020
@dagar dagar marked this pull request as ready for review April 24, 2020 17:22
@dagar
Copy link
Member Author

dagar commented Apr 24, 2020

This can be merged whether it fixes any potential numerical problem or not. The simple rotate_3f() helper is cheaper than the rotation matrix.

@dagar dagar changed the title PX4Accelerometer/PX4Gyroscope: integrated data avoid loss of numerical precision PX4Accelerometer/PX4Gyroscope: integrated data use simple rotate_3f to avoid unnecessary floating point operations Apr 24, 2020
@LorenzMeier LorenzMeier requested a review from bresch April 26, 2020 11:58
@LorenzMeier LorenzMeier merged commit 37d5d1b into master Apr 26, 2020
@LorenzMeier LorenzMeier deleted the pr-imu_integrated_numerical_precision branch April 26, 2020 12:28
dagar added a commit that referenced this pull request Apr 26, 2020
…FIFO case

This is a quick follow up fix to to a bug introduced by #14752. In the case of FIFO data (new IMU drivers) the calibration offset wasn't being applied correctly to the result of integrating the FIFO samples.

This slipped through basic sanity testing (simple bench testing, the test rack, and SITL CI) due to the calibration offsets being zeroed.
// scale calibration offset to number of samples
const Vector3f offset{_calibration_offset * _integrator_fifo_samples};
// integrated in microseconds, convert to seconds
const Vector3f delta_velocity_uncalibrated{_integration_raw * 1e-6f * dt * _scale};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can reduce the number of operations here if you add brackets:

delta_velocity_uncalibrated{_integration_raw * (1e-6f * dt * _scale)};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Drivers 🔧 Sensors, Actuators, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants