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

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

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

…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
@ivanpauno ivanpauno added this to TODO in Humble Patch Release 1 via automation 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?

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.

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

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
@audrow audrow moved this from TODO to Done in Humble Patch Release 1 Nov 23, 2022
xueying4402 pushed a commit to xueying4402/rclcpp that referenced this pull request Jul 8, 2024
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants