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

Add Support for SDF #1763

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

Amronos
Copy link

@Amronos Amronos commented Sep 25, 2024

Fixes #632
I have modified hardware_interface/src/component_parser.cpp to add support for SDF models.

To test this out the demos being added through ros-controls/gz_ros2_control#427 can be used.

Signed-off-by: Aarav Gupta <[email protected]>
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.

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.61%. Comparing base (d55def1) to head (059c1c4).

Files with missing lines Patch % Lines
hardware_interface/src/component_parser.cpp 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1763      +/-   ##
==========================================
- Coverage   87.62%   87.61%   -0.02%     
==========================================
  Files         120      120              
  Lines       12217    12230      +13     
  Branches     1093     1093              
==========================================
+ Hits        10705    10715      +10     
- Misses       1123     1125       +2     
- Partials      389      390       +1     
Flag Coverage Δ
unittests 87.61% <80.00%> (-0.02%) ⬇️

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

Files with missing lines Coverage Δ
hardware_interface/test/test_component_parser.cpp 98.83% <100.00%> (+0.01%) ⬆️
hardware_interface/src/component_parser.cpp 94.47% <66.66%> (+0.04%) ⬆️

... and 6 files with indirect coverage changes

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.

@Amronos The changes look good to me. Can you add that SDF parsing is also supported through robot_description topic in the release_notes.rst?.

Sorry for not pointing out earlier

Signed-off-by: Aarav Gupta <[email protected]>
saikishor
saikishor previously approved these changes Sep 27, 2024
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.

LGTM

@azeey
Copy link

azeey commented Oct 3, 2024

This is great! Thanks @Amronos

@Amronos
Copy link
Author

Amronos commented Oct 7, 2024

Are there any updates on when this PR can be reviewed and merged?

@christophfroehlich
Copy link
Contributor

Are there any updates on when this PR can be reviewed and merged?

Maintainers were busy with big PRs being merged the last days. We have so many other (older) PRs waiting for reviews, you can speed up the process by also reviewing them.

@Amronos
Copy link
Author

Amronos commented Oct 8, 2024

The test successfully_parse_valid_sdf is passing for me when running colcon test, but failing in the GitHub checks. Any idea why this is the case?

@bmagyar
Copy link
Member

bmagyar commented Oct 10, 2024

Do you have a non-standard URDF library installed? The CI errors come from this segment

  // parse full URDF for mimic options
  urdf::Model model;
  if (!model.initString(urdf))
  {
    throw std::runtime_error("Failed to parse URDF file");
}

located here https://github.com/ros-controls/ros2_control/pull/1763/files#diff-ae927ba56161377a082def3ca0417c1052c37949d94e27a7713e3f9f798e790eR853
and the other print comes directly from within the initString() call from here

Seems legit to me but I'm wondering how it worked for you :D

@Amronos
Copy link
Author

Amronos commented Oct 17, 2024

Do you have a non-standard URDF library installed?

I don't have a non-standard URDF library installed and the test is passing for me.
I did try rebuilding with colcon and testing multiple times. I also tried re-cloning the repo.
I also tested with a regular ROS launch file, in which I passed the SDF file to robot_state_publisher.
Is it possible that something in the CI is outdated?

@christophfroehlich
Copy link
Contributor

Is it possible that something in the CI is outdated?

I don't think so, it fails on debian, RHEL and ubuntu

@Amronos
Copy link
Author

Amronos commented Oct 17, 2024

ros-controls/gz_ros2_control#427 should help in testing this PR.

@azeey
Copy link

azeey commented Oct 17, 2024

I've locally verified that the test fails because the ros-<distro>-sdformat-urdf package is not installed. One option is to add sdformat_urdf to the package.xml. @christophfroehlich / @bmagyar is that something you'd be okay with?

@Amronos
Copy link
Author

Amronos commented Oct 18, 2024

Oh, that explains it. Thanks a lot @azeey!

@azeey
Copy link

azeey commented Oct 18, 2024

IIUC, the Source and Semi-Binary builds do not install dependencies based on the package.xml in the PR, but use a docker image built from the master branch. Should the sdformat_urdf repo be added to https://github.com/ros-controls/ros2_control/blob/master/ros2_control.jazzy.repos and https://github.com/ros-controls/ros2_control/blob/master/ros2_control.rolling.repos?

@christophfroehlich
Copy link
Contributor

No. Debian Source build uses a handcrafted docker, I can add the package there. RHEL tries to install the deps, but

ERROR: the following rosdeps failed to install
dnf: Failed to detect successful installation of [ros-jazzy-sdformat-urdf]

The package is deactivated on the buildfarm, please ping the maintainers to activate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use ros2 control with sdf model
5 participants