search

ros2 / message_filters

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

allow custom qos for message filters #41

Closed Jconn closed 5 years ago

Jconn commented 5 years ago

this fixes issue https://github.com/ros2/message_filters/issues/39 and allows me to explicitly set QoS profiles for each of my message filter subscribers

claireyywang commented 5 years ago

Thank you for submitting a PR! I added a check to prevent breaking API for current users who are not supplying their own qos_profile params. @ros2/team Should we deprecate the hardcoded queue size and just ask users to always supply their own params in the future?

wjwwood commented 5 years ago

I don't feel strongly about adding a deprecation warning. The other option is to just hard break the API post Eloquent.

I think that's the big question we need feedback on. It would be nice to have a deprecation cycle on this, and to get that deprecation in for Eloquent.

@mjcarroll I think you poked around on message filters too, do you have an opinion?

jacobperron commented 5 years ago

I agree that it would be nice to have a deprecation cycle, which should be straightforward with the Python API. It would be nice to also make the same change to the C++ implementation (there's a TODO!):

https://github.com/ros2/message_filters/blob/01e433782c3e395230016138d3d1fb91553d57c9/include/message_filters/subscriber.h#L161-L162

Maybe it's as easy as overloading that method and deprecating the old one (I haven't looked too closely).

Edit: To clarify, I'm in favor of removing the hardcoded depth setting.

claireyywang commented 5 years ago

@wjwwood @mjcarroll friendly ping. If we are okay with the current change, could I get a green check to merge? Maybe we can discuss deprecation cycle in meeting.