-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 correction quaternion option #1025
Conversation
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.
Sorry for the delayed review.
I see the point in offering the user a way to take the IMU correction log print out and pass it to the Cartographer ROS config.
Yes, I have copy-pasted the quaternion from Cartographer's log output and converted it to a urdf-readable roll-pitch-yaw angle using tf python and I didn't enjoy it.
However, this is really unrelated to ROS and it can be implement in Cartographer alone.
In my opinion, the proper way to implement this feature is to change the output of Cartographer (not _ros) to print urdf-compliant RPY angles in addition.
Essentially, Cartographer would suggest to the user: How about adding this small rotation between from your imu data frame to your tracking frame, on the side of the tracking frame?
However, such implementation should not introduce additional dependencies as the benefit does not outweigh the complexity.
Is that feasible?
@@ -29,6 +29,7 @@ float64 odometry_sampling_ratio | |||
float64 fixed_frame_pose_sampling_ratio | |||
float64 imu_sampling_ratio | |||
float64 landmarks_sampling_ratio | |||
geometry_msgs/Quaternion imu_correction |
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.
No, I don't like the idea of adding a rotation to the cartographer_ros config.
So far, we leave transforms to tf and almost all users have their extrinsics in a nice urdf file (which cartographer does not need to parse itself).
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 have copy-pasted the quaternion from Cartographer's log output and converted it to a urdf-readable roll-pitch-yaw angle using tf python and I didn't enjoy it.
We're on the same wavelength 😄 I like your idea as well. In that case, we can open an issue or even an rfc for this in the libcartographer repo.
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.
Sounds good. For additional log output and no new dependencies we don't need an rfc.
Replaced by cartographer-project/cartographer#1516 |
Hi @ojura, I'm trying to understand the meaning of
Thanks in advance! |
Hi @Ellon, you are correct about 1). About 2, it’s always a PITA to determine if it is indeed that transform or its inverse. The way I would verify would be to compare imu_correction * old sensor_to_tracking * imu_ data vs new sensor_to_tracking (with correction) * imu_data. |
I agree that it would be good to have the extrinsics configured on a single place. Having the extrinsics both in the URDF file and a lua file can be confusing. Is there any plan to fix this in cartographer [1]? Master branch moved forward and is conflicting with the branch of this PR in the mean time. I really think this is a very useful feature. |
If one wants to apply the correction quaternion from Cartographer's IMU auto-calibration, this per-trajectory option enables using it verbatim, without having to modify the TF tree.