SubscriptionOptions can not be set for message_filters::Subscriber

[ruffsl]

Feature request

When using message_filters in ros2, it'd be nice to be able to set SubscriptionOptions when creating Subscribers as one would when creating subscriptions via the underlying rclcpp::Node API. However passing such SubscriptionOptions is not yet possible using the current Subscriber constructors from the message_filters API. I'd like to suggest updating message_filters to follow that of rclcpp::Node.

Required Info:

• Operating System:
  • Ubuntu 18.04
• Installation type:
  • binaries:
• Version or commit hash:
  • ros-eloquent-message-filters/bionic,now 3.2.3-1bionic.20200121.232517 amd64

Feature description

Extend message_filters::Subscriber so that the underlying rclcpp::SubscriptionOptions can be set.

Implementation considerations

Downstream packages such as image_transport should also be updated to support this.

[claireyywang]

@ruffsl would you be interested in contributing a pull request?

[ruffsl]

I could try, but I'd prefer some patterns to follow on what the constructor signature should look like, e.g.

Subscriber(rclcpp::Node::SharedPtr node, const std::string& topic,
    const rmw_qos_profile_t qos = rmw_qos_profile_default,
    const rmw_subscription_options_t
 subscription_options = rmw_get_default_subscription_options() )

I'm a little lost on how the rest of rclcpp is or isn't dealing with optional arguments for qos and options.

https://github.com/ros2/rclcpp/blob/8525ee2eb5e07b09bc7856bf7d1dcc75d62504eb/rclcpp/include/rclcpp/subscription_factory.hpp#L56

https://github.com/ros2/rclcpp/blob/8525ee2eb5e07b09bc7856bf7d1dcc75d62504eb/rclcpp/include/rclcpp/create_subscription.hpp#L54-L57

[claireyywang]

@ruffsl this is the main way to create subscription https://github.com/ros2/rclcpp/blob/87fa896e3826ca75189e2ccf4aa07a68c51b5fdb/rclcpp/include/rclcpp/node.hpp#L186

[ruffsl]

Yes, but how exactly do we want to have the constructor for message_filters::Subscriber to look like? Do we want to remove the default options for current optional arguments, to make it more like create_subscription in rclcpp, such that qos is a required arg, and include a matching arg for MessageMemoryStrategyT while we're refactoring?

https://github.com/ros2/message_filters/blob/0b98d58e175ad93116aedb5782ce7c88f8846f3a/include/message_filters/subscriber.h#L107-L115

[claireyywang]

I remember in a previous PR https://github.com/ros2/message_filters/pull/41 we discussed deprecating default qos and make it a required arg. @wjwwood @jacobperron Is that something we want to do for Foxy?

[wjwwood]

Yes, but how exactly do we want to have the constructor for message_filters::Subscriber to look like? Do we want to remove the default options for current optional arguments, to make it more like create_subscription in rclcpp, such that qos is a required arg, and include a matching arg for MessageMemoryStrategyT while we're refactoring?

In my opinion it should be exactly like the create subscription in rclcpp, forwarding all the arguments that make sense to forward, and then only adding arguments specific to message filters.

To answer your specific questions, yes it should have QoS as a required argument, and yes it should take the message memory strategy, have a default value, and forward it along to the core create_subscription.

[jacobperron]

SubscriptionOptions can now be provided as of https://github.com/ros2/message_filters/pull/56. Please feel free to reopen if you think this does not resolve this issue.