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

Delete MoveIt fake_controller_manager #471

Merged
merged 1 commit into from
May 28, 2021

Conversation

AndyZe
Copy link
Member

@AndyZe AndyZe commented May 22, 2021

This deletes the fake_controller_manager plugin. I don't believe it's needed anymore because ros2_control supports simulation in a fairly easy way. All you need to do is specify fake components in your_robot.ros2_control.xacro. (example)

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

Then the fake controllers get launched just like a real hardware controller from a launch file:

ros2_controllers_path = os.path.join(
get_package_share_directory("moveit_resources_panda_moveit_config"),
"config",
"panda_ros_controllers.yaml",
)
ros2_control_node = Node(
package="controller_manager",
executable="ros2_control_node",
parameters=[robot_description, ros2_controllers_path],
)

This is great! It's only a few words difference between hardware and simulation.

I also think it would be good to delete this from MoveIt2 sooner rather than later, to minimize the number of people using fake_controller_manager.

Tagging @v4hn and @rhaschke because they were the code owners in MoveIt1.

@codecov
Copy link

codecov bot commented May 22, 2021

Codecov Report

Merging #471 (7e0fffe) into main (8423af3) will decrease coverage by 0.01%.
The diff coverage is 85.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #471      +/-   ##
==========================================
- Coverage   48.86%   48.85%   -0.00%     
==========================================
  Files         218      218              
  Lines       22987    22969      -18     
==========================================
- Hits        11230    11220      -10     
+ Misses      11757    11749       -8     
Impacted Files Coverage Δ
...lanning_request_adapter/planning_request_adapter.h 0.00% <ø> (ø)
..._adapter_plugins/src/fix_start_state_collision.cpp 32.82% <66.67%> (-2.80%) ⬇️
..._plugins/src/add_time_optimal_parameterization.cpp 93.75% <100.00%> (+1.45%) ⬆️
...est_adapter_plugins/src/fix_start_state_bounds.cpp 40.00% <100.00%> (-3.03%) ⬇️
...quest_adapter_plugins/src/fix_workspace_bounds.cpp 95.00% <100.00%> (+3.70%) ⬆️

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 827646f...7e0fffe. Read the comment docs.

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

I fully agree. Making ros2_control GenericSystem the default minimizes the changes for running setups on hardware and it directly supports all kinds of controllers. I also don't see the use case for the fake controller anymore. I'd still wait for a second opinion by either one of the code owners.

@@ -33,7 +33,6 @@
# FOLDER-SPECIFIC REVIEWERS:

/moveit_plugins/moveit_ros_control_interface/ @ipa-mdl @bmagyar
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove moveit_ros_control_interface .? we're not using it at all

Copy link
Member Author

Choose a reason for hiding this comment

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

How about in a new PR?

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

This needs to be rebased for us to merge it.

@AndyZe AndyZe force-pushed the andyz/delete_fake_controller_mgr branch from 0bb29c0 to 7e0fffe Compare May 28, 2021 01:34
@AndyZe AndyZe merged commit 7339c86 into moveit:main May 28, 2021
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.

4 participants