Setup assistant automating generation of controllers configs #908

mayman99 · 2018-05-21T20:32:01Z

Description

This PR automates the generation of the ros_controller config files through the Setup Assistant, this is a part of upgrading the setup assistant. Issue 894

This ros_controllers.yaml was produced using this PR foe the PR2 Robot.

I'll follow this PR with another one that exposes the options through a GUI, and add documentation for it.

Checklist

Required: Code is auto formatted using clang-format
Extended the tutorials / documentation, if necessary reference
Include a screenshot if changing a GUI
Optional: Created tests, which fail without this PR reference
Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)

…into setup_assistant_controllers

davetcoleman · 2018-05-21T21:07:25Z

moveit_setup_assistant/src/widgets/configuration_files_widget.cpp

-  // fake_controllers.yaml --------------------------------------------------------------------------------------
-  file.file_name_ = "fake_controllers.yaml";
+  // controllers.yaml --------------------------------------------------------------------------------------
+  file.file_name_ = "controllers.yaml";


ros_controller.yaml

davetcoleman · 2018-05-21T21:07:36Z

moveit_setup_assistant/include/moveit/setup_assistant/tools/moveit_config_data.h

@@ -251,7 +251,7 @@ class MoveItConfigData
  bool outputOMPLPlanningYAML(const std::string& file_path);
  bool outputKinematicsYAML(const std::string& file_path);
  bool outputJointLimitsYAML(const std::string& file_path);
-  bool outputFakeControllersYAML(const std::string& file_path);


lets keep the fake controllers also

davetcoleman · 2018-05-21T21:07:56Z

moveit_setup_assistant/templates/moveit_config_pkg_template/launch/controllers.launch

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


ros_controllers.launch

davetcoleman

Great work!

