Skip to content
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

Feature/lkw303/task estimation #408

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lkw303
Copy link
Contributor

@lkw303 lkw303 commented Mar 5, 2025

New feature implementation

Implemented feature

This pull request provides users a way to get estimated end states of the robot after carrying out a task. This can be useful for users who are making plans for tasks across multiple robots and would need to get an estimate for the duration of a task as well as the robot's remaining battery percentage after the task is completed.

This pull request depends on this pull request from rmf_api_msgs

Implementation description

The task estimation feature is implemented in the TaskManager and makes use of the TaskPlanner to estimate the end state of the robot after completing a task and returns this information to the user or the program which made the query.

How it works

  1. The user provides a task and start state of the robot in the task estimate request.
    • If no start state is provided, the TaskManager uses the expected finish state of the robot's last task by calling TaskManager::expected_finish_state().
  2. TaskManager uses the the TaskPlanner to get the end state of the robot after completing the task
  3. FleetAdapter returns the estimated result to the user.

Other notes

I would need some help on the return of the correct error codes response for the TaskManager as I am not sure where I can find the documentation on it. Currently I have just used the error code 21 here as it was one after the highest number I could find. Some help on this would be appreciated.

lkw303 and others added 3 commits March 11, 2025 14:07
…ap_commercial/cag/rmf_ros2!2)

* change repos file branch

Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>

* remove empty line

Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>

* refactored to remove redundant property in from json request

Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>

* Use modified RMF API messages

Signed-off-by: Chen Bainian <chenbn@artc.a-star.edu.sg>

* fix code style

Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>

* change duration to be in milliseconds

Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>

* validate schemas for tasks estimate requests and responses

Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>

* Check if waypoint exists on navigation graph.

Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>

* Add functionality for estimating using start state from request

Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>

* Add initial implementation for task estimate interface

Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>

Co-authored-by: Chen Bainian <chenbn@artc.a-star.edu.sg>
Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>
Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>
Signed-off-by: Kai Wen Lum <lum_kai_wen@artc.a-star.edu.sg>
@lkw303 lkw303 force-pushed the feature/lkw303/task-estimation branch from 7707833 to 2112468 Compare March 11, 2025 06:15
@lkw303
Copy link
Contributor Author

lkw303 commented Mar 11, 2025

Hi! Can I get some advice on how I could get the CI working for this pull request, which depends on another pull request from the rmf_api_msgs repository? Since the CI for this repo uses the the packages from the main branch the new generated schema hpp file isn't available when the CI for this repo is building.

@mxgrey
Copy link
Contributor

mxgrey commented Mar 11, 2025

Currently I have just used the error code 21 here as it was one after the highest number I could find.

That should be fine. We definitely need a document to standardize these integer values or else choose a different way to concretely identify errors for the next generation. Since OSRA is working on a new cross-project REP process, we can use that once it's ready.

how I could get the CI working for this pull request, which depends on another pull request from the rmf_api_msgs repository

For now we can review both PRs as one and ignore the CI failures, as long as the tests all pass locally. Once the changes are approved overall, we can first merge the rmf_api_msgs PR and then rerun the CI for this PR.

One thought that jumps out to me is I wonder if there's overlap between this and the dry run concept that was recently introduced. It's certainly not redundant since this PR introduces the ability to get estimation data back, which goes beyond the dry run feature. But it stands out to me that this PR generates a state estimate with the assumption that the new task would just be assigned to a specific robot from a user-specified initial state. On the other hand the dry run feature examines what would happen if the task was dispatched to the fleet, meaning the task could be assigned to any robot, and the tasks can be reordered to optimize their completion.

I have a few concerns around the applicability of this implementation. If you're willing to share some details about how this feature fits into a broader system (e.g. how are you using this new API to make decisions at a higher level) that may help me understand the value of this current approach, but in the meantime here are my concerns:

  1. In the PR's current implementation, the user needs to define the initial state inside the request. I understand the value of this since an external task planner may want to speculate about different possible initial states, but I think in many cases someone using this feature would want to use the finish state of already assigned tasks as the initial state for the estimate, and it doesn't look like there's a decent way to do that right now.
  2. The user gets to see what happens when exactly one task is introduced to a specific robot. Getting estimates for an entire queue of tasks would require repeated calls to this API, requiring numerous roundtrips.
  3. This doesn't give useful information about what would happen if a task is dispatched instead of directly assigned to a robot. I suspect your use case isn't using the dispatch system, but then we need to make it clear somehow that this estimation feature is only meant to be used with direct robot task assignments.

As a minimum improvement I would recommend the following changes:

  • Instead of request this field should be requests and should be an array of task requests. Then the task manager can run through the sequence as if each task request was issued in the sequence they appear in the array and produce an estimate each step of the way. A user can always just have one element in the array if they only care about estimating for one task request.
  • We can get rid of the task_estimate_request.request schema and just use task_request.json directly as the schema for each element inside requests. The "state": property can instead be an "initial_state": field that exists parallel to "requests":.
  • The initial_state field is optional, and if it is excluded by the user then we generate the initial state for the estimate based on the current direct task assignment queue of the robot.
  • result should be an array of task_estimate_result. Each element corresponds to the request of the same index from the requests field.
  • Let's rename the schema to make it explicit that this is for direct task request estimates, e.g. change the name task_estimate_request to estimate_robot_task_request. That leaves us some room to introduce an estimate_dispatch_task_request schema in the future for estimating what would happen when dispatching the task requests instead of directly assigning them.
  • Similarly task_estimate_response should be estimate_robot_task_response. task_estimate_result could be named estimate_task_result if we think it can be used as-is for both direct task requests and dispatch task requests.

Let me know what you think of these suggestions. If there's additional context you can provide about how this feature is being used, I'll be happy to take that into account for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants