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

Noetic release #89

Closed
19 of 26 tasks
rickstaa opened this issue Oct 21, 2021 · 21 comments
Closed
19 of 26 tasks

Noetic release #89

rickstaa opened this issue Oct 21, 2021 · 21 comments

Comments

@rickstaa
Copy link
Contributor

rickstaa commented Oct 21, 2021

We just released the initial version of the Noetic branch. This issue will be used for the rest of the release process.

Upstream pull requests that are required

The following upstream pull requests need to be merged for the Noetic branch to be warning and error free:

The following upstream pull request is required for the simulation to work:

Noetic branch todos

Moveit_setup_assistant todos

I found several improvements that can be made to the MSA.

Other upstream problems that still exist

Migration guide

For future reference, the following steps were performed to release the Noetic branch:

Noetic migration guide
  1. I created the initial panda robot configuration using the moveit_setup_assistant (see 8566b70). In this branch, tree/82378877879f7d40727819b1c3345eeb4308d98b was used. Similar to WIP: Generated using the latest features from MoveIt #74 I changed some posed to prevent self collisions.

  2. I modified the panda.srdf file that is created by the moveit_setup_assistant such that we can programmatically enable the gripper (see f0c2bca). This needs to be done, since the moveit_setup_assistant currently doesn't propagate xacro arguments you supply to the urdf.xacro file (see this issue). We therefore need to manually add a way to enable or disable the gripper. This can be done by creating a panda.srdf.xacro file.

  3. I replaced the sensor_3d.yaml with the example files in the perception_pipeline tutorial. Also edit the sensor_manager.launch.xml file to point to the right configuration files (see 2aaa01d).

  4. I added the rviz_tutorial flag. While doing this, I made sure that I copied the moveit.rviz and empty.rviz files from the previous ROS version. Additionally, I added the moveit_visual_tools package as a dependency (see 1d55c98).

  5. Similar to df0cbd8 I had to disable the collision checking between joint{5, 6} and joint8 since the new collision geometries that were added to https://github.com/frankaemika/franka_ros/releases/tag/0.8.0 are to coarse (see aa24fae). This should be fixed upstream by, for example, by improving the collision geometries or by giving users the ability to use meshes instead (Adds 'use_collision_mesh' arg to the robot description frankaemika/franka_ros#154).

  6. I added the panda_control_moveit_rviz.launch which can be used to control the real robot (see 36d7c9f).

  7. I added the TrajOpt files that were found in the melodic-devel branch to the repository ( see bc2bbc7 and 88fb278).

  8. I added the STOMP files that were found in the melodic-devel branch to the repository ( see 133d524) .

  9. I updated the join_limits.yaml file that was created by the MSA to enforce jerk limits. This was done by adding acceleration limits, since MoveIt does not yet support jerk limits (see 89d9b44).

  10. I added the files of the LERP planner that is created in the creating-moveit-plugins tutorial (see 85756cd). These files were copied from the melodic-devel branch.

  11. I added the demo_chomp.launch and ompl-chomp_planning_pipeline.launch.xml files needed by the chomp_planner_tutorial tutorial. I also updated the chomp config file to the one found in the melodic-devel branch (see 4c390ac and c2ccca2).

  12. I added the demo_gazebo.launch launch file which spawns a simulated Panda robot in Gazebo (see c46c81c). This launch file uses the new franka_gazebo package that was introduced in franka_ros v0.8.1.

Files that were manually created

Config folder

  • panda.srdf.xacro
  • hand.xacro
  • panda_arm.xacro
  • lerp_planning.yaml
  • stomp_planning.yaml
  • trajopt_planning.yaml
  • panda_controllers.yaml
  • panda_gripper_controllers.yaml

launch

  • demo_stomp.launch (Was not present in the melodic-devel branch).
  • demo_chomp.launch (Not used in the moveit_tutorials).
  • demo_lerp.launch (Used in the moveit_tutorials).
  • lerp_planning_pipeline.launch.xml
  • ompl-chomp_planning_pipeline.launch.xml
  • panda_gripper_moveit_controller_manager.launch.xml
  • run_benchmark_trajopt.launch
  • stomp_planning_pipeline.launch.xml
  • trajopt_planning_pipeline.launch.xml
@rickstaa
Copy link
Contributor Author

