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

Support multiple destinations with actions in nexus_transporter #69

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented Jan 27, 2025

This PR

  • Updates nexus_transporter_msgs to include a Destination msg with fields to specify the type of action to perform at the destination.
  • Updates the Itinerary and Transporter classes to support multiple Destinations.
  • Switches to SingleThreadedExecutor (default). This was previously set to MultiThreadedExecutor to deal with making a registration client request within a timer callback which leads to a deadlock. This is resolved in the same way the Workcell Orchestrator handles it, ie, relying on async client requests.
  • Move the mock_transporter plugin implementation from nexus_integration_tests to `nexus_transporter.

As a follow up, I will update rmf_request interfaces introduced in #42 with the Destination definition here and explore a native RMF plugin implementation.

Signed-off-by: Yadunund <yadunund@gmail.com>
Signed-off-by: Yadunund <yadunund@gmail.com>
Signed-off-by: Yadunund <yadunund@gmail.com>
Signed-off-by: Yadunund <yadunund@gmail.com>
Signed-off-by: Yadunund <yadunund@gmail.com>
@Yadunund Yadunund requested a review from aaronchongth January 27, 2025 21:13
Copy link
Collaborator

@aaronchongth aaronchongth 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! Left some small comments, let me know what you think

@@ -17,16 +17,16 @@

#include <nexus_transporter/Itinerary.hpp>

//==============================================================================
namespace nexus_transporter {
#include <stdexcept>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this might be left in accidentally

int32 action

# Additional parameters.
string params
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: missing EOF

int32 ACTION_DROPOFF=3

# The type of action to perform at the destination.
int32 action
Copy link
Collaborator

Choose a reason for hiding this comment

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

we probably won't need so many action types in the future, we can use an uint8 for this field


rclcpp::TimerBase::SharedPtr register_timer;


Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: extra line


RCLCPP_INFO(node->get_logger(), "Registering with system orchestrator...");
auto register_cb =
[this, node](rclcpp::Client<RegisterTransporter>::SharedFuture future)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should capture w_node instead, and lock it inside register_cb since this is an async request and node is expected to be released as _register returns before we get a response

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.

2 participants