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 moveit_simple_controller_manager #158

Merged
merged 2 commits into from
Jan 31, 2020
Merged

Port moveit_simple_controller_manager #158

merged 2 commits into from
Jan 31, 2020

Conversation

JafarAbdi
Copy link
Member

Description

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

@JafarAbdi JafarAbdi force-pushed the pr-control_interface branch 2 times, most recently from 6140d6f to ddde33d Compare January 21, 2020 17:34
@JafarAbdi JafarAbdi force-pushed the pr-control_interface branch 3 times, most recently from d6676a8 to b6f32ae Compare January 24, 2020 15:44
@JafarAbdi
Copy link
Member Author

This PR assumes controller_lists.yaml will have the following format (I added multiple controllers just for illustration)

/**:
    ros__parameters:
        controller_names:
          - position_joint_trajectory_controller
          - fake_panda_arm_controller
          - fake_hand_controller
          - franka_gripper
        fake_panda_arm_controller:
            joints:
              - panda_joint1
              - panda_joint2
              - panda_joint3
              - panda_joint4
              - panda_joint5
              - panda_joint6
              - panda_joint7
        fake_hand_controller:
            joints:
              - panda_finger_joint1
              - panda_finger_joint2
        position_joint_trajectory_controller:
           action_ns: follow_joint_trajectory
           type: FollowJointTrajectory
           default: true
           joints:
             - panda_joint1
             - panda_joint2
             - panda_joint3
             - panda_joint4
             - panda_joint5
             - panda_joint6
             - panda_joint7
        franka_gripper:
            action_ns: gripper_action
            type: GripperCommand
            default: true
            joints:
                - panda_finger_joint1
                - panda_finger_joint2

@JafarAbdi JafarAbdi force-pushed the pr-control_interface branch 2 times, most recently from 8254269 to 257cbe5 Compare January 27, 2020 09:55
JafarAbdi
JafarAbdi commented Jan 27, 2020
View reviewed changes
moveit_plugins/moveit_simple_controller_manager/src/moveit_simple_controller_manager.cpp
{
if (!isStruct(controller_list[i], { "name", "joints", "action_ns", "type" }))
std::map<std::string, rclcpp::Parameter> controller_params;
node_->get_parameters(controller_name, controller_params);
Copy link
Member Author

Choose a reason for hiding this comment

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

Or node_->get_node_parameters_interface()->get_parameters_by_prefix(controller_name, controller_params); .?

node_->get_parameters is supposed to be called with map of strings to a type (bool, int, string, ..etc) but it works with having rclcpp::Parameter not sure if it's hacky

Copy link
Member

Choose a reason for hiding this comment

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

I think it's supposed to work like that

@JafarAbdi JafarAbdi force-pushed the pr-control_interface branch 2 times, most recently from 9f1d1eb to a6d4028 Compare January 28, 2020 11:04
@JafarAbdi JafarAbdi changed the title WIP: Port moveit_simple_controller_manager Port moveit_simple_controller_manager Jan 28, 2020
henningkayser
henningkayser reviewed Jan 30, 2020
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.

Migration steps look good. We will have to test and probably revise parameter lookup (did you test this), but I agree with the general approach with the additional controller names. Please address the comments, then +1

...controller_manager/include/moveit_simple_controller_manager/action_based_controller_handle.h Outdated Show resolved Hide resolved
...controller_manager/include/moveit_simple_controller_manager/action_based_controller_handle.h Outdated Show resolved Hide resolved
...controller_manager/include/moveit_simple_controller_manager/action_based_controller_handle.h Outdated Show resolved Hide resolved
...controller_manager/include/moveit_simple_controller_manager/action_based_controller_handle.h Show resolved Hide resolved
...controller_manager/include/moveit_simple_controller_manager/action_based_controller_handle.h Outdated Show resolved Hide resolved
...mple_controller_manager/include/moveit_simple_controller_manager/gripper_controller_handle.h Show resolved Hide resolved
...t_plugins/moveit_simple_controller_manager/src/follow_joint_trajectory_controller_handle.cpp Show resolved Hide resolved
...t_plugins/moveit_simple_controller_manager/src/follow_joint_trajectory_controller_handle.cpp Show resolved Hide resolved
...t_plugins/moveit_simple_controller_manager/src/follow_joint_trajectory_controller_handle.cpp Show resolved Hide resolved
moveit_plugins/moveit_simple_controller_manager/src/moveit_simple_controller_manager.cpp Show resolved Hide resolved
@JafarAbdi JafarAbdi force-pushed the pr-control_interface branch 3 times, most recently from d5fba09 to 06ce37a Compare January 31, 2020 00:37
JafarAbdi
JafarAbdi commented Jan 31, 2020
View reviewed changes
...controller_manager/include/moveit_simple_controller_manager/action_based_controller_handle.h
@@ -51,7 +51,8 @@ namespace moveit_simple_controller_manager
class ActionBasedControllerHandleBase : public moveit_controller_manager::MoveItControllerHandle
{
public:
ActionBasedControllerHandleBase(const std::string& name) : moveit_controller_manager::MoveItControllerHandle(name)
ActionBasedControllerHandleBase(const std::string& name, const std::string& logger_name)
: moveit_controller_manager::MoveItControllerHandle(name), LOGGER(rclcpp::get_logger(logger_name))
Copy link
Member Author

Choose a reason for hiding this comment

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

The default constructor is implicitly deleted the only public constructor is the copy constructor, the constructor with string is private with get_logger as a friend to the class

@JafarAbdi
Copy link
Member Author

@henningkayser I didn't squash the last commit for you to be able to review it alone, is this how you want me to handle the logger in header file .?

@henningkayser henningkayser merged commit ab72968 into moveit:master Jan 31, 2020
@JafarAbdi JafarAbdi mentioned this pull request Feb 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henningkayser henningkayser henningkayser approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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