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

Ros2 devel #6

Merged
merged 20 commits into from
Jan 13, 2020
Merged

Conversation

Karsten1987
Copy link

This is a fair mix of commits started by @bponsler and @MarcTestier in order to port laser_proc to ros2. I am opening this in order to have a single upstream port rather than multiple forks.

This is a pretty straight forward migration without any chances in design or code style.

A ros2 port of this is further needed to unblock the migration work of URG: ros-drivers/urg_node#50

bponsler and others added 17 commits June 9, 2017 17:58
Initial port of the laser_proc package to ros 2
Removing creation of private node handle which is not allowed in ROS 2.
Fixed compile error: cannot find class_loader_register_macro.h when ament build add --isolated option
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Author

@chadrockey Do you mind having a look at this? Ideally, we would have this landed upstream and released in dashing/eloquent to push forward a release of the urg laser driver.

Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left some comments for improvements.

CMakeLists.txt Outdated Show resolved Hide resolved
include/laser_proc/laser_transport.hpp Outdated Show resolved Hide resolved
src/laser_proc.cpp Outdated Show resolved Hide resolved
src/laser_proc.cpp Outdated Show resolved Hide resolved
src/laser_proc_node.cpp Show resolved Hide resolved
src/laser_proc_ros.cpp Outdated Show resolved Hide resolved
src/laser_publisher.cpp Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
src/laser_proc_nodelet.cpp Outdated Show resolved Hide resolved
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Author

with the latest commit I've addressed some of the review points, but more over changed quite some code which was unused and can now be handled more conveniently with the ROS2 component API.

Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A few more things left open, but this is looking much better!

include/laser_proc/laser_proc.hpp Outdated Show resolved Hide resolved
include/laser_proc/laser_publisher.hpp Outdated Show resolved Hide resolved
src/laser_proc.cpp Outdated Show resolved Hide resolved
src/laser_proc.cpp Outdated Show resolved Hide resolved
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Thanks for iterating. This looks good to me now.

@clalancette
Copy link

@chadrockey Any thoughts on this PR? It looks good to me, so unless you have any thoughts or objections, @Karsten1987 or I will merge at the end of the week.

@clalancette
Copy link

All right, I'm going to go ahead and merge. If there are any comments you want to add, please feel free.

@clalancette clalancette merged commit 2a8261e into ros-perception:ros2-devel Jan 13, 2020
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