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.

Sign up for GitHub

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

Jump to bottom

Read and write ROS controllers yaml file #951

Merged

davetcoleman merged 1 commit into moveit:kinetic-devel from mayman99:gazebo_integration_trials

Jul 9, 2018

Contributor

mayman99 commented Jun 13, 2018 •

edited

Loading

Description

Adding functions to read and write ros_contollers.yaml file.
Adding an automated way to add FollowJointTrajectory action controller for each planning group, e.g.

  controller_list:
  - name: g1_controller
    action_ns: follow_joint_trajectory
    default: True
    type: FollowJointTrajectory
    joints:
      - joint1
      - joint2

@davetcoleman @mcevoyandy

Checklist

Required: Code is auto formatted using clang-format
[needed] 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)

This was referenced Jun 13, 2018

Controllers Widget #933

Closed

Setup Assistant 2 #894

Closed

mayman99 force-pushed the gazebo_integration_trials branch 2 times, most recently from 96d30f6 to 4979db2 Compare

June 14, 2018 11:01

davetcoleman requested changes

View reviewed changes

Member

davetcoleman left a comment

Great work! Sorry for the intense review, I just care a lot about quality :)

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

+              {
+                std::string name_;                 // controller name
+                std::string type_;                 // controller type
+                std::vector<std::string> joints_;  // joints names this controller

Member

davetcoleman Jun 15, 2018

comment: "joints controlled by this controller"

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

+              /**
+               * Controllers settings which may be set in the config files
+               */
+              struct Controller

Member

davetcoleman Jun 15, 2018

more descriptive name for the struct... maybe "ROSControlConfig"?

"Controller" is way too generic

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

@@ @@ -221,6 +231,15 @@ class MoveItConfigData @@
                 /// Email of the author of this config
                 std::string author_email_;
+                /// Name of the group that holds all the joints to be used by sim contollers
+                std::string controllers_initial_group_;

Member

davetcoleman Jun 15, 2018

rename sim_controllers_initial_group_

Contributor Author

mayman99 Jun 18, 2018

I removed the sim_controllers part as it is still to be decided which is the best screen to put them, I put it in the description.

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

+                std::string controllers_initial_group_;
+                /// Name of the initial pose to be used by sim contollers
+                std::string controllers_initial_pose_;

Member

davetcoleman Jun 15, 2018

sim_controllers_...

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

+                /// Name of the initial pose to be used by sim contollers
+                std::string controllers_initial_pose_;
+                /// Controllers config data

Member

davetcoleman Jun 15, 2018

more descriptive comment, define "controller" please

moveit_setup_assistant/src/widgets/controllers_widget.h Outdated

+                void editSelected();
+                /// Called from Double List widget to highlight a joint
+                // void previewClickedJoint( std::string name );

Member

davetcoleman Jun 15, 2018

remove

moveit_setup_assistant/src/widgets/planning_groups_widget.cpp Outdated

@@ @@ -282,6 +282,7 @@ void PlanningGroupsWidget::loadGroupsTree() @@
                   btn_delete_->hide();
                 }
+                current_edit_group_.clear();

Member

davetcoleman Jun 15, 2018

these kinds of changes should be in a separate PR... this one is way too big already

Contributor Author

mayman99 Jun 16, 2018

I wasn't so sure too, but from the feedback I got from PR where I removed the unused variable, "it is advised not to open too small PR"

Contributor Author

mayman99 Jun 16, 2018

Member

davetcoleman Jun 18, 2018

Yes there's a middle ground :-)

One line change is a bit small, but many hundreds of lines is getting large.

moveit_setup_assistant/src/widgets/start_screen_widget.cpp Outdated

@@ @@ -431,6 +431,10 @@ bool StartScreenWidget::loadExistingFiles() @@
                                            .append(kinematics_yaml_path.make_preferred().native().c_str()));
                 }
+                fs::path controllers_yaml_path = config_data_->config_pkg_path_;

Member

davetcoleman Jun 15, 2018

add comment explaining what this is

moveit_setup_assistant/src/widgets/controllers_widget.cpp Outdated

+              }
+              // ******************************************************************************************
+              // Add a Follow Joint Trajectory action Controller for each Planning Group

Member

davetcoleman Jun 15, 2018

