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

SystemDefaultsQoS - force to have depth > 0 for Publishers #2705

Closed
SteveMacenski opened this issue Dec 13, 2024 · 2 comments · Fixed by #2707
Closed

SystemDefaultsQoS - force to have depth > 0 for Publishers #2705

SteveMacenski opened this issue Dec 13, 2024 · 2 comments · Fixed by #2707

Comments

@SteveMacenski
Copy link
Collaborator

SteveMacenski commented Dec 13, 2024

Feature request

Feature description

For SystemDefaultsQoS to work out of the box for IPC, it needs to always strictly have a depth setting > 0. When used in a subscription out of the box, this works fine. For example:

  footprint_sub_ = create_subscription<geometry_msgs::msg::Polygon>(
    "footprint",
    nav2_util::DefaultQoS(),
    std::bind(&Costmap2DROS::setRobotFootprintPolygon, this, std::placeholders::_1));

However, when used in a publisher, it fails. For example:

  footprint_pub_ = create_publisher<geometry_msgs::msg::PolygonStamped>(
    "published_footprint", rclcpp::SystemDefaultsQoS());

or

  state_pub_ = this->create_publisher<nav2_msgs::msg::CollisionDetectorState>(
    "collision_detector_state", rclcpp::SystemDefaultsQoS());

with the error

[component_container_isolated-7] [ERROR] [1734048098.404790594] [local_costmap.local_costmap]: Original error: intraprocess communication is not allowed with a zero qos history depth value


Thus, I feel that we should update the default for SystemDefaultsQoS for publishers (or in general) to be 1 if not already > 1 so that folks migrating or implementing IPC don't run into a cacophony of issues, one for each publisher. I understand this comes from the RMW/DDS, but it seems to me this needs to be updated since SystemDefaultsQoS is a key "default" QoS profile. Having that simply not work for a core ROS 2 feature seems intrinsically problematic :-) It was a bit subtle for me to identify this, so fixing this for everyone else in the future would be ideal.

For now, I'm creating a new nav2_util::DefaultPublisherQoS() to resolve this on my end, but I believe it should be fixed in rclcpp / below for the general user case

@fujitatomoya
Copy link
Collaborator

@SteveMacenski thanks for detailed description, that really helped.

i believe this is a bug for the intra-process publisher, #2707 should fix this.

@SteveMacenski
Copy link
Collaborator Author

SteveMacenski commented Dec 14, 2024

I'll openly admit the use of qos vs get_actual_qos() is a subtlety I cannot contribute much to the discussion on :-)

I will say that in your test, I see it with use_intra_process_comms(true) without messing with the publisher-specific IPC settings like you do in your current test. I think these are functionally the same, but if you wanted to verify it fixed what I was seeing, a test might be simply:

TEST(TestPublisher, test_publisher_with_system_default_qos) {
  auto node_options = rclcpp::NodeOptions().use_intra_process_comms(true);
  auto test_node = std::make_shared<rclcpp::Node>(node_options);
  using test_msgs::msg::Empty;
  ASSERT_NO_THROW(
  {
    auto publisher = test_node->create_publisher<Empty>("topic", rclcpp::SystemDefaultsQoS());
  });
}

If you see that fail without your change and pass with it, then it fixes the issue case I see 😄

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 a pull request may close this issue.

2 participants