Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed errors when creating a custom subscriber. #2727

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from

Conversation

bks-ol
Copy link

@bks-ol bks-ol commented Jan 19, 2025

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

@bks-ol bks-ol force-pushed the feature/creating_custom_sub branch 2 times, most recently from 5dbd654 to 07f410e Compare January 19, 2025 16:22
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

bks-ol commented Jan 22, 2025

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:

@fujitatomoya
Copy link
Collaborator

@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.

@clalancette clalancette added the more-information-needed Further information is required label Jan 30, 2025
@bks-ol bks-ol force-pushed the feature/creating_custom_sub branch from 68f5ecc to ff1cf21 Compare February 3, 2025 20:59
@bks-ol
Copy link
Author

bks-ol commented Feb 3, 2025

@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
Copy link
Collaborator

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
Copy link
Contributor

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.

@bks-ol bks-ol changed the base branch from humble to rolling February 3, 2025 22:27
@bks-ol bks-ol changed the base branch from rolling to humble February 3, 2025 22:27
@bks-ol bks-ol force-pushed the feature/creating_custom_sub branch from ff1cf21 to 34ce012 Compare February 3, 2025 22:52
@bks-ol bks-ol changed the base branch from humble to rolling February 3, 2025 22:53
@jmachowinski
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants