(moveit_py) Add Trajectory Execution Manager #2406

MatthijsBurgh · 2023-10-04T08:11:25Z

Description

Add Trajectory Execution Manager.

Note: I have not been able to test the correct behaviour of all added functions. Also I skipped some functions, which are shown as comments.

Checklist

Required by CI: Code is auto formatted using clang-format
Extend the tutorials / documentation reference
Document API changes relevant to the user in the MIGRATION.md notes
Create tests, which fail without this PR reference
Include a screenshot if changing a GUI
While waiting for someone to review your request, please help review another open pull request to support the maintainers

sjahr · 2023-10-06T09:28:38Z

@MatthijsBurgh Can you run pre-commit? See here for instructions on how to do that https://moveit.ros.org/documentation/contributing/code/#pre-commit-formatting-checks

MatthijsBurgh · 2023-10-06T09:33:23Z

@sjahr done

codecov · 2023-10-06T10:49:18Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (51dfbcc) 50.39% compared to head (22c61d7) 50.79%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2406      +/-   ##
==========================================
+ Coverage   50.39%   50.79%   +0.41%     
==========================================
  Files         391      392       +1     
  Lines       31996    32170     +174     
==========================================
+ Hits        16121    16339     +218     
+ Misses      15875    15831      -44

Files	Coverage Δ
...eit_core/controller_manager/controller_manager.cpp	`85.72% <66.67%> (-14.28%)`	⬇️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

MatthijsBurgh · 2023-10-09T07:00:10Z

Sorry for breaking CI again. pre-commit didn't do its jobs correctly. I don't know how, but it is fixed again.

henningkayser

I gave this a quick pass, the bindings look all correct to me. I would appreciate if @peterdavidfagan or @JafarAbdi could verify and test this.

moveit_py/src/moveit/moveit_ros/trajectory_execution_manager/trajectory_execution_manager.cpp

moveit_py/src/moveit/moveit_ros/trajectory_execution_manager/trajectory_execution_manager.h

peterdavidfagan · 2023-10-11T16:23:47Z

Thanks for these updates @MatthijsBurgh, reviewing this is on my list for things to do, hopefully I'll get around to this during the week. I want to first fix the tutorials as these are broken.

MatthijsBurgh · 2023-10-12T10:15:30Z

Created an issue and referenced it in the code. Updated the copyright headers of only the changed files in this PR.

peterdavidfagan · 2023-10-12T11:35:25Z

Created an issue and referenced it in the code. Updated the copyright headers of only the changed files in this PR.

🚀🚀🚀

MatthijsBurgh · 2023-10-18T08:34:12Z

@peterdavidfagan friendly ping to merge this PR ;)

peterdavidfagan · 2023-10-18T10:30:02Z

Hi @MatthijsBurgh,

I do need to finish reviewing this on my side, I will look to find time this week.

mergify · 2023-10-27T14:40:19Z

This pull request is in conflict. Could you fix it @MatthijsBurgh?

peterdavidfagan

These changes LGTM. I haven't had the opportunity to test all the added functionalities but I would be in favour of merging these changes and addressing any follow ups in issues.

Thanks @MatthijsBurgh for this contribution 🚀.

moveit_py/src/moveit/moveit_ros/trajectory_execution_manager/trajectory_execution_manager.cpp

moveit_py/src/moveit/moveit_ros/trajectory_execution_manager/trajectory_execution_manager.h

…nager

sjahr

Thanks a lot for this contribution (and your patience regarding reviews). Only one thing left to be changed and then this can finally be merged!

sjahr · 2023-10-30T10:37:41Z

moveit_py/src/moveit/moveit_ros/trajectory_execution_manager/trajectory_execution_manager.cpp

