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

gigantic speed up to move group interface #1865

Merged

Conversation

azalutsky
Copy link
Contributor

Description

This speedup removes the 100ms delay during a planning time, which increases planning times by a substantial amount.
For example, my 2 robot arms without this takes 600ms+ to plan. By reducing this sleep to 1ms the plans are then in the 150ms range.

Please explain the changes you made, including a reference to the related issue if applicable

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

@azalutsky azalutsky mentioned this pull request Jan 17, 2023
6 tasks
@sjahr sjahr added backport PR requires backport to other distro branches listed in PR description backport-foxy Mergify label that triggers a PR backport to Foxy backport-humble Mergify label that triggers a PR backport to Humble and removed backport PR requires backport to other distro branches listed in PR description labels Jan 17, 2023
@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Base: 50.43% // Head: 50.42% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (bd8891e) compared to base (6711436).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1865      +/-   ##
==========================================
- Coverage   50.43%   50.42%   -0.00%     
==========================================
  Files         374      374              
  Lines       31335    31335              
==========================================
- Hits        15801    15799       -2     
- Misses      15534    15536       +2     
Impacted Files Coverage Δ
...nning_scene_monitor/src/planning_scene_monitor.cpp 45.26% <0.00%> (-0.43%) ⬇️
moveit_core/robot_state/src/robot_state.cpp 47.87% <0.00%> (-0.07%) ⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 79.93% <0.00%> (+1.14%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@tylerjw tylerjw merged commit 0642ef0 into moveit:main Jan 17, 2023
mergify bot pushed a commit that referenced this pull request Jan 17, 2023
(cherry picked from commit 0642ef0)

# Conflicts:
#	moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
mergify bot pushed a commit that referenced this pull request Jan 17, 2023
@azalutsky
Copy link
Contributor Author

Thanks guys, if anyone has any other tricks to speed up planning times- I'd love to know!

tylerjw pushed a commit that referenced this pull request Jan 17, 2023
(cherry picked from commit 0642ef0)

Co-authored-by: azalutsky <azalutsky@gmail.com>
@azalutsky
Copy link
Contributor Author

azalutsky commented Jan 18, 2023

I am noticing however with multiple robots that there is a bug somewhere with these actions.

Independent of the timeout, but the timeout alleviates it a little bit.

For dual robot, the done flag claims it plans both and completes it however only the first robot in the group executes the trajectory while the second robot stays put.

Seems to be a bug. I started tracking it but if someone has an idea I'd love to hear your thoughts as I investigate as I am super new with the codebase.

I posted in discord's bug's channel about it.

Would love a second opinion.

@azalutsky azalutsky deleted the feature/azalutsky/mgi_planning_speedup branch January 26, 2023 18:25
@rhaschke
Copy link
Contributor

@ros-planning/moveit-maintainers, why does this code use busy waiting??? There are condition variables for this purpose...

@samialperen
Copy link
Contributor

@azalutsky I did not see a performance increase even after this PR. I was expecting it would increase the performance but did not happen. Did you manage to find any other tricks to decrease planning time for humble?

@azalutsky
Copy link
Contributor Author

If you use move group interface, when you call plan the action has this 100ms latency.
What planning times are you seeing?

@samialperen
Copy link
Contributor

@azalutsky I am using Move group interface. I actually applied this fix too and it did not change anything. I am getting around 600ms planning time which does not make sense to me at all. For the same setup, I am getting less than 100ms in moveit ROS1

@azalutsky
Copy link
Contributor Author

azalutsky commented Mar 21, 2023

Is it possible you're not using moveit2 via source? Did you perhaps install moveit2 through apt-get or didn't colcon build with making sure that the package was built by source? I believe foxy requires some additional output. 600ms is almost guarantee that this change didn't go through from your source -> binary.

@samialperen
Copy link
Contributor

I built it from source and ros2 pkg prefix shows my workspace. For the reference, I am using moveit humble, (2.5.4)

@samialperen
Copy link
Contributor

I find another issue stating that foxy has better performance than humble when it comes to planning times, but I could not test it on my system since I have too many dependencies for humble, I can not change it to foxy easily

@samialperen
Copy link
Contributor

samialperen commented Mar 21, 2023

@azalutsky you were right. Somehow, there is a package collision even if ros2 pkg prefix shows the right package. After applying the fix manually, planning time reduced from 600 ms to 170ms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-foxy Mergify label that triggers a PR backport to Foxy backport-humble Mergify label that triggers a PR backport to Humble
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants