Ros2 devel #21
Ros2 devel #21
Conversation
These files were not there in sensor_msgs package of ros2 so just copied it from ROS1 and ported to ROS2 here. laser assembler uses convertPointCloudToPointCloud2() function from point_cloud_conversion.h file.
Renamed base_assembler_srv.h to base_assembler_srv.hpp. Also ported both files to ROS2 as per ROS2 migration guide.
and base_assembler_srv.h file to base_assembler_srv.hpp in previous commit so removed it here.
because message_filter.h file from tf2_ros package of bouncy release was not ported to ROS2. so picked it from link https://github.com/ros2/geometry2/blob/ros2/tf2_ros/include/tf2_ros/message_filter.h and fixed some erros related to time conversion.
… part of the communication. Request structure which is the type of the request part of the service Response structure which is the type of the request part of the service
Only porting is done but not tested.
- src/laser_scan_assembler_srv.cpp - src/merge_clouds.cpp - src/point_cloud2_assembler.cpp - src/point_cloud_assembler.cpp - src/point_cloud_assembler_srv.cpp
… to ROS2 and also tested it. non_zero_cloud_test passed successfully.
examples/periodic_snapshotter.cpp
Outdated
| #include "rclcpp/clock.hpp" | ||
| #include "std_msgs/msg/string.hpp" | ||
|
|
||
| #define ROS_ERROR printf |
There was a problem hiding this comment.
Does ROS2 not have any logging functions yet?
There was a problem hiding this comment.
ROS2 has logging functions, fixed it in the next commits.
examples/periodic_snapshotter.cpp
Outdated
|
|
||
| // Create the service client for calling the assembler | ||
| client_ = n_.serviceClient<AssembleScans>("assemble_scans"); | ||
| std::cerr<< " Inside PeriodicSnapshotter constructor " << "\n"; |
There was a problem hiding this comment.
This seems like debugging output that should be removed.
examples/periodic_snapshotter.cpp
Outdated
|
|
||
| // Create a function for when messages are to be sent. | ||
| auto timer_callback = [this]() -> void { | ||
| // msg_->name = "Hello World: " + std::to_string(count_++); |
There was a problem hiding this comment.
Please remove all the commented out code and the "Hello world" stuff.
There was a problem hiding this comment.
Removed commented out code in next commits.
| tf2_ros::TransformListener *tf_; | ||
| tf2_ros::MessageFilter<T>* tf_filter_; | ||
| rclcpp::Node::SharedPtr private_ns_; | ||
| rclcpp::Node *n_; |
There was a problem hiding this comment.
Why do you use a raw pointer for n_?
There was a problem hiding this comment.
message_filters's subscribe method requires raw node pointer.
There was a problem hiding this comment.
Right, but you can still either create a normal variable and then pass in &n_ or create a shared pointer and then pass in n_->get(). But I prefer not to store raw pointers because it's easy to forget to free them, or use after free.
There was a problem hiding this comment.
Changed raw node pointer to shared pointer in commit id 47fc3b1
|
@tfoote this PR will change the license on the laser_assembler code to Apache 2 for the ROS2 branch. That sounds like the right license to use, and I'm happy with the change, but it is technically a change from the previous BSD license. I assume this has been thought about for other repositories - are there any issues with updating the license of a package that has been around for a while? |
|
@jonbinney is right that ported code should retain the original license. Only the copyright holder can change the license on the existing code. New code can be added under a new license, but for consistency we recommend that for packages being ported new code be contributed under the original license. If there's large well separated new functionality that can be licensed separately that's fine, but in general consistency is more valuable than moving to our new preferred Apache 2.0 license. |
|
@jonbinney I agree with you. I will retain the original license. |
- Removed commented and unused code.
- Fixed logger issue for bouncy. - Modified message_filter.hpp to work on bouncy.
|
@jonbinney, |
…pp' because it is changed in filters package. - changed string type variable to std::string type.
|
@vandanamandlik thanks - I'll review this |
|
@tfoote can you comment on the question about the linter that @vandanamandlik mentions above? Is it expected that ported packages with different licenses will be flagged by the linter? |
CMakeLists.txt
Outdated
| add_executable(laser_scan_assembler_srv src/laser_scan_assembler_srv.cpp) | ||
| target_link_libraries(laser_scan_assembler_srv ${catkin_LIBRARIES} ${Boost_LIBRARIES}) | ||
| add_dependencies(laser_scan_assembler_srv ${PROJECT_NAME}_gencpp) | ||
| set( filters_FOUND 1 ) |
There was a problem hiding this comment.
Is this variable used somewhere? How do you know filters was found?
There was a problem hiding this comment.
It was workaround added previously, because find_package(filters) was giving some errors,
Updating filters package cmake has solved this issue so removed it in commit id
125b134
Updated CMakeLists.txt file from filters package has solved issue. Setting this flag is not more required.
… in laser_assembler package itself. Previously there was separate package to generate laser_assembler.srv
|
Hey, I just wanted to check and see is there anything holding back this PR? |
Initial port for Laser assembler
I have ported all files from this package to ROS2 as per ROS2 migration guide.
But I could test only laser_scan_assembler.cpp, base_assembler.h, dummy_scan_producer.cpp and test_assembler.cpp files,
Because non_zero_cloud_test from test_assembler uses only above files.