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

Read and write ROS controllers yaml file #951

Conversation

Projects
None yet
4 participants
@MohmadAyman
Copy link
Member

commented Jun 13, 2018

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

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:gazebo_integration_trials branch 2 times, most recently from 96d30f6 to 4979db2 Jun 13, 2018

@davetcoleman
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 15, 2018

Member

comment: "joints controlled by this controller"

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 15, 2018

Member

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

"Controller" is way too generic

@@ -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_;

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 15, 2018

Member

rename sim_controllers_initial_group_

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jun 18, 2018

Author Member

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.

std::string controllers_initial_group_;

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 15, 2018

Member

sim_controllers_...

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

/// Controllers config data

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 15, 2018

Member

more descriptive comment, define "controller" please

void editSelected();

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 15, 2018

Member

remove

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

current_edit_group_.clear();

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 15, 2018

Member

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

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jun 16, 2018

Author Member

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"

This comment has been minimized.

Copy link
@MohmadAyman

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 18, 2018

Member

Yes there's a middle ground :-)

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

@@ -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_;

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 15, 2018

Member

add comment explaining what this is

}

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 15, 2018

Member

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

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 15, 2018

Member

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

This comment has been minimized.

Copy link
@MohmadAyman

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:gazebo_integration_trials branch 3 times, most recently from 12067a5 to 09d4cac Jun 18, 2018

@davetcoleman
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 18, 2018

Member

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.

@MohmadAyman MohmadAyman referenced this pull request Jun 21, 2018

Merged

Simulation Screen for the SA #956

3 of 5 tasks complete

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:gazebo_integration_trials branch 2 times, most recently from c385519 to 87f7177 Jun 21, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2018

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

moveit::core::RobotModelConstPtr robot_model;
};

TEST(MoveItConfigData, ReadingControllers)

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jun 25, 2018

Author Member

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.

find_package(moveit_resources REQUIRED)
include_directories(${moveit_resources_INCLUDE_DIRS})

catkin_add_gtest(test_writting_ros_controllers test/moveit_config_data_test.cpp)

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 25, 2018

Member

"writing" (misspelled)

@@ -459,107 +614,6 @@ std::vector<OMPLPlannerDescription> MoveItConfigData::getOMPLPlanners()
return planner_des;
}

// ******************************************************************************************

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 25, 2018

Member

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

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jun 25, 2018

Author Member

Sorry I didn't understand, which files?

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 25, 2018

Member

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

#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>

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 25, 2018

Member

remove


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

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 25, 2018

Member

"This test adds..."

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jun 25, 2018

Author Member

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


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);

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 25, 2018

Member

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?

This comment has been minimized.

Copy link
@mlautman

mlautman Jun 27, 2018

Member

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.

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jun 27, 2018

Author Member

Sure, will add it in a separate PR.

// Read ros_controllers.yaml
// config_data_->inputROSControllersYAML("ros_controllers.yaml");

// Verify that we read the file correctly

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jun 25, 2018

Member

seems to be on the right track!

@MohmadAyman MohmadAyman changed the title Added a new screen for controllers Read and write ROS controllers yaml file Jun 27, 2018

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:gazebo_integration_trials branch from 87f7177 to cb70c23 Jul 2, 2018

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:gazebo_integration_trials branch from cb70c23 to ad63be1 Jul 2, 2018

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

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 2, 2018

Author Member

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 5, 2018

Member

not sure what you mean 'diff'?

This comment has been minimized.

Copy link
@mcevoyandy

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.

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 9, 2018

Author Member

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

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2018

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

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 5, 2018

Member

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)

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 9, 2018

Author Member

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?

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 5, 2018

Member

not sure what you mean 'diff'?

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

@mcevoyandy please review

@mcevoyandy

This comment has been minimized.

Copy link

commented Jul 8, 2018

@davetcoleman lgtm.

@davetcoleman davetcoleman merged commit c657650 into ros-planning:kinetic-devel Jul 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

Great job @MohmadAyman !!

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

Thank you 😺

rhaschke added a commit that referenced this pull request Jul 17, 2018

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

MohmadAyman added a commit to MohmadAyman/moveit that referenced this pull request Aug 25, 2018

@ipa-hsd ipa-hsd referenced this pull request Oct 25, 2018

Closed

Feat simple moveit controller manager #1140

0 of 7 tasks complete

@ipa-hsd ipa-hsd referenced this pull request Mar 18, 2019

Closed

Feat simple moveit controller manager #1402

0 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.