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

[WIP] [pr2eus] Remove init-robot-from-name and make-robot-interface-from-name from robot-interface.l #173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

garaemon
Copy link
Member

Do not merge yet

see discussion in jsk-ros-pkg/jsk_robot#401

@k-okada
Copy link
Member

k-okada commented Dec 12, 2018

is this duplicates of #224 ?

@pazeshun
Copy link
Collaborator

is this duplicates of #224 ?

重複しているわけではありませんが、 #224 がmergeされるならこのPRはcloseされるべきだと思います。
逆に、 #224 がcloseされるなら、このPRの趣旨を受け継ぎつつ、後方互換性を維持するため関数は削除せずに、内部で違う関数を呼ぶようにするPRを作るのがよいと思います。
#224 の結果次第です。

流れを整理すると、

jsk-ros-pkg/jsk_robot#401 のように、(ロボット名)-init関数を全廃して、robot-init関数を作ろうとする

新たに追加しようとしたものはmake-robot-interface-from-nameinit-robot-from-nameの上位互換だったので、これらの関数を消そうとこのPRができる

jsk_robotに作ろうとしていたが、jsk_pr2eusに作るべきだったということで、#175 に議論が移動

#222 ができてmergeされ、robot-init関数が誕生

#222 が使われないんじゃないかという懸念がでて #224 ができる (#175 (comment))

という感じです。

もし #224 がmergeされるなら、 #222 がなかったことになるので、make-robot-interface-from-nameinit-robot-from-nameを置き換えるものはなく、このPRはcloseされるべきです。

もし #224 がcloseされるなら、 #222 が残ることになります。

  • init-robot-from-nameについて
    Add robot init function to create *ri* and robot instance #222 が追加したrobot-init関数は、init-robot-from-nameと機能的には同じで、init-robot-from-nameでは必要だったload文が必要なくなって一般的な記述ができる代わりに、各ロボットのeusパッケージのpackage.xmlにexportタグを書く必要があります。
    init-robot-from-nameは一般的な記述を目指していたはずなので、現在のrobot-init関数が上位互換にあたります。
    また、init-robot-from-nameの悪い点として、(ロボット名)-init関数の返り値がロボットモデルのインスタンスであることを仮定していることがあり、この仮定は現在崩壊しています。
    https://github.com/jsk-ros-pkg/jsk_pr2eus/blob/master/pr2eus/pr2-interface.l#L569-L582
    以上の点から、init-robot-from-nameを書き換えて、内部でrobot-init関数を呼ぶようにするのがよいと考えます。
  • make-robot-interface-from-nameについて
    この関数は、*ri*のインスタンスをロボット名から作って返り値にしてくれる関数なので、厳密にはrobot-initで同じことができません。robot-initのようなrospack pluginsを使った実装に書きなおし、loadの必要をなくすことで、robot-initと同じ抽象度になると思います。

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.

3 participants