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

ROS Controllers screen added to SA #994

Merged
merged 6 commits into from Aug 14, 2018

Conversation

Projects
None yet
3 participants
@MohmadAyman
Copy link
Contributor

MohmadAyman commented Jul 22, 2018

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)

@MohmadAyman MohmadAyman referenced this pull request Jul 22, 2018

Closed

Setup Assistant 2 #894

6 of 6 tasks complete

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:ros_controllers_screen branch 2 times, most recently from cd43756 to 0bdb47e Jul 23, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Contributor Author

MohmadAyman commented Jul 23, 2018

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

@MohmadAyman

This comment has been minimized.

Copy link
Contributor Author

MohmadAyman commented Jul 29, 2018

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

@davetcoleman
Copy link
Member

davetcoleman left a comment

Really great work!

@@ -277,7 +269,8 @@ class MoveItConfigData
bool outputKinematicsYAML(const std::string& file_path);
bool outputJointLimitsYAML(const std::string& file_path);
bool outputFakeControllersYAML(const std::string& file_path);
void outputFollowJointTrajectoryYAML(YAML::Emitter& emitter);
void outputFollowJointTrajectoryYAML(YAML::Emitter& emitter,
std::vector<ROSControlConfig>* ros_controllers_config_copy);

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

why is this called _copy?

This comment has been minimized.

@MohmadAyman

MohmadAyman Jul 31, 2018

Author Contributor

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

@@ -277,7 +269,8 @@ class MoveItConfigData
bool outputKinematicsYAML(const std::string& file_path);
bool outputJointLimitsYAML(const std::string& file_path);
bool outputFakeControllersYAML(const std::string& file_path);
void outputFollowJointTrajectoryYAML(YAML::Emitter& emitter);
void outputFollowJointTrajectoryYAML(YAML::Emitter& emitter,

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

add function documentation for these args

ROSControlConfig* findROSControllerByName(const std::string& controller_name);

/**
* delete ros controller by name

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

Delete

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

"ROS"

@@ -1132,7 +1136,7 @@ bool MoveItConfigData::inputROSControllersYAML(const std::string& file_path)
std::ifstream input_stream(file_path.c_str());
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);

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

shouldn't this be an error or warning?

This comment has been minimized.

@MohmadAyman

MohmadAyman Jul 31, 2018

Author Contributor

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

@@ -1142,78 +1146,81 @@ bool MoveItConfigData::inputROSControllersYAML(const std::string& file_path)
YAML::Node doc = YAML::Load(input_stream);
// 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)

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

can you make map_it more descriptive / longer?

}
}
}
}
else
}
else

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

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

@@ -1433,7 +1440,7 @@ srdf::Model::Group* MoveItConfigData::findGroupByName(const std::string& name)
}

// ******************************************************************************************
// Find controller by name
// Find ros controller by name

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

ROS

} // namespace\

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

typo

return;
has_loaded = true;

const std::array<std::string, 10> types = { "effort_controllers/JointTrajectoryController",

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

rename to default_types

* 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

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

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

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:ros_controllers_screen branch from 0bdb47e to ec5004b Jul 31, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Contributor Author

MohmadAyman commented Jul 31, 2018

@davetcoleman
Addressed the comments on this one.

@@ -670,6 +671,9 @@ void MoveItConfigData::outputFollowJointTrajectoryYAML(YAML::Emitter& emitter)
// ******************************************************************************************
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_);

This comment has been minimized.

@davetcoleman

davetcoleman Jul 31, 2018

Member

instead of copy can we call this ros_controllers_config_output?

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

This comment has been minimized.

@davetcoleman

davetcoleman Jul 31, 2018

Member

instead of a pointer, pass this in by reference

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

This comment has been minimized.

@davetcoleman

davetcoleman Jul 31, 2018

Member

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

@@ -1453,6 +1478,24 @@ ROSControlConfig* MoveItConfigData::findROSControllerByName(const std::string& c
return searched_ros_controller;
}

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

This comment has been minimized.

@davetcoleman

davetcoleman Jul 31, 2018

Member

"ROS" and elsewhere

@@ -1473,9 +1516,9 @@ bool MoveItConfigData::addROSController(const ROSControlConfig& new_controller)
// ******************************************************************************************
// Gets ros_controllers_config_ vector
// ******************************************************************************************
std::vector<ROSControlConfig> MoveItConfigData::getROSControllers()
std::vector<ROSControlConfig>* MoveItConfigData::getROSControllers()

This comment has been minimized.

@davetcoleman

davetcoleman Jul 31, 2018

Member

return by reference, not pointer

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Jul 31, 2018

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

@mcevoyandy please review

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:ros_controllers_screen branch from ec5004b to 2fc77a2 Aug 2, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Contributor Author

MohmadAyman commented Aug 2, 2018

@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.

* 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

This comment has been minimized.

@davetcoleman

davetcoleman Aug 5, 2018

Member

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

* 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

This comment has been minimized.

@davetcoleman

davetcoleman Aug 5, 2018

Member

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

@@ -352,6 +362,20 @@ class MoveItConfigData
*/
bool inputKinematicsYAML(const std::string& file_path);

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

This comment has been minimized.

@davetcoleman

davetcoleman Aug 5, 2018

Member

"controllers" --> "individual controller"

@@ -361,8 +385,15 @@ class MoveItConfigData

/**
* \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

This comment has been minimized.

@davetcoleman

davetcoleman Aug 5, 2018

Member

"bool" --> "true"


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

This comment has been minimized.

@davetcoleman

davetcoleman Aug 5, 2018

Member

"bool" --> "true"

@@ -1164,6 +1222,107 @@ bool MoveItConfigData::inputKinematicsYAML(const std::string& file_path)
return true; // file created successfully
}

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

This comment has been minimized.

@davetcoleman

davetcoleman Aug 5, 2018

Member

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

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

This comment has been minimized.

@davetcoleman

davetcoleman Aug 5, 2018

Member

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

/* Author: Mohamad Ayman */

#ifndef MOVEIT_ROS_MOVEIT_SETUP_ASSISTANT_WIDGETS_CONTROLLER_EDIT_WIDGET_
#define MOVEIT_ROS_MOVEIT_SETUP_ASSISTANT_WIDGETS_CONTROLLER_EDIT_WIDGET_

This comment has been minimized.

@davetcoleman

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

This comment has been minimized.

@davetcoleman

davetcoleman Aug 5, 2018

Member

this components should be private, not public

virtual void focusGiven();

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

This comment has been minimized.

@davetcoleman

davetcoleman Aug 5, 2018

Member

make private

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Aug 5, 2018

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

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:ros_controllers_screen branch 2 times, most recently from 1330ee8 to a00ce3d Aug 6, 2018

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Aug 9, 2018

Is this ready for re-review?

@MohmadAyman

This comment has been minimized.

Copy link
Contributor Author

MohmadAyman commented Aug 9, 2018

Yes it is ready @davetcoleman

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Aug 9, 2018

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!

image

@MohmadAyman

This comment has been minimized.

Copy link
Contributor Author

MohmadAyman commented Aug 9, 2018

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

@davetcoleman
Copy link
Member

davetcoleman left a comment

Tested locally and it works :)

