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

Added demo_gazebo.launch #1051

Merged
merged 5 commits into from Nov 26, 2018

Conversation

Projects
None yet
3 participants
@MohmadAyman
Copy link
Contributor

MohmadAyman commented Sep 6, 2018

Description

This PR is about connecting the ROSControl and Simulation widgets, the following GIF sums up what this PR do.

or as video for better quality
.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Created tests, which fail without this PR reference
  • Decide if this should be cherry-picked to other current ROS branches
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:add_transmission_elements_to_urdf branch 2 times, most recently from 3c81999 to 40b0b9c Sep 6, 2018

@@ -1,3 +1,6 @@
<launch>

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 6, 2018

Author Contributor

Is modifying this file okay? like would it affect other already running stuff?

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

add a comment explaining that the contents of this file can be replaced by other controller managers as desired, but that this is a good default for many integrations including Gazebo

@@ -0,0 +1,69 @@
<launch>

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 6, 2018

Author Contributor

There is another option of editing the current demo.launch and add options to it to be able to launch gazebo too.

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

I think this second option will make demo.launch easier to maintain, rather than having a fork for gazebo

This comment has been minimized.

@gavanderhoorn

gavanderhoorn Sep 8, 2018

Member

May I suggest to not change demo.launch? MoveIt packages currently already have quite a few dependencies that make them rather involved to deploy on headless machine or non-desktop PCs (fi). If we start extending demo.launch to include Gazebo functionality, that would make Gazebo an exec_depend of every MoveIt package.

Gazebo definitely has its usages, but making it a hard dependency of a MoveIt package as it gets generated by the authoratitive tool for such packages seems like a bit of a heavyweight thing to do.

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 10, 2018

Author Contributor

Also answering your comment here .
We are not planning to make gazebo a hard dependency in anyway, it is an optional simulator which is disabled by default. if we ended up modifying the current demo.launch , gazebo will only run if the user want that by passing an argument to the launch file.

This comment has been minimized.

@davetcoleman

davetcoleman Sep 11, 2018

Member

I suggest in the package.xml we add the following:

<!-- Optional: Gazebo integration. This is a heavy dependency so is disabled by default -->
<!-- run_depend>gazebo_ros</run_depend -->

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 11, 2018

Author Contributor

I agree, added.

fs::path ros_controllers_yaml_path = config_data_->config_pkg_path_;
ros_controllers_yaml_path /= "config/ros_controllers.yaml";
config_data_->inputROSControllersYAML(ros_controllers_yaml_path.make_preferred().native().c_str());
// fs::path ros_controllers_yaml_path = config_data_->config_pkg_path_;

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 6, 2018

Author Contributor

as parsing ros_controllers.yaml is not crucial, I think it is better to disable it for now until we get this merged.

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

instead of making a Github comment about this, put this comment in the code so that it isn't lost!

@@ -131,9 +131,9 @@ SetupAssistantWidget::SetupAssistantWidget(QWidget* parent, boost::program_optio
nav_name_list_ << "Robot Poses";
nav_name_list_ << "End Effectors";
nav_name_list_ << "Passive Joints";

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 6, 2018

Author Contributor

Reorder the nav bar as it makes more sense this was, as the simulation screen uses the controllers.

This comment has been minimized.

@davetcoleman
@@ -760,10 +832,6 @@ bool MoveItConfigData::outputROSControllersYAML(const std::string& file_path)
YAML::Emitter emitter;
emitter << YAML::BeginMap;

// Start with the robot name ---------------------------------------------------

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 6, 2018

Author Contributor

According to Gazebo docs it is better to have each Robot controllers under the namespace : robot_name, but moveit_controller_manager looks for the controllers under /controller_list.
So in favor of the moveit_controller_manager the controllers and /controller_list will be with absolute namespace e.g. robot name will no longer be the name space of the controller_list.

* \brief Helper function to get the controller that is controlling the joint
* \return controller type
*/
std::string getJointHardwareInterface(std::string joint_name);

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

strings should always be passed in with const &

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 10, 2018

Author Contributor

mb 😅

// ******************************************************************************************
std::string MoveItConfigData::getJointHardwareInterface(std::string joint_name)
{
for (size_t i = 0; i < ros_controllers_config_.size(); i++)

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

++i is slightly faster so always prefer that :)

{
if (ros_controllers_config_[i].type_.substr(0, 6) == "positi")
return "hardware_interface/PositionJointInterface";
else if (ros_controllers_config_[i].type_.substr(0, 6) == "veloci")

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

why only check 6 characters and not 8?

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 10, 2018

Author Contributor

yeah you are right, would be cleaner to write the whole word.

return "hardware_interface/EffortJointInterface";
}
}
return "hardware_interface/EffortJointInterface";

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

add comment about why effort is default

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 10, 2018

Author Contributor

It is not in the else because it is default, it has to be an effort controller if not velocity or position. Controller type is chosen from a drop down and it has to be one of the three.

This comment has been minimized.

@davetcoleman

davetcoleman Sep 12, 2018

Member

sorry, i meant add a comment to the code. just copy what you just said here

This comment has been minimized.

@davetcoleman

davetcoleman Sep 18, 2018

Member

ping please add comment in code here

continue;
// Before adding inertial elements, make sure there is none and the link has collision element
if (link_element->FirstChildElement("inertial") == NULL && link_element->FirstChildElement("collision") != NULL)
if (((std::string)doc_element->Value()).find("link") != std::string::npos)

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

use static_cast instead of C-style casting

fs::path ros_controllers_yaml_path = config_data_->config_pkg_path_;
ros_controllers_yaml_path /= "config/ros_controllers.yaml";
config_data_->inputROSControllersYAML(ros_controllers_yaml_path.make_preferred().native().c_str());
// fs::path ros_controllers_yaml_path = config_data_->config_pkg_path_;

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

instead of making a Github comment about this, put this comment in the code so that it isn't lost!

@@ -0,0 +1,69 @@
<launch>

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

I think this second option will make demo.launch easier to maintain, rather than having a fork for gazebo

@@ -1,3 +1,6 @@
<launch>

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

add a comment explaining that the contents of this file can be replaced by other controller managers as desired, but that this is a good default for many integrations including Gazebo

<node name="robot_state_publisher" pkg="robot_state_publisher" type="robot_state_publisher"
respawn="false" output="screen">
<remap from="/joint_states" to="[ROBOT_NAME]/joint_states" />
</node>

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

why remove this?

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 10, 2018

Author Contributor

There is already a robot_state_publisher node in demo.launch.
removing the namespace is for the reason explained here

{
boost::filesystem::path res_path(MOVEIT_TEST_RESOURCES_DIR);
// TODO: after removing the robot namespace from ros_controllers.yaml, inputROSControllersYAML needs to be refactored
// accordingly

This comment has been minimized.

@davetcoleman

davetcoleman Sep 7, 2018

Member

disabling tests is bad practice. can you just refactor inputROSControllersYAML now?

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 7, 2018

Author Contributor

I tried tho but it wasn't so trivial, so as to get this PR merged as fast as possible I thought of postponed it to a later PR.
I can try to allocate time to do it across this weekend 🤔

This comment has been minimized.

@MohmadAyman

MohmadAyman Sep 10, 2018

Author Contributor

Done

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Sep 7, 2018

Nice video!

</include>

<!-- send robot urdf to param server -->
<param name="robot_description" [URDF_LOAD_ATTRIBUTE] />
<param name="robot_description" textfile="$(arg urdf_path)" />

<!-- push robot_description to factory and spawn robot in gazebo at the origin, change x,y,z arguments to spawn in a different position -->
<node name="spawn_gazebo_model" pkg="gazebo_ros" type="spawn_model" args="-urdf -param robot_description -model robot -x 0 -y 0 -z 0"

This comment has been minimized.

@gavanderhoorn

gavanderhoorn Sep 8, 2018

Member

This file seems to introduce a run dependency on gazebo_ros, but the PR does not seem to update the package.xml template to include that dependency.

If that was on purpose (fi to keep the nr of dependencies down and make Gazebo support optional), then I believe it would be good to document that somewhere to make it explicit.

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:add_transmission_elements_to_urdf branch 3 times, most recently from c39423f to 31d8c48 Sep 10, 2018

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Sep 11, 2018

I'm ready to approve after #1051 (comment) is addressed

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:add_transmission_elements_to_urdf branch 3 times, most recently from f1d5d0f to 9cbd22b Sep 11, 2018

@@ -371,10 +377,10 @@ class MoveItConfigData

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

This comment has been minimized.

@davetcoleman

davetcoleman Sep 12, 2018

Member

"ros_controller"

@@ -24,6 +24,8 @@
<build_depend>yaml-cpp</build_depend>
<build_depend version_gt="0.3.1">srdfdom</build_depend>

<!-- Optional: Gazebo integration. This is a heavy dependency so is disabled by default -->
<!-- run_depend>gazebo_ros</run_depend -->

This comment has been minimized.

@davetcoleman

davetcoleman Sep 12, 2018

Member

move the optional run_depend to bottom of list, not top

// ******************************************************************************************
std::string MoveItConfigData::getJointHardwareInterface(const std::string& joint_name)
{
for (size_t i = 0; i < ros_controllers_config_.size(); ++i)

This comment has been minimized.

@davetcoleman

davetcoleman Sep 12, 2018

Member

std::size_t

return "hardware_interface/EffortJointInterface";
}
}
return "hardware_interface/EffortJointInterface";

This comment has been minimized.

@davetcoleman

davetcoleman Sep 12, 2018

Member

sorry, i meant add a comment to the code. just copy what you just said here


// Loop through all controllers
// // Loop through all controllers

This comment has been minimized.

@davetcoleman

davetcoleman Sep 12, 2018

Member

too many //

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Sep 12, 2018

clang test failed

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:add_transmission_elements_to_urdf branch from c87ea94 to a68e9af Sep 13, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Contributor Author

MohmadAyman commented Sep 18, 2018

return "hardware_interface/EffortJointInterface";
}
}
return "hardware_interface/EffortJointInterface";

This comment has been minimized.

@davetcoleman

davetcoleman Sep 18, 2018

Member

ping please add comment in code here

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Sep 18, 2018

@gavanderhoorn can you give this the second needed approval?

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:add_transmission_elements_to_urdf branch from a1241c4 to 29d99d1 Sep 19, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Contributor Author

MohmadAyman commented Sep 25, 2018

Can I ask what does it mean that only ROS_DISTRO=lunar and REPO=ros fails while all the others passes in Travis?

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Sep 26, 2018

It means on the older ROS Lunar distro there is some bug. Lunar runs on Ubuntu 17.04, see http://wiki.ros.org/lunar

Looking at the travis logs, it looks like a fluke issue:

CMakeFiles/test_constraint_samplers.dir/test/pr2_arm_ik.cpp.o: file not recognized: File truncated
collect2: error: ld returned 1 exit status

I've retriggered the build

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

Open

Feat simple moveit controller manager #1140

0 of 7 tasks complete
@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Nov 21, 2018

I've just restart the build since it was broken on lunar, which is no longer being tested for.

If it passes Ill merge since @gavanderhoorn reviewed previously but went MIA

@MohmadAyman

This comment has been minimized.

Copy link
Contributor Author

MohmadAyman commented Nov 22, 2018

It still fails on lunar 🤔

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Nov 22, 2018

Then I think you need to rebase this PR against kinetic-devel (or move it to melodic).

The Travis has sense been updated: https://github.com/ros-planning/moveit/blob/kinetic-devel/.travis.yml

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:add_transmission_elements_to_urdf branch from 29d99d1 to d12d5d8 Nov 23, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Contributor Author

MohmadAyman commented Nov 23, 2018

@davetcoleman that worked!

@davetcoleman davetcoleman merged commit f90a350 into ros-planning:kinetic-devel Nov 26, 2018

1 check passed

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

davetcoleman added a commit to davetcoleman/moveit that referenced this pull request Nov 26, 2018

Added demo_gazebo.launch (ros-planning#1051)
* add transmission elements to the urdf to simulate controllers using gazebo_ros_control

* add demo_gazebo to launch gazebo at the same time as rviz

* add getjointhradwareinterface function

*  add gazebo_ros as an optional dep in SA

* move sensors_3d.yaml to templates dir in SA, add comments, replace  size_t by std::size_t
@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Nov 26, 2018

Forward port to melodic: #1231

davetcoleman added a commit that referenced this pull request Dec 3, 2018

Added demo_gazebo.launch (#1051) (#1231)
* add transmission elements to the urdf to simulate controllers using gazebo_ros_control

* add demo_gazebo to launch gazebo at the same time as rviz

* add getjointhradwareinterface function

*  add gazebo_ros as an optional dep in SA

* move sensors_3d.yaml to templates dir in SA, add comments, replace  size_t by std::size_t

ggupta9777 added a commit to ggupta9777/moveit that referenced this pull request Mar 17, 2019

Added demo_gazebo.launch (ros-planning#1051)
* add transmission elements to the urdf to simulate controllers using gazebo_ros_control

* add demo_gazebo to launch gazebo at the same time as rviz

* add getjointhradwareinterface function

*  add gazebo_ros as an optional dep in SA

* move sensors_3d.yaml to templates dir in SA, add comments, replace  size_t by std::size_t

ipa-hsd added a commit to ipa-hsd/moveit that referenced this pull request Mar 18, 2019

Added demo_gazebo.launch (ros-planning#1051)
* add transmission elements to the urdf to simulate controllers using gazebo_ros_control

* add demo_gazebo to launch gazebo at the same time as rviz

* add getjointhradwareinterface function

*  add gazebo_ros as an optional dep in SA

* move sensors_3d.yaml to templates dir in SA, add comments, replace  size_t by std::size_t

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

Open

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