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

Add tracepoint for publish/subscribe serialized message #485

Merged
merged 9 commits into from Apr 5, 2024

Conversation

h-suzuki-isp
Copy link
Contributor

Please, refer to ros2/rclcpp#2448 .
I have added the following trace points.

  • __rmw_publish_serialized_message()
    • tracepoint : rmw_publish
  • _take_serialized_message()
    • tracepoint : rmw_take

Signed-off-by: h-suzuki <h-suzuki@isp.co.jp>
Signed-off-by: h-suzuki <h-suzuki@isp.co.jp>
Signed-off-by: h-suzuki <h-suzuki@isp.co.jp>
Signed-off-by: h-suzuki <h-suzuki@isp.co.jp>
Signed-off-by: h-suzuki <h-suzuki@isp.co.jp>
Signed-off-by: h-suzuki <h-suzuki@isp.co.jp>
Signed-off-by: h-suzuki <h-suzuki@isp.co.jp>
Co-authored-by: eboasson <eb@ilities.com>
Signed-off-by: h-suzuki-isp <146712054+h-suzuki-isp@users.noreply.github.com>
@fujitatomoya
Copy link
Contributor

@mjcarroll /assign along with ros2/rmw_fastrtps#748

Copy link
Member

@christophebedard christophebedard 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. @eboasson or @mjcarroll do you want to take a(nother) look?

@h-suzuki-isp
Copy link
Contributor Author

@eboasson @mjcarroll (Cc: @christophebedard )
I'd like to get this PR to Jazzy on time.
I would appreciate it if you could review it.

Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Mostly good, one nit and I would like clarification on the impact of the API change.

rmw_cyclonedds_cpp/src/rmw_node.cpp Show resolved Hide resolved
@@ -3506,7 +3510,6 @@ static rmw_ret_t rmw_take_ser_int(
if (message_info) {
message_info_from_sample_info(info, message_info);
}

Copy link
Member

Choose a reason for hiding this comment

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

Nit: unnecessary whitespace change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2b33c0b.

@mjcarroll
Copy link
Member

Note that CI was run here: ros2/rclcpp#2448 (comment)

Signed-off-by: h-suzuki-isp <h-suzuki@isp.co.jp>
@christophebedard
Copy link
Member

I think we can probably skip CI since the only new change is a new blank line.

@mjcarroll mjcarroll merged commit 2f0e1ef into ros2:rolling Apr 5, 2024
2 checks passed
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.

None yet

5 participants