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.

Sign up for GitHub

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

Jump to bottom

Port planning_request_adapter_plugins to ROS2 #114

Merged
merged 7 commits into from
Dec 6, 2019
Merged

Port planning_request_adapter_plugins to ROS2 #114

merged 7 commits into from
Dec 6, 2019

Conversation

JafarAbdi
Copy link
Member

No description provided.

JafarAbdi pushed a commit to JafarAbdi/moveit2 that referenced this pull request Oct 28, 2019
@ahcorde
Merge pull request moveit#114 from AcutronicRobotics/fix-travis-readme
972c379 
Fix .travis.yml and README.md:
henningkayser
henningkayser reviewed Oct 29, 2019
View reviewed changes
moveit_ros/planning/planning_request_adapter_plugins/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/CMakeLists.txt Show resolved Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/src/add_time_parameterization.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/src/fix_start_state_bounds.cpp Outdated
@@ -49,23 +49,34 @@ class FixStartStateBounds : public planning_request_adapter::PlanningRequestAdap
static const std::string BOUNDS_PARAM_NAME;
static const std::string DT_PARAM_NAME;

FixStartStateBounds() : planning_request_adapter::PlanningRequestAdapter(), nh_("~")
void initialize(std::shared_ptr<rclcpp::Node>& node)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be added in the next sync and with the original commits + modification.

Copy link
Contributor

@RoboticsYY RoboticsYY Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@henningkayser I saw there was a PR about this in the original MoveIt repository. Do you mean to still use a private node in the constructor for now, and move the parameter lookup into initialize in future?

moveit_ros/planning/planning_request_adapter_plugins/src/fix_start_state_bounds.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/src/fix_start_state_bounds.cpp Outdated
{
if (!nh_.getParam(BOUNDS_PARAM_NAME, bounds_dist_))
this->node_ = node;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialize using the copy constructor

moveit_ros/planning/planning_request_adapters_plugin_description.xml Show resolved Hide resolved
@JafarAbdi JafarAbdi force-pushed the pr-planning_request_adapter_plugins branch from 66ab9aa to 571ef43 Compare November 11, 2019 14:06
henningkayser
henningkayser reviewed Nov 18, 2019
View reviewed changes
Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good so far. However, the history is broken and this branch includes features that have not been synced yet. It might be cleaner to rebase this on a synced version later on

moveit_ros/planning/planning_request_adapter_plugins/src/add_time_optimal_parameterization.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/src/list.cpp
@@ -72,6 +74,6 @@ int main(int argc, char** argv)
std::cout << " \t\t " << ad->getDescription() << std::endl;
std::cout << std::endl << std::endl;
}

rclcpp::shutdown();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this redundant?

@RoboticsYY RoboticsYY force-pushed the pr-planning_request_adapter_plugins branch from 571ef43 to 37dcf4c Compare November 26, 2019 07:24
henningkayser
henningkayser reviewed Nov 27, 2019
View reviewed changes
Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, it compiles, great work! I'm wondering if COLCON_IGNORE is actually still needed, the top-level CMakeLists should eliminate all commented subdirectories, and colcon doesn't look for nested packages recursively as far as I now.

moveit_ros/planning/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_ros/planning/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/src/add_time_optimal_parameterization.cpp Outdated Show resolved Hide resolved
@RoboticsYY RoboticsYY force-pushed the pr-planning_request_adapter_plugins branch from 6e16142 to 587321b Compare November 28, 2019 13:25
@RoboticsYY
Copy link
Contributor

Nice, it compiles, great work! I'm wondering if COLCON_IGNORE is actually still needed, the top-level CMakeLists should eliminate all commented subdirectories, and colcon doesn't look for nested packages recursively as far as I now.

COLCON_IGNOREs only apply to the ros packages under moveit_ros directory. There is no COLCON_IGNORE under the subdirectories of moveit_ros/planning now.

I run the executable through ros2 run moveit_ros_planning moveit_list_request_adapter_plugins. It successfully loaded all the plugins, but ended with a segfault before the return. I'm still trying to figure out the reason.

@RoboticsYY RoboticsYY force-pushed the pr-planning_request_adapter_plugins branch from 587321b to 9fc78a9 Compare December 1, 2019 09:36
@RoboticsYY
Copy link
Contributor

The segfault seems come from a legacy build of moveit_core. After I removed everything in the /build folder and recompiled the workspace, the segfault disappeared.

@RoboticsYY RoboticsYY force-pushed the pr-planning_request_adapter_plugins branch from 9fc78a9 to 250c638 Compare December 2, 2019 06:40
@henningkayser henningkayser force-pushed the pr-planning_request_adapter_plugins branch from 250c638 to 9e702c2 Compare December 6, 2019 19:10
@henningkayser henningkayser force-pushed the pr-planning_request_adapter_plugins branch from 1f93e41 to 3fb93d2 Compare December 6, 2019 20:52
@henningkayser henningkayser merged commit de8ea45 into moveit:master Dec 6, 2019
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
@facontidavide
Clone repository ros-planning/moveit_resources to fix issue moveit#114 (
8d1a13d 
moveit#115)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RoboticsYY RoboticsYY RoboticsYY left review comments

@henningkayser henningkayser henningkayser approved these changes

Assignees

@RoboticsYY RoboticsYY

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JafarAbdi @RoboticsYY @henningkayser