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.

Sign up for GitHub

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

Jump to bottom

apply actual QoS from rmw to the IPC publisher. #2707

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from
Open

apply actual QoS from rmw to the IPC publisher. #2707

wants to merge 2 commits into from
+43 −12

Conversation

fujitatomoya
Copy link
Collaborator

@fujitatomoya fujitatomoya commented Dec 13, 2024 •
edited
Loading

closes #2705

note: backport required to jazzy and humble.

@fujitatomoya fujitatomoya mentioned this pull request Dec 13, 2024
Open
@fujitatomoya
Copy link
Collaborator Author

@alsora i would like to request the review.

@fujitatomoya fujitatomoya requested a review from alsora December 13, 2024 08:20
@fujitatomoya fujitatomoya self-assigned this Dec 13, 2024
@fujitatomoya fujitatomoya added the bug Something isn't working label Dec 13, 2024
clalancette
clalancette reviewed Dec 13, 2024
View reviewed changes
Copy link
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 is absolutely the right change, thanks for finding it @fujitatomoya . I left one thought inline.

rclcpp/include/rclcpp/publisher.hpp Outdated
(void)topic;
(void)qos;
(void)options;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh. I'm wondering whether we should deprecate this API signature, and just reduce this to:

virtual
void
post_init_setup(rclcpp::node_interfaces::NodeBaseInterface * node_base);

As far as I can tell, there are no downstream users of this API, and just continually setting this to (void) seems useless to me. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i am okay with that.

maybe we can refactor the following, but i would like to do that with another PR.

  • introduce intra_process_init non-virtual function under Publisher and Subscriber classes, and implement whatever needs to be setup for intra-process pub/sub.
  • deprecate post_init_setup for Publisher and Subscriber classes, Subscriber one is empty already. Or we can keep it for fully virtual function? so that user can override this to setup post process initialization?

@fujitatomoya
Copy link
Collaborator Author

Pulls: #2707
Gist: https://gist.githubusercontent.com/fujitatomoya/1864914d39a602bf8236970ff6d5d595/raw/1e4e0757031d664d082ecea21c6e706f36eee0b0/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14955

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

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Dec 14, 2024 •
edited
Loading

Pulls: #2707
Gist: https://gist.githubusercontent.com/fujitatomoya/77dd98edbbde2b1cf7e7afdf880e66d3/raw/1e4e0757031d664d082ecea21c6e706f36eee0b0/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14961

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

@fujitatomoya fujitatomoya force-pushed the fujitatomoya/ipc-should-apply-actual-qos branch from 94ba4a4 to d3d745d Compare December 14, 2024 23:26
@fujitatomoya
address uncrustify warning.
9c4cdd0 
Signed-off-by: Tomoya Fujita <[email protected]>
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/ipc-should-apply-actual-qos branch from d3d745d to 9c4cdd0 Compare December 14, 2024 23:28
@fujitatomoya
Copy link
Collaborator Author

Pulls: #2707
Gist: https://gist.githubusercontent.com/fujitatomoya/0f28f78e9f92d6bde8551ba4f679d785/raw/1e4e0757031d664d082ecea21c6e706f36eee0b0/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14964

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette left review comments

@mjcarroll mjcarroll mjcarroll approved these changes

@alsora alsora Awaiting requested review from alsora

Assignees

@fujitatomoya fujitatomoya

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SystemDefaultsQoS - force to have depth > 0 for Publishers
3 participants
@fujitatomoya @clalancette @mjcarroll