Skip to content

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

Merged
clalancette merged 8 commits intoros2:rollingfrom
roncapat:humble
Mar 23, 2023
Merged

Enable TransformListener node-based constructor in Intra-process enabled components#572
clalancette merged 8 commits intoros2:rollingfrom
roncapat:humble

Conversation

@roncapat
Copy link
Copy Markdown
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
Copy Markdown

FranzAlbers commented Dec 20, 2022

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
Copy Markdown
Contributor Author

roncapat commented Dec 20, 2022 •
edited
Loading

@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
Copy Markdown

FranzAlbers commented Dec 21, 2022

@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
Copy Markdown
Contributor Author

roncapat commented Dec 21, 2022

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

@FranzAlbers
Copy link
Copy Markdown

FranzAlbers commented Dec 21, 2022 •
edited
Loading

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
Copy Markdown
Contributor Author

roncapat commented Feb 18, 2023

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

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Mar 7, 2023

@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
Copy Markdown
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
Copy Markdown
Contributor Author

roncapat commented Mar 11, 2023

@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 :)

clalancette
clalancette requested changes Mar 17, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

roncapat commented Mar 17, 2023

@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
Copy Markdown
Contributor Author

roncapat commented Mar 18, 2023 •
edited
Loading

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

Anyway, I rebased on rolling :)

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Mar 20, 2023

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.

clalancette
clalancette requested changes Mar 20, 2023
View reviewed changes
Copy link
Copy Markdown
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.

clalancette
clalancette requested changes Mar 20, 2023
View reviewed changes
roncapat and others added 2 commits March 20, 2023 19:11
@roncapat @clalancette
Doc fix
588b278 
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
@roncapat @clalancette
Doc fix
64e18a4 
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
clalancette
clalancette requested changes Mar 20, 2023
View reviewed changes
Copy link
Copy Markdown
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.

roncapat and others added 2 commits March 20, 2023 19:23
@roncapat @clalancette
Update tf2_ros/include/tf2_ros/transform_listener.h
b84fad1 
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
@roncapat @clalancette
Update tf2_ros/include/tf2_ros/transform_listener.h
88128f8 
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
clalancette
clalancette requested changes Mar 20, 2023
View reviewed changes
Copy link
Copy Markdown
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)

@roncapat @clalancette
Linting
0613a1c 
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
@roncapat
Copy link
Copy Markdown
Contributor Author

roncapat commented Mar 22, 2023

Checked offline, this should be the last lint fix!

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Mar 22, 2023 •
edited
Loading

CI:

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

@clalancette clalancette merged commit abfd97d into ros2:rolling Mar 23, 2023
This was referenced Jun 13, 2023
Merged
Merged
This was referenced Sep 21, 2023
Merged
Closed
roncapat added a commit to roncapat/geometry2 that referenced this pull request Jan 22, 2024
@roncapat
Revert "Enable TransformListener node-based constructor in Intra-proc…
be5abf3 
…ess enabled components (ros2#572)"

This reverts commit abfd97d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clalancette clalancette clalancette approved these changes

@ahcorde ahcorde Awaiting requested review from ahcorde

Assignees

@clalancette clalancette

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@roncapat @FranzAlbers @clalancette