-
Notifications
You must be signed in to change notification settings - Fork 55
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
Split itinerary and participant updates #17
Conversation
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
…rint_update' into split-itinerary-and-participant-updates
…to split-itinerary-and-participant-updates
…cipant descriptions Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
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 is looking really good. I just have a few relatively minor nitpicks/suggestions/observations.
|
||
if (wait > rmf_traffic::Duration(0)) | ||
future.wait_for(wait); | ||
void request_update(uint64_t minimum_version=0) |
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.
We should probably change this argument to std::optional<uint64_t> minimum_version = std::nullopt
, and change the RequestChanges
message to have a bool full_update
field. If update versions ever overflow (which would admittedly take a very long time with 64 bits), using 0 as the "oldest possible version" won't necessarily work.
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.
Fixed in b2643b5.
rmf_traffic_ros2/src/rmf_traffic_ros2/schedule/convert_Patch.cpp
Outdated
Show resolved
Hide resolved
rmf_traffic_ros2/src/rmf_traffic_ros2/schedule/convert_Patch.cpp
Outdated
Show resolved
Hide resolved
rmf_traffic_ros2/src/rmf_traffic_ros2/schedule/convert_ParticipantDescription.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
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.
Just one recommendation about the tuple.
rmf_traffic_ros2/src/rmf_traffic_ros2/schedule/internal_Node.hpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
…b.com/open-rmf/rmf_ros2 into split-itinerary-and-participant-updates
…atible Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
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.
The changes seem to be working 👍
New feature implementation
Closes open-rmf/rmf#8.
Implemented feature
Reworks synchronisation between a database and mirrors so that participant information is sent separately from itinerary/route information. This enables scalable synchronisation that is robust to the timing of a mirror joining a query.
This PR is part of a set of three, all of which are needed for this feature.
This PR depends on the PRs that change mirror synchronisation to use a topic, and the PRs that enable updating participant footprints.
Implementation description
A set of new messages have been created for sending participant descriptions via a topic.
The schedule node publishes a topic,
/rmf_traffic/participants
, which broadcasts all participant information when certain events occur:When a mirror receives the set of participants, it refreshes its own list to match.
To account for timing differences in receiving the list of participants and receiving the list of routes, which could mean that routes are received for participants not yet known about, a semi-caching mechanism is added. If a route is received that does not yet have a known participant, it is stored with an empty participant description. When views and snapshots are taken of a timeline, any participants/routes with empty descriptions are ignored (i.e. not given to the view).
When a mirror receives a set of participants, if it finds that a participant already exists it updates the description of that participant to match the newly-received description. A double pointer is used to store descriptions, ensuring that all places referencing the participant's description will receive the updated description immediately. To avoid snapshots having their descriptions changed (snapshots are meant to be immutable), they make a fresh copy of all descriptions when the snapshot is made.
How to test
Launch the office demo world and start a couple of tasks. Rviz should show the visualisation of the scheduled routes. Kill the RMF rviz2 node that is publishing this visualisation; the visualisation will disappear from rviz. Start the RMF rviz2 node again manually, and the schedule visualisation should appear shortly.
Notes for the reviewer
There is a lot of participant updating happening in the
rmf_traffic_schedule
node. If comments are added to theMirror::update_participants_info()
function, then this sort of output will be seen several times per second:I am not sure why this is happening and if it should be considered a problem or not.