-
Notifications
You must be signed in to change notification settings - Fork 516
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
Use fake_components::GenericSystem from ros2_control #361
Use fake_components::GenericSystem from ros2_control #361
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, that means we can drop fake_joint
from the .repos file, right?
@@ -28,8 +30,10 @@ def load_yaml(package_name, file_path): | |||
def generate_launch_description(): | |||
|
|||
# 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'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do differently?
@@ -0,0 +1,56 @@ | |||
<?xml version="1.0"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, is this required for simulated robots and could this be generated from inline tags in the description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
<xacro:include filename="panda.ros2_control.xacro" /> | ||
<xacro:include filename="panda_hand.ros2_control.xacro" /> | ||
|
||
<xacro:panda_ros2_control name="PandaFakeSystem" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Meh, I guess I'm OK with it. The |
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. |
d4a883b
to
36687da
Compare
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'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be removed once ros-controls/ros2_control#310 is merged
Rebase on master |
I moved the config files to |
Could you please mention them in a review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
0dd32fc
to
714dca2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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")], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -46,7 +50,7 @@ def generate_launch_description(): | |||
ompl_planning_pipeline_config['move_group'].update(ompl_planning_yaml) | |||
|
|||
# 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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.....)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
moveit_ros/moveit_servo/test/launch/test_servo_pose_tracking.test.py
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/test/launch/move_group_launch_test_common.py
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/test/launch/move_group_launch_test_common.py
Show resolved
Hide resolved
moveit_ros/planning_interface/test/launch/move_group_launch_test_common.py
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/test/launch/move_group_ompl_constraints.test.py
Outdated
Show resolved
Hide resolved
54a89cb
to
b336039
Compare
rebase on the main branch |
d29fd81
to
deece60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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....
The MoveIt cpp demo currently fails for me, any ideas?
`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 |
I think something is missing with the Servo demo
List controllers like this:
Shows that the joint trajectory controller isn't active:
|
deece60
to
4430a2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Description
Depends on ros-controls/ros2_control#323 (merged)
Allows us to stop using the
fake_joint
packageChecklist
FakeSystem