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.

Sign up for GitHub

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

Jump to bottom

Port sensor_manager of moveit_core to ROS 2.0 #11

Merged
merged 1 commit into from
Feb 20, 2019
Merged

Port sensor_manager of moveit_core to ROS 2.0 #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion moveit_core/sensor_manager/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
install(DIRECTORY include/ DESTINATION ${CATKIN_GLOBAL_INCLUDE_DESTINATION})
install(DIRECTORY include/ DESTINATION include)
Copy link
Member

Choose a reason for hiding this comment

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

@mlautman is this best practice? should we ask dirk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taken from https://index.ros.org/doc/ros2/Contributing/Migration-Guide/.

Copy link
Contributor

Choose a reason for hiding this comment

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

As stated somewhere else, I believe GNUInstallDirs is the better approach here - from a maintainer's point of few at least.
@dirk-thomas I believe you were involved with the migration instructions, right?
Did you know about that cmake module or did you deliberately ignore it?
The only possible shortcoming I can see is that you might end up with lib<arch> folders instead of lib depending on the context

Copy link
Contributor

Choose a reason for hiding this comment

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

Using GNUInstallDirs or not is pretty much orthogonal to the migration instructions. You can either use it or not - in ROS 1 as well as in ROS 2 packages.

Copy link
Member

Choose a reason for hiding this comment

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

It sounds like switching to GNUInstallDirs is outside the scope of a first-pass moveit port to ROS2, and @vmayoral 's approach is good

8 changes: 4 additions & 4 deletions moveit_core/sensor_manager/include/moveit/sensor_manager/sensor_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@
#include <vector>
#include <string>
#include <moveit/macros/class_forward.h>
#include <moveit_msgs/RobotTrajectory.h>
#include <geometry_msgs/PointStamped.h>
#include <moveit_msgs/msg/robot_trajectory.hpp>
#include <geometry_msgs/msg/point_stamped.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, being rather ignorant of the current state of ROS2, I really wonder what should be the beautiful upside of changing includes like this, apart from making huge code-bases incompatible...

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, the rational is that this way, it's possible to have services, actions and msgs that have the same name (but I'm not 100% certain).

Copy link
Member

Choose a reason for hiding this comment

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

^ that is correct. For example in ROS1 we had the standard of adding "Action" at the end of every action message type to avoid conflicts in messages or services, but it was decided (I have no opinion) to enforce this more rigorously through folders and namespaces


/// Namespace for the base class of a MoveIt! sensor manager
namespace moveit_sensor_manager
Expand Down Expand Up @@ -99,8 +99,8 @@ class MoveItSensorManager
/// of this function can perform checks on the safety of the trajectory.
/// The function returns true on success (either completing execution succesfully or computing a trajecotory
/// successufully)
virtual bool pointSensorTo(const std::string& name, const geometry_msgs::PointStamped& target,
moveit_msgs::RobotTrajectory& sensor_trajectory) = 0;
virtual bool pointSensorTo(const std::string& name, const geometry_msgs::msg::PointStamped& target,
moveit_msgs::msg::RobotTrajectory& sensor_trajectory) = 0;
};
}

Expand Down