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

Port rdf_loader to ROS 2 #76

Closed
wants to merge 3 commits into from

Conversation

vmayoral
Copy link
Contributor

@vmayoral vmayoral commented Apr 5, 2019

No description provided.

@vmayoral vmayoral mentioned this pull request Apr 5, 2019
@vmayoral
Copy link
Contributor Author

vmayoral commented Apr 8, 2019

Same as #62 (comment). Please consider these changes.

ros::NodeHandle nh("~");
auto start = std::chrono::system_clock::now();
// TODO (anasarrak): Add the correct node_handle name
auto node = rclcpp::Node::make_shared("/");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big TODO. What is our strategy for Nodes in MoveIt2? Should we always check that rclcpp has been initialized first? Seems like something we should do with intention.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: change the api for rdf_loader so that the user must pass in a node.


std::string cmd = "rosrun xacro xacro ";
//TODO (anasarrak): Test ros2 xacro https://github.com/ros/xacro/tree/ros2 // https://github.com/bponsler/xacro
std::string cmd = "ros2 run xacro xacro";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a demonstration of the RDF Loader loading it loading a robot description? Seems like a test that we should really consider adding

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've actually used xacro to transform from a .urdf.xacro to a .urdf for our models. The problem is that there is actually a ros2 branch for xacro, but is not oficially ported to ros2, the cmakelist is still with catkin... There is an open PR but I've been having problems with it.

The only working one is this one, and I managed to transform the urdf.xacro with the following command:

xacro --inorder mara_robot_camera_top.urdf.xacro -o mara_robot_camera_top.urdf

@@ -49,48 +49,56 @@
#include <fstream>
#include <streambuf>
#include <algorithm>
#include <chrono>

rclcpp::Logger LOGGER = rclcpp::get_logger("moveit").get_child("rdf_loader");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be inside a namespace like the other loggers.
If you wanted to use namespace rdf_loader then you could also find replace rdf_loader::RDFLoader:: with RDFLoader:: for every method in the namespace block

@henningkayser
Copy link
Member

Closed in favor of #104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants