Fixed errors when creating a custom subscriber.

[bks-ol]

It was impossible to create a custom subscriber inherited from rclcpp::Subscriber, I fixed this bug and added a test to check correctness.

[fujitatomoya]

i think that allowing the user application to create the custom subscription inherited from rclcpp::Subscriber would be nice.
but this PR targets to humble, and there are many test failures.

can you address that retargeting this PR against rolling that is the main development branch and all test failures?

note that, since this is breaking ABI change, we cannot backport this to already released distros that are jazzy and humble.

[bks-ol]

i think that allowing the user application to create the custom subscription inherited from rclcpp::Subscriber would be nice. but this PR targets to humble, and there are many test failures.

can you address that retargeting this PR against rolling that is the main development branch and all test failures?

note that, since this is breaking ABI change, we cannot backport this to already released distros that are jazzy and humble.

No, it doesn't break the API because there are template parameters in the function signature. There are no parameters in the internal call stack.

Please take a look:

rclcpp/rclcpp/include/rclcpp/create_subscription.hpp

Line 44 in 07f410e

namespace detail

rclcpp/rclcpp/include/rclcpp/node_impl.hpp

Line 85 in 07f410e

template<

[fujitatomoya]

@bks-ol ah i see, this does not change templated parameter, since it is already there.

can you address that retargeting this PR against rolling that is the main development branch and all test failures?

but this is still what we need to do to proceed this PR.

[bks-ol]

@bks-ol ah i see, this does not change templated parameter, since it is already there.

can you address that retargeting this PR against rolling that is the main development branch and all test failures?

but this is still what we need to do to proceed this PR.

I have fixed the errors and would like to get your feedback.

I can make another pull request to rolling later, but for now I need a merge with humble, okay? :)

[fujitatomoya]

I can make another pull request to rolling later, but for now I need a merge with humble, okay? :)

I do not think so 😓 usually we fix the problem in rolling, and then if that is ABI/API compatible (it does not break the user space), we can work on the backport after soak time...

CC: @clalancette @mjcarroll @jmachowinski

[clalancette]

I do not think so 😓 usually we fix the problem in rolling, and then if that is ABI/API compatible (it does not break the user space), we can work on the backport after soak time...

Yeah, we need to get it into rolling first. Besides the fact that we need it to have soak time, we also want to make sure that the fixes are always in the latest release, and the way we guarantee that is by always targeting the latest release first.

[jmachowinski]

@bks-ol Out of curiosity, why do you need to inherit from rclcpp::Subscription in the first place ?

[bks-ol]

@fujitatomoya Hello! Everything is done!

[fujitatomoya]

lgtm with a couple of minor comments.

[fujitatomoya]

@mjcarroll @jmachowinski @alsora either of you, can you take a look?

[fujitatomoya]

@bks-ol thanks for the patience and effort, appreciate it. let's wait for the 2nd review for this.

[fujitatomoya]

Pulls: #2727
Gist: https://gist.githubusercontent.com/fujitatomoya/c0eb64b8725e475fdc01c47961b19ec7/raw/8dd8072602bdaaaf0b0391fff4bdd9e6d9b790ed/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15290

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[bks-ol]

@bks-ol thanks for the patience and effort, appreciate it. let's wait for the 2nd review for this.

Let's continue!

[bks-ol]

@fujitatomoya Hello! :)

[fujitatomoya]

@bks-ol someone else needs to review this, we are pretty much occupied for next distro release for now.

[dbarker]

Thanks for the PR @bks-ol. I've recently run into this limitation also when trying to create a derived Subscription class in order to add opentelemetry instrumentation. The fix lgtm with just a few ci issues to address.