Skip to content

MuJoCo ros2 control plugins#133

Open
saikishor wants to merge 11 commits intoros-controls:mainfrom
pal-robotics-forks:mujoco_ros2_control_plugins
Open

MuJoCo ros2 control plugins#133
saikishor wants to merge 11 commits intoros-controls:mainfrom
pal-robotics-forks:mujoco_ros2_control_plugins

Conversation

@saikishor
Copy link
Member

This PR proposes an approach to easily integrate the user's code to fetch information from the mujoco system hardware component, and decouples the complexity into the main code, and also gives the user some flexibility.

@saikishor
Copy link
Member Author

@Juliaj FYI

@Juliaj
Copy link
Contributor

Juliaj commented Mar 3, 2026

@Juliaj FYI

@saikishor, thanks, this is awesome. I can think two use cases immediately

  • Get velocimeter without exposing it 😃
  • Potential reward calculation

Will look for opportunities to utilize it.

@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2026

Codecov Report

❌ Patch coverage is 74.13793% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.92%. Comparing base (0795d08) to head (09a3a20).

Files with missing lines Patch % Lines
...ujoco_ros2_control/src/mujoco_system_interface.cpp 68.75% 7 Missing and 3 partials ⚠️
...control_plugins/src/heartbeat_publisher_plugin.cpp 77.27% 0 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #133      +/-   ##
==========================================
+ Coverage   72.89%   72.92%   +0.02%     
==========================================
  Files           9       12       +3     
  Lines        2509     2567      +58     
  Branches      248      266      +18     
==========================================
+ Hits         1829     1872      +43     
- Misses        572      579       +7     
- Partials      108      116       +8     
Flag Coverage Δ
unittests 72.92% <74.13%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...os2_control_plugins/heartbeat_publisher_plugin.hpp 100.00% <100.00%> (ø)
...ntrol_plugins/mujoco_ros2_control_plugins_base.hpp 100.00% <100.00%> (ø)
...control_plugins/src/heartbeat_publisher_plugin.cpp 77.27% <77.27%> (ø)
...ujoco_ros2_control/src/mujoco_system_interface.cpp 53.60% <68.75%> (+0.41%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@saikishor
Copy link
Member Author

@saikishor, thanks, this is awesome. I can think two use cases immediately

  • Get velocimeter without exposing it 😃
  • Potential reward calculation

Will look for opportunities to utilize it.

The velocimeter part yes. If you want any other link, other than the floating base this should be the way to use it. The floating base part is already integrated in the simulator.

Regarding the reward calculation, I don't think it makes sense here during sim-to-sim

You can use this to have the contact detection as you wanted initially 😊

@saikishor
Copy link
Member Author

@eholum The pixi build is now failing when added new packages. I'll try to take a look at it

Copy link
Collaborator

@eholum-nasa eholum-nasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Mostly just minor nits. @Juliaj I'd be curious to know more about your thoughts, too, if you had the time to take a look!

Comment on lines +1092 to +1093
// Default: load the heartbeat publisher plugin
RCLCPP_INFO(get_logger(), "No 'mujoco_plugins' parameter found, loading default HeartbeatPublisherPlugin");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to throw the heartbeat plugin into the list here?

That said, do we want this to be loaded by default? It's pretty innocuous I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. In my previous implementation i was using it to test it's functionality

Comment on lines +1078 to +1080
// find the motion key: after 'motions.', and before the next '.'
const auto plugin_key = param.substr(init_position, param.find_first_of('.', init_position) - init_position);
// Add the motion to the set of unique values
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// find the motion key: after 'motions.', and before the next '.'
const auto plugin_key = param.substr(init_position, param.find_first_of('.', init_position) - init_position);
// Add the motion to the set of unique values
// find the plugin key: after 'motions.', and before the next '.'
const auto plugin_key = param.substr(init_position, param.find_first_of('.', init_position) - init_position);
// Add the plugin to the set of unique values

Comment on lines +1065 to +1066
// Load MuJoCo ROS2 Control plugins
try
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm annoying, but would you mind kicking this out to an internal helper function just for clarity and to make the init method a little shorter?

virtual bool init(rclcpp::Node::SharedPtr node, const mjModel* model, mjData* data) = 0;

/**
* @brief Update the plugin (called every simulation step)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is called during read right? So it will run at the control loop rate? Might be worth calling it out.

void HeartbeatPublisherPlugin::update(const mjModel* /*model*/, mjData* data)
{
auto current_time = std::chrono::steady_clock::now();
auto elapsed = std::chrono::duration_cast<std::chrono::seconds>(current_time - last_publish_time_);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it was intentional or not but did you want to use the sim time here rather than wall clock time? They might be different! Not a big deal either way since it's an example, but I would explicitly call it out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you say, it makes sense.

Maybe I should also forward the rclcpp::Time here? That way everyone can use it for the timestamp to publish data

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think users (and we) have options! Could either just use the node's time since that will pull the sim time, or even just the data->time if that's what people are interested in? I don't think we explicitly have to pass the time stamp through here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants