Incorrect frame_id and child_frame_id
Closed this issue · 5 comments
Describe the bug
The frame_id of the /ekf/velocity topic and the child_frame_id of the odometry earth topics (/ekf/odometry_earth, /gnss_1/odometry_earth, and /gnss_2/odometry_earth) topic are incorrect.
According to the ROS wiki page, the twist message published on the /ekf/velocity topic is in NED frame. Thus, the frame_id of the ROS message should be the map frame, whose name is configured by means of the the map_frame_id parameter. However, the publisher uses the frame_id parameter (i.e. the sensor frame), see line 139 of publishers.cpp.
For the three odometry earth topics (/ekf/odometry_earth, /gnss_1/odometry_earth, and /gnss_2/odometry_earth) , the odometry messages have both fields, frame_id as well as child_frame_id, set to the earth_frame_id parameter, see lines 132-133 and lines 141-142 in publishers.cpp. The child_frame_id of the messages should be set to the frame_id parameter instead of the earth_frame_id parameter. Otherwise the reported poses make no sense. See the odometry message definition.
To Reproduce
Clone the ros2 branch of the repository and activate the mentioned topics in the configuration by setting their rate >0. Then do
ros2 topic echo /ekf/velocity
ros2 topic echo /ekf/odometry_earth
ros2 topic echo /gnss_1/odometry_earth
ros2 topic echo /gnss_2/odometry_earth
Expected behavior
The messages on /ekf/velocity topic use the map_frame_id parameter for the frame_id in the header. The messages on the odometry earth topics use the frame_id parameter for the child_frame_id message field.
Environment
- OS: Ubuntu 20.04
- Architecture: x86_64
- ROS Version: humble (from source)
- Version: ros2-4.0.0
- Sensor(s): 3DM-GQ7
Modifications
No modifications have been performed.
@martfra Thank you very much for your feedback. I think there are two seperate issues here.
/ekf/velocity
The TwistWithCovarianceStamped message does not explicitly state what the frame_id in the header should represent. Because of this we struggled to figure out how we should make this data available but eventually landed on the current solution as the frame_id allows users to lookup the vehicle frame lever arm offset for where the velocity was sampled and therefore could be used for angular rate compensation. However, our main purpose of this change was to make the driver easier to integrate with other services. Are you attempting to use the driver with another service that expects a certain frame_id in the ekf/velocity message?
/*/odometry_earth
When initially determining how we would output these messages, we looked at the noetic definition of the Odometry message and as it states there:
The twist in this message should be specified in the coordinate frame given by the child_frame_id
And that holds true for these messages since the twist component is populated with ECEF velocity. However, after looking at the more recent message definition you sent and thinking about the use cases that most consumers of the odometry message would have, I agree that the child_frame_id should be changed to the value of frame_id and instead of ECEF velocity, it should be populated with vehicle frame velocity. I will be making those changes soon
@robbiefish, thank you for the explanation.
No, I am not using the ekf/velocity topic with another service. It was just confusing to me because just by reading the frame_id field I concluded that the velocity is given in the same frame. I think this is the only way the twist message makes sense. Do you have specific services in mind which expect the current frame_id?
Thank you for considering to implement changes for the ekf/odometry_earth topic. This would also make the ekf/odometry_earth topic consistent with the ekf/odometry_map topic. The messages on the ekf/odometry_map topic have the child_frame_id set to the frame_id parameter and populate velocity in vehicle frame.
This issue is stale because it has been open for 2 weeks with no activity. If the issue is still not resolved, please leave a comment describing what is still not working
The changes to /*/odometry_earth have been integrated into 4.1.0
I looked around to see if there were any other nodes which published NED or ECEF velocity so that we could behave the same as them. I ended up finding that the ublox also uses the frame_id of the sensor. It is not a very large sample group to pull from, but I am going to keep the frame_id of the velocity messages the same for now. If this becomes a big issue in the future, we can reassess this.
Thanks for the update. Let's close this issue.