-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
IMU: parameterize IMU integration time (IMU_INTEG_RATE) #14759
Conversation
Historically the IMU integration time was 4000 us (250 Hz) and only very recently changed to 2500 us (400 Hz). We could also consider if 5000 us (200 Hz) is a better default. By default on most systems the rate controller (inner loop) is running at 800 Hz (synchronized with primary gyro). |
Changing the integration from 2500 us -> 5000 us saves about 7% cpu on F4 boards. This is spread out over running the attitude controller, the ekf2 frontend (heavier than expected), and a bit of logging/mavlink (HIGHRES_IMU). |
912f92b
to
f4f154d
Compare
f4f154d
to
7732f2d
Compare
Yet another factor to consider for the default is the impact on default logging rate and ekf2 replay. On a typical simple multicopter configuration the nominal logging rate changes from ~ 47 KiB/s (IMU integration 2500 us) down to ~32 KiB/s (IMU integration 5000 us). That being said, our top priority should be optimizing control and we'll work back 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.
Making 200Hz the default attitude update rate also for multicopters makes sense to me.
|
||
if (imu_integration_time_us != _param_imu_integ_time.get()) { | ||
_param_imu_integ_time.set(imu_integration_time_us); | ||
_param_imu_integ_time.commit_no_notification(); |
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.
Isn't it a problem that all Gyroscope and Accelerometer drivers would try to correct the out of range parameter more or less at the same time?
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.
I would at least define the [1000, 20000] interval shared to make sure it's always in sync.
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.
Yes I didn't like that part either. Very soon (early in the v1.12 release cycle) the plan is to gut all of this code from PX4Accelerometer/PX4Gyroscope and move it "downstream" to sensors/vehicle_imu where we can effectively synchronize accel + gyro.
One more question I forgot to ask in the review: Is there a specific reason why the new parameter is an interval while all others I know of are rates in Hz? Just thinking about the user perspective. |
Not really, I was thinking about it from the perspective of what was already hard coded and aligning with the current ecl/EKF filter update expressed as an interval (10 ms). We reset the integrator and publish the integrated value every ____ microseconds. |
Default changed to 5000 us. |
SITL appears to be broken with 5000 us IMU integration. |
@dagar we might have to adapt the gazebo side too: https://github.com/PX4/sitl_gazebo/blob/0823f5e006011d36858c2dd39881874c1edac10b/worlds/empty.world#L31-L33 |
- this goes along with the change PX4 side PX4/PX4-Autopilot#14759
- this goes along with the change PX4 side PX4/PX4-Autopilot#14759
- this goes along with the change PX4 side PX4/PX4-Autopilot#14759
CI SITL MC_avoidance and MC_safe_landing still failing due to the sitl_gazebo version. |
- this goes along with the change PX4 side PX4/PX4-Autopilot#14759
1ca9532
to
aa84ac2
Compare
I've made 2 changes to the PR.
|
3ed7a8a
to
570ee7a
Compare
a9ebe46
to
4fddd84
Compare
This effectively controls the
vehicle_attitude
publication rate and subsequently how fast the attitude controllers run. It's not something I expect regular users to to change, but this is better than having a magic hard coded number.We might want to consider lowering this by default for FW.