Skip to content

allow construction of tf broadcaster from node object (not a pointer)#555

Merged
clalancette merged 1 commit intoros2:rollingfrom
alsora:asoragna/tf-broadcaster-node-object
Sep 29, 2022
Merged

allow construction of tf broadcaster from node object (not a pointer)#555
clalancette merged 1 commit intoros2:rollingfrom
alsora:asoragna/tf-broadcaster-node-object

Conversation

@alsora
Copy link
Copy Markdown
Contributor

@alsora alsora commented Sep 26, 2022

Follow up PR to #552.
Addresses #554

@alsora
allow construction of tf broadcaster from node object (not a pointer)
c95c2ac 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
Copy link
Copy Markdown
Contributor Author

alsora commented Sep 26, 2022

@clalancette @ijnek I ended up taking a different approach to address the problem.
This does not require to add/deprecate constructors.
I added a unit-test to verify that we can build both from pointers as well as non-pointers.

jacobperron
jacobperron approved these changes Sep 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jacobperron jacobperron left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

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

@jacobperron
Copy link
Copy Markdown
Member

jacobperron commented Sep 28, 2022

Bizarre, I'm not sure why Windows is experiencing build errors.

ijnek
ijnek approved these changes Sep 29, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ijnek ijnek left a comment

Choose a reason for hiding this comment

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

LGTM, added tests look good too.

@clalancette clalancette mentioned this pull request Sep 29, 2022
Merged
@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Sep 29, 2022

Bizarre, I'm not sure why Windows is experiencing build errors.

Well, at the very least it does look like there are some standard header includes missing in the rclcpp node_interface headers. I've opened up ros2/rclcpp#2018 to fix that, which may also fix this.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Sep 29, 2022

All right, I've merged in the rclcpp PR (thanks @alsora for the review). Let's see if it fixes CI here:

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

clalancette
clalancette approved these changes Sep 29, 2022
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.

This looks good to me, and has green CI.

I also verified that it fixes the test case that @ijnek had on his branch. So I'm going to go ahead and merge this, thanks for the fix!

@clalancette clalancette linked an issue Sep 29, 2022 that may be closed by this pull request
TransformBroadcaster takes shared pointer instead of raw node #554
Closed
@clalancette clalancette merged commit de08d17 into ros2:rolling Sep 29, 2022
@ijnek
Copy link
Copy Markdown
Contributor

ijnek commented Sep 30, 2022

Thanks @alsora for taking on the work!

@alsora alsora deleted the asoragna/tf-broadcaster-node-object branch September 30, 2022 06:01
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

@jacobperron jacobperron jacobperron approved these changes

+1 more reviewer

@ijnek ijnek ijnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TransformBroadcaster takes shared pointer instead of raw node

4 participants

@alsora @jacobperron @clalancette @ijnek