+      Manages the trajectory execution.
+      )")
+
+      .def("isManagingControllers", &trajectory_execution_manager::TrajectoryExecutionManager::isManagingControllers,


Suggested change

.def("isManagingControllers", &trajectory_execution_manager::TrajectoryExecutionManager::isManagingControllers,

.def("is_managing_controllers", &trajectory_execution_manager::TrajectoryExecutionManager::isManagingControllers,

Shouldn't the python functions be defined in snake_case (camelCase is used for cpp functions)? I think it is done like this everywhere else, so we should be consistent about it. Can you update the other function signatures as well?

We need to update many of the existing cpp methods as there are discrepancies elsewhere in the library which may have lead to @MatthijsBurgh using different conventions.

Not sure if there is an easy way to add this to auto formatter (clang-format) but I agree with the above convention.

Added issue: #2499

I think that got covered by #2177 or do you mean something else @peterdavidfagan? That PR also added clang-tidy checks to warn about cpp function names that don't follow the conventions. But don't think there are checks to catch convention mistakes in .def("...", ..).

49052c1 ❤️.

My fork was outdated only seeing these updates now. Thanks so much.

As for .def I would personally need to become more familiar with clang-tidy, maybe there is a way to get a rule which also flags .def. For now I am happy to be more vigilant about this when pushing code or reviewing.

Awesome, thanks for opening the issue though!

So the python functions will be snake_case?

MatthijsBurgh · 2023-11-10T17:12:53Z

@sjahr I fixed the styling. I think then this one is ready to be merged too, correct?

* (moveit_py) add trajectory execution manager * (moveit_py) add __bool__ to ExecutionStatus * (moveit_py) Update copyright header of changed files * (moveit_py) add comment referencing issue * Rename init_trajectory_execution_manager -> initTrajectoryExecutionManager * (moveit_py) python functions snake_case * (moveit_py) fix styling --------- (cherry picked from commit 76a488b) # Conflicts: # moveit_py/src/moveit/planning.cpp

sjahr requested review from peterdavidfagan, henningkayser and JafarAbdi October 6, 2023 09:10

MatthijsBurgh force-pushed the traj_exec_manager branch from 9719861 to be8b177 Compare October 6, 2023 09:33

MatthijsBurgh force-pushed the traj_exec_manager branch from be8b177 to d798100 Compare October 6, 2023 09:35

MatthijsBurgh force-pushed the traj_exec_manager branch 2 times, most recently from d37246e to cbdf49e Compare October 9, 2023 06:58

henningkayser approved these changes Oct 10, 2023

View reviewed changes

MatthijsBurgh added 2 commits October 12, 2023 11:39

(moveit_py) add trajectory execution manager

98a16dd

(moveit_py) add __bool__ to ExecutionStatus

5c1ba00

MatthijsBurgh force-pushed the traj_exec_manager branch from cbdf49e to 5c1ba00 Compare October 12, 2023 09:39

(moveit_py) Update copyright header of changed files

d5601c7

MatthijsBurgh mentioned this pull request Oct 12, 2023

(moveit_py) Extend Trajectory Execution Manager #2442

Open

(moveit_py) add comment referencing issue

9fc02e0

MatthijsBurgh force-pushed the traj_exec_manager branch from 4d5c8ba to 9fc02e0 Compare October 12, 2023 11:51

henningkayser added the backport-iron Mergify label that triggers a PR backport to Iron label Oct 24, 2023

peterdavidfagan approved these changes Oct 30, 2023

View reviewed changes

Merge branch 'main' into traj_exec_manager

42abd13

sjahr reviewed Oct 30, 2023

View reviewed changes

moveit_py/src/moveit/moveit_ros/trajectory_execution_manager/trajectory_execution_manager.cpp Outdated Show resolved Hide resolved

sjahr reviewed Oct 30, 2023

View reviewed changes

moveit_py/src/moveit/moveit_ros/trajectory_execution_manager/trajectory_execution_manager.h Outdated Show resolved Hide resolved

Rename init_trajectory_execution_manager -> initTrajectoryExecutionMa…

bd2be40

…nager

sjahr reviewed Oct 30, 2023

View reviewed changes

peterdavidfagan mentioned this pull request Oct 30, 2023

moveit_py: fix function signature formatting #2499

Closed

MatthijsBurgh added 3 commits October 31, 2023 10:34

(moveit_py) python functions snake_case

b4a686a

Merge remote-tracking branch 'upstream/main' into traj_exec_manager

9585809

(moveit_py) fix styling

22c61d7

sjahr enabled auto-merge (squash) November 10, 2023 17:13

sjahr approved these changes Nov 10, 2023

View reviewed changes

sjahr merged commit 76a488b into moveit:main Nov 10, 2023

mergify bot mentioned this pull request Nov 10, 2023

(moveit_py) Add Trajectory Execution Manager (backport #2406) #2522

Closed

MatthijsBurgh deleted the traj_exec_manager branch November 12, 2023 09:36

MatthijsBurgh mentioned this pull request Nov 12, 2023

(moveit_py) fix pyi files #2526

Merged

6 tasks

sea-bass mentioned this pull request Dec 1, 2023

Fix trajectory execution manager comments for docs builds #2563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(moveit_py) Add Trajectory Execution Manager #2406

(moveit_py) Add Trajectory Execution Manager #2406

MatthijsBurgh commented Oct 4, 2023 •

edited

Loading

sjahr commented Oct 6, 2023

MatthijsBurgh commented Oct 6, 2023

codecov bot commented Oct 6, 2023 •

edited

Loading

MatthijsBurgh commented Oct 9, 2023 •

edited

Loading

henningkayser left a comment

peterdavidfagan commented Oct 11, 2023 •

edited

Loading

MatthijsBurgh commented Oct 12, 2023

peterdavidfagan commented Oct 12, 2023

MatthijsBurgh commented Oct 18, 2023

peterdavidfagan commented Oct 18, 2023

mergify bot commented Oct 27, 2023

peterdavidfagan left a comment

sjahr left a comment

sjahr Oct 30, 2023

peterdavidfagan Oct 30, 2023 •

edited

Loading

peterdavidfagan Oct 30, 2023

sjahr Oct 30, 2023 •

edited

Loading

peterdavidfagan Oct 30, 2023 •

edited

Loading

sjahr Oct 30, 2023

MatthijsBurgh Oct 30, 2023

MatthijsBurgh commented Nov 10, 2023

	.def("isManagingControllers", &trajectory_execution_manager::TrajectoryExecutionManager::isManagingControllers,
	.def("is_managing_controllers", &trajectory_execution_manager::TrajectoryExecutionManager::isManagingControllers,

(moveit_py) Add Trajectory Execution Manager #2406

(moveit_py) Add Trajectory Execution Manager #2406

Conversation

MatthijsBurgh commented Oct 4, 2023 • edited Loading

Description

Checklist

sjahr commented Oct 6, 2023

MatthijsBurgh commented Oct 6, 2023

codecov bot commented Oct 6, 2023 • edited Loading

Codecov Report

MatthijsBurgh commented Oct 9, 2023 • edited Loading

henningkayser left a comment

Choose a reason for hiding this comment

peterdavidfagan commented Oct 11, 2023 • edited Loading

MatthijsBurgh commented Oct 12, 2023

peterdavidfagan commented Oct 12, 2023

MatthijsBurgh commented Oct 18, 2023

peterdavidfagan commented Oct 18, 2023

mergify bot commented Oct 27, 2023

peterdavidfagan left a comment

Choose a reason for hiding this comment

sjahr left a comment

Choose a reason for hiding this comment

sjahr Oct 30, 2023

Choose a reason for hiding this comment

peterdavidfagan Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

peterdavidfagan Oct 30, 2023

Choose a reason for hiding this comment

sjahr Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

peterdavidfagan Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

sjahr Oct 30, 2023

Choose a reason for hiding this comment

MatthijsBurgh Oct 30, 2023

Choose a reason for hiding this comment

MatthijsBurgh commented Nov 10, 2023

MatthijsBurgh commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 6, 2023 •

edited

Loading

MatthijsBurgh commented Oct 9, 2023 •

edited

Loading

peterdavidfagan commented Oct 11, 2023 •

edited

Loading

peterdavidfagan Oct 30, 2023 •

edited

Loading

sjahr Oct 30, 2023 •

edited

Loading

peterdavidfagan Oct 30, 2023 •

edited

Loading