ros2_control icon indicating copy to clipboard operation
ros2_control copied to clipboard

[CM] Remove support for the description parameter and use only ~/robot_description topic

Open destogl opened this issue 1 year ago • 10 comments

Part of the #1237.

Has to be adjusted after #1354 is merged.

destogl avatar Feb 01 '24 19:02 destogl

May I raise the question (has probably been discussed elsewhere, but I couldn't find a note in #940): Why do we subscribe to ~/robot_description? Isn't the usual workflow that we have /robot_state_publisher and /controller_manager, where /robot_state_publisher publishes /robot_description? This would leave everyone having to remap controller_manager/robot_description in their launchfiles. Am I missing something or is this intended behavior?

If we instead subscribed to robot_description there would not necessarily be any migration necessary, once we merge this, as CM will silently use the description topic from the joint state publisher instead of the parameter that people might still send to the CM node. In that case they just would have to remove the parameter and that's it.

fmauch avatar Feb 01 '24 20:02 fmauch

May I raise the question (has probably been discussed elsewhere, but I couldn't find a note in #940): Why do we subscribe to ~/robot_description? Isn't the usual workflow that we have /robot_state_publisher and /controller_manager, where /robot_state_publisher publishes /robot_description? This would leave everyone having to remap controller_manager/robot_description in their launchfiles. Am I missing something or is this intended behavior?

This is intended, but soley by myself :)

The idea was to work with the local namespace. Reading your comment, you are right. for the cases where one has multiple controller managers, one should remap those instead making people to do remapping by default.

destogl avatar Feb 02 '24 15:02 destogl

so... should we change that?

fmauch avatar Feb 02 '24 15:02 fmauch

so... should we change that?

I'll adjust!

destogl avatar Feb 02 '24 17:02 destogl

So when using multiple controller managers, we remap the topic within the launch file like this?

remappings=[
            ('/robot_description', '/custom_1/robot_description'),
        ]

Am I right?

saikishor avatar Feb 02 '24 17:02 saikishor

So when using multiple controller managers, we remap the topic within the launch file like this?

remappings=[
            ('/robot_description', '/custom_1/robot_description'),
        ]

Am I right?

That's the idea.

destogl avatar Feb 02 '24 17:02 destogl

This pull request is in conflict. Could you fix it @destogl?

mergify[bot] avatar Feb 25 '24 09:02 mergify[bot]

This pull request is in conflict. Could you fix it @destogl?

I'll make a PR to this PR's branch as mentioned in #1410


Edit: Looking into the conflicts present, my changes seem to be causing only minor issues so I think we should wait for #1354 and then fix all conflicts at once.

fmauch avatar Feb 28 '24 16:02 fmauch

This pull request is in conflict. Could you fix it @destogl?

mergify[bot] avatar Apr 29 '24 12:04 mergify[bot]

Now conflicting

bmagyar avatar Jun 25 '24 11:06 bmagyar

I've resolved the conflicts on my fork: https://github.com/fmauch/ros2_control/commit/7418dbbb540e041d043888b5a6c7dfe820a5c9c7 I could either make a PR to this PR's branch, or I could make a new PR replacing this or a maintainer could push my merge commit to this PR's branch. I think the latter is the best option, as then the history of this PR will be retained and git history will stay somehow understandable.

The tests run fine and I tested the UR driver against this locally.

fmauch avatar Jul 09 '24 09:07 fmauch