Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Description topic in controller manager not configurable #1138

Closed
domire8 opened this issue Oct 17, 2023 · 6 comments
Closed

Description topic in controller manager not configurable #1138

domire8 opened this issue Oct 17, 2023 · 6 comments
Labels

Comments

@domire8
Copy link

domire8 commented Oct 17, 2023

https://github.com/ros-controls/ros2_control/blame/bd6911d663abb687dc956bf19a104d03648f8985/controller_manager/src/controller_manager.cpp#L324

  robot_description_subscription_ = create_subscription<std_msgs::msg::String>(
    "~/robot_description", rclcpp::QoS(1).transient_local(),
    std::bind(&ControllerManager::robot_description_callback, this, std::placeholders::_1));

Hello guys, I have a question regarding this relatively new feature in the controller manager. Why can the topic of this subscription not be configured? This means that there always needs to be a mapping between the global /robot_description topic from the robot state publisher and this <ns>/controller_manager/robot_description topic, no?

Maybe I also didn't get what the intended usage here is, but I'd be happy if you could explain. Thank you very much!

@destogl
Copy link
Member

destogl commented Nov 28, 2023

Hi @domire8,

Yes, this is the intended usage. Using mapping is not an issue; it is one line in the launch file, but it increases awareness. I want to make it evident to the user and disallow "automatic" functionality here. In general, one can have multiple controller managers. Therefore it makes sense to have this separation here. What we should improve is the documentation on this functionality and examples. They are not given, since not many people use this at the moment, but rather pass the robot description as a parameter.

@fmauch
Copy link
Contributor

fmauch commented Nov 28, 2023

May I raise the point that a using the ~/robot_description needs requires every user to use a remap, where I think in most setups the controller_manager is started alongside the robot_state_publisher. So using robot_description by default instead will work for most people out of the box while also working correctly with different descriptions in different namespaces.

I understand that the intention is to raise awareness to users in order to not wonder why things break as soon as they do more complicated setups, but in general, whenever I create a setup that is going away from the default I expect things to require being remapped, etc.

In my humble opinion a good out-of-the-box experience for most setups is preferable to raising awareness that the topic could be somewhere else.

@domire8
Copy link
Author

domire8 commented Nov 28, 2023

Thanks for taking the time to reply @destogl.

In general, one can have multiple controller managers.

I agree there, we have applications where we use several of those. However, I find it still unusual and not traditional that the description topic is then under <namespace>/ControllerManager/robot_description.

An alternative would be to put it under <namespace>/robot_description. This way, if there is only one controller manager in the global namespace, the topic stays at /robot_description where it always was. If you have several controller managers, the topics will be <namespace_1>/robot_description and <namespace_2>/robot_description.

It's a small detail and I also like when the intended usage is very clear but I'm also a bit afraid like @fmauch that the out of the box experience for simple setups becomes more complicated this way.

@bmagyar
Copy link
Member

bmagyar commented Nov 28, 2023

In my humble opinion a good out-of-the-box experience for most setups is preferable to raising awareness that the topic could be somewhere else.

This is the way

@destogl
Copy link
Member

destogl commented Feb 19, 2024

should be adjusted and closed by #1358

@christophfroehlich
Copy link
Contributor

I'll close this issue as we merged the PR yesterday. If concerns remain, feel free to reopen or open a new (update) issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants