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

Component parser: Get mimic information from URDF #1256

Merged

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Dec 28, 2023

I started implementing the parsing of information about mimic joints from the URDF (instead of duplicating information in the ros2_control tag).
This will fix #652:

After a discussion with @destogl it seems we converged on a solution very close to the above.

Overview: Using a mimic joint should work out-of-the-box, on by default. Users who want to opt-in can do so in the configuration of their hardware component. The mimic property should be defined in the wider URDF section, not under the <ros2_control> tag.

In the example with the setup below, ros2_control should do the check on each joint to figure out if it is a mimic joint or not. This could be part of JointInfo (will need to reach up to the full urdf to find it) ((implementation detail))

<joint name="right_finger_joint" type="prismatic">
  <axis xyz="0 1 0"/>
  <origin xyz="0.0 -0.48 1" rpy="0.0 0.0 0.0"/>
  <parent link="base"/>
  <child link="finger_right"/>
  <limit effort="1000.0" lower="0" upper="0.38" velocity="10"/>
</joint>
<joint name="left_finger_joint" type="prismatic">
  <mimic joint="right_finger_joint" multiplier="1" offset="0"/>
  <axis xyz="0 1 0"/>
  <origin xyz="0.0 0.48 1" rpy="0.0 0.0 3.1415926535"/>
  <parent link="base"/>
  <child link="finger_left"/>
  <limit effort="1000.0" lower="0" upper="0.38" velocity="10"/>
</joint>
<ros2_control>
  <joint name="right_finger_joint">
    <command_interface name="effort"/>
    <state_interface name="position"/>
    <state_interface name="velocity"/>
    <state_interface name="effort"/>
  </joint>
  <joint name="left_finger_joint">
    <command_interface name="position"/>
    <state_interface name="position"/>
    <state_interface name="velocity"/>
    <state_interface name="effort"/>
  </joint>
</ros2_control>

If one wants to disable mimic behaviour for some reason, they can still turn this functionality off by adding mimic="false" to the joint like so

  <joint name="left_finger_joint" mimic="false">
    <command_interface name="position"/>
    <state_interface name="position"/>
    <state_interface name="velocity"/>
    <state_interface name="effort"/>
  </joint>

Originally posted by @bmagyar in #652 (comment)

Decisions in WG meeting on 2024 01 03:

  • mimic="false" should be for opt-out; missing or mimic="true" has the same effect
  • component_parser throws an error if an invalid URDF is given. Additionally, I added a check if a joint defined in the ros2_control tag is available in the URDF and throw an error otherwise. Is this ok, or are there any use-cases for an incomplete URDF?
  • As a consequence of the above point: The tests failed, because there are some interfaces like "configuration/max_tcp_jerk" defined as joint. IMHO we should use the semantic information of the type and changed them to be of gpio type.

migration notes

I moved existing migration notes directly below the ros2_control heading and added migration notes for this change. Or should we put them at top-level? (can be done in a different PR).

image

Copy link

codecov bot commented Dec 28, 2023

Codecov Report

Attention: Patch coverage is 89.23077% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 76.14%. Comparing base (35bb5f7) to head (3a1c1cd).
Report is 15 commits behind head on master.

❗ Current head 3a1c1cd differs from pull request most recent head 931f108. Consider uploading reports for the commit 931f108 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1256      +/-   ##
==========================================
+ Coverage   75.51%   76.14%   +0.63%     
==========================================
  Files          41       41              
  Lines        3328     3396      +68     
  Branches     1921     1957      +36     
==========================================
+ Hits         2513     2586      +73     
+ Misses        421      420       -1     
+ Partials      394      390       -4     
Flag Coverage Δ
unittests 76.14% <89.23%> (+0.63%) ⬆️

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

Files Coverage Δ
...terface/include/mock_components/generic_system.hpp 100.00% <ø> (ø)
...e_interface/src/mock_components/generic_system.cpp 79.18% <100.00%> (-1.17%) ⬇️
hardware_interface/src/component_parser.cpp 92.83% <88.88%> (-0.42%) ⬇️

... and 5 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.

I'll continue my review for the other files later

hardware_interface/src/component_parser.cpp Outdated Show resolved Hide resolved
hardware_interface/src/component_parser.cpp Show resolved Hide resolved
hardware_interface/include/hardware_interface/tools.hpp Outdated Show resolved Hide resolved
hardware_interface/src/component_parser.cpp Outdated Show resolved Hide resolved
hardware_interface/src/component_parser.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

mergify bot commented Jan 4, 2024

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

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.

Rest of the changes, look fine to me

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.

The changes look good to me.
Just few minor suggestions :)

hardware_interface/src/component_parser.cpp Outdated Show resolved Hide resolved
hardware_interface/src/component_parser.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

mergify bot commented Jan 12, 2024

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

Copy link
Contributor

mergify bot commented Jan 25, 2024

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

Copy link
Contributor

mergify bot commented Feb 1, 2024

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

@christophfroehlich christophfroehlich force-pushed the component_parser_add_mimic branch 2 times, most recently from 3a1c1cd to 48ee90e Compare February 15, 2024 13:15
@saikishor
Copy link
Member

Niceee!
Thanks for fixing it. Can we get this merged soon? I have 2 more PRs waiting for these changes.

bmagyar
bmagyar previously approved these changes Mar 25, 2024
destogl
destogl previously approved these changes Mar 26, 2024
const auto & joint = hw_info.joints.at(i);

// Search for mimic joints defined in ros2_control tag (deprecated)
// TODO(christophfroehlich) delete deprecated config with ROS-J
Copy link
Member

Choose a reason for hiding this comment

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

Can you open follow up on this by removing it add ading migration notes to ROS-J. Even better make 2 PR from it, so we can merge already migration notes, but then later (end of april) delete the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -169,18 +299,18 @@ const auto hardware_resources =
<state_interface name="velocity"/>
<state_interface name="acceleration"/>
</joint>
<joint name="configuration">
<gpio name="configuration">
Copy link
Member

Choose a reason for hiding this comment

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

This should be also marked in the migration notes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. I added a screenshot in the PR summary on top.

bmagyar
bmagyar previously approved these changes Mar 27, 2024
@christophfroehlich
Copy link
Contributor Author

@Bence @destogl Let's merge this and then ros-controls/control.ros.org#262 to put the migration notes on the top level of the docs.

@bmagyar
Copy link
Member

bmagyar commented Mar 28, 2024

Say no more!

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

Successfully merging this pull request may close these issues.

Clean definition of mimic joints under ros2_control URDF tag
4 participants