ROS Controllers screen added to SA by mayman99 · Pull Request #994 · moveit/moveit

mayman99 · 2018-07-22T10:18:27Z

Description

This screen main purpose is to automate the process of generating ROS Controllers configuration.

The generated ROS Controllers config is launched by ros_controllers.launch which makes simulating a robot in gazebo possible.

This PR is part of upgrading the Setup Assistant.

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)

mayman99 · 2018-07-23T20:25:38Z

If pull request #969 turned to need the new struct to be a class, this will too.

mayman99 · 2018-07-29T07:08:14Z

Friendly ping @davetcoleman
If pull request #969 turned to need the new struct to be a class, this probably will too.

davetcoleman

Really great work!

davetcoleman · 2018-07-30T16:37:48Z

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

  bool outputFakeControllersYAML(const std::string& file_path);
-  void outputFollowJointTrajectoryYAML(YAML::Emitter& emitter);
+  void outputFollowJointTrajectoryYAML(YAML::Emitter& emitter,
+                                       std::vector<ROSControlConfig>* ros_controllers_config_copy);


why is this called _copy?

Because this function modifies the ros_controllers_config_ object, we don't want to modify the original object here.

davetcoleman · 2018-07-30T16:37:59Z

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

  bool outputJointLimitsYAML(const std::string& file_path);
  bool outputFakeControllersYAML(const std::string& file_path);
-  void outputFollowJointTrajectoryYAML(YAML::Emitter& emitter);
+  void outputFollowJointTrajectoryYAML(YAML::Emitter& emitter,


add function documentation for these args

davetcoleman · 2018-07-30T16:38:25Z

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

+  ROSControlConfig* findROSControllerByName(const std::string& controller_name);
+
+  /**
+   * delete ros controller by name


davetcoleman · 2018-07-30T16:39:22Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

  if (!input_stream.good())
  {
-    ROS_ERROR_STREAM_NAMED("ros_controller.yaml", "Unable to open file for reading " << file_path);
+    ROS_INFO_STREAM_NAMED("ros_controller.yaml", "Does not exist " << file_path);


shouldn't this be an error or warning?

I agree a warning maybe better here, initially the package might not has a ros_controllers.yaml, so a warn would do I believe.

davetcoleman · 2018-07-30T16:39:48Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

    // Read the robot name

-    for (YAML::const_iterator controller_it = doc.begin(); controller_it != doc.end(); ++controller_it)
+    for (YAML::const_iterator map_it = doc.begin(); map_it != doc.end(); ++map_it)


can you make map_it more descriptive / longer?

davetcoleman · 2018-07-30T16:43:11Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

            }
-            else
+          }
+          else


this is very difficult to follow, please split into helper functions

davetcoleman · 2018-07-30T16:43:39Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp


 // ******************************************************************************************
-// Find controller by name
+// Find ros controller by name


davetcoleman · 2018-07-30T16:44:12Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

 }

-}  // namespace
+}  // namespace\


davetcoleman · 2018-07-30T16:45:34Z

moveit_setup_assistant/src/widgets/controller_edit_widget.cpp

+    return;
+  has_loaded = true;
+
+  const std::array<std::string, 10> types = { "effort_controllers/JointTrajectoryController",


rename to default_types

davetcoleman · 2018-07-30T16:46:05Z

moveit_setup_assistant/src/widgets/controller_edit_widget.h

+ *     copyright notice, this list of conditions and the following
+ *     disclaimer in the documentation and/or other materials provided
+ *     with the distribution.
+ *   * Neither the name of Willow Garage nor the names of its


note "Willow Garage" is still in the license here and elsewhere. replace with your name

mayman99 · 2018-07-31T22:36:09Z

@davetcoleman
Addressed the comments on this one.

davetcoleman · 2018-07-31T23:29:02Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

 bool MoveItConfigData::outputROSControllersYAML(const std::string& file_path)
 {
+  // Copy ros_control_config_ to a new vector to avoid modifying it
+  std::vector<ROSControlConfig> ros_controllers_config_copy(ros_controllers_config_);


instead of copy can we call this ros_controllers_config_output?

davetcoleman · 2018-07-31T23:29:23Z

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

+   * @return void
+   */
+  void outputFollowJointTrajectoryYAML(YAML::Emitter& emitter,
+                                       std::vector<ROSControlConfig>* ros_controllers_config_copy);


instead of a pointer, pass this in by reference

davetcoleman · 2018-07-31T23:32:23Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

        // Loop through all controllers
-        if (const YAML::Node& controller = controller_it->second)
+        for (YAML::const_iterator controller_it = controllers.begin(); controller_it != controllers.end();
+             ++controller_it)


move the contents of this for loop into a helper function to make this more readable... named perhaps processROSController(controller_it, ...)

davetcoleman · 2018-07-31T23:32:39Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

 }

