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 option to automatically generate ros2_control's fake components description file #1283

Closed

Conversation

JafarAbdi
Copy link
Member

@JafarAbdi JafarAbdi commented May 27, 2022

Description

Reduces the number of required trajectory execution config files for sim by 3 (out of 5), all the information needed to generate the fake components description file is already in the URDF file

See the discussion in #1261

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #1283 (7cd6bc3) into main (934ac23) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1283      +/-   ##
==========================================
- Coverage   61.48%   61.47%   -0.01%     
==========================================
  Files         274      274              
  Lines       24940    24940              
==========================================
- Hits        15332    15329       -3     
- Misses       9608     9611       +3     
Impacted Files Coverage Δ
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 75.36% <0.00%> (-1.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 934ac23...7cd6bc3. Read the comment docs.

Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

I'm OK with this. It looks like you satisfied my concern from #1261

Comment on lines +93 to +95
<state_interface name="velocity">
<param name="initial_value">0.0</param>
</state_interface>
Copy link
Member

Choose a reason for hiding this comment

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

This defaults to 0.

Suggested change
<state_interface name="velocity">
<param name="initial_value">0.0</param>
</state_interface>
<state_interface name="velocity"/>

Copy link
Member Author

Choose a reason for hiding this comment

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

I was getting a warning about not explicitly setting this, @destogl is this needed?

"initial_positions"
]
if initial_positions is None:
# For backward compatibility check for config/initial_positions.yaml
Copy link
Member

Choose a reason for hiding this comment

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

I think you should just print a warning here. No need to maintain backward compatability in my opinion

moveit_configs_utils/test/test_moveit_resources_configs.py Outdated Show resolved Hide resolved
@DLu
Copy link
Contributor

DLu commented May 31, 2022

As mentioned in the maintainers meeting last week, I have some code that does this the MSA side. Currently both fanuc and panda have it as part of their moveit configs.

I guess my main question is: when is the ros2_control tag need to be used?

@AndyZe
Copy link
Member

AndyZe commented May 31, 2022

I guess my main question is: when is the ros2_control tag need to be used?

Any time you're using ros2_control at all, that tag has to be in the URDF somewhere. Simulation or hardware

@DLu
Copy link
Contributor

DLu commented May 31, 2022

Any time you're using ros2_control at all, that tag has to be in the URDF somewhere. Simulation or hardware

What is the "fake" part then?

@AndyZe
Copy link
Member

AndyZe commented Jun 1, 2022

What is the "fake" part then?

Sorry I didn't understand the question completely at first. For you auto-generating these templates, this should always be fine. It's what allows the RViz simulation without needing Gazebo/Ignition. That's a nice, simple default that ought to be available for everybody in my opinion.

            <hardware>
                <plugin>fake_components/GenericSystem</plugin>
            </hardware>

If people want to start using real hardware, they'll need to add their own tag. Like here for the UR driver:

        <xacro:if value="${sim_gazebo}">
          <plugin>gazebo_ros2_control/GazeboSystem</plugin>
        </xacro:if>
        <xacro:if value="${sim_ignition}">
          <plugin>ign_ros2_control/IgnitionSystem</plugin>
        </xacro:if>
        <xacro:if value="${use_fake_hardware}">
          <plugin>fake_components/GenericSystem</plugin>
          <param name="fake_sensor_commands">${fake_sensor_commands}</param>
          <param name="state_following_offset">0.0</param>
        </xacro:if>
        <xacro:unless value="${use_fake_hardware or sim_gazebo or sim_ignition}">
          <plugin>ur_robot_driver/URPositionHardwareInterface</plugin>

@JafarAbdi JafarAbdi force-pushed the pr-auto_generate_ros2_control branch from 75e5380 to 7cd6bc3 Compare June 1, 2022 20:20
@mergify
Copy link

mergify bot commented Jun 3, 2022

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

@DLu
Copy link
Contributor

DLu commented Jun 16, 2022

@JafarAbdi Do you think that #1299 covers everything you wanted to do with this PR?

@mergify
Copy link

mergify bot commented Sep 27, 2022

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

@tylerjw
Copy link
Member

tylerjw commented Nov 18, 2022

@AndyZe or @JafarAbdi do you know what should be done next with this change?

@mergify
Copy link

mergify bot commented May 8, 2023

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

@tylerjw
Copy link
Member

tylerjw commented Aug 23, 2023

This PR has many conflicts and was abandoned. It seems like a useful change so I'll create an issue to track finishing this work.

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.

None yet

4 participants