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

Adding NodeInterfaces to Buffer #656

Merged
merged 6 commits into from
Mar 26, 2024
Merged

Conversation

CursedRock17
Copy link
Contributor

Resolves #580 by adding node_interfaces support to the constructor of tf2_ros::Buffer. Currently, this is still a work in progress to resolve the test failures. Similar to PR #576

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@clalancette clalancette marked this pull request as draft March 11, 2024 12:50
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@CursedRock17 CursedRock17 marked this pull request as ready for review March 12, 2024 13:39
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>
@CursedRock17
Copy link
Contributor Author

colcon build and colcon test both run fine locally, is there a reason the CI is failing with such a weird error on here? Also, is it better to add another class constructor for Buffer that takes in the node_interfaces and calls create_service as the only call, or can we stay simple with the basic universal constructor?

@ahcorde
Copy link
Contributor

ahcorde commented Mar 18, 2024

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

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

*/
TF2_ROS_PUBLIC Buffer(
Copy link
Contributor

Choose a reason for hiding this comment

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

restore TF2_ROS_PUBLIC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I imagine that's the only thing throwing it off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So TF2_ROS_PUBLIC is essentially

#define TF2_ROS_EXPORT __declspec(dllexport)

or some similar form of dllexport. Thus, it cannot exist in the scope of a definition of it's application according to the docs. We have to immediately define the constructor for Buffer since it's a template. As for removing the macro, the original CI job has linking errors in getFrames and onTimeJump which are the only two private functions used in the constructor of Buffer. I assume that if we mark those functions as able to link, Buffer shouldn't have problems with anything else

Adding `TF2_ROS_PUBLIC` to `Buffer` 

Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>
@ahcorde
Copy link
Contributor

ahcorde commented Mar 20, 2024

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

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@ahcorde
Copy link
Contributor

ahcorde commented Mar 21, 2024

  • Windows Build Status

@ahcorde
Copy link
Contributor

ahcorde commented Mar 25, 2024

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

@ahcorde ahcorde requested a review from clalancette March 25, 2024 20:37
@ahcorde ahcorde merged commit 0e505aa into ros2:rolling Mar 26, 2024
1 of 2 checks passed
CursedRock17 added a commit to CursedRock17/geometry2 that referenced this pull request Apr 3, 2024
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: Lucas Wendland <82680922+CursedRock17@users.noreply.github.com>
CursedRock17 added a commit to CursedRock17/geometry2 that referenced this pull request Apr 4, 2024
CursedRock17 added a commit to CursedRock17/geometry2 that referenced this pull request Apr 4, 2024
@methylDragon
Copy link

methylDragon commented Jun 29, 2024

Just a quick PSA to use NodeInterfaces (no expected/obligated actionable, more a request for help if anyone stumbles across this).
I understand using the template approach helps prevent rippling out changes to any dependents of buffer.

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.

Lifecycle node support for tf2_ros::Buffer
4 participants