+// ******************************************************************************************
+// Deletes a ros controller by name


"ROS" and elsewhere

davetcoleman · 2018-07-31T23:32:53Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

 // Gets ros_controllers_config_ vector
 // ******************************************************************************************
-std::vector<ROSControlConfig> MoveItConfigData::getROSControllers()
+std::vector<ROSControlConfig>* MoveItConfigData::getROSControllers()


return by reference, not pointer

davetcoleman · 2018-07-31T23:34:21Z

Looks like your other PR caused merge conflicts. otherwise its looking very close!

@mcevoyandy please review

mayman99 · 2018-08-02T07:48:45Z

@davetcoleman
Addressed the comments both here and in the doc, I thought of not squashing the commits to the review easier and when approved I can squash them.

davetcoleman · 2018-08-05T19:05:29Z

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

+   * Helper function for writing follow joint trajectory ROS controllers to ros_controllers.yaml
+   * @param YAML Emitter - yaml emitter used to write the config to the ROS controllers yaml file
+   * @param vector<ROSControlConfig> - a copy of ROS controllers config which will be modified in the function
+   * @return void


no need to have this line @return if its void, please remove

davetcoleman · 2018-08-05T19:05:34Z

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

+   * Helper function for writting Joint State ROS controllers to ros_controller.yaml
+   * @param YAML Emitter - yaml emitter used to write the config to the ROS controllers yaml file
+   * @param vector<ROSControlConfig> - a copy of ROS controllers config which will be modified in the function
+   * @return void


no need to have this line @return if its void, please remove

davetcoleman · 2018-08-05T19:06:16Z

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


+  /**
+   * Helper function for parsing ros_controllers.yaml file
+   * @param YAML::Node - controllers to be parsed


"controllers" --> "individual controller"

davetcoleman · 2018-08-05T19:06:33Z

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


  /**
   * \brief Add a Follow Joint Trajectory action Controller for each Planning Group
+   * \return bool if controllers were added to the ros_controllers_config_ data structure


"bool" --> "true"

davetcoleman · 2018-08-05T19:06:39Z

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

+
+  /**
+   * \brief Add a Joint State Controller for each Planning Group
+   * \return bool if controllers were added to the ros_controllers_config_ data structure


"bool" --> "true"

davetcoleman · 2018-08-05T19:08:11Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

 }

+// ******************************************************************************************
+// Helper function for parsing ros_controllers.yaml file


be more descriptive in comment, currently this comment is identical to line 1272. what is different about this function from processROSControllers?

davetcoleman · 2018-08-05T19:09:09Z

moveit_setup_assistant/src/widgets/controller_edit_widget.cpp

+void ControllerEditWidget::loadControllersTypesComboBox()
+{
+  // Only load this combo box once
+  static bool has_loaded = false;


do not use static, rather make this a member variable of the class

davetcoleman · 2018-08-05T19:10:19Z

moveit_setup_assistant/src/widgets/controller_edit_widget.h

+/* Author: Mohamad Ayman */
+
+#ifndef MOVEIT_ROS_MOVEIT_SETUP_ASSISTANT_WIDGETS_CONTROLLER_EDIT_WIDGET_
+#define MOVEIT_ROS_MOVEIT_SETUP_ASSISTANT_WIDGETS_CONTROLLER_EDIT_WIDGET_


add "H" at end: http://wiki.ros.org/CppStyleGuide#A.23ifndef_guards

davetcoleman · 2018-08-05T19:10:42Z

moveit_setup_assistant/src/widgets/controller_edit_widget.h

+
+  // ******************************************************************************************
+  // Qt Components
+  // ******************************************************************************************


this components should be private, not public

davetcoleman · 2018-08-05T19:10:55Z

moveit_setup_assistant/src/widgets/ros_controllers_widget.h

+  virtual void focusGiven();
+
+  // ******************************************************************************************
+  // Qt Components


make private

davetcoleman · 2018-08-05T19:15:31Z

Review from runtime:

Change screen title: "ROS Control Controllers"
Change description sentence: "Configure MoveIt! to work with ROS Control to control the robot's physical hardware."
Both of the "Auto" buttons have the "A" underlined, but this defeats the purpose of the hotkey shortcut. Choose a different key for the second button

davetcoleman · 2018-08-09T17:52:51Z

Is this ready for re-review?

mayman99 · 2018-08-09T18:51:36Z

Yes it is ready @davetcoleman

davetcoleman · 2018-08-09T21:22:10Z

This segfaults when I choose a perception sensor then change screens.

Apply this PR to moveit source install
git clone franka_ros
catkin build
Start the MSA
Create new moveit_config package by loading URDF from ws_moveit/src/franka_ros/franka_description/robot
Choose the perception screen
Choose a sensor
Change screens
Segfault!

mayman99 · 2018-08-09T22:23:16Z

@davetcoleman I believe this makes sense, since the pull request #1013 [fix bug in perception screen] is not merged yet.

davetcoleman

Tested locally and it works :)

mcevoyandy · 2018-08-13T17:36:22Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+      // If the setting has joints then it is a controller that needs to be parsed
+      if (!control_setting.joints_.empty())
+      {
+        if (const YAML::Node& urdf_node = controller_it->second["type"])


combine if statements?

I don't think I can put more than one condition in an if statement that has initialization in it, I'm not 100% if it is not possible, but I've tried and it didn't work.

mcevoyandy · 2018-08-13T18:16:36Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

 bool MoveItConfigData::outputROSControllersYAML(const std::string& file_path)
 {
+  // Copy ros_control_config_ to a new vector to avoid modifying it
+  std::vector<ROSControlConfig> ros_controllers_config_output(ros_controllers_config_);


Cand the copy of outputFollowJointTrajectoryYAML be moved into outputFollowJointTrajectoryYAML()? It seems it's not used elsewhere in the function. Also, can you add a comment as to why you are erasing things from the copy?

I believe you meant ros_controllers_config_output which is a copy of ros_controllers_config_, It is used in the rest of the function outputROSControllersYAML, thats why it is defined there not in outputFollowJointTrajectoryYAML.

I'll add a comment the explains why I erase the controller that I write to the file.

addressed all comments.

ros_controllers_config_output is not used in outputROSControllersYAML(), only in outputFollowJointTrajectoryYAML(). please move it there since its only actually used in that helper function

ros_controllers_config_output is used in outputROSControllersYAML() here .

mcevoyandy · 2018-08-13T18:44:21Z

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+    // Writes Follow Joint Trajectory ROS controllers to ros_controller.yaml
+    outputFollowJointTrajectoryYAML(emitter, ros_controllers_config_output);

    {


Remove bracket

thats right, thats scoping didn't really make sense

davetcoleman · 2018-08-14T06:51:33Z

cherry-picked to melodic

great job!

* Added ros controllers screen to SA * GUI and code fixes * added a ros_control namespace * better docs, moved variables from public to private * reverted joint state controllers change * adding a minor comment

mayman99 mentioned this pull request Jul 22, 2018

Setup Assistant 2 #894

Closed

6 tasks

mayman99 force-pushed the ros_controllers_screen branch 2 times, most recently from cd43756 to 0bdb47e Compare July 23, 2018 12:26

davetcoleman requested changes Jul 30, 2018

View reviewed changes

mayman99 force-pushed the ros_controllers_screen branch from 0bdb47e to ec5004b Compare July 31, 2018 22:35

davetcoleman requested changes Jul 31, 2018

View reviewed changes

mayman99 force-pushed the ros_controllers_screen branch from ec5004b to 2fc77a2 Compare August 2, 2018 07:46

davetcoleman requested changes Aug 5, 2018

View reviewed changes

mayman99 force-pushed the ros_controllers_screen branch 2 times, most recently from 1330ee8 to a00ce3d Compare August 6, 2018 22:33

davetcoleman approved these changes Aug 13, 2018

View reviewed changes

mcevoyandy reviewed Aug 13, 2018

View reviewed changes

mayman99 force-pushed the ros_controllers_screen branch from a00ce3d to c5c667c Compare August 13, 2018 20:31

mayman99 added 6 commits August 14, 2018 01:17

Added ros controllers screen to SA

72de640

GUI and code fixes

90b68a4

added a ros_control namespace

6d56553

better docs, moved variables from public to private

f975472

reverted joint state controllers change

99a7d65

adding a minor comment

ba56e13

mayman99 force-pushed the ros_controllers_screen branch from c5c667c to ba56e13 Compare August 13, 2018 22:23

davetcoleman approved these changes Aug 14, 2018

View reviewed changes

davetcoleman merged commit bd7e1c1 into moveit:kinetic-devel Aug 14, 2018

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 Nov 5, 2021

MSA: Rework ROS controllers config #2945

Merged

JafarAbdi pushed a commit to JafarAbdi/moveit that referenced this pull request Mar 24, 2022

Remove 'using namespace' from header files. (moveit#994)

877d0bd

Conversation

mayman99 commented Jul 22, 2018

Description

Checklist

Uh oh!

mayman99 commented Jul 23, 2018

Uh oh!

mayman99 commented Jul 29, 2018

Uh oh!

davetcoleman 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!

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!

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!

mayman99 commented Jul 31, 2018

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!

davetcoleman commented Jul 31, 2018

Uh oh!

mayman99 commented Aug 2, 2018

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!

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!

davetcoleman commented Aug 5, 2018

Uh oh!

davetcoleman commented Aug 9, 2018

Uh oh!

mayman99 commented Aug 9, 2018

Uh oh!

mayman99 commented Aug 9, 2018 •

edited

Loading