not to hold callback group in subscription #1833

iuhilnehc-ynos · 2021-12-03T02:40:50Z

keep consistency with other cases to not hold callback group in subscription

The above link can jump but not show the correct location. Copy comments

Because I want to make the subscription keep consistent behavior with other entities.
I found other entities not to hold the callback group, refer to #1754 (comment), so I think it's better not to hold a callback group in the subscription.

That sounds fine, but it doesn't seem to be needed in this PR (which seems to be a replacement of #1754).
I would rather see this done in another PR if it's not needed here.

NOTE:
the callback group for subscription is needed (kept) here,

rclcpp/rclcpp/include/rclcpp/create_subscription.hpp

Line 137 in 536df11

node_topics_interface->add_subscription(sub, options.callback_group);

not in the subscription

rclcpp/rclcpp/include/rclcpp/subscription.hpp

Line 146 in 536df11

options_(options),

alsora · 2021-12-08T14:20:03Z

Woudln't be a better approach to remove the callback group field from the SubscriptionOptionsWithAllocator structure?
Then we can have create_subscription function take the callback group as a separate argument.

I think that this makes it much clearer because:

you get the same approach as other entities
in create_subscription.hpp you don't have to be careful at which one to use among options and options_copy
in the subscription code you don't have an empty/unused field

iuhilnehc-ynos · 2021-12-09T02:06:29Z

@alsora

Thank you for your response.

Woudln't be a better approach to remove the callback group field from the SubscriptionOptionsWithAllocator structure?
Then we can have create_subscription function take the callback group as a separate argument.

Yes, make it as other entities did. It's ideal.
(Or make it as weak_ptr, or add a new structure kept in the subscription, etc.)

From my point of view, I just considered not breaking ABI because it affects so many other repositories.

I'd like to hear others' opinions about your recommendation. After that, I'll update it.

alsora · 2021-12-09T05:55:22Z

Ok, IMO we shouldn't be afraid to break API/ABI to have clean and consistent code.
Especially considering that this goes to master and the next release is in 6 months.

If you end up following my suggestion, you will have to update the documentation to mention this breaking changes.
https://github.com/ros2/ros2_documentation

clalancette · 2021-12-09T13:37:51Z

Ok, IMO we shouldn't be afraid to break API/ABI to have clean and consistent code.

Yes, agreed. ABI, in particular, we don't guarantee between releases at all, and we always rebuild all downstream packages. If we are going to break API, we should discuss it a little further and also probably have a deprecation cycle, but we can do that as well.

All of that is to say is that on Rolling, we should do what we need to make the code "best", and then worry about API/ABI more fully if we try to backport it to a released version.

iuhilnehc-ynos · 2021-12-10T03:32:12Z

@clalancette

Yes, agreed. ABI, in particular, we don't guarantee between releases at all, and we always rebuild all downstream packages. If we are going to break API, we should discuss it a little further and also probably have a deprecation cycle, but we can do that as well.

All of that is to say is that on Rolling, we should do what we need to make the code "best", and then worry about API/ABI more fully if we try to backport it to a released version.

Thank you, got it. To separate the callback_group will be my final choice if there is no other way.

After re-consideration, I think to separate the callback_group from SubscriptionOptions seems unreasonable because it's an option of SubscriptionOptions for users.

What about saving the necessary variable in the subscription, what do you think? @wjwwood
From

rclcpp/rclcpp/include/rclcpp/subscription.hpp

Lines 396 to 401 in 0775e2f

    
             /// Copy of original options passed during construction. 
        
             /** 
        
              * It is important to save a copy of this so that the rmw payload which it 
        
              * may contain is kept alive for the duration of the subscription. 
        
              */ 
        
             const rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> options_;

to

    * It is important to save a copy of this so that the rmw payload which it
    * may contain is kept alive for the duration of the subscription.
    */
-  const rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> options_;
+  std::shared_ptr<rclcpp::detail::RMWImplementationSpecificSubscriptionPayload>
+    rmw_implementation_payload_;

Notice that GenericSubscription does not copy the options.

alsora · 2021-12-10T08:39:52Z

After re-consideration, I think to separate the callback_group from SubscriptionOptions seems unreasonable because it's an option of SubscriptionOptions for users.

This is a rather arbitrary consideration IMO. There isn't a clear distinction of what should go into that data-structure and what not.

Anyhow, I'm ok with your proposal of only storing the rmw_payload rather than the full options.
This assumes that after construction we don't need anymore the options for "regular operations".

alsora · 2021-12-12T10:52:59Z

rclcpp/include/rclcpp/create_subscription.hpp

@@ -173,6 +174,57 @@ template<
  typename SubscriptionT = rclcpp::Subscription<MessageT, AllocatorT>,
  typename MessageMemoryStrategyT = typename SubscriptionT::MessageMemoryStrategyType,
  typename NodeT>
+[[deprecated("use another overloaded method create_subscription instead")]]


we should mention that the purpose of the deprecation is to have a separate argument for the callback group

I think the message in the deprecated can be either the reason or suggestion, what about adding the reason here?

I'll follow your suggestion, what about

Suggested change

[[deprecated("use another overloaded method create_subscription instead")]]

[[deprecated(

"To Keep consistency with other cases(Client, Service, Timer, etc) not hold callback_group,\n" \

"the callback_group in the SubscriptionOptionsBase will be removed in the future since the\n" \

"SubscriptionOptions is held in the Subscription and then the callback_group is depend\n" \

"on the lifetime of Subscription.\n" \

"Use\n" \

" template<\n" \

" typename MessageT,\n" \

" typename CallbackT,\n" \

" typename AllocatorT = std::allocator<void>,\n" \

" typename SubscriptionT = rclcpp::Subscription<MessageT, AllocatorT>,\n" \

" typename MessageMemoryStrategyT = typename SubscriptionT::MessageMemoryStrategyType\n" \

" typename NodeT>\n" \

" typename std::shared_ptr<SubscriptionT>\n" \

" create_subscription(\n" \

" NodeT & node,\n" \

" const std::string & topic_name,\n" \

" const rclcpp::QoS & qos,\n" \

" CallbackT && callback,\n" \

" const SubscriptionOptionsWithAllocator<AllocatorT> & options =(\n" \

" SubscriptionOptionsWithAllocator<AllocatorT>()\n" \

" ),\n" \

" rclcpp::CallbackGroup::SharedPtr group = nullptr,\n" \

" typename MessageMemoryStrategyT::SharedPtr msg_mem_strat = (\n" \

" MessageMemoryStrategyT::create_default()\n" \

" )\n" \

" );\n" \

"instead"

)]]

i think suggestion would be okay enough, since reason can be found on git history and on this issue.

Actually, I agree that the deprecation message should be more specific. While some one could figure out why this change is happening by looking at the git/github history, that's not reasonable for the average user.

On the other hand I think the suggestion above is a bit too verbose.

I would propose something like:

Suggested change

[[deprecated("use another overloaded method create_subscription instead")]]

[[deprecated(

"use the signature which has the callback group before the message memory strategy, i.e. "

"create_subscription(node, topic_name, qos, callback, options, callback_group, msg_mem_strat)"

)]]

I believe this suggestion is still under the line limit, but check the linters if you take it more or less as-is.

rclcpp/test/rclcpp/test_memory_strategy.cpp

fujitatomoya

rclcpp/rclcpp/include/rclcpp/parameter_client.hpp

