Use fake_components::GenericSystem from ros2_control by JafarAbdi · Pull Request #361 · moveit/moveit2

JafarAbdi · 2021-02-09T14:46:58Z

Description

Depends on ros-controls/ros2_control#323 (merged)

Allows us to stop using the fake_joint package

Checklist

Update servo to use the FakeSystem

henningkayser

Awesome, that means we can drop fake_joint from the .repos file, right?

moveit_demo_nodes/run_move_group/config/panda_controllers.yaml

henningkayser · 2021-02-09T14:54:30Z

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

    # planning_context
-    robot_description_config = load_file('moveit_resources_panda_description', 'urdf/panda.urdf')
-    robot_description = {'robot_description' : robot_description_config}
+    robot_description_config = xacro.process_file(os.path.join(get_package_share_directory('run_moveit_cpp'),


What does this do differently?

henningkayser · 2021-02-09T14:56:33Z

moveit_demo_nodes/run_moveit_cpp/config/panda.ros2_control.xacro

@@ -0,0 +1,56 @@
+<?xml version="1.0"?>


Interesting, is this required for simulated robots and could this be generated from inline tags in the description?

This file is required by all robots running on ros2_control. The generation for a robot is not so simple. This has to be actually provided with the robot's driver.

You could generate some general descriptions from the joints in URDF and assume they all have can receive position commands and provide position feedback. I have already some templates for this. This will work perfectly with the MoveIt --> Joint Trajectory Controller --> Fake System. Nevertheless, when using real hardware those interfaces have to correspond to the driver.

If you have any questions regarding the new ros2_control Architecture or FakeRobot (the same as the FakeSystem) just ping me :)

henningkayser · 2021-02-09T14:57:56Z

moveit_demo_nodes/run_moveit_cpp/config/panda.urdf.xacro

+    <xacro:include filename="panda.ros2_control.xacro" />
+    <xacro:include filename="panda_hand.ros2_control.xacro" />
+
+    <xacro:panda_ros2_control name="PandaFakeSystem" />


So, I assume this is optional... What happens when you want to run the driver? Adding face controller parameters to the description somehow feels strange

I'd suggest injecting either the fake system of the non-fake one in the launch file or via a xacro argument, they should be considered mutually exclusive

codecov · 2021-02-09T15:47:59Z

Codecov Report

Merging #361 (e40a3ef) into main (c9c61a6) will increase coverage by 0.06%.
The diff coverage is n/a.

❗ Current head e40a3ef differs from pull request most recent head de7bc90. Consider uploading reports for the commit de7bc90 to get more accurate results

@@            Coverage Diff             @@
##             main     #361      +/-   ##
==========================================
+ Coverage   53.32%   53.38%   +0.06%     
==========================================
  Files         210      210              
  Lines       18814    18814              
==========================================
+ Hits        10031    10042      +11     
+ Misses       8783     8772      -11

Impacted Files	Coverage Δ
moveit_ros/moveit_servo/src/servo_calcs.cpp	`80.13% <0.00%> (+0.62%)`	⬆️
moveit_ros/moveit_servo/src/pose_tracking.cpp	`75.17% <0.00%> (+0.66%)`	⬆️
...meterization/work_space/pose_model_state_space.cpp	`84.56% <0.00%> (+1.48%)`	⬆️
moveit_ros/moveit_servo/src/collision_check.cpp	`76.12% <0.00%> (+1.50%)`	⬆️
..._core/collision_detection/src/collision_common.cpp	`100.00% <0.00%> (+57.15%)`	⬆️

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 c9c61a6...de7bc90. Read the comment docs.

AndyZe

I'm not sure I like this. The beauty of fake_joint_driver is it launches exactly like the real hardware driver.

Notice, lines of code went up by 120. That's not a good sign.

AndyZe · 2021-02-10T18:22:54Z

Meh, I guess I'm OK with it. The panda_hand.ros2_control.xacro will need to get added for the hardware robot, anyway. That's where most of the new lines of code come from.

destogl · 2021-02-11T15:49:51Z

Notice, lines of code went up by 120. That's not a good sign.

The lines are mostly YAML and xacro files, as you mentioned. They are needed anyway for the ros2_control. So there is no additional lines compared to the hardware.

Why @JafarAbdi added some things I am also not sure. I'll provide the FakeRobot example in the ros2_control_demo repository, so you can compare it to the "real" hardware.

JafarAbdi · 2021-02-25T19:32:20Z

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

+    load_joint_state_controller = ExecuteProcess(cmd=['ros2 control load_start_controller joint_state_controller'], shell=True, output='screen')
+    load_controllers = [load_joint_state_controller]
+    # load panda_arm_controller
+    load_controllers += [ExecuteProcess(cmd=['ros2 control load_configure_controller panda_arm_controller'],


Should be removed once ros-controls/ros2_control#310 is merged

JafarAbdi · 2021-02-25T19:33:28Z

Rebase on master

JafarAbdi · 2021-02-25T19:34:04Z

I moved the config files to panda_moveit_config, moveit/moveit_resources#51 should be merged first

JafarAbdi · 2021-02-25T19:36:15Z

Why @JafarAbdi added some things I am also not sure.

Could you please mention them in a review?

AndyZe

I think this change in Servo publish_fake_jog_commands.cpp would fix the timestamp issue:

  Time pub_period(0, 50 /* ms */);
  auto callback = [captured_pub, captured_node]() -> void {
    auto pub_ptr = captured_pub.lock();
    auto node_ptr = captured_node.lock();
    if (!pub_ptr || !node_ptr)
    {
      return;
    }
    auto msg = std::make_unique<geometry_msgs::msg::TwistStamped>();
    msg->header.stamp = node_ptr->now() + pub_period;
    msg->header.frame_id = "panda_link0";
    msg->twist.linear.x = 0.3;
    msg->twist.angular.z = 0.5;
    pub_ptr->publish(std::move(msg));
  };
  auto timer = node->create_wall_timer(pub_period, callback);

I don't really know how time works in ROS2 yet. That syntax is prob wrong

destogl

I suggest a few changes here. It is confusing for me to have so many copy-pasted launch files. This makes this really hard to maintain. Would be possible to add repetitive code in an import file?

destogl · 2021-03-01T07:59:46Z

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

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


Why not use controllers_yaml variable? What is difference between panda_controller.yaml and panda_ros_controllers.yaml?

Also, it is cleaner if the path is joint in a variable that is then used as a parameter.

panda_ros_controllers.yaml is the config file for ros2_controllers whereas panda_controller.yaml is used to configure moveit simple controller which is what MoveIt use to execute its trajectories by sending them to ros2_control trajectory controller

destogl · 2021-03-01T08:04:01Z

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py


    # Trajectory Execution Functionality
-    controllers_yaml = load_yaml('run_move_group', 'config/controllers.yaml')
+    controllers_yaml = load_yaml('moveit_resources_panda_moveit_config', 'config/panda_controllers.yaml')


If this is something MoveIt related, why not call it panda_moveit_controllers.yaml? People could misunderstand this as something ros2_control related. (This is maybe just my ros2_control background, I don't know.....)

I agree with you, I'll rename it to panda_moveit_simple_controllers

moveit_demo_nodes/run_moveit_cpp/launch/run_moveit_cpp.launch.py

moveit_demo_nodes/run_move_group/launch/run_move_group.launch.py

moveit_ros/moveit_servo/test/launch/test_servo_pose_tracking.test.py

moveit_ros/planning_interface/test/launch/move_group_launch_test_common.py

moveit_ros/planning_interface/test/launch/move_group_ompl_constraints.test.py

JafarAbdi · 2021-03-08T17:46:32Z

rebase on the main branch

destogl

Besides one comment probably resulting from my lack of understanding of the MoveIt code, everything looks very good!

Happy to see you are following ros2_control and ros2_controllers master branches :)

destogl · 2021-03-21T14:57:26Z

moveit_demo_nodes/run_moveit_cpp/launch/run_moveit_cpp_fake_controller.launch.py

-    controllers_yaml = load_yaml('run_moveit_cpp', 'config/fake_controllers.yaml')
-    fake_controller = {'moveit_fake_controller_manager': controllers_yaml,
+    moveit_simple_controllers_yaml = load_yaml('moveit_resources_panda_moveit_config', 'config/fake_controllers.yaml')
+    fake_controller = {'moveit_fake_controller_manager': moveit_simple_controllers_yaml,


Do you need CM in MoveIt? Since you are starting ros2_control with the fake hardware I am not sure what is the purpose of this. I am probably missing something....

AndyZe · 2021-03-23T15:23:12Z

The MoveIt cpp demo currently fails for me, any ideas?

ros2 launch run_moveit_cpp run_moveit_cpp.launch.py

`future: <Task finished name='Task-2' coro=<LaunchService._process_one_event() done, defined at /opt/ros/foxy/lib/python3.8/site-packages/launch/launch_service.py:271> exception=SubstitutionFailure("package 'run_moveit_cpp' found at '/home/andy/ws_schilling/install/run_moveit_cpp', but libexec directory '/home/andy/ws_schilling/install/run_moveit_cpp/lib/run_moveit_cpp' does not exist")>

JafarAbdi · 2021-03-23T15:26:58Z

The MoveIt cpp demo currently fails for me, any ideas?

ros2 launch run_moveit_cpp run_moveit_cpp.launch.py

`future: <Task finished name='Task-2' coro=<LaunchService._process_one_event() done, defined at /opt/ros/foxy/lib/python3.8/site-packages/launch/launch_service.py:271> exception=SubstitutionFailure("package 'run_moveit_cpp' found at '/home/andy/ws_schilling/install/run_moveit_cpp', but libexec directory '/home/andy/ws_schilling/install/run_moveit_cpp/lib/run_moveit_cpp' does not exist")>

Replace this line with RUNTIME DESTINATION lib/${PROJECT_NAME}

AndyZe · 2021-03-23T15:36:00Z

I think something is missing with the Servo demo

ros2 launch moveit_servo servo_cpp_interface_demo.launch.py

List controllers like this:

ros2 control list_controllers

Shows that the joint trajectory controller isn't active:

panda_arm_controller[joint_trajectory_controller/JointTrajectoryController] finalized

henningkayser

Great improvement, all demos are working great for me now. @AndyZe's issue also seems resolved. I just pushed a minor fix to the destination path for run_ompl_constrained_planning. I actually wasn't aware that bin has been exposing all executables globally when doing a symlink install (did anything change there?). We should do a repo-wide refactoring for all runtime destinations.

henningkayser reviewed Feb 9, 2021

View reviewed changes

This was linked to issues Feb 9, 2021

Use new ros2_control interfaces #283

Closed

Switch moveit2 to use ros2_control debian #332

Closed

AndyZe reviewed Feb 10, 2021

View reviewed changes

AndyZe mentioned this pull request Feb 10, 2021

Add package with "Fake" components for simple integration of framework ros-controls/ros2_control#323

Merged

destogl mentioned this pull request Feb 21, 2021

Add joystick to servo generic component #369

Closed

10 tasks

JafarAbdi force-pushed the pr-ros2_control_fake_system branch from d4a883b to 36687da Compare February 25, 2021 19:31

JafarAbdi commented Feb 25, 2021

View reviewed changes

AndyZe reviewed Feb 26, 2021

View reviewed changes

JafarAbdi force-pushed the pr-ros2_control_fake_system branch from 0dd32fc to 714dca2 Compare February 28, 2021 01:13

destogl reviewed Mar 1, 2021

View reviewed changes

JafarAbdi force-pushed the pr-ros2_control_fake_system branch from 54a89cb to b336039 Compare March 8, 2021 17:46

JafarAbdi force-pushed the pr-ros2_control_fake_system branch 3 times, most recently from d29fd81 to deece60 Compare March 20, 2021 00:54

destogl approved these changes Mar 21, 2021

View reviewed changes

JafarAbdi added 4 commits March 25, 2021 14:24

Remove fake-joint from .repos + update ros2_control branches

aff29c9

Fix target dependencies for rviz_plugin_render_tools

ebaffff

Move moveit_test_utils outside BUILD_TESTING if statement

cd4f8a3

Port planning_interface tests to use GenericSystem from ros2_control

5385094

JafarAbdi added 5 commits March 25, 2021 14:24

Port moveit_servo to use GenericSystem from ros2_control

e898cdc

Port moveit_demo_nodes to use GenericSystem from ros2_control

4553dd7

Remove mongodb_server_node from test launch file

ecb1b3e

Run pre-commit

4fe4f62

Fix installation directory for run_moveit_cpp

4430a2a

JafarAbdi force-pushed the pr-ros2_control_fake_system branch from deece60 to 4430a2a Compare March 25, 2021 15:02

JafarAbdi and others added 2 commits March 25, 2021 15:58

Fix bug introduced during rebase

c93d86c

Fix runtime path in run_ompl_constrained_planning

de7bc90

henningkayser approved these changes Mar 25, 2021

View reviewed changes

henningkayser merged commit 2620f7b into moveit:main Mar 25, 2021

chaos4ros2 mentioned this pull request Apr 12, 2021

The run_move_group demo has the same problem like run_moveit_cpp demo #415

Closed

sjahr mentioned this pull request Apr 16, 2021

Update hybrid planning feature branch #426

Merged

6 tasks

Conversation

JafarAbdi commented Feb 9, 2021 • edited by AndyZe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

henningkayser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AndyZe left a comment

Choose a reason for hiding this comment

Uh oh!

AndyZe commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

destogl commented Feb 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JafarAbdi commented Feb 25, 2021

Uh oh!

JafarAbdi commented Feb 25, 2021

Uh oh!

JafarAbdi commented Feb 25, 2021

Uh oh!

AndyZe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

destogl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JafarAbdi Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JafarAbdi commented Mar 8, 2021

Uh oh!

destogl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyZe commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JafarAbdi commented Mar 23, 2021

Uh oh!

AndyZe commented Mar 23, 2021

Uh oh!

henningkayser left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

JafarAbdi commented Feb 9, 2021 •

edited by AndyZe

Loading

codecov bot commented Feb 9, 2021 •

edited

Loading

AndyZe commented Feb 10, 2021 •

edited

Loading

AndyZe left a comment •

edited

Loading

JafarAbdi Mar 8, 2021 •

edited

Loading

AndyZe commented Mar 23, 2021 •

edited

Loading

henningkayser left a comment •

edited

Loading