search

ros-aldebaran / romeo_robot

ROS stack for Aldebaran's Romeo robot
4 stars 7 forks source link

fixed and tested dcm bringup #4

Closed nlyubova closed 8 years ago

mikaelarguedas commented 9 years ago

no problem for urdf and xacro update with new sensor. +1 for https://github.com/nlyubova/romeo_robot/commit/95b1220aab95808a48d21c25821bbf0e45dec8c0 why remove the Head controller ? why remove the namespace in dcm ?

nlyubova commented 9 years ago

There is still some problem with hands in urdf and xacro, because opening and closing works inverse (should be from 1 to 0 but not from 0 to 1)

I thought I have added the head controller back :) and the RightHand controller as well, no?

We removed the namespace only for joints_states at the moment. Do you think we should keep it?

mikaelarguedas commented 9 years ago

ok for the hands I remember the issue I though it was solved, but maybe it was the ROMEO I was using who had the hands inverted... Can you confirm with motion to be sure that we are consistent with choregraphe?

No pb for the the namespace, you've been consistent in removing it everywhere it was needed so it's good for me.

For the controllers, have you tested LeftHand and RightHand controller with the joint changes you made ? We had this joints at first and we had to put the other wrists joints in the hand controller, I don't remember why.

vrabaud commented 8 years ago

can you please rebase and push again ? Thx

nlyubova commented 8 years ago

I've rebased it, Also added a new romeo_bringup (similar to nao_bringup) and few changes in controllers names to make compatible with the current moveit_config version

vrabaud commented 8 years ago

@nlyubova , please make several commits so that we can track differences ... You created a new package, white space changes, launch file updates and URDF updates in only one commit ... Several commits help reverting easily and understand better what happened. You can probably create several pull requests too.

nlyubova commented 8 years ago

@vrabaud , thank you for your advice. Definitely, my pull request was not good! but now it is much better: I assume that these commits are the most important, and all the rest can be skipped for a while.

vrabaud commented 8 years ago

@keulYSMB , can you please review ? lgtm except for the first commit where I don't understand why the robot_state_publisher is removed. Thx both.

nlyubova commented 8 years ago

yes, I've removed the robot_state_publisher and added it to the romeo_moveit_config/launch/demo.launch because otherwise I had several overlapping robots published. @keulYSMB do you agree to remove it?

nlyubova commented 8 years ago

done with commented blocks, they are removed

vrabaud commented 8 years ago

for future pull request: if you change the style (like indentation), please make a different commit for that: it makes things easier to review.

vrabaud commented 8 years ago

Thx, releasing now.