Skip to content
This repository has been archived by the owner on Jul 22, 2021. It is now read-only.

Feature/valet: Reservation system for RMF #297

Closed
wants to merge 24 commits into from
Closed

Conversation

arjo129
Copy link
Member

@arjo129 arjo129 commented Feb 16, 2021

This is the initial API and code for reserving locations. Eventually this code will be used for reserving resources like chargers and parking slots. The basic api can be found in rmf_traffic/include/reservations/reservation.hpp.

Additionally, for ros2 based systems a node and messages are provided in the rmf_traffic_ros2 package.

The core data structure used is an std::map which is used to sort the reservations by starting time. Upon a reservation being requested, the rmf_traffic::reservations::Implementation::is_free function is used to check if that specific reservation is available.

  • For finite length reservations. It performs a simple binary search to see if the starting time and ending time fall within the same time gap. It then checks the previous reservation (if it exists) to see if there is overlap. Since we can assume the previous schedule is consistent, the current schedule should also be consistent. Time complexity for this is O(logn).

  • For indefinite reservations we just look at the reservation with the last starting time in the map and compare if its end time is before or equal to the start time of the incoming reservation. Since we can assume the map contains a consistent schedule, therefore the item with the last starting time will also have the last ending time. This provides an O(1) lookup time.

Unit tests for the API are also provided in this PR.

Several follow-up PRs need to be added. The first PR will be related to parking. This PR will involve a small modification to the planner and loop requests so that the planner will allow for a certain waiting time. The second PR will add persistence to the reservation node so that it can be gracefully restarted.

@arjo129 arjo129 changed the title Feature/valet Feature/valet: Reservation system for RMF Feb 16, 2021
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 marked this pull request as ready for review February 24, 2021 05:15
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Copy link
Member

@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.

I think this is a great start, but I have a lot of feature and design ideas that I'm going to need to outline somewhere else and share with you when it's ready. I've also left some immediate feedback down below.

I think we'll need to hold off on merging this until after the migration since it will take considerable time to develop the features that I have in mind. Luckily this PR almost exclusively touches on new files, so there should be no issue with just manually copying them over into the new repo.

ReservationId reservation_id() const;

/// Get the reservation way point
const std::string waypoint() const;
Copy link
Member

Choose a reason for hiding this comment

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

Since this is aimed at being a general reservation system, we should probably call this a resource() instead of a waypoint().

/// otherwise returns a Reservation object.
std::optional<Reservation> reserve(
const rmf_traffic::Time time,
const std::vector<std::string>& vertices,
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this resources instead of vertices.

/// Do not use this to construct reservations unless you are deseriallizing/
/// Serializing resolutions. If you wish to make a reservation see
/// the \ref{ReservationSystem} class.
Reservation(
Copy link
Member

Choose a reason for hiding this comment

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

To ensure forward ABI compatibility, we should only define a default constructor and then we should take care of initializing the fields internally.

You can use the rmf_traffic::agv::Plan::Waypoint class as an example for how to do this.

@@ -0,0 +1,96 @@
/*
* Copyright (C) 2020 Open Source Robotics Foundation
Copy link
Member

Choose a reason for hiding this comment

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

I think these licenses should probably use 2021 at this point.

std::unordered_map<ReservationId,
std::string> _location_by_reservation_id;

std::mutex _mutex;
Copy link
Member

Choose a reason for hiding this comment

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

Let's not use a mutex in this class. Mutexes can add potentially unnecessary overhead.

The classes in rmf_traffic offer no guarantees about being simultaneous read/write thread-safe. Managing threads and simultaneous access is considered the responsibility of the user of rmf_traffic, not the responsibility of rmf_traffic itself.

The only time we'd want to use a mutex in rmf_traffic is if we're doing memoization for a const-qualified function and we need to modify the internal cache when it may have multiple simultaneous readers.


//==============================================================================
/// Manages reservations of waypoints within the class
class ReservationSystem
Copy link
Member

Choose a reason for hiding this comment

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

To align better with rmf_traffic::schedule::Database we could consider naming this class something like rmf_traffic::reservations::Database.

*/

#ifndef RMF_TRAFFIC__RESERVATIONS_HPP
#define RMF_TRAFFIC__RESERVATIONS_HPP
Copy link
Member

Choose a reason for hiding this comment

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

Let's name this header after the class that it's responsible for defining rather than just reservations.hpp. Alternatively we could consider just making the header rmf_traffic/reservations.hpp if we feel certain that the whole reservation system is best to hold entirely in one header.

@mxgrey
Copy link
Member

mxgrey commented Feb 25, 2021

I've created a Google Doc with an outline of the kinds of features and behaviors that I think we'll need out of the reservation system. The hope here is to make the reservation system more like the traffic system, where it can automatically adjust to the constantly changing conditions out in the field.

You'll find that the requested features and behaviors are going to add a lot of design and implementation complexity, so it's going to demand quite a bit more work for this PR.

Ultimately I think we'll want to have these following classes, each defined in their own header:

  • rmf_traffic::reservations::Viewer: Pure abstract interface that allows reservations to be inspected, but not modified
  • rmf_traffic::reservations::Writer: Pure abstract interface that allows reservations to be requested
  • rmf_traffic::reservations::Database: Implementation for both Viewer and Writer. This will be the backbone of the reservation node.
  • rmf_traffic::reservations::[Reservation|Ticket|Handle|Participant] I'm not exactly sure what to name this one, but it should be the reservation system's equivalent of the traffic schedule's Participant class.
  • rmf_traffic::reservations::Snapshot: The reservation system equivalent of the rmf_traffic::schedule::Snapshot class.
  • rmf_traffic_ros2::reservations::Node: Acts as the central authority over reservations, similar to rmf_traffic_ros2::schedule::Node and rmf_traffic_ros2::blockade::Node. The topics that this class listens to should be parameterized so that it can be used for multiple different reservation systems.
  • rmf_traffic_ros2::reservations::Mirror: The reservation system equivalent of the rmf_traffic_ros2::schedule::Mirror. This allows ROS2 nodes to inspect the current reservations. It provides a snapshot feature so you can get a frozen snapshot of the current state of reservations at any time. The topics that this class listens to should be parameterized so that it can be used for multiple different reservation systems.
  • rmf_traffic_ros2::reservations::Writer: The reservation system equivalent of the rmf_traffic_ros2::schedule::Writer. This allows ROS2 nodes to make reservation requests. It returns rmf_traffic::reservations::Reservation objects asynchronously, after receiving a reply from the rmf_traffic::reservations::Node. The topics that this class listens to should be parameterized so that it can be used for multiple different reservation systems.

@arjo129
Copy link
Member Author

arjo129 commented Feb 25, 2021 via email

@arjo129
Copy link
Member Author

arjo129 commented Mar 4, 2021

Moving to new repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants