search

osrf / rmf_core

Provides the centralized functions of RMF: scheduling, etc.
Apache License 2.0
102 stars 41 forks source link

Feature/dispatcher refactor #302

Open youliangtan opened 3 years ago

youliangtan commented 3 years ago
Yadunund commented 3 years ago

High level feedback: It seems like one of the fundamental goals of this PR is to populate the TaskSummary msg's task_profile field. This requires the rmf_fleet_adapter::Task object to store the TaskProfile msg for the corresponding request. But I feel approach here with creating a new rmf_task_ros2::Description object and using this to generate a rmf_task::Request and rmf_fleet_adapter::Task is overly complicating the implementation. For starters, for every new type of request/task to support, users have to implement a rmf_task::Request::Description object and the newly introduced rmf_task_ros2::Description object (which shouldn't really be defined in rmf_task_ros2 as it is meant to be a public API but that is a different matter). My suggestion would be to first store all incoming TaskProfiles in a map inside internal_FleetUpdateHandle.hpp. Maybe 

std::unordered_map<std::string, rmf_task_msgs::msg::TaskProfile> task_profile_map;

Here we are mapping Request ids to their TaskProfile msgs. This map can be populated inside FleetUpdateHandle::dispatch_request_cb() Then you only have to make two modifications:

  1. Add set and get methods to rmf_fleet_adapter::Task for setting and getting the TaskProfile object
  2. Modify the signature of rmf_fleet_adapter::TaskManager::set_queue(~) to additionally accept the task_profile_map. So change to void set_queue(const std::vector<Assignment>& assignments, std::unordered_map<std::string, rmf_task_msgs::msg::TaskProfile> task_profile_map);. Inside the definition of this function, right before we add the task to the queue, we can call the set TaskProfile method and pass task_profile_map[assignment.request()->id()]. I think this is much simpler approach to achieving the end goal without all the modifications to FleetUpdateHandle, TaskManager and Task as seen in this PR.

Secondly, I assumed the refactor would shift a lot of the header files from rmf_task_ros2 to rmf_task but only the Evaluator.hpp was moved. Do you think its possible to have abstract interfaces of Dispatcher, Auctioneer and MinmalBidder into rmf_task and then only have their sample implementations (which use ROS 2) inside rmf_task_ros2? We should also move the implementations of Evaluator (LeastFleetDiffCostEvaluator, LeastFleetCostEvaluator, QuickestFinishEvaluator) which currently live inside Evaluator.hpp to rmf_task_ros2