[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 (1ed61ef) to head (366f65c).
Report is 2 commits behind head on master.

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

…rrect parameter.

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 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 maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 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

Open

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 suggested 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 added 3 commits April 1, 2024 21:05

Fix typo when merging master.

b54fa76

Prepara RM for overriding.

9c5b684

Change how update rate is passed to RM so that we always work with co…

2c0a022

…rrect parameter.

destogl dismissed bmagyar’s stale review via 2c0a022 April 1, 2024 19:09

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

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

codecov bot commented Feb 1, 2024 •

edited

fmauch left a comment

mergify bot commented Feb 25, 2024

destogl commented Apr 2, 2024 •

edited

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

mergify bot commented Jun 17, 2024

saikishor left a comment

destogl commented Jun 19, 2024 •

edited

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

Are you sure you want to change the base?

[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

Other Changes

codecov bot commented Feb 1, 2024 • edited

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

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

mergify bot commented Jun 17, 2024

saikishor left a comment

Choose a reason for hiding this comment

destogl commented Jun 19, 2024 • edited

[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

codecov bot commented Feb 1, 2024 •

edited

destogl commented Apr 2, 2024 •

edited

olivier-stasse commented Apr 5, 2024 •

edited

destogl commented Jun 19, 2024 •

edited