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.

Sign up for GitHub

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

Jump to bottom

Fix subscription.is_serialized() for callbacks with message info #1950

Merged
merged 2 commits into from
Jun 10, 2022
Merged

Fix subscription.is_serialized() for callbacks with message info #1950

merged 2 commits into from
Jun 10, 2022

Conversation

ivanpauno
Copy link
Member

I think this was broken when support for type adaption was added.
Currently if you create a serialized subscription with a callback taking both the message and the message info, the executor throws an exception the first time a message is dispatched.

This fixes the issue.

@ivanpauno
Fix subscription.is_serialized() for callbacks with message info argu…
93ca78a 
…ment

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the bug Something isn't working label Jun 7, 2022
@ivanpauno ivanpauno requested a review from wjwwood June 7, 2022 19:29
@ivanpauno ivanpauno self-assigned this Jun 7, 2022
@clalancette
Copy link
Contributor

Yikes, we probably didn't notice because there wasn't a test.

Can you add tests that exercise this?

@ivanpauno
Add tests + please linters
199d36b 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

Can you add tests that exercise this?

I have added some tests for AnySubscriptionCallback covering this case.
See 199d36b.

I would also like to have a test with a publisher and a subscription for each of the callbacks, and make sure the message arrives.
We don't currently have tests like that in rclcpp, I think that we added all of them in test_rclcpp (system_tests repo), so I could add one doing something like that there.

@ivanpauno ivanpauno requested a review from clalancette June 8, 2022 19:36
@clalancette
Copy link
Contributor

I would also like to have a test with a publisher and a subscription for each of the callbacks, and make sure the message arrives.
We don't currently have tests like that in rclcpp, I think that we added all of them in test_rclcpp (system_tests repo), so I could add one doing something like that there.

Yeah, we typically have tests like that in test_rclcpp or test_communication. I think it would be fine if you wanted to add some tests there, though I would be careful about adding all combinations; I don't want to explode our CI times.

clalancette
clalancette approved these changes Jun 10, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI (I'd be sure to run CI with --packages-above rclcpp)

@ivanpauno
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member Author

windows issues are unrelated (happening in nightlies as well https://ci.ros2.org/view/nightly/job/nightly_win_rel/2329/msbuild/new/).
Going in!

@ivanpauno ivanpauno merged commit 8e6a6fb into master Jun 10, 2022
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/fix-is-serialized-for-callback-with-info branch June 10, 2022 20:18
xueying4402 pushed a commit to xueying4402/rclcpp that referenced this pull request Jul 8, 2024
@ivanpauno
Fix subscription.is_serialized() for callbacks with message info (ros…
4404c47 
…2#1950)

* Fix subscription.is_serialized() for callbacks with message info argument

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Add tests + please linters

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@xueying4402 xueying4402 mentioned this pull request Jul 8, 2024
Closed
roscan-tech pushed a commit to roscan-tech/rclcpp that referenced this pull request Sep 8, 2024
@ivanpauno
Fix subscription.is_serialized() for callbacks with message info (ros…
86e2ea5 
…2#1950)

* Fix subscription.is_serialized() for callbacks with message info argument

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Add tests + please linters

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: roscan-tech <liwei4402@mail2.sysu.edu.cn>
@fujitatomoya fujitatomoya mentioned this pull request Oct 4, 2024
Merged
fujitatomoya pushed a commit that referenced this pull request Oct 7, 2024
@roscan-tech @ivanpauno
Fix subscription.is_serialized() for callbacks with message info (#1950…
28de27e 
…) (#2622)

* Fix subscription.is_serialized() for callbacks with message info argument

* Add tests + please linters

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: roscan-tech <liwei4402@mail2.sysu.edu.cn>
Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@alsora alsora alsora approved these changes

@clalancette clalancette clalancette approved these changes

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

Assignees

@ivanpauno ivanpauno

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivanpauno @clalancette @alsora @fujitatomoya