Possible to read robot_description from topic

[pucciland95]

Hi,

first thing: thank you for developing this repo. It is very useful.
I noticed that it is necessary to pass the robot_desciption as a param from the launch file.
Since our ROS2 stack requires it to be passed from topic I added this functionality. Therefore, it is not necessary anymore to pass it from the launch file.

Let me know what you think and whether it is useful

[eholum]

Thanks @pucciland95! Definitely seems like the right way to go for me. Just adding some comments from the peanut gallery.

[pucciland95]

Hi @eholum and thanks for reviewing,

I should have made all the changes you asked for. Let me know if you think there is still some more work to be done.

Cheers,

Niccolo

[eholum]

Thanks @pucciland95! Sorry if my comments opened a few more questions... There are some param layering questions that I had.

[pucciland95]

Thanks @pucciland95! Sorry if my comments opened a few more questions... There are some param layering questions that I had.

Don't worry, it is totally fine 😄
I answer to all your comments. Feel free to make as many comments as you like

[eholum]

Works for me!

I will have to defer to @sangteak601 and whether or not he wants to take a look as well, though.

[sangteak601]

Thank you very much for your contribution. It looks great and will definitely improve usability. I have just a few comments.

[pucciland95]

After applying all the requested changes the code crashes since the cm_executor_ is neither passed from the outside of the init() function nor defined inside it.
The controller_manager needs both it as an argument to be spinned:

controller_manager_ = std::make_shared<controller_manager::ControllerManager>(
  std::move(resource_manager), cm_executor_, "controller_manager", node_->get_namespace());
cm_executor_->add_node(controller_manager_);

An executor needs to be present for this code to work either by passing it from outside the MujocoRos2Control or defining it in the init() function.

[sangteak601]

Please check my other comments link1 link2.

[pucciland95]

Hi, can I have feedback on the changes?

I would like to detach my fork since I want to bring the repo in another direction, but I think if I do, I break the PR. Therefore, before detaching, I would like to close this PR.

[eholum]

Sorry for the delay @pucciland95! This all continues to work for me. Added a note about the executor stuff, which I also don't think is a big deal since there's still some cleanup todo around nodes and threading that should be handled in a later PR.

[pucciland95]

Reverted spin to spin_once as requested

[pucciland95]

Just a gentle ping

[sangteak601]

Thank you for the reminder. The conflicts should be resolved before this can be merged.

[pucciland95]

Fixed merge conflict

[sangteak601]

Thank you again for your contribution!