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

[CM] Remove support for the description parameter and use only robot_description topic #1358

Merged
merged 9 commits into from
Jul 17, 2024

Conversation

destogl
Copy link
Member

@destogl destogl commented Feb 1, 2024

Part of the #1237.

Has to be adjusted after #1354 is merged.

@fmauch
Copy link
Contributor

fmauch commented Feb 1, 2024

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.

@destogl
Copy link
Member Author

destogl commented Feb 2, 2024

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.

@fmauch
Copy link
Contributor

fmauch commented Feb 2, 2024

so... should we change that?

@destogl
Copy link
Member Author

destogl commented Feb 2, 2024

so... should we change that?

I'll adjust!

@saikishor
Copy link
Member

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?

@destogl
Copy link
Member Author

destogl commented Feb 2, 2024

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.

Copy link
Contributor

mergify bot commented Feb 25, 2024

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

@fmauch
Copy link
Contributor

fmauch commented Feb 28, 2024

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.

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

mergify bot commented Apr 29, 2024

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

@bmagyar
Copy link
Member

bmagyar commented Jun 25, 2024

Now conflicting

@fmauch
Copy link
Contributor

fmauch commented Jul 9, 2024

I've resolved the conflicts on my fork: fmauch@7418dbb 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.

…-param

Merge current master into cm-remove-robot-description
@christophfroehlich christophfroehlich changed the title [CM] Remove support for the description parameter and use only ~/robot_description topic [CM] Remove support for the description parameter and use only robot_description topic Jul 14, 2024
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM
fyi @fmauch I've added a commit and added this change to the migration guide.

@destogl destogl merged commit 9d1aff9 into master Jul 17, 2024
13 of 20 checks passed
@destogl destogl deleted the cm-remove-robot-description-from-param branch July 17, 2024 17:35
christophfroehlich added a commit that referenced this pull request Aug 12, 2024
* [ResourceManager] Make destructor virtual for use in derived classes (#1607)

* Fix typos in test_resource_manager.cpp (#1609)

* [CM] Remove support for the description parameter and use only `robot_description` topic (#1358)

---------

Co-authored-by: Felix Exner <exner@fzi.de>
Co-authored-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>

* move critical variables to the private context (#1623)

* Fix controller starting with later load of robot description test (#1624)

* Fix the duplicated restart of the controller_manager services initialization

* Scope the ControllerManagerRunner to avoid malloc and other test issues

* reorder the tests for consistent log at the end

* Remove noqa (#1626)

* Unused header cleanup (#1627)

* Create debugging.rst (#1625)


---------

Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
Co-authored-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>

* Update changelogs

* 4.14.0

* add missing rclcpp logging include for Humble compatibility build (#1635)

* [CM] Remove deprecated spawner args (#1639)

* Add a pytest launch file to test ros2_control_node (#1636)

* Fix rst markup (#1642)

* Fix rqt_cm paragraph

* Fix indent

* CM: Add missing includes (#1641)

* [RM] Add `get_hardware_info` method to the Hardware Components (#1643)

* Fix the namespaced controller_manager spawner + added tests (#1640)

* Bump version of pre-commit hooks (#1649)

Co-authored-by: christophfroehlich <3367244+christophfroehlich@users.noreply.github.com>

* Add missing include for executors (#1653)

* Update changelogs

* 4.15.0

* refactor SwitchParams to fix the incosistencies in the spawner tests (#1638)

* Modify test with missing CM to have a timeout

* Catch exception when CM services are not found

And print the error and exit in the application

* Exit with code 1 on unreached CM

---------

Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
Co-authored-by: Parker Drouillard <parker@pepcorp.ca>
Co-authored-by: Dr. Denis <denis@stoglrobotics.de>
Co-authored-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Co-authored-by: Henry Moore <henry.moore@picknik.ai>
Co-authored-by: Lennart Nachtigall <firesurfer127@gmail.com>
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: christophfroehlich <3367244+christophfroehlich@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants