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

Port trajectory_execution_manager to ROS2 #110

Merged

Conversation

JafarAbdi
Copy link
Member

No description provided.

@JafarAbdi JafarAbdi force-pushed the pr-trajectory_execution_manager branch from 1d975d3 to 3a7e25b Compare October 28, 2019 09:37
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.

There are a lot of changes to address here. Replacing dynamic_reconfigure and defining how we want to handle parameters in general should happen before merging this PR.

${rclcpp_LIBRARIES}
)

if(WIN32)
Copy link
Member

Choose a reason for hiding this comment

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

I'm hesitant to tinker with windows support here already. But we should definitely keep the tests. I didn't look into running/fixing up the moveit_core tests so could you extract them here for now and move them to another branch+issue (just add comment+description to #106)?

{
dynamic_reconfigure_server_.setCallback(
boost::bind(&DynamicReconfigureImpl::dynamicReconfigureCallback, this, _1, _2));
// TODO (anasarrak): generate a similar thing for ros2 using the parameters
Copy link
Member

Choose a reason for hiding this comment

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

Yep, we will have to find a solution for this. It's supposed to be much easier in ROS 2 (http://design.ros2.org/articles/ros_parameters.html) as we have parameter callbacks already available.
I'll start looking into this once moveit_core is merged.

@RoboticsYY RoboticsYY force-pushed the pr-trajectory_execution_manager branch from ad3d09e to f51f9f1 Compare December 10, 2019 09:35
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.

@RoboticsYY Great progress so far, thanks for fixing this up.

@RoboticsYY RoboticsYY force-pushed the pr-trajectory_execution_manager branch from 25d6e0d to 376a031 Compare December 16, 2019 09:28
@RoboticsYY RoboticsYY force-pushed the pr-trajectory_execution_manager branch 6 times, most recently from 3d56cf1 to d49b1fd Compare January 10, 2020 07:45
@RoboticsYY RoboticsYY force-pushed the pr-trajectory_execution_manager branch from d49b1fd to 41aebbb Compare January 15, 2020 07:40
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 implementation looks good to me! Could you start squashing the commits, rebase and address the final remarks? We should get PSM merged before this, though.

@RoboticsYY RoboticsYY force-pushed the pr-trajectory_execution_manager branch from 41aebbb to ecc8749 Compare January 17, 2020 10:00
@RoboticsYY RoboticsYY force-pushed the pr-trajectory_execution_manager branch 4 times, most recently from 1d4076c to 3919980 Compare January 29, 2020 03:00
@JafarAbdi JafarAbdi force-pushed the pr-trajectory_execution_manager branch 2 times, most recently from a05bac0 to c3b283b Compare January 29, 2020 19:59
@JafarAbdi JafarAbdi force-pushed the pr-trajectory_execution_manager branch from c3b283b to 45b5ecd Compare January 29, 2020 20:01
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.

Looks good! lgtm

@henningkayser henningkayser merged commit cfbe97f into moveit:master Jan 30, 2020
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
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.

3 participants