Expose subscription options #53
Conversation
Signed-off-by: Audrow Nash <[email protected]>
| rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> options = | ||
| rclcpp::SubscriptionOptionsWithAllocator<AllocatorT>()) = 0; |
There was a problem hiding this comment.
It would be nice if we could introduce these options without breaking API, but I'm not sure if that's possible in a pure virtual class.
To be honest, it's not clear to me why we have this abstract class to begin with, it doesn't appear to be used anywhere (besides being implemented by Subscriber below) 🤔 It might be safer to omit the changes to SubscriberBase and add new overloads to Subscriber to incorporate the subscriber options.
Pinging @mjcarroll for thoughts (I think you're the maintainer?).
There was a problem hiding this comment.
The way to avoid breaking API is to keep the old
virtual void subscribe(
rclcpp::Node::SharedPtr node,
const std::string& topic,
const rmw_qos_profile_t qos = rmw_qos_profile_default) = 0;and add a new
virtual void subscribe(
rclcpp::Node::SharedPtr node,
const std::string& topic,
const rmw_qos_profile_t qos,
rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> options)
{
(void)options;
// --> warn about method not overriden here <--
this->subscribe(node, topic, qos);
};There was a problem hiding this comment.
To be honest, it's not clear to me why we have this abstract class to begin with
It doesn't seem to be meant to be extended by external users, so it might be ok to just add new virtual methods to it.
There was a problem hiding this comment.
If it's not meant to be extended, then I think we could just add the new method to the concrete class.
We could go so far as to deprecate the pure virtual class and/or remove it in a separate PR, to avoid similar API/ABI issues in the future.
There was a problem hiding this comment.
I'm also okay with your proposal to implement and warn about the non-overridden method #53 (comment)
Signed-off-by: Audrow Nash <[email protected]>
| @@ -43,6 +43,7 @@ | |||
| namespace message_filters | |||
| { | |||
|
|
|||
| template <typename AllocatorT=std::allocator<void>> | |||
There was a problem hiding this comment.
I don't think that making this virtual class templated makes much sense, I would just use rclcpp::SubscriptionOptions below
| rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> options = | ||
| rclcpp::SubscriptionOptionsWithAllocator<AllocatorT>()) = 0; |
There was a problem hiding this comment.
The way to avoid breaking API is to keep the old
virtual void subscribe(
rclcpp::Node::SharedPtr node,
const std::string& topic,
const rmw_qos_profile_t qos = rmw_qos_profile_default) = 0;and add a new
virtual void subscribe(
rclcpp::Node::SharedPtr node,
const std::string& topic,
const rmw_qos_profile_t qos,
rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> options)
{
(void)options;
// --> warn about method not overriden here <--
this->subscribe(node, topic, qos);
};| rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> options = | ||
| rclcpp::SubscriptionOptionsWithAllocator<AllocatorT>()) = 0; |
There was a problem hiding this comment.
To be honest, it's not clear to me why we have this abstract class to begin with
It doesn't seem to be meant to be extended by external users, so it might be ok to just add new virtual methods to it.
|
Closed in favor of #56. |
This PR is needed for QoS overrides in subscriptions, as in ros-perception/image_pipeline#651.