-
Notifications
You must be signed in to change notification settings - Fork 65
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
base: main
Are you sure you want to change the base?
Conversation
…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>
7707833
to
2112468
Compare
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 |
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.
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 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:
As a minimum improvement I would recommend the following changes:
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. |
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 theTaskPlanner
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
TaskManager
uses the expected finish state of the robot's last task by callingTaskManager::expected_finish_state()
.TaskManager
uses the theTaskPlanner
to get the end state of the robot after completing the taskFleetAdapter
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 code21
here as it was one after the highest number I could find. Some help on this would be appreciated.