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

Publish fleet and task updates over ROS 2 if websocket is not provided #383

Merged
merged 12 commits into from
Oct 4, 2024

Conversation

aaronchongth
Copy link
Member

@aaronchongth aaronchongth commented Aug 22, 2024

New feature implementation

Implemented feature

Allows fleet_state_update, fleet_log_update, task_state_update, and task_log_update to be published over ROS 2 when websocket server URL is not provided.

Works with open-rmf/rmf-web#1003

Implementation description

The motivation is related to open-rmf/rmf-web#899, where non-reproducible issues with websockets occur unpredictably in production environments, causing various systems to fail.

This allows the option to mitigate issues with the upstream websocketpp library, and just rely on ROS 2 to publish updates instead.

This will also allow the use of ROS 2 introspection tools in production if network instability occurs, which hopefully is a much better experience than debugging websocket connections.

Testing

Instead of running demos with the server_uri, just run

ros2 run rmf_demos_gz office.launch.xml #headless:=1

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@koonpeng
Copy link

koonpeng commented Aug 23, 2024

Can it publish regardless if websocket is enabled?

Connection issues aside, I feel that the split of multiple transport is causing unnecessary complexities as well. websockets is the "public" api but it doesn't expose many important information (like door, lifts, maps etc), the ros2 side is "internal", does expose them but it doesn't expose task states etc. So in practice, it is very likely for downstream to have to listen to both transports and deal with 2 different message schema format.

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth
Copy link
Member Author

As per discussion, publishing over ROS 2 regardless of websocket availability makes sense

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
koonpeng
koonpeng previously approved these changes Sep 24, 2024
@@ -294,9 +300,15 @@ class Dispatcher::Implementation
this->handle_dispatch_ack(*msg);
});

task_state_update_pub = node->create_publisher<TaskStateUpdateMsg>(
rmf_task_ros2::TaskStateUpdateTopicName,
rclcpp::ServicesQoS().keep_last(10).reliable().transient_local());

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for spotting that, I've corrected them to be the same 07b9c6a

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth aaronchongth enabled auto-merge (squash) October 2, 2024 16:54
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Just one very minor recommendation on QoS history length for log topics, and then I'll be happy to approve.

@@ -192,6 +192,15 @@ TaskManagerPtr TaskManager::make(
self->_handle_request(request->json_msg, request->request_id);
});

auto reliable_transient_qos =
rclcpp::ServicesQoS().keep_last(10).reliable().transient_local();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is going to be used for logs which could experience sudden bursts that we don't want to miss, I would suggest having at least .keep_last(100) for logs. But since task states always send out the latest state, replacing whatever was sent before, 10 would be sufficient.

Maybe we should have a different history value for each.

FleetStateUpdateTopicName, reliable_transient_qos);
handle->_pimpl->fleet_log_update_pub =
handle->_pimpl->node->create_publisher<FleetLogUpdateMsg>(
FleetLogUpdateTopicName, reliable_transient_qos);
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend using 100 history for this log topic as well.

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Looks good!

@aaronchongth aaronchongth merged commit e4e82e1 into main Oct 4, 2024
1 of 6 checks passed
@aaronchongth aaronchongth deleted the feat/updates-over-ros2 branch October 4, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants