Add base twist plugin for commanding the mobile base - #254
Conversation
62d5903 to
26cbb60
Compare
|
I'm not sure of the approach I'm choosing here, right now what I do is the mjData that we pass to the update method of the plugin, I'm setting them to NaNs, and then if the plugin sets them to a value, then that's what I'm using to override the original values in the mjStep |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
==========================================
+ Coverage 79.96% 80.55% +0.59%
==========================================
Files 27 30 +3
Lines 4642 4907 +265
Branches 518 538 +20
==========================================
+ Hits 3712 3953 +241
- Misses 657 668 +11
- Partials 273 286 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
eholum-nasa
left a comment
There was a problem hiding this comment.
Funnily enough we've been considering this exact problem for our ridgeback.
We do whole body motion planning with Phoebe which requires "joints" to attach the base to world (linear x, y, and a rotational yaw), so those joints existed anyway. To make the base into a magic carpet we just make those joints real and exclude contacts between the wheels, base, and floor. From there it's just a forward command controller fed from a mecanum drive controller -> joint command republisher. The full changes are in:
NASA-JSC-Robotics/phoebe_bridgeback#49
Where the magic carpet commands are just handled here.
Would something like that support your use case? We've found it more useful for handling the sim2realness of an actual mobile base
I feel like it is less invasive than adding another piece of overridable data for the plugin.
This might work too. However, I think there is quite some difference in the approach, for a beginner user modifying the XML to add new joints to make it work, instead with the proposed approach, adding a plugin to a yaml already gives him the flexibility to start and run without modifying much his simulation setup. I think it is a big win in this scenario. Moreover, Gazebo also offered similar kind of plugin in the past : https://github.com/ros-simulation/gazebo_ros_pkgs/blob/noetic-devel/gazebo_plugins/src/gazebo_ros_planar_move.cpp. I'm a bit aligned with the proposed approach for the simplicity on the user end. |
|
I agree with @saikishor that the python republishing thing is pretty complicated for new users, but also with @eholum-nasa that needing to modify Is there a solution where we don't touch the mujoco_ros2_control source and instead create an actual C++ ROS 2 controller that can publish the base in this ideal form? Then for newcomers it's just a matter of activating this controller and publishing |
As of now, there is no easier way as the floating base joints are not exposed as controllable interfaces through ROS Controls interfaces. Even though we expose, we don't have a n out-of-the-box controller that does that work for you. I can rethink this part. I agree that touching the mujoco_simulation file is not ideal, as per every implementation we come across limitations on that end. |
|
I do think our solution to use a "magic carpet controller" is a reasonable one, https://github.com/NASA-JSC-Robotics/phoebe_bridgeback/pull/49/changes I don't think those changes were too significant, a lot of them were just to handle that we have a crazy number of deployment modes to support for Phoebe. That said, the controller + the linear rails is a very straightforward way to end up with perfect odometry if that's what you're looking for? And it matches our existing control setup with the mecanum drive so it's still somewhat seamless from nav2's perspective? |
I don't know If this is the way. I don't think it is right to impose everyone to set these linear rails just to be able to be able to track their base without slipping. I think it is better to leave it simple like a plugin no?. Even with this plugin, we are publishing to the same topic /cmd_vel, so everything should be working as it is supposed to work with the proposed approach in this PR right? |
| * @note data->qvel is NaN-filled before every plugin's update() runs this cycle. Write a finite value into any | ||
| * entry to request a hard velocity override on that DOF -- it bypasses the normal mass/contact dynamics for that | ||
| * DOF this step, overwriting mj_data_->qvel directly right before the next mj_step. Leave an entry NaN to keep its | ||
| * normal physics-driven value untouched. This is the only channel a plugin has to influence qvel: unlike | ||
| * ctrl/qfrc_applied/xfrc_applied, data->qvel here cannot be read for the body's actual velocity, since it is not | ||
| * restored to a real value until after every plugin's update() has run this cycle. |
There was a problem hiding this comment.
This feels like a large can of worms to open for the sake of free-flying magic carpets, especially when the engine already supports this through other approaches (like the rails). That said, I think we can all agree that MuJoCo's wheel/contact model makes this genuinely hard, and I don't disagree with providing a mechanism to make it easier.
If we're going to open this can of worms, I think it's worth revisiting an idea we've bumped up against a few times - expanding the plugin interface with a pre-step hook that gives plugins direct access to mj_data_ immediately before mj_step in the physics loop.
Something like:
virtual void update_pre_step(const mjModel* model, mjData* data)
{
(void)model;
(void)data;
}
If we threw that in and put the CB into the PhysicsLoop then plugins really would have full god-tier access to the sim. We could remove both the qvel_override_staged_ and the xfrc_plugin_desired_ staging mechanisms. This would give plugins way more power, but IMO that would be the "right" thing to do rather than to continue digging very deep hooks deep into simulate. I am not the biggest fan of the use-case-specific staging vectors that we have to handle... so maybe that would make things easier for the next time a plugin needs access to a new field?
Thoughts?
I think I would be down to move forward with this for now if we commit ourselves to a refactor like what I've proposed above rather than add any more one-offs (rule of three).
There was a problem hiding this comment.
I agree with you. I'm not proud of this implementation. Moreover, if any plugin uses qvel for their use case, then they will receive NaNs instead of a valid qvel. This is something I dislike in this approach. I also agree with the limitations and how much this is causing the design to keep editing the primary system interface.
ndunkelb-nasa
left a comment
There was a problem hiding this comment.
Just a couple of comments to check some implementation details!
| - ``bool`` | ||
| - ``false`` | ||
| - Subscribe to ``geometry_msgs/TwistStamped`` instead of ``geometry_msgs/Twist`` (e.g. for | ||
| Nav2, which publishes stamped twists by default in some configurations). |
There was a problem hiding this comment.
I would add to this that if you set this to true, the header is thrown away, bc sometimes the assumption, especially with cmd_timeout might be that the stamp needs to be close enough to real time to work.
| if (body_id_ < 0 || qvel_adr_ < 0) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think you took care of these failure cases in init(), right?
| // Step 2 - a stale (or never-received) command is treated as a zero-velocity | ||
| // command, i.e. the base is commanded to stop rather than coast on the last override. | ||
| double vx_cmd = 0.0, vy_cmd = 0.0, wz_cmd = 0.0; | ||
| if (cached_cmd_.valid) |
There was a problem hiding this comment.
Seems like this is never not valid? Just set to true when initialized, then not ever set to false...
eholum-nasa
left a comment
There was a problem hiding this comment.
Definitely does what it promises! Side note that I appreciate all the warnings about breaking physics.
Screen.Recording.2026-08-05.at.9.51.34.AM.mov
Just a minor nit and the philosophical design question, but otherwise I think this is in a good state.
| wheels and ground have zero friction, which is now largely cosmetic -- propulsion no longer goes | ||
| through wheel-ground contact at all, so it does not depend on friction either way. | ||
|
|
||
| .. warning:: |
There was a problem hiding this comment.
Nice, this explains the danger well!
| } | ||
| else | ||
| { | ||
| twist_sub_ = node_->create_subscription<geometry_msgs::msg::Twist>(cmd_vel_topic, rclcpp::SystemDefaultsQoS(), |
There was a problem hiding this comment.
My ignorance on ros2_controllers, but are there controllers for wheeled bases that ingest twists and not stamped twists?
There was a problem hiding this comment.
Nav2 over the last few ROS distros has attempted to migrate from Twist to TwistStamped:
https://discourse.openrobotics.org/t/nav2-support-for-twiststamped-available-rolling-jazzy/35995
Note the post also mentions Gazebo and ros2_control, so ... probably stamped is the way to go since it's a superset of twist?
There was a problem hiding this comment.
Yes, it is better to go with TwistStamped. For humble versions, Nav2 was still with Twist IIRC. Having an option to toggle helps. Maybe I can change the default
| double vy{ 0.0 }; | ||
| double wz{ 0.0 }; | ||
| rclcpp::Time time{ 0, 0, RCL_ROS_TIME }; | ||
| bool valid{ false }; |
There was a problem hiding this comment.
I don't think valid is necessary? Could remove it and just let the timeout check either catch it, but everything defaults to 0 anyway.
|
I'll respond to you guys tonight once I reach home. Thank you for the review |
Description
This PR adds a new plugin that can control the base of the robot by commanding directly the floating joint it is connected to directly; it resembles the ROS planar move plugin that Gazebo offers. This is very reliable as the users doesn't have to spend time modeling their wheels and contact friction etc, and this basically helps to fast prototype
Is this user-facing behavior change?
NO
Did you use Generative AI?
Claude
Additional Information
Grabacion.de.pantalla.desde.27-07-26.15_26_29.1.webm