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

Flexible task definitions #39

Merged
merged 88 commits into from
Feb 13, 2022
Merged

Flexible task definitions #39

merged 88 commits into from
Feb 13, 2022

Conversation

mxgrey
Copy link
Contributor

@mxgrey mxgrey commented Sep 3, 2021

This is a draft PR for prototyping and discussing the redesign that was initially brought here. As of right now most of the new classes are unimplemented, but I think it's worth discussing the proposed APIs sooner rather than later. I'm still very open to renaming or reorganizing the API names/structures. Since all the interfaces are still very abstract, it may be difficult to understand, but I'll attempt to have some example classes implemented early next week to make this proposal more clear.

Most of the changes are based on this discussion, but I'll highlight some aspects where this PR diverges a bit from what was originally proposed:

A Task is an object that generates phases

In the original discussion I proposed that a Task should be a sequence of phases. After some more thinking and conversations, I concluded that this is too rigid of a definition. Instead I'm proposing an API where a Task is an object that generates phases. The way a Task generates phases is completely up to the implementer. This allows more complex Task behavior than we were originally targeting, including the possibility of implementing behavior trees which was proposed by @aarushg22.

For developer convenience, we will be providing a Task implementation called rmf_task::sequence::Task which is a Task that is defined by a sequence of phases. This is what was originally proposed, but now it's simply one way to define a task, not the only way.

Planning is effectively the same

Besides some refactoring/renaming, all the APIs for task planning remain effectively the same. The new phase sequence task type has its own implementation of the Request::Description interface. Other task types can implement their own Request::Description interfaces with no regard for the phase sequence task type.

Tasks are generated by factories

The new class rmf_task::execute::TaskFactory is able to convert a rmf_task::Request into an active, running Task object. A phase sequence task will be generated by this factory whenever such a task is requested. Downstream developers can inject their own custom task types into this factory.

The new class rmf_task::sequence::PhaseFactory is able to convert from a phase description into an active, running phase for the new rmf_task::sequence::Task class. Downstream developers can inject their own custom phase types into this factory.

@codecov
Copy link

codecov bot commented Sep 3, 2021

Codecov Report

Merging #39 (cc8bf3d) into main (a548bac) will decrease coverage by 3.60%.
The diff coverage is 29.56%.

❗ Current head cc8bf3d differs from pull request most recent head 956cf5b. Consider uploading reports for the commit 956cf5b to get more accurate results

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
- Coverage   38.76%   35.16%   -3.61%     
==========================================
  Files          29       64      +35     
  Lines        1656     2420     +764     
  Branches     1002     1322     +320     
==========================================
+ Hits          642      851     +209     
- Misses        248      567     +319     
- Partials      766     1002     +236     
Flag Coverage Δ
tests 35.16% <29.56%> (-3.61%) ⬇️

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

Impacted Files Coverage Δ
rmf_task/include/rmf_task/CompositeData.hpp 0.00% <0.00%> (ø)
rmf_task/include/rmf_task/Constraints.hpp 0.00% <ø> (ø)
rmf_task/include/rmf_task/Log.hpp 0.00% <0.00%> (ø)
rmf_task/include/rmf_task/Parameters.hpp 0.00% <ø> (ø)
rmf_task/include/rmf_task/Payload.hpp 0.00% <0.00%> (ø)
rmf_task/include/rmf_task/Request.hpp 100.00% <ø> (ø)
rmf_task/include/rmf_task/TaskPlanner.hpp 50.00% <ø> (ø)
rmf_task/include/rmf_task/VersionedString.hpp 0.00% <0.00%> (ø)
rmf_task/include/rmf_task/detail/Backup.hpp 0.00% <0.00%> (ø)
..._task/include/rmf_task/events/SimpleEventState.hpp 0.00% <0.00%> (ø)
... and 52 more

Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
@cnboonhan
Copy link
Contributor

I noticed that there is an Activator Class: https://github.com/open-rmf/rmf_task/blob/redesign_v2/rmf_task/include/rmf_task/Activator.hpp#L27

but also a Activator alias to rmf_task::Activator::Activate<Description>: https://github.com/open-rmf/rmf_task/blob/redesign_v2/rmf_task/test/mock/MockDelivery.hpp#L36

Would it be confusing?

Yadunund and others added 2 commits January 20, 2022 16:49
* Cherry pick diff for PerformAction from new_descriptions

Signed-off-by: Yadunund <[email protected]>

* Add missing utils

Signed-off-by: Yadunund <[email protected]>

* Add category to PerformAction make

Signed-off-by: Yadunund <[email protected]>

* Fix bug in estimate_remaining_time()

Signed-off-by: Yadunund <[email protected]>

* Update utils

Signed-off-by: Yadunund <[email protected]>

* Update copyright

Signed-off-by: Yadunund <[email protected]>
Yadunund and others added 3 commits January 27, 2022 20:27
* Fix bad optional

Signed-off-by: Yadunund <[email protected]>

* Fix segfaults

Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Xi Yu Oh <[email protected]>
@mxgrey mxgrey marked this pull request as ready for review February 12, 2022 17:25
@mxgrey mxgrey merged commit 27f2865 into main Feb 13, 2022
@mxgrey mxgrey deleted the redesign_v2 branch February 13, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants