Feature/finishing task

[Yadunund]

This PR introduces two features:

1. Fix Distinguish between requested tasks and automatic tasks rmf_ros2#36 Adds a bool automatic parameter to Request to discern requests that are user and auto generated
2. Fix Configure task planner to always append a desired task at the end of the plan #27 . A new pure abstract RequestFactory is introduced with a make_reqeust() function that returns a ConstReqeustPtr. This RequestFactory can be optionally passed into TaskPlanner::optimal_plan(). Once the planner finds an solution for the set of requests, it will append the assignment for the RequestFactory->make_request() at the end. Requests generated by the factory must have automatic == true so that they are not confused with user submitted ones. This will help downstream fleet adapters ignore such requests when replanning.

When the reservation system is completed, we can add a NavigateToReservedSpot factory that will always make an agent navigate to its assigned spot.

Update:
3. Add TaskPlanner::Options class that bundles greedy, interrupter and finishing_request objects.
4. TaskPlanner::optimal_plan() and TaskPlanner::greedy_plan() replaced by TaskPlanner::plan() with an overload that accepts non-default Options.
5. Renamed ReturnToChagerFactory to ParkRobotFactory

[codecov]

Codecov Report

Merging #28 (0353e21) into main (8349287) will decrease coverage by 0.07%.
The diff coverage is 28.09%.

@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
- Coverage   38.90%   38.82%   -0.08%     
==========================================
  Files          27       29       +2     
  Lines        1501     1656     +155     
  Branches      905     1002      +97     
==========================================
+ Hits          584      643      +59     
- Misses        221      248      +27     
- Partials      696      765      +69     

Flag	Coverage Δ
tests	38.82% <28.09%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
rmf_task/include/rmf_task/Estimate.hpp	50.00% <ø> (-50.00%)	⬇️
rmf_task/include/rmf_task/Request.hpp	100.00% <ø> (ø)
rmf_task/include/rmf_task/agv/Constraints.hpp	0.00% <ø> (ø)
rmf_task/include/rmf_task/agv/Parameters.hpp	0.00% <ø> (ø)
rmf_task/include/rmf_task/agv/State.hpp	0.00% <ø> (ø)
rmf_task/include/rmf_task/agv/TaskPlanner.hpp	50.00% <0.00%> (-16.67%)	⬇️
rmf_task/src/rmf_task/requests/Clean.cpp	0.00% <0.00%> (ø)
rmf_task/test/unit/agv/test_TaskPlanner.cpp	25.18% <18.80%> (+0.92%)	⬆️
rmf_task/src/rmf_task/agv/TaskPlanner.cpp	43.75% <30.15%> (-3.02%)	⬇️
...mf_task/src/rmf_task/requests/ParkRobotFactory.cpp	55.00% <55.00%> (ø)
... and 10 more

[mxgrey]

The approach and implementation all looks good! I've just left a few comments about places where documentation can be improved or filled in.

[Yadunund]

@mxgrey I've added documentation for the classes introduced here and also for the existing API where lacking.

I just noticed that the implementations ChargeBatteryFactory and ReturnToChargerFactory are included inside rmf_task/requests/factory/ while the namespacing is simply rmf_task::requests::. is this ok or should I update the namespacing to rmf_task::requests::factory?

Secondly, should we replace TaskPlanner::greedy_plan() and TaskPlanner::optimal_plan() with a single TaskPlanner::plan() method that accepts a greedy boolean parameter? I feel this would clean up the API. Let me know if this makes sense and whether to make this change in this PR or a separate one.

[mxgrey]

I just noticed that the implementations ChargeBatteryFactory and ReturnToChargerFactory are included inside rmf_task/requests/factory/ while the namespacing is simply rmf_task::requests::. is this ok or should I update the namespacing to rmf_task::requests::factory?

I think it's a good idea to have the namespacing match the file system, but in this case I'd recommend moving the implementations of those classes into rmf_task/requests/ rather than adding a factory namespace, since the classes already have the word Factory in their names. Alternatively we could name the classes rmf_task::requests::factory::ChargeBattery, but that might create confusion if we ever expose a rmf_task::requests::ChargeBattery class to the public API.

Secondly, should we replace TaskPlanner::greedy_plan() and TaskPlanner::optimal_plan() with a single TaskPlanner::plan() method that accepts a greedy boolean parameter?

Let's provide some kind of TaskPlanner::Options PIMPL class that contains a bool greedy() field. Then whenever we want to offer more options for the planner, we add it to that class instead of changing the function signature (making the API+ABI more stable).

[Yadunund]

Let's provide some kind of TaskPlanner::Options PIMPL class that contains a bool greedy() field.

Tiny clarification: currently the fleet adapter calls the optimal planner on the main thread. In the future we might want to call the greedy planner on the main thread to quickly respond to bids and have the optimal planner run on a different thread. The results from the optimal planner will be used to update assignments if the bid is awarded. I could be wrong here, but would changing the greedy value in the Options class in one thread, affect the other planner running in the other thready?

[mxgrey]

I could be wrong here, but would changing the greedy value in the Options class in one thread, affect the other planner running in the other thready?

This is something that we need to make sure to implement correctly. But if we implement it following the model of rmf_traffic::agv::Planner then this not an issue at all.

To be clear, each Options instance will belong to each separate call to the TaskPlanner::plan(~) function in the exact same way that each bool greedy instance would belong to each separate call to TaskPlanner::plan(~) if we were to add a bool greedy parameter. The purpose of the Options class is simply to bundle up any additional parameters that are needed by the plan(~) function into a class that provides API+ABI stability so that we can add more parameters at any time in the future without any harm to the API or ABI. If you're familiar with kwargs in Python, it's roughly similar to that.

[Yadunund]

The purpose of the Options class is simply to bundle up any additional parameters that are needed by the plan(~) function into a class that provides API+ABI stability so that we can add more parameters at any time in the future without any harm to the API or ABI.

ooh so you're suggesting to modify the signature of TaskPlanner::plan() to accept a TaskPlanner::Options object? I originally assumed you meant to modify the constructor of TaskPlanner to take in TaskPlanner::Options similar to the planner in rmf_traffic

[mxgrey]

Right, but notice that the constructor of rmf_traffic::agv::Planner is taking in a parameter for the default options, meaning those options will only be used when someone calls Planner::plan without providing any options. Anytime someone calls Planner::plan while specifying some options, those specified options will be used instead of the default. Each call to Planner::plan is completely independent from each other.

Whether or not you want the constructor of the TaskPlanner class to take in a default option set is a separate design question.

[Yadunund]

Aah that's cool! Will modify accordingly.

[Yadunund]

Does also moving the interrupter and finishing_request into the TaskPlanner::Options class make sense?

[mxgrey]

Yeah, that's a great idea. Bundling all of those into Options makes perfect sense 👍

[Yadunund]

@mxgrey I've added TaskPlanner::Options class that presently accepts greedy, interrupter and finishing_request parameters.
optimal_plan and greedy_plan have been replaced with plan(). There is an overloaded plan() which accepts Options to override the default ones.

[mxgrey]

This looks great; thanks for iterating!

We should wait until open-rmf/rmf_ros2#108 is fully settled and approved before we merge this one, since the API breakages will have a cascading effect if there's a big gap between merging the PRs.