-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Update subscription callback signature and parameterize IPC #5804
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
Opening this as draft so we can discuss how to parameterize IPC in the following places:
|
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
SteveMacenski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have those metrics you posted on discourse on a page about IPC to showcase that it can be helpful, but also could hurt depending on the optimizations made by the middleware and your particular operating environment (so its best to test and find what is best for you).
This could be a root-page on docs.nav2.org, or perhaps on the Tuning Guide? I think a big part of this contribution is going to be related to documentation rather than programming to make folks aware (1) that it exists, (2) its not always better on, and (3) describing the metrics they might expect to see from your experiments to help pass judgement
nav2_route/src/route_server.cpp
Outdated
| auto msg = std::make_unique<visualization_msgs::msg::MarkerArray>(utils::toMsg(graph_, | ||
| route_frame_, this->now())); | ||
| graph_vis_publisher_->publish(std::move(msg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just return a unique pointer instead. Save a large copy.
nav2_route/src/route_server.cpp
Outdated
| if (graph_loader_->loadGraphFromFile(graph_, id_to_graph_map_, request->graph_filepath)) { | ||
| goal_intent_extractor_->setGraph(graph_, &id_to_graph_map_); | ||
| graph_vis_publisher_->publish(utils::toMsg(graph_, route_frame_, this->now())); | ||
| auto msg = std::make_unique<visualization_msgs::msg::MarkerArray>(utils::toMsg(graph_, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
nav2_rviz_plugins/src/route_tool.cpp
Outdated
| void RouteTool::update_route_graph(void) | ||
| { | ||
| graph_vis_publisher_->publish(nav2_route::utils::toMsg(graph_, "map", node_->now())); | ||
| auto msg = std::make_unique<visualization_msgs::msg::MarkerArray>(nav2_route::utils::toMsg(graph_, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
| std::string(node->get_name()) + "_" + client_node_name + "_rclcpp_node"); | ||
| auto options = node->get_node_options(); | ||
| options = options.arguments(new_arguments); | ||
| options = options.use_intra_process_comms(true).arguments(new_arguments); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in others: obviously we need to parameterize this (as you know)
I assume the issue is that you are doing this in the constructor so you don't have access to While I didn't test, I think something like this should work We can have this as a member of the Nav2 Lifecycle Node (though maybe implemented as a free function so it can be used independently as well. |
|
This pull request is in conflict. Could you fix it @mini-1235? |
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Sorry, I should be more specific. What I wanted to discuss is whether we want to enable this via launch files or yaml configuration. In fe1d30d I tried enabling IPC by adding extra_arguments in the node launch (for composable nodes), this works for (1),(3),(4):
For (2), I need to modify it to For (5), I am not sure if it is actually used in the current codebase, so I didn't modify it for now Let me know which approach you would prefer |
I have opened a PR here ros-navigation/docs.nav2.org#835 to document IPC. I also briefly mention composition, RMW, and QoS. Let me know if there are other details you would like me to add :) |
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Description of how this change was tested
Future work that may be required in bullet points
For Maintainers:
backport-*.