search

ros2 / message_filters

BSD 3-Clause "New" or "Revised" License
76 stars 66 forks source link

Adapt Subscriber class to be used with LifecycleNode instances #37

Closed jcmonteiro closed 3 years ago

jcmonteiro commented 5 years ago

All message filters used to work only with rclcpp::Node instances. It is not possible to use them with rclcpp_lifecycle::LifecycleNode instances.

fmrico commented 4 years ago

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.

@dirk-thomas @tfoote

Thanks Francisco

jcmonteiro commented 4 years ago

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.

mjcarroll commented 4 years ago
Michael-Equi commented 4 years ago

Is this going to be merged in anytime soon? @mjcarroll @tfoote

ivanpauno commented 4 years ago

@Karsten1987 ping from waffle meeting

allenh1 commented 3 years ago

Any update on this patch? I would be glad to drive it forward if @jcmonteiro no longer wants to push this forward

jcmonteiro commented 3 years ago

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 😊

allenh1 commented 3 years ago

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?

jcmonteiro commented 3 years ago

I believe it would be better to push into this PR since it has already gone through a review process.