[RM] Rename `load_urdf` method to `load_and_initialize_components` and add error handling there to avoid stack crashing when error happens. #1354

destogl · 2024-02-01T16:19:46Z

Functionality discussed in the meeting on 31st of January 2024.

Don't crash is something fails when hardware compoentns are loaded and initialized and enable new sending of robot description in that case.

First part of functinality in #1237.

Other Changes

Adjustment how update_rate is passed to RM - now it is always passed after parameter is read.
enabling override of loading and parsing URDF in RM so that simulators can now use this to simplify data access. For example see Simplify access for robot description from CM by overriding RM. gz_ros2_control#265
Check the example 9 in Update Example 9 to use Gazebo Sim and .xml launch files ros2_control_demos#486
Webots people are notified: API and robot description handling changes in CM and RM cyberbotics/webots_ros2#916

load_urdf() -> load_and_initialize_components()

…topic.

…lization of the hardware components.

…rst one fails.

codecov · 2024-02-01T16:32:08Z

Codecov Report

Attention: Patch coverage is 89.40397% with 16 lines in your changes missing coverage. Please review.

Project coverage is 87.79%. Comparing base (fbb893b) to head (ff682bd).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1354      +/-   ##
==========================================
+ Coverage   87.70%   87.79%   +0.08%     
==========================================
  Files         102      102              
  Lines        8704     8764      +60     
  Branches      780      787       +7     
==========================================
+ Hits         7634     7694      +60     
- Misses        790      792       +2     
+ Partials      280      278       -2

Flag	Coverage Δ
unittests	`87.79% <89.40%> (+0.08%)`	⬆️

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

Files	Coverage Δ
.../include/controller_manager/controller_manager.hpp	`100.00% <100.00%> (ø)`
...ager/test/test_controller_manager_urdf_passing.cpp	`100.00% <100.00%> (ø)`
...rface/test/mock_components/test_generic_system.cpp	`99.75% <100.00%> (ø)`
...rface_testing/test/test_components/test_sensor.cpp	`68.75% <100.00%> (-18.75%)`	⬇️
...e_interface_testing/test/test_resource_manager.cpp	`99.32% <100.00%> (+0.02%)`	⬆️
...e_interface_testing/test/test_resource_manager.hpp	`93.33% <100.00%> (ø)`
...ace_testing/test/test_components/test_actuator.cpp	`91.48% <0.00%> (-4.07%)`	⬇️
...rface_testing/test/test_components/test_system.cpp	`89.36% <66.66%> (-10.64%)`	⬇️
hardware_interface/src/resource_manager.cpp	`73.56% <93.82%> (+2.65%)`	⬆️
controller_manager/src/controller_manager.cpp	`74.41% <50.00%> (-0.15%)`	⬇️

... and 1 file with indirect coverage changes

fmauch

This seems fine and reflects the discussion in my opinion. I left a couple of comments/suggestions, but they should not be blocking this.

hardware_interface_testing/test/test_resource_manager.cpp

controller_manager/src/controller_manager.cpp

hardware_interface/src/resource_manager.cpp

hardware_interface/include/hardware_interface/resource_manager.hpp

hardware_interface/src/resource_manager.cpp

hardware_interface_testing/test/test_components/test_sensor.cpp

mergify · 2024-02-25T09:01:02Z

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

Co-authored-by: Sai Kishor Kothakota <[email protected]>

hardware_interface/src/resource_manager.cpp

hardware_interface_testing/test/test_components/test_sensor.cpp

hardware_interface/include/hardware_interface/resource_manager.hpp

….hpp

destogl · 2024-04-02T11:13:11Z

pending a Gazebo fixup PR

see ros-controls/gz_ros2_control#265

saikishor

minor comments

controller_manager/include/controller_manager/controller_manager.hpp

controller_manager/src/controller_manager.cpp

hardware_interface/src/resource_manager.cpp

controller_manager/src/controller_manager.cpp

christophfroehlich · 2024-04-02T21:16:26Z

pending a Gazebo fixup PR

see ros-controls/gz_ros2_control#265

Do we want to maintain gazebo_ros2_control until EOL of Gazebo Classic? (at least for building it from source on jammy)

destogl · 2024-04-03T08:59:52Z

pending a Gazebo fixup PR

see ros-controls/gz_ros2_control#265

Do we want to maintain gazebo_ros2_control until EOL of Gazebo Classic? (at least for building it from source on jammy)

I don't want to put any effort there anymore. I am not using it or recommend to use it, so I think this will be a reson more for people to switch.

Co-authored-by: Sai Kishor Kothakota <[email protected]>

saikishor

Looks good to me! :)
Thanks for the changes. I will try to review also the PR of gz_ros2_control soon

christophfroehlich · 2024-04-04T19:52:20Z

pending a Gazebo fixup PR

see ros-controls/gz_ros2_control#265

Do we want to maintain gazebo_ros2_control until EOL of Gazebo Classic? (at least for building it from source on jammy)

I don't want to put any effort there anymore. I am not using it or recommend to use it, so I think this will be a reson more for people to switch.

I still have to use it, because modern gazebo is not feature equal yet (e.g, no kinematic loops). As a consequence, we will already break the rolling-on-humble compatibility.

olivier-stasse · 2024-04-05T05:09:04Z

I still have to use it, because modern gazebo is not feature equal yet (e.g, no kinematic loops). As a consequence, we will already break the rolling-on-humble compatibility.

It looks like there is a way to handle parallel structure here gazebosim/gz-physics#25 (comment) using DetachableJoint

mergify · 2024-06-17T18:14:40Z

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

hardware_interface/src/resource_manager.cpp

saikishor

@destogl Looking great!
Thanks for following up. We should get this merged soon! :)

destogl · 2024-06-19T18:08:46Z

Tests are clean on rolling:

Hardware Interface:

build/hardware_interface/Testing/20240619-1807/Test.xml: 6 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface/test_results/hardware_interface/test_component_interfaces.gtest.xml: 9 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface/test_results/hardware_interface/test_component_parser.gtest.xml: 40 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface/test_results/hardware_interface/test_generic_system.gtest.xml: 22 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface/test_results/hardware_interface/test_inst_hardwares.gtest.xml: 3 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface/test_results/hardware_interface/test_joint_handle.gtest.xml: 5 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface/test_results/hardware_interface/test_macros.gtest.xml: 2 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface_testing/Testing/20240619-1745/Test.xml: 2 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface_testing/test_results/hardware_interface_testing/test_resource_manager.gtest.xml: 27 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface_testing/test_results/hardware_interface_testing/test_resource_manager_prepare_perform_switch.gtest.xml: 5 tests, 0 errors, 0 failures, 0 skipped

Hardware Interface Testing:

build/hardware_interface_testing/Testing/20240619-1808/Test.xml: 2 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface_testing/test_results/hardware_interface_testing/test_resource_manager.gtest.xml: 27 tests, 0 errors, 0 failures, 0 skipped
build/hardware_interface_testing/test_results/hardware_interface_testing/test_resource_manager_prepare_perform_switch.gtest.xml: 5 tests, 0 errors, 0 failures, 0 skipped

bmagyar

Thank you!

bmagyar and others added 5 commits January 31, 2024 20:16

__ -> _

a31acf8

load_urdf() -> load_and_initialize_components()

Added functionality and tests for getting robot description from the …

c47ac65

…topic.

Fixed compilation and tests.

c1ba8bc

Don't throw exception if something goes wrong with loading and initia…

26585af

…lization of the hardware components.

Make sure that we can send new robot description over topic is the fi…

62e4384

…rst one fails.

destogl self-assigned this Feb 1, 2024

github-actions bot requested review from aprotyas, bmagyar, DasRoteSkelett, fmauch, livanov93 and VX792 February 1, 2024 16:20

destogl added bug enhancement high-priority backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. backport-iron labels Feb 1, 2024

destogl added 2 commits February 1, 2024 19:44

Add some additional tests.

2e13c3c

Merge branch 'master' into rename_load_urdf_method

90df9a3

destogl mentioned this pull request Feb 1, 2024

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

Merged

fmauch approved these changes Feb 1, 2024

View reviewed changes

hardware_interface_testing/test/test_resource_manager.cpp Outdated Show resolved Hide resolved

hardware_interface_testing/test/test_resource_manager.cpp Show resolved Hide resolved

Optimize tests according to comments.

15fb5c6

saikishor requested changes Feb 5, 2024

View reviewed changes

Apply suggestions from code review

0c6761f

Co-authored-by: Sai Kishor Kothakota <[email protected]>

destogl commented Feb 28, 2024

View reviewed changes

destogl added 2 commits February 28, 2024 18:30

Update hardware_interface/include/hardware_interface/resource_manager…

4c03455

….hpp

Merge branch 'master' into rename_load_urdf_method

abbd74f

destogl requested a review from saikishor February 28, 2024 17:37

destogl requested review from bmagyar and saikishor April 2, 2024 11:15

saikishor reviewed Apr 2, 2024

View reviewed changes

controller_manager/include/controller_manager/controller_manager.hpp Outdated Show resolved Hide resolved

controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved

hardware_interface/src/resource_manager.cpp Show resolved Hide resolved

saikishor reviewed Apr 2, 2024

View reviewed changes

Apply suggestions from code review

f3a9cad

Co-authored-by: Sai Kishor Kothakota <[email protected]>

saikishor approved these changes Apr 3, 2024

View reviewed changes

destogl mentioned this pull request Jun 17, 2024

Remove hardcoded default update rate in Resource Manager - might cause issues with simulators #1575

Open

Merge branch 'master' into rename_load_urdf_method

fd49fd4

destogl commented Jun 17, 2024

View reviewed changes

hardware_interface/src/resource_manager.cpp Show resolved Hide resolved

destogl requested review from fmauch and saikishor June 17, 2024 18:41

Fix merge error.

9246300

saikishor approved these changes Jun 18, 2024

View reviewed changes

destogl added 3 commits June 19, 2024 19:51

Fix tests and remove dupplicated test after changes.

069572f

Correct formatting.

b2bd5b6

Correct formatting.

366f65c

Merge branch 'master' into rename_load_urdf_method

ff682bd

bmagyar approved these changes Jun 25, 2024

View reviewed changes

bmagyar merged commit 017f0da into master Jun 25, 2024
14 of 19 checks passed

bmagyar deleted the rename_load_urdf_method branch June 25, 2024 10:56

christophfroehlich mentioned this pull request Jul 5, 2024

Simplify access for robot description from CM by overriding RM ros-controls/gazebo_ros2_control#349

Merged

mergify bot mentioned this pull request Jul 6, 2024

Simplify access for robot description from CM by overriding RM. (backport #265) ros-controls/gz_ros2_control#364

Merged

saikishor mentioned this pull request Jul 8, 2024

make ResourceManager destructor virtual for derived classes #1607

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RM] Rename `load_urdf` method to `load_and_initialize_components` and add error handling there to avoid stack crashing when error happens. #1354

[RM] Rename `load_urdf` method to `load_and_initialize_components` and add error handling there to avoid stack crashing when error happens. #1354

destogl commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

fmauch left a comment

mergify bot commented Feb 25, 2024

destogl commented Apr 2, 2024 •

edited

Loading

saikishor left a comment

christophfroehlich commented Apr 2, 2024

destogl commented Apr 3, 2024

saikishor left a comment

christophfroehlich commented Apr 4, 2024

olivier-stasse commented Apr 5, 2024 •

edited

Loading

mergify bot commented Jun 17, 2024

saikishor left a comment

destogl commented Jun 19, 2024 •

edited

Loading

bmagyar left a comment

[RM] Rename load_urdf method to load_and_initialize_components and add error handling there to avoid stack crashing when error happens. #1354

[RM] Rename load_urdf method to load_and_initialize_components and add error handling there to avoid stack crashing when error happens. #1354

Conversation

destogl commented Feb 1, 2024 • edited Loading

Other Changes

codecov bot commented Feb 1, 2024 • edited Loading

Codecov Report

fmauch left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 25, 2024

destogl commented Apr 2, 2024 • edited Loading

saikishor left a comment

Choose a reason for hiding this comment

christophfroehlich commented Apr 2, 2024

destogl commented Apr 3, 2024

saikishor left a comment

Choose a reason for hiding this comment

christophfroehlich commented Apr 4, 2024

olivier-stasse commented Apr 5, 2024 • edited Loading

mergify bot commented Jun 17, 2024

saikishor left a comment

Choose a reason for hiding this comment

destogl commented Jun 19, 2024 • edited Loading

bmagyar left a comment

Choose a reason for hiding this comment

[RM] Rename `load_urdf` method to `load_and_initialize_components` and add error handling there to avoid stack crashing when error happens. #1354

[RM] Rename `load_urdf` method to `load_and_initialize_components` and add error handling there to avoid stack crashing when error happens. #1354

destogl commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

destogl commented Apr 2, 2024 •

edited

Loading

olivier-stasse commented Apr 5, 2024 •

edited

Loading

destogl commented Jun 19, 2024 •

edited

Loading