Skip to content

Pass URDF to controllers on init#1088

Merged
bmagyar merged 9 commits intoros-controls:masterfrom
bmagyar:pass-urdf-to-controllers
Nov 6, 2023
Merged

Pass URDF to controllers on init#1088
bmagyar merged 9 commits intoros-controls:masterfrom
bmagyar:pass-urdf-to-controllers

Conversation

@bmagyar
Copy link
Copy Markdown
Member

@bmagyar bmagyar commented Jul 26, 2023

This should help avoiding extra legwork in controllers to get access to the /robot_description.

@bmagyar bmagyar force-pushed the pass-urdf-to-controllers branch from 7f832b4 to c80c1e6 Compare July 26, 2023 18:28
aprotyas
aprotyas approved these changes Jul 26, 2023
View reviewed changes
controller_interface/include/controller_interface/controller_interface_base.hpp
std::vector<hardware_interface::LoanedStateInterface> state_interfaces_;
unsigned int update_rate_ = 0;
bool is_async_ = false;
std::string urdf_ = "";
Copy link
Copy Markdown
Member

@aprotyas aprotyas Jul 26, 2023

Choose a reason for hiding this comment

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

Should this be a std::optional<std::string> urdf_ = std::nullopt instead?

Copy link
Copy Markdown
Member Author

@bmagyar bmagyar Oct 22, 2023

Choose a reason for hiding this comment

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

I don't want to communicate the option of not being set as anything close to valid. The empty string initial value is only there so we can test if it was properly set...

I also thought about how we could do this better, a const string would be nice but that can only be set once which doesn't work well with the longer term idea of getting URDF updates from topics.

saikishor
saikishor reviewed Jul 27, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

The provision of the URDF directly to the controllers is a brilliant idea. This reduces the hassles one should go through to set up their controllers.

ros2_control/controller_manager/src/controller_manager.cpp

Line 1143 in 757ccd4

controller.c->init(controller.info.name, get_namespace()) ==

However, I think there is a missing part in assigning this URDF from the controller_manager's end, and at least one test that verifies the robot_description string from the topic or param is the same as that inside the controller.
Another test that checks when a URDF is updated in the topic, the newly initialized controllers get the new URDF rather than the old one.

Brilliant work 👏

destogl
destogl previously approved these changes Jul 31, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

This looks good!

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Sep 18, 2023

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

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 22, 2023 •
edited
Loading

Codecov Report

Merging #1088 (caef34d) into master (bd6911d) will decrease coverage by 0.08%.
The diff coverage is 11.86%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
- Coverage   31.62%   31.55%   -0.08%     
==========================================
  Files          94       94              
  Lines       10838    10878      +40     
  Branches     7419     7454      +35     
==========================================
+ Hits         3428     3433       +5     
+ Misses        804      802       -2     
- Partials     6606     6643      +37
Flag Coverage Δ
unittests 31.55% <11.86%> (-0.08%) ⬇️

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

Files Coverage Δ
...controller_interface/controller_interface_base.hpp 33.33% <ø> (-16.67%) ⬇️
...roller_interface/src/controller_interface_base.cpp 48.14% <100.00%> (+1.99%) ⬆️
...er_interface/test/test_controller_with_options.hpp 45.00% <100.00%> (ø)
.../include/controller_manager/controller_manager.hpp 18.18% <ø> (ø)
...r_manager/test/test_controller/test_controller.hpp 100.00% <ø> (ø)
...roller_failed_init/test_controller_failed_init.cpp 55.55% <ø> (ø)
...roller_failed_init/test_controller_failed_init.hpp 20.00% <ø> (ø)
...er_interface/test/test_controller_with_options.cpp 29.62% <0.00%> (ø)
...oller_interface/test/test_controller_interface.cpp 21.79% <0.00%> (ø)
...rface/test/test_chainable_controller_interface.cpp 14.44% <0.00%> (ø)
... and 3 more

@saikishor saikishor mentioned this pull request Oct 22, 2023
Merged
bmagyar
bmagyar commented Oct 23, 2023
View reviewed changes
bmagyar
bmagyar commented Oct 23, 2023
View reviewed changes
bmagyar
bmagyar commented Oct 23, 2023
View reviewed changes
@bmagyar bmagyar added the holding label Nov 4, 2023
@christophfroehlich christophfroehlich mentioned this pull request Nov 4, 2023
Closed
@bmagyar bmagyar merged commit b33e579 into ros-controls:master Nov 6, 2023
@bmagyar bmagyar mentioned this pull request Nov 6, 2023
Merged
@destogl destogl mentioned this pull request Nov 28, 2023
Open
6 tasks
@bmagyar bmagyar mentioned this pull request Feb 29, 2024
Closed
@christophfroehlich christophfroehlich mentioned this pull request Mar 19, 2024
Closed
@christophfroehlich christophfroehlich mentioned this pull request Mar 27, 2024
Open
This was referenced Mar 28, 2024
Merged
Merged
@Wiktor-99 Wiktor-99 mentioned this pull request Nov 18, 2024
Merged
@christophfroehlich christophfroehlich mentioned this pull request Jan 19, 2025
Merged
@jellehierck jellehierck mentioned this pull request Apr 7, 2025
Open
@sea-bass
Copy link
Copy Markdown

sea-bass commented Sep 6, 2025 •
edited
Loading

Hey @bmagyar -- I am working with some folks that are using Humble and would like this feature backported. Would you be okay with this, or does this break ABI in some way?

EDIT: Yeah this definitely breaks ABI... ugh...

@christophfroehlich
Copy link
Copy Markdown
Member

christophfroehlich commented Sep 8, 2025

Yes, I don't think it makes sense to backport this.
Are you aware that we support building the current development version (aka rolling) on humble? 5min build time in your workspace, and you've got all the new features ;)

@sea-bass
Copy link
Copy Markdown

sea-bass commented Sep 8, 2025

I was not aware it still built on humble, thanks!

@saikishor saikishor deleted the pass-urdf-to-controllers branch September 8, 2025 08:50
@bmagyar
Copy link
Copy Markdown
Member Author

bmagyar commented Sep 8, 2025 via email

@saikishor
Copy link
Copy Markdown
Member

saikishor commented Sep 8, 2025

I was not aware it still built on humble, thanks!

@sea-bass You can find more information here:
https://control.ros.org/humble/doc/getting_started/getting_started.html#building-from-source

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

Reviewers

@saikishor saikishor saikishor left review comments

@destogl destogl destogl left review comments

@duringhof duringhof Awaiting requested review from duringhof

@erickisos erickisos Awaiting requested review from erickisos

@qiayuanl qiayuanl Awaiting requested review from qiayuanl

@sachinkum0009 sachinkum0009 Awaiting requested review from sachinkum0009

+1 more reviewer

@aprotyas aprotyas aprotyas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

holding rolling

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@bmagyar @sea-bass @christophfroehlich @saikishor @destogl @aprotyas