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

Add under review passthru controllers from upstream ros_controllers #41

Closed
wants to merge 2 commits into from

Conversation

egordon
Copy link
Contributor

@egordon egordon commented Aug 12, 2021

While waiting on a review of ros-controls/ros_controllers#569

This creates the following controllers:

rewd_controllers/JointGroupEffortController
rewd_controllers/JointGroupVelocityController
rewd_controllers/JointGroupPositionController

With only the joint_names argument.

Allows for either a subscriber float array connection-less interface (via the command topic) or a JointGroupCommandAction (from pr_control_msgs, see personalrobotics/pr_control_msgs#3)

@egordon egordon requested review from amalnanavati and a team August 12, 2021 23:38
@egordon egordon self-assigned this Aug 12, 2021
{
try
{
joints_.push_back(hw->getHandle(joint_names_[i]));

Choose a reason for hiding this comment

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

To protect against users calling init twice, perhaps clear joints_ before adding the joints to it?

(And/or add an is_initialized bool that is set to true at the end of this function and is checked at the beginning of this function?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Added

Copy link

@amalnanavati amalnanavati left a comment

Choose a reason for hiding this comment

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

With the exception of the change in init (which imo should have been in the original ROS code as well), this looks good.

FYI, I effectively reviewed the upstream commit to ros-controllers as opposed to this one, to better capture the diff.

Copy link

@amalnanavati amalnanavati left a comment

Choose a reason for hiding this comment

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

LGTM

@egordon
Copy link
Contributor Author

egordon commented Aug 24, 2021

Closing. Superseded by personalrobotics/pr_ros_controllers#26

@egordon egordon closed this Aug 24, 2021
@egordon egordon deleted the egordon/ros_controllers branch August 24, 2021 23:03
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.

2 participants