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

Setup assistant automating generation of controllers configs #908

Conversation

Projects
None yet
4 participants
@MohmadAyman
Copy link
Member

commented May 21, 2018

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)

@MohmadAyman MohmadAyman reopened this May 21, 2018

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 21, 2018

Member

ros_controller.yaml

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 21, 2018

Member

lets keep the fake controllers also

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 21, 2018

Member

ros_controllers.launch

@MohmadAyman MohmadAyman referenced this pull request May 22, 2018

Closed

Setup Assistant 2 #894

6 of 6 tasks complete
@davetcoleman
Copy link
Member

left a comment

Great work!

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 24, 2018

Member

please use underscore for var names per ros style guidelines

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 24, 2018

Member

use more specific name than just groups

std::vector<std::vector<std::string>> groups;
std::vector<std::string> groupJoints;

// Loop through groups

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 24, 2018

Member

expand comment: why loop through?

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 24, 2018

Member

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

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 24, 2018

Member

explain why you are iterating through the joints

emitter << YAML::Value << YAML::BeginSeq;

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 24, 2018

Member

std::size_t

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

emitter << YAML::BeginMap;

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 24, 2018

Member

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;
output_stream << emitter.c_str();
output_stream.close();

return true; // file created successfully

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 24, 2018

Member

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?

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman May 25, 2018

Author Member

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?

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 29, 2018

Member

Sure, no problem.

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 24, 2018

Member

use ROS_ERROR_STREAM_NAMED

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman May 25, 2018

Author Member

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 29, 2018

Member

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.

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman May 29, 2018

Author Member

Okay I understand, Thanks.

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

<!-- load the controllers -->

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 24, 2018

Member

be consistent with capitalization in comments. i personally prefer capitalizing

@davetcoleman

This comment has been minimized.

Copy link
Member

commented May 24, 2018

@mcevoyandy please review

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented May 25, 2018

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

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented May 25, 2018

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

@rhaschke
Copy link
Collaborator

left a comment

Generally approve. Some minor comments remain.


<!-- 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"/>

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 26, 2018

Collaborator

@willcbaker Is using absolute namespaces compatible to 4598f66?

This comment has been minimized.

Copy link
@willcbaker

willcbaker May 27, 2018

Contributor

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?

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman May 29, 2018

Author Member

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

This comment has been minimized.

Copy link
@rhaschke

rhaschke Jul 4, 2018

Collaborator

Yes, I think so.

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 4, 2018

Author Member

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?

<!-- 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" />

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 26, 2018

Collaborator

Same here.

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

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 26, 2018

Collaborator

outputROSControllersYAML?

MohmadAyman added some commits May 29, 2018

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 29, 2018

Member

how about namespace = "setup_assistant"

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 29, 2018

Collaborator

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 29, 2018

Member

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:
ros-planning/moveit.ros.org#166

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman May 30, 2018

Author Member

So do we go back to ROS_ERROR_STREAM?

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 30, 2018

Collaborator

That's my suggestion. @davetcoleman?

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 30, 2018

Member

I prefer the consistency of always using NAMED as I've just attempted to document in ros-planning/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.

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 30, 2018

Collaborator

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

@@ -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"/>

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 29, 2018

Collaborator

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

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman May 29, 2018

Author Member

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.

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

MSA: new function to generate ROS controllers config (#908)
- ros_controllers.launch
- ros_controllers.yaml
@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2018

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

@rhaschke rhaschke closed this May 30, 2018

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

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

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.