// 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"])

This comment has been minimized.

@mcevoyandy

mcevoyandy Aug 13, 2018

combine if statements?

This comment has been minimized.

@MohmadAyman

MohmadAyman Aug 13, 2018

Author Contributor

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.

@@ -667,6 +668,9 @@ void MoveItConfigData::outputFollowJointTrajectoryYAML(YAML::Emitter& emitter)
// ******************************************************************************************
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_);

This comment has been minimized.

@mcevoyandy

mcevoyandy Aug 13, 2018

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?

This comment has been minimized.

@MohmadAyman

MohmadAyman Aug 13, 2018

Author Contributor

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.

This comment has been minimized.

@MohmadAyman

MohmadAyman Aug 13, 2018

Author Contributor

addressed all comments.

This comment has been minimized.

@davetcoleman

davetcoleman Aug 13, 2018

Member

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

This comment has been minimized.

@MohmadAyman

MohmadAyman Aug 13, 2018

Author Contributor

ros_controllers_config_output is used in outputROSControllersYAML() here .

// This function writes FollowJointTrajectory controllers for each planning group to ros_controller.yaml.
outputFollowJointTrajectoryYAML(emitter);
// Writes Follow Joint Trajectory ROS controllers to ros_controller.yaml
outputFollowJointTrajectoryYAML(emitter, ros_controllers_config_output);

{

This comment has been minimized.

@mcevoyandy

mcevoyandy Aug 13, 2018

Remove bracket

This comment has been minimized.

@MohmadAyman

MohmadAyman Aug 13, 2018

Author Contributor

thats right, thats scoping didn't really make sense

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:ros_controllers_screen branch from a00ce3d to c5c667c Aug 13, 2018

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:ros_controllers_screen branch from c5c667c to ba56e13 Aug 13, 2018

@davetcoleman davetcoleman merged commit bd7e1c1 into ros-planning:kinetic-devel Aug 14, 2018

1 check passed

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

This comment has been minimized.

Copy link
Member

davetcoleman commented Aug 14, 2018

cherry-picked to melodic

great job!

davetcoleman added a commit that referenced this pull request Aug 14, 2018

ROS Controllers screen added to SA (#994)
* 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
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.