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

Enable TransformListener node-based constructor in Intra-process enabled components #572

Merged
merged 8 commits into from
Mar 23, 2023

Conversation

roncapat
Copy link
Contributor

@roncapat roncapat commented Dec 9, 2022

Fixes #571

With this patch, TransformListener can be constructed using existing node references, when those nodes are in fact Intra-Process Enabled C++ ROS2 Components.

Intra-process communication is disabled per-subscription.

@FranzAlbers
Copy link

I'm running into a similar issue like #571, but with a StaticTransformBroadcaster.

Maybe this PR can be extended for the StaticTransformBroadcaster and TransformBroadcaster?

@roncapat roncapat changed the title Enable TransformListener node-based constructor in Intra-process enaled components Enable TransformListener node-based constructor in Intra-process enabled components Dec 20, 2022
@roncapat
Copy link
Contributor Author

roncapat commented Dec 20, 2022

@FranzAlbers so you compose a StaticTransformBroadcaster and it doesn't work when intra_process_Comms are enabled via component param?

EDIT: since I will probably try to compose some StaticTransformBroadcaster in my launchers, additional feedback from you could help me fix the problem you are mentioning.

@FranzAlbers
Copy link

@roncapat I want to compose a node with my camera driver, image_proc::RectifyNode, and image_proc::FlipNode (see ros-perception/image_pipeline#756) as components. To avoid unnecessary copies of the images, I want to use_intra_process_comms.

This works fine for my camera driver and the RectifyNode, but not for the FlipNode since it also uses a StaticTransformBroadcaster with transient_local durability. Thus, I am running into the following error during startup:
Component constructor threw an exception: intraprocess communication allowed only with volatile durability
Without the StaticTransformBroadcaster, intraprocess communications run fine for the FlipNode.

However, I quickly tried this myself by setting options.use_intra_process_comm = rclcpp::IntraProcessSetting::Disable; in line 67 of static_transform_broadcaster.h, but I am still running into the same error. Not sure what I am missing here.

@roncapat
Copy link
Contributor Author

Ok, I will check also that. Seems a simple job, like what already included in this PR.

@FranzAlbers
Copy link

FranzAlbers commented Dec 21, 2022

After cleaning and rebuilding the image_flip package it works as expected.

I think editing static_transform_broadcaster.h similarly to this PR should do the trick.

@clalancette clalancette self-assigned this Jan 12, 2023
@roncapat
Copy link
Contributor Author

@clalancette is there anything I should do to improve the PR?

@clalancette
Copy link
Contributor

@clalancette is there anything I should do to improve the PR?

Can you please add in a test to show that this fixes the problem?

@roncapat
Copy link
Contributor Author

roncapat commented Mar 7, 2023

Of course, I'll try (and write back if not able to figure out how to do it properly).

@roncapat roncapat force-pushed the humble branch 2 times, most recently from cc081d5 to e3d3981 Compare March 11, 2023 14:01
@roncapat
Copy link
Contributor Author

@clalancette I force-pushed a commit with a new test triggering the issue with intraprocess enabled.

So that:

  • if you check out the first commit, a new test appear (and fails)
  • if you then check the second one, the new test passes

Hope this covers your request :)

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.

Can you please retarget this to the rolling branch? Once we have it in there, we can consider backporting to humble. Thanks.

@roncapat roncapat changed the base branch from humble to rolling March 17, 2023 16:55
@roncapat roncapat requested a review from ahcorde as a code owner March 17, 2023 16:55
@roncapat
Copy link
Contributor Author

@clalancette yes (did a mess now with github portal trying to switch base branch...). During weekend I'll do the proper moves/rebase sorry :)

@roncapat roncapat changed the base branch from rolling to humble March 17, 2023 16:56
@roncapat roncapat changed the base branch from humble to rolling March 18, 2023 13:31
@roncapat
Copy link
Contributor Author

roncapat commented Mar 18, 2023

What's the difference between Hpr and Rpr build?
Is it Humble vs. Rolling?

Anyway, I rebased on rolling :)

@clalancette
Copy link
Contributor

What's the difference between Hpr and Rpr build?
Is it Humble vs. Rolling?

Yes, exactly. There is some bug here where if you change branches, it still tries to build on the old branch. So that's why the Hpr job is failing, but we can ignore that.

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.

One more small thing to fix, then I think this is ready for CI.

tf2_ros/include/tf2_ros/transform_listener.h Show resolved Hide resolved
tf2_ros/include/tf2_ros/transform_listener.h Outdated Show resolved Hide resolved
tf2_ros/include/tf2_ros/transform_listener.h Outdated Show resolved Hide resolved
roncapat and others added 2 commits March 20, 2023 19:11
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
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.

(trailing whitespace makes uncrustify unhappy)

We are getting closer.

tf2_ros/include/tf2_ros/transform_listener.h Outdated Show resolved Hide resolved
tf2_ros/include/tf2_ros/transform_listener.h Outdated Show resolved Hide resolved
roncapat and others added 2 commits March 20, 2023 19:23
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
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.

(it's probably best if you run colcon test --packages-select tf2_ros before submitting again)

tf2_ros/include/tf2_ros/transform_listener.h Outdated Show resolved Hide resolved
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
@roncapat
Copy link
Contributor Author

Checked offline, this should be the last lint fix!

@clalancette
Copy link
Contributor

clalancette commented Mar 22, 2023

CI:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TransformListener does not work in composed, intra-process enabled nodes
3 participants