"Add a FollowJointTrajectory action controller from ros_control for each Planning Group"

moveit_setup_assistant/src/widgets/controllers_widget.cpp Outdated

+              // ******************************************************************************************
+              // Add a Follow Joint Trajectory action Controller for each Planning Group
+              // ******************************************************************************************
+              void ControllersWidget::addDefaultControllers()

Member

davetcoleman Jun 15, 2018

can you also, by default, add a joint_state_controller/JointStateController? those are equally important

Contributor Author

mayman99 Jun 17, 2018

Yes I already do that in moveit_data_config @ https://github.com/ros-planning/moveit/pull/951/files#diff-b722193e66bd85583ac56b9d68186447R666

mayman99 force-pushed the gazebo_integration_trials branch 3 times, most recently from 12067a5 to 09d4cac Compare

June 18, 2018 19:54

davetcoleman reviewed

View reviewed changes

Member

davetcoleman left a comment

This is looking really good! All it needs is the unit test.

moveit_setup_assistant/src/widgets/setup_assistant_widget.h Outdated

@@ @@ -220,6 +221,7 @@ private Q_SLOTS: @@
                 EndEffectorsWidget* efw_;
                 VirtualJointsWidget* vjw_;
                 PassiveJointsWidget* pjw_;
+                ControllersWidget* cw_;

Member

davetcoleman Jun 18, 2018

these abbreviated variable names are bad practice, but they are my fault 7 years ago when I was a younger programmer. can you add a TODO to make them not abbreviated? I don't want to make this PR any bigger itself.

mayman99 mentioned this pull request

Simulation Screen for the SA #956

Merged

5 tasks

mayman99 force-pushed the gazebo_integration_trials branch 2 times, most recently from c385519 to 87f7177 Compare

June 21, 2018 13:33

Contributor Author

mayman99 commented Jun 23, 2018

As suggested by @davecoleman this PR now does only deal with reading and writing the ros_controllers.yaml file.

mayman99 commented

View reviewed changes

moveit_setup_assistant/test/moveit_config_data_test.cpp Outdated

+                moveit::core::RobotModelConstPtr robot_model;
+              };
+              TEST(MoveItConfigData, ReadingControllers)

Contributor Author

mayman99 Jun 25, 2018

The test is still not completely implemented, I've a problem setting the robot_model in config_data. I've spend some time trying to figure out how to set it in the test, so for efficiency I thought I would ask for ideas here.

davetcoleman reviewed

View reviewed changes

moveit_setup_assistant/CMakeLists.txt Outdated

+                find_package(moveit_resources REQUIRED)
+                include_directories(${moveit_resources_INCLUDE_DIRS})
+                catkin_add_gtest(test_writting_ros_controllers test/moveit_config_data_test.cpp)

Member

davetcoleman Jun 25, 2018

"writing" (misspelled)

moveit_setup_assistant/src/tools/moveit_config_data.cpp Outdated

                 return planner_des;
               }
-              // ******************************************************************************************

Member

davetcoleman Jun 25, 2018

just curious - why move these files? it makes reviewing harder because i can't tell what has actually changed

Contributor Author

mayman99 Jun 25, 2018

Sorry I didn't understand, which files?

Member

davetcoleman Jun 25, 2018

oops i mean functions.... why move the functions to a different location in the file?

moveit_setup_assistant/test/moveit_config_data_test.cpp Outdated

+              #include <boost/filesystem.hpp>  // for creating folders/files
+              #include <moveit/setup_assistant/tools/moveit_config_data.h>
+              // #include <moveit/setup_assistant/src/widgets/controllers_widget.h>

Member

davetcoleman Jun 25, 2018

remove

moveit_setup_assistant/test/moveit_config_data_test.cpp Outdated


		// #include <moveit/setup_assistant/src/widgets/controllers_widget.h>

		// This tests adding FollowJointTrajectory controllers for each planning group to ros_controller.yaml

Member

davetcoleman Jun 25, 2018

"This test adds..."

Contributor Author

mayman99 Jun 25, 2018

This tests adding FollowJointTrajectory controllers, isn't that Okay?

moveit_setup_assistant/test/moveit_config_data_test.cpp

+                  srdf_model.reset(new srdf::Model());
+                  std::string xml_string;
+                  std::fstream xml_file((res_path / "pr2_description/urdf/robot.xml").string().c_str(), std::fstream::in);

Member

davetcoleman Jun 25, 2018

lets move away from using the PR2 but rather the Franka Emika robot instead. we might need to add Franka Emika into moveit_resources. @mlautman any opinions?

Contributor

mlautman Jun 27, 2018

I'm strongly in favor of replacing the PR2 with the Panda wherever possible. @mohmadAyman a small side project that would be a huge improvement would be to add the Panda to moveit_resources so that it can be used in this test.

Contributor Author

mayman99 Jun 27, 2018 •

edited

Loading

Sure, will add it in a separate PR.

moveit_setup_assistant/test/moveit_config_data_test.cpp Outdated

+                // Read ros_controllers.yaml
+                // config_data_->inputROSControllersYAML("ros_controllers.yaml");
+                // Verify that we read the file correctly

Member

davetcoleman Jun 25, 2018

seems to be on the right track!

mayman99 changed the title ~~Added a new screen for controllers~~ Read and write ROS controllers yaml file

mayman99 force-pushed the gazebo_integration_trials branch from 87f7177 to cb70c23 Compare

July 2, 2018 06:42


          Parsing ros controllers

ad63be1

mayman99 force-pushed the gazebo_integration_trials branch from cb70c23 to ad63be1 Compare

July 2, 2018 06:53

mayman99 commented

View reviewed changes

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+              // ******************************************************************************************
+              // Output kinematic config files
+              // ******************************************************************************************
+              bool MoveItConfigData::outputKinematicsYAML(const std::string& file_path)

Contributor Author

mayman99 Jul 2, 2018

@davetcoleman
I didnt change anything in these functions, does git sees that as a diff because I've added the setRobotModel function above?

Member

davetcoleman Jul 5, 2018

not sure what you mean 'diff'?

mcevoyandy Jul 8, 2018

Looks like you moved them up in the file? I think this is why git sees this as a large diff.

Contributor Author

mayman99 Jul 9, 2018

Sorry just saw the comment, I mean the difference that git saw.

Contributor Author

mayman99 commented Jul 3, 2018

Ping @davetcoleman @mcevoyandy
are there any further comments concerning this PR?

davetcoleman approved these changes

View reviewed changes

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+              void MoveItConfigData::setRobotModel(robot_model::RobotModelPtr robot_model)
+              {
+                robot_model_ = robot_model;
+                robot_model_const_ = robot_model;

Member

davetcoleman Jul 5, 2018

just curious: do you know why i originally had to add a const and non-const version of robot_model? this is very hacky. shame on me (~6 years ago)

Contributor Author

mayman99 Jul 9, 2018

Well the robot_model_const_ is only used in getRobotModel, while robot_model_ is used whenever we are to edit sth in the robot model.

We can remove robot_model_const_ and just const_cast the robot_model_ in the getRobotModel(), does that sound better?

moveit_setup_assistant/src/tools/moveit_config_data.cpp

+              // ******************************************************************************************
+              // Output kinematic config files
+              // ******************************************************************************************
+              bool MoveItConfigData::outputKinematicsYAML(const std::string& file_path)

Member

davetcoleman Jul 5, 2018

not sure what you mean 'diff'?

Member

davetcoleman commented Jul 5, 2018

@mcevoyandy please review

mcevoyandy commented Jul 8, 2018

@davetcoleman lgtm.

davetcoleman merged commit c657650 into moveit:kinetic-devel

Member

davetcoleman commented Jul 9, 2018

Great job @mohmadAyman !!

Contributor Author

mayman99 commented Jul 9, 2018

Thank you 😺

rhaschke pushed a commit that referenced this pull request


          Parsing ros controllers (#951)

dg-shadow pushed a commit to shadow-robot/moveit that referenced this pull request


          Parsing ros controllers (moveit#951)

554bf0d

mayman99 added a commit to mayman99/moveit that referenced this pull request


          Parsing ros controllers (moveit#951)

e4f4aea

hsd-dev mentioned this pull request

Feat simple moveit controller manager #1140

Closed

7 tasks

darkdragon-001 mentioned this pull request

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

Closed

hsd-dev mentioned this pull request

Feat simple moveit controller manager #1402

Closed

7 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet