Adapt Subscriber class to be used with LifecycleNode instances#37
Adapt Subscriber class to be used with LifecycleNode instances#37jcmonteiro wants to merge 1 commit intoros2:masterfrom
Conversation
|
Hi @jcmonteiro I see that this PR is waiting to be accepted since August. I wonder if it can be merged. It could benefit some improvements in nav2 that I am working on, among others. Thanks |
|
Hello, I had to return to ros melodic after changing jobs, so it's been a while since I've used ros2. Nonetheless, I've used this fork a lot in my previous job without problems, in particular with my other PR #38. |
|
|
Is this going to be merged in anytime soon? @mjcarroll @tfoote |
tfoote
left a comment
There was a problem hiding this comment.
Looks good to me. @Karsten1987 do you have any concerns?
|
@Karsten1987 ping from waffle meeting |
Karsten1987
left a comment
There was a problem hiding this comment.
Sorry for being late here, that initial tagging somehow got lost in between all GH notifications.
The code looks good to me, I am requesting changes though because I think the rclcpp_lifecycle dependency is purely related to tests. See inline comments for details.
|
|
||
| <build_depend>builtin_interfaces</build_depend> | ||
| <build_depend>rclcpp</build_depend> | ||
| <build_depend>rclcpp_lifecycle</build_depend> |
There was a problem hiding this comment.
I think rclcpp_lifecycle should be listed as a test_depend only.
| find_package(ament_cmake_python REQUIRED) | ||
| find_package(ament_cmake_ros REQUIRED) | ||
| find_package(rclcpp REQUIRED) | ||
| find_package(rclcpp_lifecycle REQUIRED) |
There was a problem hiding this comment.
I think this call to find_package(rclcpp_lifecycle REQUIRED) should be under the BUILD_TESTING section and only be used when building tests.
|
Any update on this patch? I would be glad to drive it forward if @jcmonteiro no longer wants to push this forward |
|
Hi @allenh1. According to the previous comments, I believe the changes should be minor. If you can take them, I'd be glad to see this PR merged 😊 |
Absolutely. There were a few extra changes that needed to be made (that have been made to message filters since you submitted this patch). I haven't addressed @Karsten1987's review points yet, but I think he's absolutely right about the dependency so I'll push that on my branch. Would people prefer I submit a different PR or hijack this one? |
|
I believe it would be better to push into this PR since it has already gone through a review process. |
All message filters used to work only with rclcpp::Node instances. It is not possible to use them with rclcpp_lifecycle::LifecycleNode instances.