search

UTNuclearRoboticsPublic / jog_arm

A real-time robot arm jogger.
45 stars 22 forks source link

Bug fix and cleaning #52

Closed ycantin closed 6 years ago

ycantin commented 6 years ago

Bug fix : Silent out of bound access in checkIfJointsWithinBounds wreak havoc later (random segfault/memory corruption) : joint->getFirstVariableIndex() don't match and exceed new_jt_traj.points[0].velocities indexes when there is more joints in the robot model than in the move_group used.

There's some access check (try/catch std::out_of_range) but they don't work with [], they need .at() accessors. I'm wondering if hardening the code ([] to .at()) is worth the effort.

Cleanup : Some bits gathered when debugging, see if that's ok for you.

AndyZe commented 6 years ago

Can you roll back your work-in-progress commit and do a separate PR? That will make it easier on me.

This generally looks good but I'll have to do a more thorough review tomorrow.

ycantin commented 6 years ago

Do you mean one PR per commit, or an all-in-on commit ? (I'm not very familiar with git workflow)

AndyZe commented 6 years ago

A new PR for your passive joint filtering. It looks unrelated to the other bug fixes.

On Tue, Jul 31, 2018, 6:32 PM ycantin notifications@github.com wrote:

Do you mean one PR per commit, or an all-in-on commit ? (I'm not very familiar with git workflow)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/UTNuclearRoboticsPublic/jog_arm/pull/52#issuecomment-409400935, or mute the thread https://github.com/notifications/unsubscribe-auth/AKwvqSpwQBvjDozri6MDJcQUDwHR8b1hks5uMOkDgaJpZM4VjfJR .

AndyZe commented 6 years ago

This looks great up to Remove unused checkIfImminentCollision parameter 59ecdbf. I appreciate your attention to detail. However, I definitely don't want to merge 23bd830. I'm on the fence about ea94279 and wanted to ask some more questions.

So, I'm planning to close this PR. Can you please make a new branch with the commits I want to keep: git checkout 59ecdbf git checkout -b ycantin-bug-fixes

Then make a new PR from that branch. I would definitely accept it.

AndyZe commented 6 years ago

@ycantin Re. filtering the passive joints out of the trajectory message: did you see any performance issues on your robot? Or what was the motivation?

It seems like there wouldn't be any harm with leaving the passive joints in the message, but I don't really have a way to test it.

Thanks a lot for all the bug fixes.

ycantin commented 6 years ago

I work with 6 3DOF legs and ikfast, but i was not able to get MoveIt to work with 3dof only (any hint ?), so ended adding 3 passive joints to the model to enable transform6D IK.

The other way around would be to consider these added joints as real, and discard them on the controller's level, but this mean doubling the size of all ros messages.

Moveit filters the passive joints before sending the final trajectory, so i wanted to do the same in jog_arm.