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

Rework execution #459

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Rework execution #459

wants to merge 8 commits into from

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented May 4, 2023

This is a draft I had lying around for a while to rework MTC solution execution. Instead of calling the move_group's capability, the idea is to directly interface MoveIt's TrajectoryExecutionManager and thus allow callbacks for each sub-trajectory segment. To this end, this PR reimplements MoveIt's PlanExecution to:

  • allow for both start and end callbacks for each segment in an ExecutableMotionPlan (the original PlanExecution only allowed for end effects)
    Of course, in principle it would suffice to have start or end effects only. However, I found it simpler to define preparatory and cleanup tasks of a specific segment within that segment instead of shifting one or the other to the previous/next segment.
  • to run a sequence of related trajectories without risking interference with another caller

The latter functionality was also implemented by moveit/moveit#3243 meanwhile.

The old Task::execute() method was moved into a free function execute(), still calling the move_group capability.
However, a new free function executableMotionPlan(SolutionBase) is provided to create the ExecutableMotionPlan for use with the new PlanExecution.

This needs to be consolidated with moveit/moveit#3243.
@cambel and @v4hn: Can we organize a video conference to discuss open TODOs?

@rhaschke rhaschke linked an issue May 4, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Patch coverage: 14.00% and project coverage change: -0.70 ⚠️

Comparison is base (75e4260) 58.65% compared to head (a8255bd) 57.94%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
- Coverage   58.65%   57.94%   -0.70%     
==========================================
  Files          90       92       +2     
  Lines        8424     8523      +99     
==========================================
- Hits         4940     4938       -2     
- Misses       3484     3585     +101     
Impacted Files Coverage Δ
core/include/moveit/task_constructor/execution.h 0.00% <0.00%> (ø)
core/include/moveit/task_constructor/storage.h 86.96% <ø> (ø)
core/include/moveit/task_constructor/task.h 33.34% <ø> (+8.34%) ⬆️
core/src/storage.cpp 76.69% <0.00%> (-7.62%) ⬇️
core/src/task.cpp 78.24% <0.00%> (-1.99%) ⬇️
core/src/execution.cpp 15.48% <15.48%> (ø)
demo/src/pick_place_task.cpp 96.75% <50.00%> (-0.02%) ⬇️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cambel
Copy link

cambel commented May 8, 2023

@rhaschke sure

@v4hn
Copy link
Contributor

v4hn commented May 8, 2023

I wasn't aware you had that lying around. Cool!

I'm open for a call on TODOs, but from my perspective the main points are

  • we need thorough reviews for Simultaneous trajectory execution moveit#3243 so that it can be confidently merged to replace the current execution. I can't put priorities on things I don't need in my own work for a while already and I'm quite ashamed I did not get around to do it in all this time.
  • MTC execution came up quite often during the GSoC meetings with @cambel as the implemented approach is right in the middle of PlanExecution, TrajectoryExecution, and MTC execution capabilities. I would hope/expect that this draft can be based on the new execution system and from your explanations the main thing missing are the start callbacks in @cambel's system (which I agree make a lot of sense to add).

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.

Execute solution with MoveItCpp
3 participants