search

segwayrmp / segway_rmp

ROS package for interfacing with Segway's RMP{50,100,200,400} series robotic platforms.
http://www.ros.org/wiki/segway_rmp
11 stars 16 forks source link

Publish odom twist in child_frame_id #28

Open mkoval opened 9 years ago

mkoval commented 9 years ago

This node currently publishes the twist component in the world frame, instead of the body frame. This differs from the nav_msg/Odometry message description, which says:

The twist in this message should be specified in the coordinate frame given by the child_frame_id

This pull request fixes this issue by making segway_rmp publish twists in the child_frame_id. It also fixes a bug that caused yaw rate to be reported as z-down. We had to make both of these changes to successfully use the output of segway_rmp as an input to the robot_localization package (see https://github.com/cra-ros-pkg/robot_localization/issues/221).

wjwwood commented 9 years ago

Perhaps this is the change makes the code more correct, but it is also a very breaking behavioral change. Some people may be relying on this behavior, wrong as it is. Since I don't use this anymore, I'll defer to the other maintainers which use it.

acarrillo commented 9 years ago

I also bumped into this for my own project about a year ago, and it left me headdesking pretty hard. I ended up making my own filter function that properly mutated the message fields before passing it onwards to robot_localization. Doing this right might be one of those band-aids where the sooner it's ripped off, the better! If others have run into this issue before in their projects, they probably remember it, since it's rather insidious, so the question is whether there's a loud, clear way to communicate the change to the right parties should someone decide to run their code against the latest segway_rmp package. Even after hacking to make the system work, I would welcome a change that requires me to un-do my hack.

Just my $0.02!

Best, -- Alex C.

On Thu, Aug 13, 2015 at 1:55 PM, William Woodall notifications@github.com wrote:

Perhaps this is the change makes the code more correct, but it is also a very breaking behavioral change. Some people may be relying on this behavior, wrong as it is. Since I don't use this anymore, I'll defer to the other maintainers which use it.

— Reply to this email directly or view it on GitHub https://github.com/segwayrmp/segway_rmp/pull/28#issuecomment-130841805.