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 subscriber for JointTrajectory as /command #37

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

k-okada
Copy link
Member

@k-okada k-okada commented Sep 17, 2014

/command is general API[1] that is widly used together with JointTrajectoryAction, I need to use this IF for calibration program[2]

[1] http://wiki.ros.org/joint_trajectory_controller?distro=indigo
[2] ros-perception/calibration#31

@aginika
Copy link
Contributor

aginika commented Oct 29, 2014

+1

@rethink-imcmahon
Copy link
Contributor

I will take this pull request into consideration. It's a reasonable request, but I need to see if it fits into the joint trajectory action server upgrades that I've been working on in the im_inverse_dynamics_feedforward branch.

@rethink-imcmahon rethink-imcmahon removed their assignment Jun 3, 2016
@k-okada
Copy link
Member Author

k-okada commented Jul 16, 2016

resolved conflicts

@@ -68,6 +69,7 @@ def __init__(self, limb, reconfig_server, rate=100.0,
FollowJointTrajectoryAction,
execute_cb=self._on_trajectory_action,
auto_start=False)
self._command = rospy.Subscriber('robot/limb/' + limb + '/command', JointTrajectory, self._on_joint_trajectory)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would advocate for the following namespacing for clarity:

/robot/limb/right/joint_trajectory/command
/robot/limb/right/joint_trajectory/state
/robot/limb/right/joint_trajectory/query_state

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for comment, fixed,

self._pid[jnt].initialize()

def _on_joint_trajectory(self, trajectory):
joint_names = trajectory.joint_names
Copy link
Contributor

Choose a reason for hiding this comment

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

As this trajectory callback can execute at the same time as the Action callback, I think it would make sense to have a mutex lock on both to prevent simultaneous joint commands from being sent into the robot.

@IanTheEngineer
Copy link
Contributor

@k-okada I'm very sorry your pull request has languished for nearly two years :/ . I think this topic interface is a valuable addition to the joint trajectory action server, and want to incorporate it if possible. Since the master branch is considered "live", can you target your pull requests at the development branch?

I am off-the-grid the next week on vacation, but I would like to address your pull requests the first week in August when I come back.

@k-okada
Copy link
Member Author

k-okada commented Jul 22, 2016

ok, i have rebased on development branch and add mutex lock code. I added mutex lock within on_trajectory_action method (5b954f9#diff-393fafc5e38a00ca79cd9d500e6ab03cR350) at 5b954f9#diff-393fafc5e38a00ca79cd9d500e6ab03cR421, based on # Acquire Mutex comment
(the lock code is removed at https://github.com/RethinkRobotics/baxter_interface/pull/51/files), and within on_trajectory_command method at 5b954f9#diff-393fafc5e38a00ca79cd9d500e6ab03cR582

But I do not think this will prevent simultaneous joint commands execution, we have to lock more wider range, i.e. entire on_trajectory_action and on_trajectory_command, and also I think we should support "override / trajectory replacement" feature as described in http://wiki.ros.org/robot_mechanism_controllers/JointTrajectoryActionController#Trajectory_Replacement, I'll work on this once all my PR is merged.

Have a nice vacation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants