search

heuristicus / spot_ros

ROS driver for controlling Boston Dynamics' Spot robot
https://heuristicus.github.io/spot_ros/
Other
268 stars 136 forks source link

optionally publish parent odom tf #105

Closed sei-jmattson closed 1 year ago

sei-jmattson commented 1 year ago

This supports case where a payload publishes map and odom frames and doesn't want the platform interfering.

jeremysee2 commented 1 year ago

Seems fine, what would be the scenario where this flag would be useful?

sei-jmattson commented 1 year ago

If one has a custom sensor payload that publishes odom/map, and spot_ros is publishing odom as parent of body also, the tf tree gets broken. This PR defaults to "no change" but allows us to ignore spot_ros odom tf. Maybe there is a better way? But we've been using this in our fork (and hoping to eliminate that fork!)

jeremysee2 commented 1 year ago

That makes sense to me, the change isn't obtrusive and people who don't make use of it can leave it on the default value.

heuristicus commented 1 year ago

I'm not sure I fully understand why the TF tree breaks in that case. It's permissible to publish odom->map and odom->body at the same time.

If the problem is a name clash for some reason, it should be a fairly small change to modify the frame names to fix things, so this modification shouldn't be needed.

This is a minor change, but I'd like to understand the rationale a bit more.

sei-jmattson commented 1 year ago

With our custom payload, the tf tree is map->odom->body; so competing publishers of body's parent breaks the tree.

For discussion it might be helpful to distinguish between a payload feeding the platform vs. the payload driving the platform. In that second case, one might want the payload to handle tf tree above body.

sei-jmattson commented 1 year ago

Let's pause this PR? You've prompted me to try again getting our payload to play nice with spot_ros' odom->body tf.

TankyFranky commented 1 year ago

I would very much promote this change. The way spot_ros.py and ros_helpers.py publishes the odom->body transform, it breaks other existing localization packages. Weirdly enough I was trying to come up with a solution for this today, and the suggestion was posted 3 hours ago :thinking: . I will be adding this to my local version.

This change would allow spot to localize using the robot_localization package using additional on board sensors.