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

Fix crash for missing urdf in resource manager #1301

Conversation

saikishor
Copy link
Member

@saikishor saikishor commented Jan 16, 2024

Fixes #1299

These changes are needed after the merging of #1271 and #1272. It seems like the gazebo_ros2_control doesn't load the URDF into the resource manager, and this is causing the CM services not to start.

https://github.com/saikishor/gazebo_ros2_control/blob/60e4b75f4a786c9563d7dd97c6d08a31ca69c5b1/gazebo_ros2_control/src/gazebo_ros2_control_plugin.cpp#L366-L371

The gazebo_ros2_control doesn't load_urdf into the resource manager instead, it sets up the components and initializes it. Loading the URDF with the load_urdf method will probably fail, because this method originally loads and initializes the hardware interfaces and this step might probably fail.

Solution 1

I thought it would be wiser to utilize the size of the components to check if the URDF is loaded. This way without any changes from the gazebo_ros2_control side, we can have this issue fixed.

Solution 2

We can either go with this fix or add a new default argument to the load_urdf method to not load_and_initialize_components

void ResourceManager::load_urdf(const std::string & urdf, bool validate_interfaces)
{
is_urdf_loaded__ = true;
const std::string system_type = "system";
const std::string sensor_type = "sensor";
const std::string actuator_type = "actuator";
const auto hardware_info = hardware_interface::parse_control_resources_from_urdf(urdf);
for (const auto & individual_hardware_info : hardware_info)
{
if (individual_hardware_info.type == actuator_type)
{
std::scoped_lock guard(resource_interfaces_lock_, claimed_command_interfaces_lock_);
resource_storage_->load_and_initialize_actuator(individual_hardware_info);
}
if (individual_hardware_info.type == sensor_type)
{
std::lock_guard<std::recursive_mutex> guard(resource_interfaces_lock_);
resource_storage_->load_and_initialize_sensor(individual_hardware_info);
}
if (individual_hardware_info.type == system_type)
{
std::scoped_lock guard(resource_interfaces_lock_, claimed_command_interfaces_lock_);
resource_storage_->load_and_initialize_system(individual_hardware_info);
}
}
// throw on missing state and command interfaces, not specified keys are being ignored
if (validate_interfaces)
{
validate_storage(hardware_info);
}
std::lock_guard<std::recursive_mutex> guard(resources_lock_);
read_write_status.failed_hardware_names.reserve(
resource_storage_->actuators_.size() + resource_storage_->sensors_.size() +
resource_storage_->systems_.size());
}

This is also interesting as in the future, if we want to get the URDF information into the resource manager to parse things like limits or other entities

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (3309c6d) 48.02% compared to head (89ec65d) 48.01%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1301      +/-   ##
==========================================
- Coverage   48.02%   48.01%   -0.02%     
==========================================
  Files          41       41              
  Lines        3525     3526       +1     
  Branches     1912     1913       +1     
==========================================
  Hits         1693     1693              
  Misses        442      442              
- Partials     1390     1391       +1     
Flag Coverage Δ
unittests 48.01% <60.00%> (-0.02%) ⬇️

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

Files Coverage Δ
hardware_interface/src/resource_manager.cpp 48.83% <60.00%> (-0.08%) ⬇️

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.

I confirm that it works with ros2_control_demo_example_9 without the PR to gazebo_ros2_control.

This is also interesting as in the future, if we want to get the URDF information into the resource manager to parse things like limits or other entities

This PR does not break the use of the URDF in the ressource_manager?

A small note: For semantic reasons, maybe a change to load_urdf instead of check for the component sizes would be better.

But this is a quick fix for the broken simulation now. (I guess ign_ros2_control is affected too?)

@saikishor
Copy link
Member Author

A small note: For semantic reasons, maybe a change to load_urdf instead of check for the component sizes would be better.

Thanks for the feedback. Tomorrow, I'll work on the changes to load_urdf method

@saikishor saikishor marked this pull request as draft January 16, 2024 21:57
@saikishor saikishor marked this pull request as ready for review January 17, 2024 09:26
Copy link
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.

Looks good! Thanks!

@destogl destogl enabled auto-merge (squash) January 17, 2024 15:59
@destogl destogl added backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. backport-iron labels Jan 17, 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

@destogl destogl merged commit 510ba73 into ros-controls:master Jan 17, 2024
14 checks passed
mergify bot pushed a commit that referenced this pull request Jan 17, 2024
* check the components size instead of the variable (fixes #1299)
* add new load_and_initialize_components default argument to the load_urdf method

(cherry picked from commit 510ba73)

# Conflicts:
#	hardware_interface/src/resource_manager.cpp
mergify bot pushed a commit that referenced this pull request Jan 17, 2024
* check the components size instead of the variable (fixes #1299)
* add new load_and_initialize_components default argument to the load_urdf method

(cherry picked from commit 510ba73)
saikishor added a commit to saikishor/ros2_control that referenced this pull request Jan 18, 2024
* check the components size instead of the variable (fixes ros-controls#1299)
* add new load_and_initialize_components default argument to the load_urdf method
saikishor added a commit to saikishor/ros2_control that referenced this pull request Jan 18, 2024
* check the components size instead of the variable (fixes ros-controls#1299)
* add new load_and_initialize_components default argument to the load_urdf method
destogl pushed a commit that referenced this pull request Jan 18, 2024
* check the components size instead of the variable (fixes #1299)
* add new load_and_initialize_components default argument to the load_urdf method

(cherry picked from commit 510ba73)
@saikishor saikishor deleted the fix_crash_for_missing_urdf_in_resource_manager branch August 17, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Humble] Gazebo simulation failing after merging #1272
3 participants