Develop

[aaronchongth]

This PR introduces a complete overhaul and redesign of free_fleet. This attempts to create multiple layers of abstractions that would accommodate different software configurations, mobile platforms and UIs, in order to be more widely used as a generic fleet management system.

The objective of this PR while development is still underway, is to have the API reviewed and modified if needed, such that further developments that use these new abstractions can be adapted without too many breaking changes.

Inspired and modeled after the fleet adapters from the RMF project, we have added these abstractions,

• Middleware - Split between ClientMiddleware and ServerMiddleware, allows communication between the fleet manager and the mobile platform client. With this in place, users can implement and use their preferred middleware, for example ROS, ROS 2, different implementations of DDS, REST, MQTT or other web APIs.

• Command Handle - This is used by the client to control the mobile platform, users will be able to implement this to interact with their preferred navigation stack or robot controlling API.

• Status Handle - This is used by the client to gather information in order to update the fleet management system. The implementation will also be dependent on the mobile platform's API.

Other features:

• Added a generalized console logging feature, which can be used throughout free_fleet, https://github.com/open-rmf/free_fleet/blob/develop/free_fleet/include/free_fleet/Console.hpp, will consider using spdlog for future iterations, Replace logging system with spdlog #101.

• Added basic Executor and Worker pattern for both the clients and managers

• No dependency on ROS or ROS2, instead, any relevant implementations will be ported to free_fleet_ros1 and free_fleet_ros2, both which are under development.

• This repository contains the package free_fleet_cyclonedds, which is a ready-to-use implementation of Middleware using CycloneDDS, release 0.8.0beta5, the same release version on ROS2 Galactic.

• Message definitions have been pimpl-ized, https://github.com/open-rmf/free_fleet/tree/develop/free_fleet/src/messages

Also fixes:

1. Reactive message subscriptions - Message subscriptions to be made reactive instead of polling #88
2. Messages are generated during build - Generate message source files from IDL during build #90
3. Code style linting - Add style linting #100

[mxgrey]

I think this PR is taking the free_fleet in a really great direction.

Based on ongoing discussions about "What is Free Fleet?", I anticipate we'll want another very significant round of redesign on top of this one, so let's keep the version number in alpha/beta range at 0.1.0 or 0.2.0 so we're not over-committing to the current API.

In addition to the in-line feedback, I wonder if we should reconsider the use of namespaces. I'm not sure agv is actually a very meaningful namespace for this library. We might want to instead group classes into namespace based on which component in the pipeline they are used by, e.g. free_fleet::server:: and free_fleet::client.

[aaronchongth]

@mxgrey, thanks for the review so far! I have a much better understanding of how a reactive system can be useful in this scenario now, as well as how working towards a feature-based system would be great.

I haven't given much thought about the use of namespaces, other than using them as a way to keep things within categories or subsets (i.e. agv, transport, requests), I'll look into getting them towards component pipeline based as you mentioned. I'll develop these locally for now.

I've addressed most of the design concerns and got to implement what we have discussed previously regarding requests, I'd be keen to get your review again before any new commits. Let me know what you think.

[mxgrey]

The architecture is generally looking really good!

I'm leaving a final round of pedantic suggestions to tidy up the implementation before merging, but they'll mostly be minor changes.

[mxgrey]

I'm about halfway through reviewing the code, and this new design is looking really great.

Most of my comments so far are pretty minor nitpicks, although the issues I've raised about capturing this are important to avoid race conditions and/or use-after-free bugs.

[mxgrey]

Let's change the documentation to say "If you robot has no docking abilities, implement this by simply triggering docking_finished_callback() immediately". If the function is implemented as a no-op, then the fleet manager could get stuck forever waiting for the docking command to finish.

On the other hand, for error handling it might be good if there's a way to indicate whether the docking procedure is recognized by the client. Maybe we should rework this function so that it returns a boolean: true if the docking command is recognized, and false if it's not recognized. If the function returns true, then the fleet manager will wait for the docking_finished_callback to be triggered; if it returns false, then the fleet manager should log an error/warning and not bother waiting for the docking_finished_callback to be triggered.

[aaronchongth]

That makes sense, thanks! e58cc91

[mxgrey]

I'm not sure it's obvious what "Forward" or "Backward" mean in this context. Since this interface is specific to free_fleet, why don't we say from_fleet_to_robot(~) and from_robot_to_fleet(~)?

[mxgrey]

Follow-up: I see that later on we give names like to_robot to the CoordinateTransformer object. That does help alleviate my concern above. I'm just a little bit worried that encoding the direction of the transformation in the variable name could allow the meaning to get accidentally lost, e.g. when passing the CoordinateTransformer to a function. For example, the compiler wouldn't stop this from happening:

void do_some_transformation(const CoordinateTransformer& to_fleet)
{
  ...
}

void some_other_function(const CoordinateTransformer& to_robot)
{
  do_some_transformation(to_robot);
  ...
}

I'm not sure if this is worth being concerned about, so I'll leave it up to however you feel about the points I've raised to decide whether we should (1) keep things the way they are, or (2) encode the meaning of the transformation directions into the class definition.

[mxgrey]

When the TrackingState value is Lost the std::size_t component of the tracking estimation doesn't have any meaning, right?

In that case, I'd recommend the following:

enum class TrackingMode : uint8_t
{
  OnWaypoint,
  OnLane
};

struct TrackingState
{
  TrackingMode mode;
  std::size_t waypoint;
};

std::optional<TrackingState> tracking_estimation() const;

When the robot is lost, tracking_estimation would return a std::nullopt.

[mxgrey]

Similar to the previous comment, capturing this in a lambda that will be run in some other scope at an arbitrary time is unsafe, because you can't be sure that this will still be alive when the lambda is triggered. This should also be refactored to use a std::shared_ptr or std::weak_ptr of the shared data structure.

[aaronchongth]

Moved fields into a separate data structure, and passed shared pointers to lambdas, e8bec64

[mxgrey]

For each robot, any new new task (really, command) should supersede previous tasks (really, commands). But in the current implementation, the robot will be forced to acknowledge every single request that gets sent.

As an alternative approach, I might recommend mapping unacknowledged tasks/commands with the robot ID as the key, and only keeping up to one unacknowledged task at a time (the one with the highest task ID value) per robot. I'd also recommend wrapping that check-and-overwrite into a simple member function that each of these request_... functions can easily call.

[mxgrey]

I've finished the review now and only have a few very small nitpicks to add. It's looking really great; thanks for all the massive iterating you've done 👍