-
Notifications
You must be signed in to change notification settings - Fork 23
Feature/Idle Robot Preferred Task Planning #129
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: kjchee <[email protected]>
Signed-off-by: kjchee <[email protected]>
Signed-off-by: kjchee <[email protected]>
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.
Overall I'm not convinced that putting a bias towards robots that are in an idle state is the most meaningful or effective strategy to maximize utilization of the robots. I suspect it might make more sense to use a bias (cost weight) that favors using robots with a higher expected battery level.
That being said, since the approach in this PR is simpler to implement, opt-in, and should be intuitive enough for system integrators to get a desirable behavior, I'm okay with merging this as a core feature. I've just left a few remarks where I think there are opportunities to improve the code.
/// busy robots can finish tasks sooner. Only the shortest finish time | ||
/// idle robots will be considered. | ||
/// | ||
enum class ExpansionPolicy |
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.
I'm not sure if enum is the right data structure to have here. There's a possibility that some policy options in the future may need additional information, or maybe multiple policy options can be active at once.
I would recommend just adding a new method to Options for
bool prioritize_idle_robots() const;
Options& prioritize_idle_robots(bool choice);
std::function<bool()> interrupter = nullptr, | ||
ConstRequestFactoryPtr finishing_request = nullptr); | ||
ConstRequestFactoryPtr finishing_request = nullptr, | ||
ExpansionPolicy expansion_policy = ExpansionPolicy::ShortestFinishTime); |
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.
I suggest leaving the constructor as-is and just having a method to set prioritize_idle_robots
after constructing this class.
|
||
// is this index an initially-idle robot and now still has no non-charging assignment? | ||
std::vector<bool> idle_and_unassigned(initial_states.size(), false); | ||
for (std::size_t i = 0; i < initial_states.size(); ++i) |
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.
Style nitpick: always use { ... }
around a for-block.
// is this index an initially-idle robot and now still has no non-charging assignment? | ||
std::vector<bool> idle_and_unassigned(initial_states.size(), false); | ||
for (std::size_t i = 0; i < initial_states.size(); ++i) | ||
idle_and_unassigned[i] = |
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.
Maybe I'm overlooking something, but I feel like it would be simpler to do
std::unordered_set<std::size_t> idle_and_unassigned;
for (std::size_t i = 0; i < initial_sates.size(); ++i)
{
if (initial_states[i].is_idle() && !has_non_charging(parent->assigned_tasks[i])
{
idle_and_unassigned.insert(i);
}
}
const bool any_idle_still_unassigned = !idle_and_unassigned.empty();
Then later you can use idle_and_unassigned
as a list of which robots should be preferred.
Grey, you are referring to using cost system to select the 'best candidate' for the next rmf_task/rmf_task/src/rmf_task/TaskPlanner.cpp Line 1024 in b765a66
And the cost calculation is based on what user prefer, eg. he prefers higher battery level + idle state robot, then inside the cost calculation, there is some equation like:
|
And at the fleet_config, maybe SI just need to set the task_assignment_strategy to idle+higher_battery or idle+fastest or default (fastest)... Then, inside rmf core, we will auto assign each of the w1,w2,w3 to the strategy they opted? So that, user/SI do not need to touch/set the weightage w1,w2,w3... |
That's right.
Also correct.
I'm a little ambivalent about this detail. I can see good arguments for not allowing the SI to set the weights directly, but I can also see sound arguments for why they should be able to, since different applications may care about these different factors to varying degrees. If we were to go this route, I would probably allow the weights to be set by the SI, but also provide a catalog of profiles that have preset weight values. |
sure, flexible enough for all type of SI/deployments.
Regarding this draft cost calculation idea, would it be ok to proceed with this direction? Do you have any feedback or suggestions for improvement? |
If you're okay with taking that approach, I'm happy to support it via code review.
I think you've captured the idea quite well in your formula. A few additional thoughts that come to mind:
|
yes, i will proceed and update with new commits, thanks for your time for reviewing!
ok, i understand this point.
ok, if i understand correctly, for the example of batteryPenalty:
Then,
So to achieve this outcome. |
More specifically 3 seconds of robot time per fraction of battery spent. I agree with your representation of
I might recommend reframing With an assigned task count as the |
got it, this is shaping up to be a more complete solution, will start working on it |
Signed-off-by: kjchee <[email protected]>
@mxgrey, this PR is ready for review, thanks! I put the newly proposed config at open-rmf/rmf_demos#332. Also, i've updated the rmf_ros2 portion in the original rmf_ros2 PR open-rmf/rmf_ros2#466. I've tested the following with rmf_demos Office:
Also, to check on the actual cost, can uncomment some std::cout in |
New feature implementation
Implemented feature
This PR is related to feature proposal: #127
This update introduces a new parameter in the task planner called ExpansionPolicy. It defines the policy used when the task planner assigns unassigned-task during node expansion. Currently, the available options are:
When IdlePreferred is selected, the planner prioritizes assigning tasks to idle robots (robot that are not executing any task and currently still unassigned during node expansion) during node expansion, rather than solely picking the robot with the shortest estimated finish time. However, if all robot are idle or all robot are busy, then it falls back to the default strategy of selecting the best candidate based on shortest estimated finish time.
This ExpansionOption can be extended in the future to support additional strategies, eg. IdlePreferredHighBattery (preferred idled robot with highest soc) and etc...
Implementation description
rmf_task::TaskPlanner::Options
called expansion_policy, it is updated in FleetUpdateHandle.cpp (via fleet_config.yaml), and passed into Task Planner.expand_shortest_finish_time_robots()
expand_fastest_idle_robots()
rmf_task::TaskPlanner::ExpansionPolicy expansion_policy
GenAI Use
We follow OSRA's policy on GenAI tools
Generated-by: