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

[pr2eus/robot-interface.l] use follow joint trajectory action #237

Merged
merged 1 commit into from
Sep 13, 2016

Conversation

furushchev
Copy link
Member

this change requires
jsk-ros-pkg/jsk_robot#623

@furushchev furushchev force-pushed the use-follow-base-traj branch 2 times, most recently from 9eee2d2 to bf12051 Compare July 1, 2016 08:53
…tion for base trajectory action

* [pr2eus/robot-interface.l] fix: wrong code
@furushchev
Copy link
Member Author

rebased origin/master

@@ -1556,7 +1555,7 @@ send-action [ send message to action server, it means robot will move ]"
(send msg :points (list pt1 pt2)))
))
(send goal :goal :trajectory msg)
(when send-action and move-base-trajectory-action
Copy link
Member

@k-okada k-okada Aug 12, 2016

Choose a reason for hiding this comment

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

why this syntax error did not caught by test ???? af18a5a (#210)

Copy link
Member Author

Choose a reason for hiding this comment

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

@k-okada Because of luck of test case on go-pos-unsafe?
I think it is used only for sending action to pr2_base_trajectory_action controller.

@k-okada
Copy link
Member

k-okada commented Aug 27, 2016

@snozawa, @YoheiKakiuchi , @mmurooka , please check if this change did not trouble your software.

@furushchev
Copy link
Member Author

@snozawa @YoheiKakiuchi @mmurooka Well, did you check this? Any problem?
Cc: @k-okada

@k-okada
Copy link
Member

k-okada commented Oct 13, 2016

It seems we do have problem on this change, @YuOhara, @oshiroy
start-jsk/rtmros_common#970 (comment)

@furushchev
Copy link
Member Author

@k-okada OK. The previous question was just timed out...
Should I wait, or need revert? 🙃

@k-okada
Copy link
Member

k-okada commented Oct 13, 2016

we do not have to revert, just to remind that even if no one respond in
github, it does not mean they have not problem.
I think I have commented somewhere, what's will happen if we change
"fullbody_controller/joint_trajectory_action" to
"fullbody_controller/follow_joint_trajectory_action" ?

https://github.com/jsk-ros-pkg/jsk_pr2eus/pull/237/files#diff-15f6920b924513f6027d36b361f85bbdR793

◉ Kei Okada

2016-10-13 17:33 GMT+09:00 Furushchev [email protected]:

@k-okada https://github.com/k-okada OK. The previous question was just
timed out...
Should I wait, or need revert? 🙃


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#237 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAeG3JXjwc77dZUR27GCl9emHMpCS-0Sks5qzezYgaJpZM4I7Yjf
.

@mmurooka
Copy link
Member

@furushchev , Sorry for late.
I do not understand well about ros joint controller, and I should learn but cloud you help?
(commit fa32b11 is a little bit difficult to read because the difference is large.)

What is the difference between pr2_controllers_msgs::JointTrajectoryAction and control_msgs::FollowJointTrajectoryAction?
Are those compatible?
Why do you want to use control_msgs::FollowJointTrajectoryAction?
In which euslisp method of which class, are those type of ros messages used (for PR2 and HRP2 if you know)?

@furushchev
Copy link
Member Author

furushchev commented Oct 13, 2016

@mmurooka No problem.
You can compare the definition at https://github.com/PR2/pr2_controllers/blob/hydro-devel/pr2_controllers_msgs/action/JointTrajectory.action and https://github.com/ros-controls/control_msgs/blob/indigo-devel/control_msgs/action/FollowJointTrajectory.action for example.

controller_msgs/FollowJointTrajectoryAction differs from pr2_controller_msgs/JointTrajectoryAction in some points:

  • ActionGoal torelance option is added (but ignorable)
  • ActionResult returns error_code
  • ActionFeedback returns error, desired, actual joint angles at each points while joints are moving.
  • no dependency to pr2 related packages

I think this pull request is not so much complicated. Please have a look at only robot-interface.l, I just change msg types and add conditions for feedback.

@furushchev
Copy link
Member Author

Theoretically, If you don't care about making use of feedback and error code, we can migrate by only replacing msg type name in all codes,

@mmurooka
Copy link
Member

@furushchev , sorry, your PR difference was simple enough.

今までまったく見ていなかった部分で理解しきれていないですが,
PR2の場合の関節やベースのactionlibのサーバはどこにあるでしょうか.そちらも一緒に変わってる?

HRP2のactionlibのサーバは
https://github.com/start-jsk/rtmros_common/blob/master/hrpsys_ros_bridge/src/HrpsysSeqStateROSBridge.h#L60-L61 にあって,これを変更しないといけなかったのでマージまってください
というべきだったということ?

@furushchev
Copy link
Member Author

@mmurooka

PR2の場合の関節やベースのactionlibのサーバはどこにあるでしょうか.そちらも一緒に変わってる?

https://github.com/PR2/pr2_controllers/blob/hydro-devel/robot_mechanism_controllers/include/robot_mechanism_controllers/joint_trajectory_action_controller.h
が実装だと思います。

http://wiki.ros.org/pr2_controllers_msgs や http://answers.ros.org/question/67079/how-to-depend-on-pr2_controllers_msgs/?answer=67134#post-id-67134
にある通り、pr2_controller_msgsよりcontrol_msgsを使うことが推奨されています。

HRP2のactionlibのサーバは
https://github.com/start-jsk/rtmros_common/blob/master/hrpsys_ros_bridge/src/HrpsysSeqStateROSBridge.h#L60-L61 にあって,これを変更しないといけなかったのでマージまってください
というべきだったということ?

ソースが長くて、よくは読んでいませんが、これはpr2_controller_msgsとcontrol_msgsが両方使われているように見えます。僕が上に挙げたPR2のaction serverの実装のようにpr2_controller_msgsのほうが単にcontrol_msgsのback compatibilityのために使われているのなら、変更はいらないし、そうでないならfollow_joint_trajectoryに変える必要があります。その場合pr2_controller_msgsを消せるので、依存関係はシンプルになりそうです。

@mmurooka
Copy link
Member

http://wiki.ros.org/pr2_controllers_msgs や http://answers.ros.org/question/67079/how-to-depend-on-pr2_controllers_msgs/?answer=67134#post-id-67134
にある通り、pr2_controller_msgsよりcontrol_msgsを使うことが推奨されています。

PR2のcontrollerのサーバはpr2_controller_msgsとcontrol_msgsをどちらも受け取ることができるようになっているということで合ってる?

@furushchev
Copy link
Member Author

はい。

2016年10月14日(金) 15:47 Masaki Murooka [email protected]:

http://wiki.ros.org/pr2_controllers_msgs
http://answers.ros.org/question/67079/how-to-depend-on-pr2_controllers_msgs/?answer=67134#post-id-67134
にある通り、pr2_controller_msgsよりcontrol_msgsを使うことが推奨されています。

PR2のcontrollerのサーバはpr2_controller_msgsとcontrol_msgsをどちらも受け取ることができるようになっているということで合ってる?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#237 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB0B0NvS8VoNCuswtAufFmQllyMeX4udks5qzyWbgaJpZM4I7Yjf
.

-- ⌘ Yuki Furuta

@mmurooka
Copy link
Member

https://github.com/jsk-ros-pkg/jsk_pr2eus/pull/237/files#diff-15f6920b924513f6027d36b361f85bbdL794
がcontrollerのactionのtopic(actionlibの場合はtopicとは呼ばないのかな.)の名前に対応するかな.
このPRの変更では関節のcontrollerについては,
topicの型は換えているけれど,topicの名前は変えていないように読めるけれど,
PR2のcontrollerのサーバは,同じtopic名で,pr2_controller_msgsとcontrol_msgsの両方を読むことができる?仮にそうなら rostopic infoしたら何が起きるんだろう.

HRP2のcontrollerのサーバでは,
pr2_controller_msgsとcontrol_msgsでtopic名が違う
https://github.com/start-jsk/rtmros_common/blob/master/hrpsys_ros_bridge/src/HrpsysSeqStateROSBridge.h#L60-L61
https://github.com/start-jsk/rtmros_common/blob/master/hrpsys_ros_bridge/src/HrpsysSeqStateROSBridge.cpp#L35-L36
ので,
start-jsk/rtmros_common#970
のPRが必要になっているみたい.

@furushchev
Copy link
Member Author

furushchev commented Oct 14, 2016

@mmurooka なるほど、僕はfullbodyについて使っていないので、ユーザから特に返信がなく、単に変更を忘れたままmergeされたということになりますね。
https://github.com/jsk-ros-pkg/jsk_pr2eus/pull/237/files#diff-15f6920b924513f6027d36b361f85bbdL794
も同様に変えるのが混乱がないと思います。

@mmurooka
Copy link
Member

@furushchev
Copy link
Member Author

furushchev commented Oct 14, 2016

control_msgs/FollowJointTrajectoryActionを使っているならrobot-interface.lを変えたほうがいいと思います。
理由はシンプルだから、サブクラスで再実装する必要がないから、です。

1点だけ気になるのが、トピックの命名規則は普通はfollow_joint_trajectory_actionでなくて、follow_joint_trajectoryですので、ここは共通化したほうがいいかなと思っています。

@mmurooka
Copy link
Member

前提にcontrol_msgs/FollowJointTrajectoryActionの名前は,
"fullbody_controller/joint_trajectory_action"ではなくて当然"fullbody_controller/follow_joint_trajectory_action"だという前提があるかな.

他のロボットでcontrol_msgs/FollowJointTrajectoryActionだけれど,
"fullbody_controller/joint_trajectory_action"という名前のロボットはさすがにないだろうということ?

@mmurooka
Copy link
Member

#237 (comment) の疑問を調べてみて,
PR2でも,
別のtopic名でpr2_controllers_msgsとcontrol_msgsがあって,
fa32b11#diff-15f6920b924513f6027d36b361f85bbdL796
でPR2専用のpr2-interface.lでtopic名を変えていたんですね.

そうすると,たぶん
#237 (comment)
ということなので,
start-jsk/rtmros_common#970 よりも #250 がいいということになりそうです.

# PR2体内で
$ rostopic info /l_arm_controller/joint_trajectory_action/goal | grep Type
Type: pr2_controllers_msgs/JointTrajectoryActionGoal


$ rostopic info /l_arm_controller/follow_joint_trajectory/goal | grep Type
Type: control_msgs/FollowJointTrajectoryActionGoal

@furushchev
Copy link
Member Author

はい。説明不足ですみませんでした。fetchなどでもそうなっているはずです。

@YoheiKakiuchi
Copy link
Member

start-jsk/rtmros_common#970 の意図としては、
follow_joint_trajectory_actionの名前をつけた時に長すぎたなと思っていて、
PR2のfollow_trajectory_actionの方が名前としてはいいなと言うのがあって、
robot-interface.lはそのままにして、名前変えたいなあということです。
start-jsk/rtmros_common#971

@mmurooka
Copy link
Member

#237 (comment)
で確かめた時にはfollow_joint_trajectory_actionでしたが,

PR2のfollow_trajectory_actionの方が名前としてはいいなと言うのがあって、

とういのはどういうことでしたでしょうか.

@YoheiKakiuchi
Copy link
Member

YoheiKakiuchi commented Oct 14, 2016

follow_joint_trajectory_action (HrpsysSeqStateROSBridge)

follow_joint_trajectory (PR2)
ですね。

@mmurooka
Copy link
Member

なるほど,どちらのPRが良いか判断していただいて,
現状pull origin/masterすると動かないことになってしまうので,
問題なさそうならどちらかをmergeいただけると嬉しいです.

@furushchev
Copy link
Member Author

furushchev commented Oct 14, 2016

「どちらも "fullbody_controller/follow_joint_trajectory**_action"** にする」に+1

@mmurooka
Copy link
Member

https://github.com/start-jsk/rtmros_common/blob/master/hrpsys_ros_bridge/src/HrpsysSeqStateROSBridge.cpp#L35-L36
は,HRP2以外のJAXONやたぶんHIROも使っていてすぐに変えることはできないので,
当分の間はfollow_joint_trajectoryで我慢するしかなくて,
現状ロボットが動かないのでどうにかしないといけなくて,
取り急ぎ start-jsk/rtmros_common#970 をするということなんだと思います.

@furushchev
Copy link
Member Author

@mmurooka ??すみません、僕が理解していないだけかもしれないですが、follow_joint_trajectoryで我慢するんじゃなくて、むしろ全部follow_joint_trajectoryにするのが良いと思っています。

@mmurooka
Copy link
Member

すみません,
当分の間はfollow_joint_trajectory_actionで我慢するしかなくて,
が正しいです.

@furushchev
Copy link
Member Author

@mmurooka なるほど、ようやくわかりました。
では #250 + comment(あとで名前をfollow_joint_trajectoryに結局変えるのか変えないかに依る)に+1したいと思います。

@mmurooka
Copy link
Member

たぶん#237 (comment) で言っていることは,
#250 の出番はなくて,
1. start-jsk/rtmros_common#970 をマージ (これでまずは動く状態に戻せる)
2. robot-interfaceのaction_lib名をfollow_joint_trajectoryにする(これは#250 とは違う)
3. https://github.com/start-jsk/rtmros_common/blob/master/hrpsys_ros_bridge/src/HrpsysSeqStateROSBridge.cpp#L35-L36
の名前を変える,と同時に start-jsk/rtmros_common#970 をrevertする
ということだと思います.

@YoheiKakiuchi
Copy link
Member

名前は変えたいけど別議論ということで、
#250 を採用で
start-jsk/rtmros_common#970 をクローズしました。
名前と古いtrajectoryactionについては、ここで議論
start-jsk/rtmros_common#971

@snozawa
Copy link
Contributor

snozawa commented Oct 17, 2016

すいません、出遅れました。
まだ中身がおえてないですが、これ解決or議論が収束したんですっけ?

@mmurooka
Copy link
Member

はい,jsk_pr2eusとrtmros_commonのどちらもorigin/master最新で
hrpsys_ros_bridge系のロボットも動くようになりました.

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.

5 participants