CCNYRoboticsLab / imu_tools

ROS tools for IMU devices
Other
913 stars 428 forks source link

TF confusion #29

Closed mikepurvis closed 10 years ago

mikepurvis commented 10 years ago

It seems very odd to me that the frame_id from data_raw is not simply copied directly to the data output message.

What is the rationale for the fixed frame value, and the optional TF publication?

mintar commented 10 years ago

Very good point; I think you're right that this is a bug (especially because the default value of fixed_frame is set to odom, which doesn't make sense and only confuses new users). I always set the fixed_frame value to imu, which happens to be the frame_id of the data_raw topic, so in effect it is copying the frame_id.

About the publish_tf parameter: I think this is just for debugging, when you want to visualize the IMU output without running robot_pose_ekf.

I'd support ignoring the fixed_frame param and copying the frame_id directly, as suggested.

If we also remove the fixed_frame param completely, we'll also have to remove the publish_tf param (because it uses fixed_frame as a parent frame for the transform). Maybe it's best to keep both for backwards compatibility and easy testing.

The only catch is that I have no idea whether people already use the fixed_frame param to do weird things, and changing the behaviour would silently break their setup. Any thoughts? @mikepurvis? @jspricke?

jspricke commented 10 years ago

I would simply remove this line: https://github.com/ccny-ros-pkg/imu_tools/blob/master/imu_filter_madgwick/src/imu_filter.cpp#L230, it got introduced in #5. @sameerparekh any comments?

mintar commented 10 years ago

+1

mikepurvis commented 10 years ago

LGTM