rickstaa commented Oct 21, 2021

If people want to help to finalize the Noetic release, please tag me and state which TODO you are working on. I will then update the TODO in the issue report above.

@rickstaa
Copy link
Contributor Author

rickstaa commented Oct 21, 2021

I just performed some tests on the simulated and real robot, and it looks like the code that remaps /joint_states to /joint_states_desired is no longer needed (see rickstaa#54):

https://github.com/ros-planning/panda_moveit_config/blob/78e783a1cd979b312f582748ce3f7426d16258a0/launch/move_group.launch#L78

https://github.com/ros-planning/panda_moveit_config/blob/78e783a1cd979b312f582748ce3f7426d16258a0/launch/demo.launch#L36

#9 (comment) states the following about why this code was used in the past:

This code was needed
the joint states published in /joint_states aren't suitable for planning trajectories with MoveIt, but still required for other uses, such as e.g. visualization of the current robot state in RViz.

@rhaschke, @fwalch Can you one of you validate that this code is redundant ? It looks that in franka v0.8.1 the joint_states and joint_states_desired topics both use the sensor_msgs/JointState.msg(see https://github.com/frankaemika/franka_ros/blob/da13eb1d6f5cc9600dbb22a60759016e715e8d27/franka_control/src/franka_state_controller.cpp#L179-L194). To my knowledge MoveIt can work with this message (see https://ros-planning.github.io/moveit_tutorials/doc/planning_scene_monitor/planning_scene_monitor_tutorial.html?highlight=jointstate#currentstatemonitor).

@fwalch
Copy link

fwalch commented Oct 22, 2021

@rickstaa I think this is still required. Both have the same data type, but the data comes from different sources (robot_state_.q vs robot_state_.q_d).

@rhaschke
Copy link
Contributor

rhaschke commented Oct 22, 2021

@fwalch, what's the semantic difference between these two sources / topics?
Usually, the robot should publish its current joint values on joint_states. What's the meaning of joint_states_desired?
If these are commanded (but not yet reached) joint values, this is just for info and neither MoveIt nor any other ROS node actually cares about them.

@v4hn
Copy link

v4hn commented Oct 22, 2021

Wow, that's a long list! Thank you a lot for working on noetic support and digging through all of that @rickstaa! 🏅

If these are commanded (but not yet reached) joint values, this is just for info and neither MoveIt nor any other ROS node actually cares about them.

With more and more online controller support it can make a lot of sense to replace the actual with the desired state in tight loops. But then there needs to be a mechanism in place that will abort if the actual state moves to far away from desired.

Also it can be a way to work around unstable controllers, e.g., make MoveIt assume joint values of 0.5 when the controller jitters around +-.02.

Both case should not be relevant for the panda here though, so I agree that MoveIt should not silently get the (semantically) wrong topic!

@rickstaa
Copy link
Contributor Author

rickstaa commented Oct 22, 2021

@fwalch I think @rhaschke is right. If we look at the primitive in the libfranka package, they are equal (see https://github.com/frankaemika/libfranka/blob/f1f46fb008a37eb0d1dba00c971ff7e5a7bfbfd3/include/franka/robot_state.h#L212-L222). To check if we still need to remap the joint_states topic we have to consider three use cases: simulation, visualization and the real robot.

Simulation

In the simulation, both robot_state_.q and robot_state_.q_d are populated using joint->position (see https://github.com/frankaemika/franka_ros/blob/da13eb1d6f5cc9600dbb22a60759016e715e8d27/franka_gazebo/src/franka_hw_sim.cpp#L459-L467).

Rviz visualization

In the RVIZ visualization (i.e. demo.launch) the /joint_states are created by a fake joint_state_publisher. Here, the desired_joint_states and thus the following code is to my knowledge not needed:

https://github.com/ros-planning/panda_moveit_config/blob/78e783a1cd979b312f582748ce3f7426d16258a0/launch/demo.launch#L36

Real robot

On the real robot (i.e. panda_control_moveit_rviz.launch) the /joint_states_desired topic is created by a joint_state_publisher (see https://github.com/frankaemika/franka_ros/blob/da13eb1d6f5cc9600dbb22a60759016e715e8d27/franka_control/launch/franka_control.launch#L25-L30). This joint_state_publisher uses the franka_state_controller/joint_states_desired topic for creating the /joint_states_desired topic. I didn't dive deep enough into the code, but I think the franka_state_controller receives the joint_states_desired directly from the FCI through the FrankaStateHandle (see https://github.com/frankaemika/franka_ros/blob/da13eb1d6f5cc9600dbb22a60759016e715e8d27/franka_hw/include/franka_hw/franka_state_interface.h#L52-L53). Based on that, I think what @v4hn explained is right.

I therefore think MoveIt should subscribe to the real robot states (i.e., joint_states) and not the desired states (i.e., joint_states_desired). I tested the panda_control_moveit_rviz.launch file, without the remapping, on the real robot and have not experienced any issues.

@rhaschke
Copy link
Contributor

@fwalch and @rickstaa could we have a phone/video conference with responsible people at Franka to discuss the list of open issues in #89 (comment)? @fwalch, I'm contacting you as you appeared in most github PRs / issues. Please feel free to dispatch this request to another person if needed. I prepared a doodle to negotiate a time slot.

@rickstaa
Copy link
Contributor Author

rickstaa commented Oct 22, 2021

@rhaschke Of course, I'm happy to join the meeting to provide details about the upstream pull requests I created. In the meantime, please allow me to quickly describe all the upstream pull requests to make sure everybody has a clear picture.

Upstream pull requests related to the panda_moveit_config package

Here again, I will make a distinction between the real/visualized and simulated robot. From all the pull requests described below, franka_ros#181 is the most important. The other pull request will improve the user experience.

Visualized or real robot

The noetic-devel branch is ready to be used with both the RVIZ simulation and the real robot. There are, however, several problems with the way @frankaemika defined the collision geometries:

Simulated robot

  • The current FrankaHWSim interface does not contain any of the motion generators that are present in the FCI. As a result, the position_joint_trajectory_controller that was is used in the melodic-devel branch can not be used with the simulated robot. I created franka_ros#181 to allow us to use the position_joint_trajectory_controller in simulation. I chose the position_joint_trajectory_controller controller since the effort joint trajectory controller requires PID gains to be tuned when working on the real robot. The PID gains used for the motion generators in franka_ros#181 are okay, but not perfect. If @frankaemika improves these PID gains and merged franka_ros#181 the noetic-devel and melodic-devel (see Integrate with Gazebo #68) branches work with the franka_gazebo simulation.
  • I noticed that the FrankaHWSim interface does not allow MoveIt to set the desired gripper width. The current version only allows the gripper to close and open. I created franka_ros/pull/173 to address this issue. While doing so, the Franka team and I discovered some instabilities in the Franka simulation that prevent the gripper from working properly (see the pull request for more info).

Other pull requests

Apart from the pull requests above, I created franka_ros#177 to address an issue with the simulated gravity compensation. This pull request is related to my research but should not be needed for the noetic-devel of the panda_moveit_config package.

@rickstaa
Copy link
Contributor Author

@rhaschke Quick question. Based on the discussion above, I think we can drop the following TODO:

We have to check if remaping 'joint_states' to 'joint_states_desired' in the move_group.launch file is still needed (see Remaps move_group 'joint_states' to 'joint_states_desired' rickstaa#54).

Or do you first want to confirm with the @frankaemika team how the joint_states_desired works in the new version?

@rhaschke
Copy link
Contributor

I strongly believe that we can remove joint_states_desired. But, indeed, it would be great to get some feedback from @frankaemika first.

@rickstaa
Copy link
Contributor Author

@rhaschke I will keep it onto our to-do list till we talked to Franka.

This was referenced Oct 24, 2021
@rhaschke
Copy link
Contributor

@rickstaa, I have worked myself through your list of TODOs and would like to discuss them with you.
Do you have time for a chat? Please send me your contacts (email and/or phone) to [email protected]. Thanks.

@rickstaa
Copy link
Contributor Author

rickstaa commented Oct 25, 2021

@rhaschke That's great! I send you a mail.

@gollth
Copy link
Contributor

gollth commented Oct 25, 2021

@fwalch and @rickstaa could we have a phone/video conference with responsible people at Franka to discuss the list of open issues in #89 (comment)? @fwalch, I'm contacting you as you appeared in most github PRs / issues. Please feel free to dispatch this request to another person if needed. I prepared a doodle to negotiate a time slot.

Hello @rhaschke,

thanks for the Doodle, definitively makes sense to discuss these issues personally. We entered slots in your Doodle. If we don't find a common slot, feel free to propose more times.

Looking forward to it.

@v4hn
Copy link

v4hn commented Oct 25, 2021 via email

@rhaschke
Copy link
Contributor

rhaschke commented Nov 5, 2021

@v4hn, the results of our phone-conf meeting with Franka are summarized here.
@rickstaa, I addressed various issues of your list in moveit/moveit#2932, moveit/moveit#2945, moveit/moveit#2946.
I'm tempted to recreate the noetic-devel branch starting from these latest MSA templates.

This was referenced Nov 7, 2021
@rickstaa
Copy link
Contributor Author

rickstaa commented Nov 12, 2021

Closed because this has been replaced by #96.

The Gazebo-capable URDF generated by MSA doesn't show the robot in Gazebo, when using moveit_resources_panda_moveit_config. However, it seems to be there. It's just not visible?!

Has been addressed in https://github.com/rickstaa/moveit/tree/add_MSA_write_gazebo_urdf_feature. The MSA now checks whether the specified urdf is Gazebo compatible and otherwise saves it into the generated <ROBOT>_moveit_config. I will create a pull request after I performed some last tests and add the last warning popup (probably on Sunday). This popup will come up when a supplied urdf is invalid and lets people know about the file generation behaviour and points to http://gazebosim.org/tutorials?tut=inertia.

@rhaschke Can you double-check to see if there are still todos missing from #69. I will also address moveit/moveit#2955 on Sun.

@rhaschke
Copy link
Contributor

The Gazebo-capable URDF generated by MSA doesn't show the robot in Gazebo, when using moveit_resources_panda_moveit_config. However, it seems to be there. It's just not visible?!

Has been addressed in https://github.com/rickstaa/moveit/tree/add_MSA_write_gazebo_urdf_feature. The MSA now checks whether the specified urdf is Gazebo compatible and otherwise saves it into the generated <ROBOT>_moveit_config.

I think it is reasonable to provide an option to save the generated, Gazebo-compatible URDF. However, I suggest providing a save button in the corresponding widget, as the .urdf file will be saved in an external (non moveit-config) package. All other files listed in the config-files widget, are related to the moveit-config package only.

However, my problem wasn't that I couldn't save the file. Of course, I did that.
Nevertheless, Gazebo doesn't visualize the robot for me (although it is loaded and simulated). I guess, some material properties are not correctly configured for Gazebo:
image

@rickstaa
Copy link
Contributor Author

Strange, I remember that the urdf worked for me when using the steps in the tutorials moveit_setup_assistant. I will investigate this weekend.

The current implementation is as follows:

  • If the simulation widget is not used, no gazebo_panda.urdf will be added to the moveit_config folder. The file will be shown but is disabled. The demo launch file is added but uses the original urdf. My plan, however, was only to add the gazebo launch files when the simulation widget is used.
  • If the simulation widget is used and the provide urdf is Gazebo compatible, no gazebo_panda.urdf will be added to the moveit_config folder. The file will also be disabled in the file overview.
  • if the widget is used by provided urdf is not Gazebo compatible, the new gazebo urdf is added to the moveit_config, and the gazebo launch files will use this urdf while the other launch files use the original urdf. My plan was here to show a warning.

I can, however, change it in any way we like. I will create a draft pull request so we can discuss it there. My main goal was to fix the gazebo demo launch files by providing a have to automatically add the gazebo urdf when the provided urdf is not Gazebo compatible.

@rhaschke
Copy link
Contributor

As I said, I generally like and appreciate your proposal. We can use the generated gazebo-compatible file in two ways:

  • Overwrite the original file in the robot's description package. This option should be offered in the simulation widget.
  • Alternatively, create the corresponding file as part of the moveit_config package. In that case, it should be listed in the config file widget.

demo_gazebo.launch should use one or the other file. Both options are mutually exclusive.

@rickstaa
Copy link
Contributor Author

@rhaschke I took your suggestions into account. You can find the tested version of my proposal at moveit/moveit#2960.

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

No branches or pull requests

5 participants