Lines 214 to 215 in ee20dd3

    
           const rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> & options = ( 
        
             rclcpp::SubscriptionOptionsWithAllocator<AllocatorT>()

this also needs to be updated accordingly?

the same fix needs to be applied to rclcpp::PublisherOptionsBase as follow-up? other entities have dedicated argument for rclcpp::CallbackGroup, if i am not missed.

fujitatomoya · 2021-12-13T19:30:32Z

rclcpp/include/rclcpp/create_subscription.hpp

@@ -173,6 +174,57 @@ template<
  typename SubscriptionT = rclcpp::Subscription<MessageT, AllocatorT>,
  typename MessageMemoryStrategyT = typename SubscriptionT::MessageMemoryStrategyType,
  typename NodeT>
+[[deprecated("use another overloaded method create_subscription instead")]]


i think suggestion would be okay enough, since reason can be found on git history and on this issue.

fujitatomoya · 2021-12-13T22:08:12Z

note for myself, this must be tested with --packages-above-and-dependencies rclcpp rclcpp_lifecycle in CI.

iuhilnehc-ynos · 2021-12-14T00:45:10Z

rclcpp/rclcpp/include/rclcpp/parameter_client.hpp

Lines 214 to 215 in ee20dd3

const rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> & options = (

rclcpp::SubscriptionOptionsWithAllocator<AllocatorT>()

this also needs to be updated accordingly?

Thank you, @fujitatomoya. I'll update it.

the same fix needs to be applied to rclcpp::PublisherOptionsBase as follow-up? other entities have dedicated argument for rclcpp::CallbackGroup, if i am not missed.

Yes, I'll update it in the next PR if this one can be merged.

fujitatomoya · 2021-12-21T19:51:28Z

CI:

Linux
Linux-aarch64
macOS
Windows

iuhilnehc-ynos · 2021-12-22T07:03:58Z

rerun CI from #1833 (comment)

Linux
Linux-aarch64
macOS
Windows

The downstream repositories have the deprecated warning message as we expected.
(NOTE: I can update them after it's merged. If someone would like to fix them, I'd appreciate it.)

Update:
the failure cases in windows seem not related to current PR, because they exists in the https://ci.ros2.org/job/ci_windows/16088/ and https://ci.ros2.org/job/ci_windows/16099/

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya · 2022-01-03T19:15:50Z

The downstream repositories have the deprecated warning message as we expected.
(NOTE: I can update them after it's merged. If someone would like to fix them, I'd appreciate it.)

(downstream packages are in ros2 repository, so we should address them at the same time, i guess.) anyway, all done.

New CI:

Linux
Linux-aarch64
macOS
Windows

fujitatomoya · 2022-01-07T17:26:44Z

CI(retry):

Linux
Linux-aarch64
macOS
Windows

iuhilnehc-ynos · 2022-01-11T05:41:21Z

A failure test case in the CI. Log as below,

[ RUN      ] TestTimeSource.clock_sleep_until_with_ros_time_basic
terminate called after throwing an instance of 'std::system_error'
  what():  Resource deadlock avoided

I need to reproduce it and confirm if it was introduced by this PR.

Barry-Xu-2018 · 2022-01-11T07:42:55Z

[ RUN      ] TestTimeSource.clock_sleep_until_with_ros_time_basic
terminate called after throwing an instance of 'std::system_error'
  what():  Resource deadlock avoided

This problem isn't introduced by this PR.
I can reproduce this on master branch.
A new issue #1861 is created for this issue.

clalancette · 2022-05-09T15:23:28Z

@fujitatomoya I can go take a look and merge them all in, but do you know why CI is failing in #1833 (comment) ?

fujitatomoya · 2022-05-09T16:54:11Z

@clalancette test repo file is old, i will rebase the repo file and restart the CI against master branch.

fujitatomoya · 2022-05-09T16:58:22Z

CI(retry with https://gist.githubusercontent.com/fujitatomoya/bb7f9773ee426b740bf9af56f48d2598/raw/d48ac2c96b48f37e2916ca365a97635328929441/ros2.repos):

Linux
Linux-aarch64
Windows

wjwwood

Maybe I missed the reason why, but unless there is a reason for it, the new signatures added to support the old argument order need a deprecation warning in code.

rclcpp/include/rclcpp/create_subscription.hpp

rclcpp/include/rclcpp/node.hpp

fujitatomoya · 2022-05-09T21:15:24Z

@iuhilnehc-ynos besides @wjwwood previous comments, i think we need to rebase this. i will do the dependent packages accordingly.

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

…ription Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Chen Lihui <lihui.chen@sony.com>

remove depreacted in the template function since it's not supported. (ignored by gcc and failure in clang++) Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

iuhilnehc-ynos · 2022-05-10T03:38:40Z

retry CI based on #1833 (comment)

Linux
Linux-aarch64
Windows

iuhilnehc-ynos · 2022-05-10T05:07:03Z

The failed test can't be reproduced in my local env(ubuntu20, ubuntu22).

colcon test --packages-select rclcpp --ctest-args -R test_publisher_subscription_count_api

@ros-pull-request-builder retest this please

fujitatomoya · 2022-05-10T18:22:43Z

there are 3 more repositories to address this deprecation. i will do that later.

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya · 2022-05-10T22:28:31Z

CI (https://gist.githubusercontent.com/fujitatomoya/275ad49b0cdb2603b04a2f87a3022ce7/raw/e8b389df88e75cd2264c8572a14ba710958d5028/ros2.repos):

Linux
Linux-aarch64
Windows

fujitatomoya · 2022-05-11T03:28:49Z

windows failure https://ci.ros2.org/job/ci_windows/17074/ is unrelated.

@wjwwood @clalancette

I think this is ready to merge, the followings must go with this.

wjwwood

The changes lgtm, with a few exceptions that I commented on.

Also we need to document this. I've asked folks where it should be documented, we're in a bit of a strange state at the moment where we don't want to put it in humble, but we don't have an I-turtle release page yet so we cannot put it under the "changes since the last release" section there yet. I'll update when I have an idea where to put it.

Finally, after re-reviewing this and seeing the changes (that we realized were needed after properly adding the deprecation warnings) and after my comments here (ros2/realtime_support#115 (review)), I'm questioning if this is the right direction to take.

It's true that this makes subscriptions "more like" the other entities, but I think that's less to do with them being right and subscription being wrong and more to do with the fact that subscriptions were the first to get their own options structure due to the number and complexity of them.

So I think a better alternative might be to instead change the option classes to add (yet another) layer to them. That would allow for us to fix this problem without new signatures of create_subscription() and without any deprecations (maybe). What I was imagining was a new base class for the subscription (and publisher) options which did not contain problematic options (from a lifespan perspective) and the subscription would hold on to that base class while the user would use the full derived class to set up the options.

Also, it occurs to me that really the callback group and msg mem strat are executor specific, which is related to a shift in class layout that needs to happen to make wait set based use of our entities more natural, so really we should have like ExecutableSubscriptionOptions inherit from SubscriptionOptionsWithAllocator inherit from SubscriptionOptionsBase, with a user-friendly SubscriptionOptions which is just an alias to the ExecutableSubscriptionOptions.

Something like this sketch:

// everything except the allocator and the callback group/msg mem strat
class SubscriptionOptionsBase;
// all the normal subscription options and the allocator
template<typename AllocatorT>
class SubscriptionOptionsWithAllocator<AllocatorT>
: public SubscriptionOptionsBase;
// all the normal subscription options plus things needed
// to use a subscription with an executor
template<typename AllocatorT>
class ExecutableSubscriptionOptions<AllocatorT>
: public SubscriptionOptionsWithAllocator<AllocatorT>;

{
  // user code
  SubscriptionOptions so;
  so.callback_group = custom_callback_group;
  so.message_memory_strategy = custom_mms;
  auto sub = node->create_subscription<...>("...", 10, my_callback, so);
  // could also support, but maybe deprecate (would affect less users):
  // node->create_subscription<...>("...", 10, my_callback, so, custom_mms);

  SubscriptionOptionsBase after = sub->get_subscription_options();
}

I won't block this pr on this suggestion, but I do think it's the better way to solve the problem. As we add more options the awkwardness of passing nullptr's for unused settings will just get worse.

wjwwood · 2022-05-19T00:11:49Z

rclcpp/include/rclcpp/create_subscription.hpp

@@ -173,6 +174,57 @@ template<
  typename SubscriptionT = rclcpp::Subscription<MessageT, AllocatorT>,
  typename MessageMemoryStrategyT = typename SubscriptionT::MessageMemoryStrategyType,
  typename NodeT>
+[[deprecated("use another overloaded method create_subscription instead")]]


Actually, I agree that the deprecation message should be more specific. While some one could figure out why this change is happening by looking at the git/github history, that's not reasonable for the average user.

On the other hand I think the suggestion above is a bit too verbose.

I would propose something like:

Suggested change

[[deprecated("use another overloaded method create_subscription instead")]]

[[deprecated(

"use the signature which has the callback group before the message memory strategy, i.e. "

"create_subscription(node, topic_name, qos, callback, options, callback_group, msg_mem_strat)"

)]]

I believe this suggestion is still under the line limit, but check the linters if you take it more or less as-is.

wjwwood · 2022-05-19T00:15:50Z

rclcpp/include/rclcpp/subscription_options.hpp

@@ -53,6 +60,7 @@ struct SubscriptionOptionsBase
    RMW_UNIQUE_NETWORK_FLOW_ENDPOINTS_NOT_REQUIRED;

  /// The callback group for this subscription. NULL to use the default callback group.
+  [[deprecated("use create_subscription with a dedicated callback_group instead")]]


Similarly this deprecation notice could be better I think, maybe:

Suggested change

[[deprecated("use create_subscription with a dedicated callback_group instead")]]

[[deprecated("pass the callback group directly to the method creating the subscription")]]

I avoided create_subscription because it's not an exact symbol and it's not clear from the text (without context) if it is supposed to be a function or class or what. Since there is both a free-function and several iterations of the method on the Node class and the LifecycleNode class, it's better we just say it like I suggested, I think.

wjwwood · 2022-05-19T00:17:41Z

rclcpp/test/rclcpp/test_memory_strategy.cpp

@@ -362,12 +356,13 @@ TEST_F(TestMemoryStrategy, get_group_by_subscription) {
        memory_strategy()->get_group_by_subscription(subscription, weak_groups_to_nodes));
    }  // callback_group goes out of scope
    EXPECT_EQ(
-      callback_group,
+      nullptr,


This doesn't seem right to me, are we just "silencing" this test rather than fixing it?

callback_group goes out of scope, I think it should get nullptr like service.

iuhilnehc-ynos · 2022-05-19T02:23:31Z

@wjwwood
Holding a callback group in the subscription is not a big deal. Considering this PR is not good enough, I decide to close it rather than merge it.

@fujitatomoya
Thanks for your help. I am sorry about these PRs.

I'll create another PR if I can use an elegant method to fix this issue in the future.

fujitatomoya · 2022-05-19T03:10:59Z

As we add more options the awkwardness of passing nullptr's for unused settings will just get worse.

agree on this.

Thanks for your help. I am sorry about #1833 (comment).

no problem. those are just adjustments to suppress the warnings.

maybe we can come back on this issue with different approach.

iuhilnehc-ynos force-pushed the topic-subscription-not-hold-callback-group branch from 6a6d7c9 to 7f02ce6 Compare December 7, 2021 02:06

iuhilnehc-ynos force-pushed the topic-subscription-not-hold-callback-group branch 3 times, most recently from 7c5df21 to 126eeee Compare December 12, 2021 02:36

alsora reviewed Dec 12, 2021

View reviewed changes

rclcpp/test/rclcpp/test_memory_strategy.cpp Show resolved Hide resolved

fujitatomoya reviewed Dec 13, 2021

View reviewed changes

iuhilnehc-ynos force-pushed the topic-subscription-not-hold-callback-group branch from 948946e to 52e9814 Compare December 22, 2021 02:32

fujitatomoya added a commit to fujitatomoya/examples that referenced this pull request Jan 3, 2022

separate callback_group from subscription options

6c17511

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya mentioned this pull request Jan 3, 2022

separate callback_group from subscription options ros2/examples#335

Closed

fujitatomoya added a commit to fujitatomoya/geometry2 that referenced this pull request Jan 3, 2022

separate callback_group from subscription options

b5addcf

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya mentioned this pull request Jan 3, 2022

separate callback_group from subscription options ros2/geometry2#492

Closed

fujitatomoya added a commit to fujitatomoya/system_tests that referenced this pull request Jan 3, 2022

separate callback_group from subscription options

068f671

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya mentioned this pull request Jan 3, 2022

separate callback_group from subscription options ros2/system_tests#492

Closed

iuhilnehc-ynos mentioned this pull request Jan 4, 2022

deprecated callback_group from publisher option #1853

Closed

clalancette assigned fujitatomoya Feb 4, 2022

wjwwood requested changes May 9, 2022

View reviewed changes

rclcpp/include/rclcpp/create_subscription.hpp Outdated Show resolved Hide resolved

rclcpp/include/rclcpp/node.hpp Outdated Show resolved Hide resolved

fujitatomoya added a commit to fujitatomoya/examples that referenced this pull request May 9, 2022

separate callback_group from subscription options

82513df

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya added a commit to fujitatomoya/geometry2 that referenced this pull request May 9, 2022

separate callback_group from subscription options

246d709

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya added a commit to fujitatomoya/system_tests that referenced this pull request May 9, 2022

separate callback_group from subscription options

8a86b1c

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

Chen Lihui and others added 8 commits May 10, 2022 11:12

keep consistency with other cases to not hold callback group in subsc…

bf88bf7

…ription Signed-off-by: Chen Lihui <lihui.chen@sony.com>

separate the callback_group out of the subscription options

93ee932

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

address review issues and make deprecated cycle work fine

aef0005

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Chen Lihui <lihui.chen@sony.com>

fix unscritify and update deprecated info for the callback_group

e3e68e1

remove depreacted in the template function since it's not supported. (ignored by gcc and failure in clang++) Signed-off-by: Chen Lihui <lihui.chen@sony.com>

update comments, and note that these template functions can be removed

6e7ee70

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

update comments for doxygen

3594739

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

rename group with callback_group

35e18c2

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

add deprecated for function template

49e677d

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

iuhilnehc-ynos force-pushed the topic-subscription-not-hold-callback-group branch from a9601ed to 49e677d Compare May 10, 2022 03:36

fujitatomoya added a commit to fujitatomoya/realtime_support that referenced this pull request May 10, 2022

separate callback_group from subscription options

1a4f694

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya mentioned this pull request May 10, 2022

separate callback_group from subscription options ros2/realtime_support#115

Closed

fujitatomoya added a commit to fujitatomoya/demos that referenced this pull request May 10, 2022

separate callback_group from subscription options

acd6243

aligned with ros2/rclcpp#1833 Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya mentioned this pull request May 10, 2022

separate callback_group from subscription options ros2/demos#563

Closed

wjwwood requested changes May 19, 2022

View reviewed changes

iuhilnehc-ynos closed this May 19, 2022

-[[deprecated("use another overloaded method create_subscription instead")]]
+[[deprecated(
+  "To Keep consistency with other cases(Client, Service, Timer, etc) not hold callback_group,\n" \
+  "the callback_group in the SubscriptionOptionsBase will be removed in the future since the\n" \
+  "SubscriptionOptions is held in the Subscription and then the callback_group is depend\n" \
+  "on the lifetime of Subscription.\n" \
+  "Use\n" \
+  "  template<\n" \
+  "    typename MessageT,\n" \
+  "    typename CallbackT,\n" \
+  "    typename AllocatorT = std::allocator<void>,\n" \
+  "    typename SubscriptionT = rclcpp::Subscription<MessageT, AllocatorT>,\n" \
+  "    typename MessageMemoryStrategyT = typename SubscriptionT::MessageMemoryStrategyType\n" \
+  "    typename NodeT>\n" \
+  "  typename std::shared_ptr<SubscriptionT>\n" \
+  "  create_subscription(\n" \
+  "    NodeT & node,\n" \
+  "    const std::string & topic_name,\n" \
+  "    const rclcpp::QoS & qos,\n" \
+  "    CallbackT && callback,\n" \
+  "    const SubscriptionOptionsWithAllocator<AllocatorT> & options =(\n" \
+  "      SubscriptionOptionsWithAllocator<AllocatorT>()\n" \
+  "    ),\n" \
+  "    rclcpp::CallbackGroup::SharedPtr group = nullptr,\n" \
+  "    typename MessageMemoryStrategyT::SharedPtr msg_mem_strat = (\n" \
+  "      MessageMemoryStrategyT::create_default()\n" \
+  "    )\n" \
+  "  );\n" \
+  "instead"
+)]]

	const rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> & options = (
	rclcpp::SubscriptionOptionsWithAllocator<AllocatorT>()

	[[deprecated("use create_subscription with a dedicated callback_group instead")]]
	[[deprecated("pass the callback group directly to the method creating the subscription")]]

not to hold callback group in subscription #1833

not to hold callback group in subscription #1833

Conversation

iuhilnehc-ynos commented Dec 3, 2021 • edited Loading

alsora commented Dec 8, 2021

iuhilnehc-ynos commented Dec 9, 2021

alsora commented Dec 9, 2021 • edited Loading

clalancette commented Dec 9, 2021

iuhilnehc-ynos commented Dec 10, 2021

alsora commented Dec 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujitatomoya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujitatomoya commented Dec 13, 2021

iuhilnehc-ynos commented Dec 14, 2021

fujitatomoya commented Dec 21, 2021

iuhilnehc-ynos commented Dec 22, 2021 • edited Loading

fujitatomoya commented Jan 3, 2022

fujitatomoya commented Jan 7, 2022

iuhilnehc-ynos commented Jan 11, 2022

Barry-Xu-2018 commented Jan 11, 2022

clalancette commented May 9, 2022

fujitatomoya commented May 9, 2022

fujitatomoya commented May 9, 2022

wjwwood left a comment

Choose a reason for hiding this comment

fujitatomoya commented May 9, 2022

iuhilnehc-ynos commented May 10, 2022

iuhilnehc-ynos commented May 10, 2022 • edited Loading

fujitatomoya commented May 10, 2022

fujitatomoya commented May 10, 2022

fujitatomoya commented May 11, 2022

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iuhilnehc-ynos commented May 19, 2022 • edited Loading

fujitatomoya commented May 19, 2022

iuhilnehc-ynos commented Dec 3, 2021 •

edited

Loading

alsora commented Dec 9, 2021 •

edited

Loading

iuhilnehc-ynos commented Dec 22, 2021 •

edited

Loading

iuhilnehc-ynos commented May 10, 2022 •

edited

Loading

iuhilnehc-ynos commented May 19, 2022 •

edited

Loading