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 accepting robot description using parameter and harden the behavior when getting it from topic. #1237

Closed
wants to merge 25 commits into from

Conversation

destogl
Copy link
Member

@destogl destogl commented Dec 18, 2023

#1218 should go before this PR.

Should I split this into multiple PRs? There is a bit of a mix now with small code restructurings, renaming and similar.

…hrow errors when comparing URDF and exported interfaces but add warning about this.
@destogl destogl force-pushed the cm-fix-calls-to-prepare-and-perform-switch branch from d8f03ec to d5b1276 Compare December 18, 2023 14:08
Copy link
Contributor

@olivier-stasse olivier-stasse left a comment

Choose a reason for hiding this comment

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

LGTM.
I do not think that this PR needs to be splitted.
It would be nice to comment the logic of the tests.

Copy link
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.

Some partial comments for now. I will review the rest later

}
init_resource_manager(robot_description_);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
init_resource_manager(robot_description_);
init_resource_manager(robot_description_);
init_services();

Copy link
Member Author

@destogl destogl Jan 26, 2024

Choose a reason for hiding this comment

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

There is a reason why I moved init_service into callback. The idea is that they are not available until there is HW. Otherwise, it happens that the spawner fails before the controller manager manages to get the robot description and initialize HW. When services are not available, spawners are waiting for them.

I can understand that this is not logical on the first view. Maybe there is another better solution, but I wanted to avoid adding additional flags to synchronize that.

controller_manager/src/controller_manager.cpp Show resolved Hide resolved
<< e.what());
"ResourceManager has already loaded an urdf file. Ignoring attempt to reload a robot "
"description file.");
return;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return;
if(!robot_description_notification_timer_->is_canceled())
{
robot_description_notification_timer_->cancel();
}
return;

Copy link
Member

Choose a reason for hiding this comment

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

can this happen at all?

Comment on lines 312 to 318
robot_description_notification_timer_ = create_wall_timer(
std::chrono::seconds(1),
[&]()
{
RCLCPP_WARN(
get_logger(), "Waiting for data on '~/robot_description' topic to finish initialization");
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
robot_description_notification_timer_ = create_wall_timer(
std::chrono::seconds(1),
[&]()
{
RCLCPP_WARN(
get_logger(), "Waiting for data on '~/robot_description' topic to finish initialization");
});
// This is needed to start the CM services when the URDF is already loaded
if (resource_manager_->is_urdf_already_loaded())
{
init_services();
}
else
{
robot_description_notification_timer_ = create_wall_timer(
std::chrono::seconds(1),
[&]()
{
RCLCPP_WARN(
get_logger(), "Waiting for data on '~/robot_description' topic to finish initialization");
});
}

Copy link
Member Author

@destogl destogl Jan 26, 2024

Choose a reason for hiding this comment

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

You are right! but I think that the solution is rather to initialize the timer before subscriber. Then we have less code and it should work in both cases.

controller_manager/src/controller_manager.cpp Show resolved Hide resolved
Copy link
Contributor

mergify bot commented Jan 25, 2024

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

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

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

Comparison is base (25aa5d3) 47.53% compared to head (62a3692) 47.66%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1237      +/-   ##
==========================================
+ Coverage   47.53%   47.66%   +0.13%     
==========================================
  Files          41       41              
  Lines        3547     3539       -8     
  Branches     1930     1914      -16     
==========================================
+ Hits         1686     1687       +1     
+ Misses        459      449      -10     
- Partials     1402     1403       +1     
Flag Coverage Δ
unittests 47.66% <36.66%> (+0.13%) ⬆️

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

Files Coverage Δ
.../include/controller_manager/controller_manager.hpp 18.18% <ø> (ø)
hardware_interface/src/resource_manager.cpp 48.54% <50.00%> (-0.29%) ⬇️
controller_manager/src/controller_manager.cpp 39.29% <30.95%> (+0.47%) ⬆️

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Please separate into 3.

  • remove robot description parameter
  • interfaces validation
  • remove the deprecated parameters component state after start

load_urdf() -> load_and_initialize_components()
Copy link
Contributor

mergify bot commented Feb 1, 2024

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

Copy link
Contributor

mergify bot commented Apr 29, 2024

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

@christophfroehlich
Copy link
Contributor

The last PR was merged yesterday, I think this can be closed.

@christophfroehlich christophfroehlich deleted the cm-fix-calls-to-prepare-and-perform-switch branch July 18, 2024 06:43
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