davetcoleman · 2018-05-24T05:21:15Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+{
+  // Cache the joints' names.
+  std::vector<std::vector<std::string>> groups;
+  std::vector<std::string> groupJoints;


please use underscore for var names per ros style guidelines

davetcoleman · 2018-05-24T05:21:28Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+bool MoveItConfigData::outputControllersYAML(const std::string& file_path)
+{
+  // Cache the joints' names.
+  std::vector<std::vector<std::string>> groups;


use more specific name than just groups

davetcoleman · 2018-05-24T05:22:33Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+  std::vector<std::vector<std::string>> groups;
+  std::vector<std::string> groupJoints;
+
+  // Loop through groups


expand comment: why loop through?

davetcoleman · 2018-05-24T05:23:08Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+      if (joint->isPassive() || joint->getMimic() != NULL || joint->getType() == robot_model::JointModel::FIXED)
+        continue;
+      else
+      {


don't mix using brackets without using brackets... all or none

davetcoleman · 2018-05-24T05:23:27Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+    // Get list of associated joints
+    const robot_model::JointModelGroup* joint_model_group = getRobotModel()->getJointModelGroup(group_it->name_);
+    const std::vector<const robot_model::JointModel*>& joint_models = joint_model_group->getActiveJointModels();
+    // Iterate through the joints


explain why you are iterating through the joints

davetcoleman · 2018-05-24T05:24:14Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+    emitter << YAML::Value << YAML::BeginSeq;
+
+    // Iterate through the joints
+    for (int j = 0; j < groups[i].size(); j++)


std::size_t

davetcoleman · 2018-05-24T05:26:29Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+  emitter << YAML::Newline << YAML::Comment("Creates the /joint_states topic necessary in ROS");
+  emitter << YAML::EndMap;
+
+  emitter << YAML::BeginMap;


what if in order to track the begin and end of maps, brackets were used to indent these sections? e.g.

emitter << YAML::BeginMap; { emitter << YAML::Key << "joint_state_controller"; emitter << YAML::Value << YAML::BeginMap; ... } emitter << YAML::EndMap;

davetcoleman · 2018-05-24T05:27:20Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+  output_stream << emitter.c_str();
+  output_stream.close();
+
+  return true;  // file created successfully


this function is very long and readability is low. can it be separated out into a few smaller functions for each section of the yaml?

I think I can split it, but I think later we will have to change this function when we expose some of the options to be configured by the GUI, would it be okay to leave it as a one piece until then?

Sure, no problem.

davetcoleman · 2018-05-24T05:27:51Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+  std::ofstream output_stream(file_path.c_str(), std::ios_base::trunc);
+  if (!output_stream.good())
+  {
+    ROS_ERROR_STREAM("Unable to open file for writing " << file_path);


use ROS_ERROR_STREAM_NAMED

Why do you recommend using the Named version here?
All the functions here that cant output a file uses this
ROS_ERROR_STREAM("Unable to open file for writing " << file_path);

See the style guidelines under "ROS":
http://moveit.ros.org/documentation/contributing/code/

It makes filtering all the command line output much easier and cleaner. All of the MSA should be updated to use NAMED, but that belongs in a separate simple PR that doesn't clutter this one.

Okay I understand, Thanks.

davetcoleman · 2018-05-24T05:28:31Z

moveit_setup_assistant/templates/moveit_config_pkg_template/launch/ros_controllers.launch

+  <!-- Load joint controller configurations from YAML file to parameter server -->
+  <rosparam file="$(find [GENERATED_PACKAGE_NAME])/config/controllers.yaml" command="load"/>
+
+  <!-- load the controllers -->


be consistent with capitalization in comments. i personally prefer capitalizing

davetcoleman · 2018-05-24T05:29:29Z

@mcevoyandy please review

…into setup_assistant_controllers

mayman99 · 2018-05-25T13:14:58Z

Sorry didn't see the comments till now, I'll have a look at them later today.
For now I got the output configuration to work with 'moveit_sim_controller'.

mayman99 · 2018-05-25T23:28:59Z

Thanks for the review!
Addressed the comments, please have a look.
this is a ros_conttollers.yaml produced for the ur5 and tested with moveit_sim_controller
@davetcoleman @mcevoyandy

…into setup_assistant_controllers

rhaschke

Generally approve. Some minor comments remain.

rhaschke · 2018-05-26T15:12:48Z

moveit_setup_assistant/templates/moveit_config_pkg_template/launch/ros_controllers.launch

+
+  <!-- Load the controllers -->
+  <node name="controller_spawner" pkg="controller_manager" type="spawner" respawn="false"
+    output="screen" ns="/[GENERATED_PACKAGE_NAME]" args="spawn joint_state_controller position_trajectory_controller"/>


@willcbaker Is using absolute namespaces compatible to 4598f66?

I don't believe so, I would advise against absolute namespacing.
Unable to test or review until next week, seems changing this way with generated package name will require pushing down into package name namespace?

So you recommend to remove the ns argument from here and from the robot_state_publisher node? right?

Yes, I think so.

This ros_controllers.launch is intended to be used with gazebo, and according to ros_control tutorials, this is how gazebo_ros_control plugin is defined in the urdf

<gazebo> <plugin name="gazebo_ros_control" filename="libgazebo_ros_control.so"> <robotNamespace>/MYROBOT</robotNamespace> </plugin> </gazebo>

So if we were to launch the controllers with not the robot_name as a namespace , and leave the plugin element <robotNamespace> blank, how would we be able to use simulate multiple robot for example?
Is there a solution for a situation like that?

rhaschke · 2018-05-26T15:13:03Z

moveit_setup_assistant/templates/moveit_config_pkg_template/launch/ros_controllers.launch

+  <!-- Convert joint states to TF transforms for rviz, etc -->
+  <node name="robot_state_publisher" pkg="robot_state_publisher" type="robot_state_publisher"
+    respawn="false" output="screen">
+    <remap from="/joint_states" to="/[GENERATED_PACKAGE_NAME]/joint_states" />


rhaschke · 2018-05-26T15:13:50Z

moveit_setup_assistant/include/moveit/setup_assistant/tools/moveit_config_data.h

@@ -254,6 +254,7 @@ class MoveItConfigData
  bool outputKinematicsYAML(const std::string& file_path);
  bool outputJointLimitsYAML(const std::string& file_path);
  bool outputFakeControllersYAML(const std::string& file_path);
+  bool outputControllersYAML(const std::string& file_path);


outputROSControllersYAML?

…into setup_assistant_controllers

…d ros_error by ros_error_named

davetcoleman · 2018-05-29T18:34:31Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

@@ -732,7 +732,7 @@ bool MoveItConfigData::outputControllersYAML(const std::string& file_path)
  std::ofstream output_stream(file_path.c_str(), std::ios_base::trunc);
  if (!output_stream.good())
  {
-    ROS_ERROR_STREAM("Unable to open file for writing " << file_path);
+    ROS_ERROR_STREAM_NAMED("ros_controllers_config", "Unable to open file for writing " << file_path);


how about namespace = "setup_assistant"

-1 The package name (which is moveit_setup_assistant) is already part of the logger name. No need for an additional name at all!

Good point about the duplicate namespace, I should have recommended the file name "moveit_config_data" as that's the standard we usually follow. I really do like using _NAMED though. I've just attempted to document this standard better:
moveit/moveit.ros.org#166

So do we go back to ROS_ERROR_STREAM?

That's my suggestion. @davetcoleman?

I prefer the consistency of always using NAMED as I've just attempted to document in moveit/moveit.ros.org#166. Perhaps a Github issue should be opened to specifically debate this. Either way, having the namespace certainly does not hurt anything so I would like to just merge this PR.

@davetcoleman I don't agree. If you just have a redundant logger name like xxx.xxx, there is no reason to insist in NAMED. When we recently reworked the logging in some other MoveIt packages, we also dropped the extra NAME in such cases.

rhaschke · 2018-05-29T20:40:15Z

moveit_setup_assistant/templates/moveit_config_pkg_template/launch/ros_controllers.launch

@@ -6,12 +6,12 @@

  <!-- Load the controllers -->
  <node name="controller_spawner" pkg="controller_manager" type="spawner" respawn="false"
-    output="screen" ns="/[GENERATED_PACKAGE_NAME]" args="spawn joint_state_controller position_trajectory_controller"/>
+    output="screen" ns="[ROBOT_NAME]" args="spawn joint_state_controller position_trajectory_controller"/>


Looks like the ns prefix is very specific to your application. Why do you prefer ROBOT_NAME over GENERATED_PACKAGE_NAME now?

After looking at gazebo_ros_demos/rrbot_control.launch , I've found we use the robot name not the package name for the namespace.
Which is also done in the ros_control tutorial here.

…into setup_assistant_controllers

- ros_controllers.launch - ros_controllers.yaml

rhaschke · 2018-05-30T21:19:56Z

@MohmadAyman Please do not merge the devel branch into pull-request branches - never ever.
Instead you should rebase your PR to the latest devel branch.
Otherwise the commit history is screwed up and we have a really hard time to follow in the git tree what happened. Also, we cannot easily squash-merge the PR.

I manually merged this now in 89f55aa.

- ros_controllers.launch - ros_controllers.yaml

mayman99 added 6 commits May 19, 2018 11:32

add new function for generating controllers

fa6a3d5

output controllers file

3fc2678

Merge branch 'kinetic-devel' of https://github.com/ros-planning/moveit …

644884e

…into setup_assistant_controllers

cleaning a comment

bf5d682

removed fake controllers

8d765c6

added controllers launch file

63cfe2a

mayman99 closed this May 21, 2018

mayman99 reopened this May 21, 2018

davetcoleman reviewed May 21, 2018

View reviewed changes

re-added fake controllers for backward compatibility

b06bc6f

mayman99 mentioned this pull request May 22, 2018

Setup Assistant 2 #894

Closed

6 tasks

mayman99 added 5 commits May 22, 2018 19:51

fixed clang format error

6c5380d

renamed controllers launch to ros_controllers_launch

e9e9d31

output the joints in each group

7a6c6e8

replaced controllers.launch by ros_controllers.launch

faaa778

fixed clang formating errors

ce69772

davetcoleman requested changes May 24, 2018

View reviewed changes

mayman99 added 2 commits May 25, 2018 15:10

added MoveIt-specific simulation settings

e48a590

Merge branch 'kinetic-devel' of https://github.com/ros-planning/moveit …

b75e461

…into setup_assistant_controllers

improved documentaion and style

c2cc610

mayman99 added 3 commits May 26, 2018 01:39

replace int by std::size_t

7ed61e0

renamed controllers.yaml to ros_controllers.yaml

b72b3e8

Merge branch 'kinetic-devel' of https://github.com/ros-planning/moveit …

07e3ac8

…into setup_assistant_controllers

rhaschke requested changes May 26, 2018

View reviewed changes

mayman99 added 3 commits May 29, 2018 13:33

Merge branch 'kinetic-devel' of https://github.com/ros-planning/moveit …

6dcadd2

…into setup_assistant_controllers

renamed outputControllersYAML to outputROSControllersYAML and replace…

322c6d5

…d ros_error by ros_error_named

removed absolute namespacing

2364497

mayman99 added 3 commits May 29, 2018 15:19

use relative namespacing

b44c197

fix function name in config files widget

92544a1

use robot name for namespacing

0c1b91e

davetcoleman approved these changes May 29, 2018

View reviewed changes

rhaschke mentioned this pull request May 29, 2018

remove unused variable in moveit_config_data -> setup assistant #924

Merged

5 tasks

rhaschke requested changes May 29, 2018

View reviewed changes

mayman99 added 3 commits May 30, 2018 00:47

Merge branch 'kinetic-devel' of https://github.com/ros-planning/moveit …

64a266c

…into setup_assistant_controllers

Merge branch 'kinetic-devel' of https://github.com/ros-planning/moveit …

3f9ee4c

…into setup_assistant_controllers

rename namespace to setup_assistant

c929678

rhaschke pushed a commit that referenced this pull request May 30, 2018

MSA: new function to generate ROS controllers config (#908)

89f55aa

- ros_controllers.launch - ros_controllers.yaml

rhaschke closed this May 30, 2018

rhaschke pushed a commit to ubi-agni/moveit that referenced this pull request May 30, 2018

MSA: new function to generate ROS controllers config (moveit#908)

bf97fbd

- ros_controllers.launch - ros_controllers.yaml

dg-shadow pushed a commit to shadow-robot/moveit that referenced this pull request Jul 30, 2018

MSA: new function to generate ROS controllers config (moveit#908)

a086414

- ros_controllers.launch - ros_controllers.yaml

darkdragon-001 mentioned this pull request Jan 22, 2019

How to use ros_controllers.yaml and ros_controllers.launch from new setup assistant? #1314

Closed

rhaschke mentioned this pull request Oct 23, 2021

demo_gazebo.launch with standard ROS packages rhaschke/panda_moveit_config#9

Closed

5 tasks

Setup assistant automating generation of controllers configs #908

Setup assistant automating generation of controllers configs #908

Conversation

mayman99 commented May 21, 2018 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davetcoleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davetcoleman commented May 24, 2018

mayman99 commented May 25, 2018

mayman99 commented May 25, 2018 • edited Loading

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayman99 May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayman99 May 29, 2018 • edited Loading

Choose a reason for hiding this comment

rhaschke commented May 30, 2018

mayman99 commented May 21, 2018 •

edited

Loading

mayman99 commented May 25, 2018 •

edited

Loading

mayman99 May 30, 2018 •

edited

Loading

mayman99 May 29, 2018 •

edited

Loading