Skip to content

Fix subscription instrumentation for ConstSharedPtr[WithInfo]Callback#1872

Merged
clalancette merged 1 commit intoros2:foxyfrom
christophebedard:christophebedard/fix-sub-instrumentation-callback
Jan 31, 2022
Merged

Fix subscription instrumentation for ConstSharedPtr[WithInfo]Callback#1872
clalancette merged 1 commit intoros2:foxyfrom
christophebedard:christophebedard/fix-sub-instrumentation-callback

Conversation

@christophebedard
Copy link
Copy Markdown
Member

@christophebedard christophebedard commented Jan 20, 2022 •
edited
Loading

Subscription callbacks of types ConstSharedPtrCallback and ConstSharedPtrWithInfoCallback were not getting registered.

See: https://gitlab.com/ros-tracing/tracetools_analysis/-/issues/47#note_817008890

Switching to something like this (in galactic) would be better, but the change would be less minimal:

rclcpp/rclcpp/include/rclcpp/any_subscription_callback.hpp

Lines 372 to 384 in 468cbab

void
register_callback_for_tracing()
{
#ifndef TRACETOOLS_DISABLED
std::visit(
[this](auto && callback) {
TRACEPOINT(
rclcpp_callback_register,
static_cast<const void *>(this),
tracetools::get_symbol(callback));
}, callback_variant_);
#endif // TRACETOOLS_DISABLED
}

Signed-off-by: Christophe Bedard bedard.christophe@gmail.com

@christophebedard
Fix subscription instrumentation for ConstSharedPtr[WithInfo]Callback
fdb8b9e 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Jan 20, 2022

CI for rclcpp and tracetools+tracetools_test:

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

clalancette
clalancette reviewed Jan 20, 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.

Just so I'm clear about this; this is only a problem in Foxy, correct? Because Galactic and Rolling use the std::visit pattern, they don't have this issue?

@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Jan 20, 2022

Exactly! It's certainly a more foolproof way to do this.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Jan 20, 2022

Exactly! It's certainly a more foolproof way to do this.

Perfect, thanks for clarifying.

clalancette
clalancette approved these changes Jan 20, 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.

Looks good to me with green CI.

@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Jan 21, 2022

CMake warnings on Windows look unrelated.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Jan 21, 2022

CMake warnings on Windows look unrelated.

Yeah, I'm pretty sure those are well-known on Foxy. @jacobperron OK to merge this one?

fujitatomoya
fujitatomoya approved these changes Jan 21, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

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

this makes sense to me. i do not think https://ci.ros2.org/job/ci_windows/16303/cmake/new/ is unrelated to this PR.

@clalancette clalancette merged commit 7e1740a into ros2:foxy Jan 31, 2022
@christophebedard christophebedard deleted the christophebedard/fix-sub-instrumentation-callback branch January 31, 2022 14:39
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

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@christophebedard @clalancette @fujitatomoya