urata_hrpsys_config.pyを分離する

[kyawawa]

@YutaKojio さんと話したのですが，ロボット毎の設定が全てurata_hrpsys_config.pyに書かれており，非常に編集しづらいかつ，gitのdiffも見づらい状況になっています．
jaxon_hrpsys_config.pyやchidori_hrpsys_config.pyでURATAHrpsysConfiguratorを継承したクラスを作り，そこで設定を行おうと思うのですが問題のある方はいますでしょうか？ >
@kindsenior @YuyaNagamatsu @shintarokkk

[YuyaNagamatsu]

いいと思いますが，ただ，
今後*_hrpsys_config.pyに書くべき内容のフォーマットに仕様変更があるたびに，それに気づいた人が自分のロボットのコンフィグだけ編集してロボットごとにフォーマットがバラバラになる状況が容易に想像できるので，
できる限りそうなりにくいような配慮があるといいと思います．
例えばhrpsys.hrpsys_configのAPIを直接たたく文は極力共通のURATAHrpsysConfigurator内に一般化して定義するなど．

[shintarokkk]

@kyawawa
良いと思います。

[kyawawa]

ロボット毎の継承にすると
https://github.com/start-jsk/rtmros_choreonoid/blob/master/hrpsys_choreonoid_tutorials/src/hrpsys_choreonoid_tutorials/choreonoid_hrpsys_config.py
など，URATAHrpsysConfiguratorを継承しているクラスの記述が大変になりそうだということに気付いて手が止まりました．

[k-okada]

今後*_hrpsys_config.pyに書くべき内容のフォーマットに仕様変更があるたびに，それに気づいた人が自分のロボットのコンフィグだけ編集してロボットごとにフォーマットがバラバラになる状況が容易に想像できるので，できる限りそうなりにくいような配慮があるといいと思います．

これは重要だと思います。この瞬間はバラバラになりましたね、でおわりますが、１年たつと、あのファイルは整理されていなくて使いずらい、新しいのを作るべきだ、となって、無限ループに入ります。

URATAHrpsysConfiguratorを継承しているクラスの記述

は何が書いてあるんだろう？ connectConstraintForceLoggerPorts はなんだろう？ 一般の二足歩行ロボットに必要なrあ、さらに上流のConfigurator に渡すようなことも視野に入れるとよいと思います。 パット見でabcとrfsensorをここでつなげているのは何か変な気がします。 理想を言うと、ロボット毎に必要な情報は実機が持っているので、そこから必要なソースコード、あるいは、データは作られるべきです。 例えば、簡単なところでは、どのRTCを組み込むかは人が書いてあるとしても、そうだった時の残りの作業は記述のひつようはないですね。 https://github.com/start-jsk/rtmros_choreonoid/blob/master/hrpsys_choreonoid_tutorials/src/hrpsys_choreonoid_tutorials/choreonoid_hrpsys_config.py#L33 あるいは、ロボットのモデルはModelLoaderにはいっているから、コンパイル時にurdfやeuslispのモデルも、本当は作らなくてよいようになっています。 が、あえて、それをやっている、ということは押さえておくとよいと思います。

…

-- ◉ Kei Okada

-- ◉ Kei Okada 2018年12月22日(土) 19:19 Tatsuya Ishikawa <notifications@github.com>:

ロボット毎の継承にすると https://github.com/start-jsk/rtmros_choreonoid/blob/master/hrpsys_choreonoid_tutorials/src/hrpsys_choreonoid_tutorials/choreonoid_hrpsys_config.py など，URATAHrpsysConfiguratorを継承しているクラスの記述が大変になりそうだということに気付いて手が止まりました． — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[kyawawa]

これは重要だと思います。この瞬間はバラバラになりましたね、でおわりますが、１年たつと、あのファイルは整理されていなくて使いずらい、新しいのを作るべきだ、となって、無限ループに入ります。

はい．この点に関しては気をつけながら作業を進めていきます．

URATAHrpsysConfiguratorを継承しているクラスの記述

に該当するファイルの中身をあまり理解できておらず，Choreonoidや実機特有の設定が書いていそうだということしか分かっていません．
経緯はわかりませんがabcとsensorをつなげている部分は現在はコメントアウトされており，rtcレベルで見てもabcがセンサ情報を使っている部分はないため，消して良い部分だと思われます．
コメントアウトされていませんでした．(rtcレベルの話で)こちらはref force周りで必要なんでしたっけ？ > @YutaKojio
Choreonoid以外でも必要なのであれば，上流に流していきます．
その他で上流に渡せそうな箇所は渡していくつもりです．

例えば、簡単なところでは、どのRTCを組み込むかは人が書いてあるとしても、そうだった時の残りの作業は記述のひつようはないですね。

ここの理解が及ばなかったのですが，例えば今の場合どのような部分が必要のないところに当たるのでしょうか？
また，あえて，というのはどのような判断の下で行われるべきなのでしょうか(実行の複雑さ，速度，ソフトウェアの分離などでしょうか．)

[YutaKojio]

コメントアウトされていませんでした．(rtcレベルの話で)こちらはref force周りで必要なんでしたっけ？

ログを増やしているだけで制御には関係ないですが，おそらくchoreodnoiの導入当初のデバッグ用とかでしょうか？？

Choreonoid以外でも必要なのであれば，上流に流していきます．
その他で上流に渡せそうな箇所は渡していくつもりです．

ロボットの歩行制御等の確認用のログとしても必要なさそうなので，上流に流さず消すだけでいいと思います．

[YoheiKakiuchi]

何回か、同じ議論をしている気がしていて、少し整理したいので一旦待ってもらえますか。

[YoheiKakiuchi]

@YutaKojio さんと話したのですが，ロボット毎の設定が全てurata_hrpsys_config.pyに書かれており，非常に編集しづらいかつ，gitのdiffも見づらい状況になっています．

本当にこれが達成されるかほどほどに疑問

例えばhrpsys.hrpsys_configのAPIを直接たたく文は極力共通のURATAHrpsysConfigurator内に一般化して定義するなど．

これについては賛成
過去に、hrpsysのidl (STやautobalancerなど)の変更で変更が必須になることがあって、
個別にすべてに対応する必要があって、それはどうにかなって欲しい

[kyawawa]

何回か、同じ議論をしている気がしていて、少し整理したいので一旦待ってもらえますか。

整理していただいた後でも構わないですが，過去の議論のスレッドやログがあれば共有していただけると助かります．

本当にこれが達成されるかほどほどに疑問

if文を消してファイルとクラスを分けるだけで可読性は上がると考えていたのですがイマイチでしょうか．
ここの方針が間違っていると全てが無駄なので，何か意見ある方はお願いします．

過去に、hrpsysのidl (STやautobalancerなど)の変更で変更が必須になることがあって、

こちら，該当コミットなどはありますでしょうか．

[YoheiKakiuchi]

整理していただいた後でも構わないですが，過去の議論のスレッドやログがあれば共有していただけると助かります．

https://github.com/jsk-ros-pkg/trans_system/pull/401
#333
ここらへんでしょうか。もっと継承の構成とかで議論があった気がするけど見つからない。

パラメータ設定部分が分かりにくいのはそうだろうと思うのですが、pythonまわりのわかりにくさの根本は、
どういう継承になっていて、どこにファイルがあって、メソッド、パラメータがどこで設定されているか分からない
ということなのだろうと思います。
なので、まずはこのpythonまわりのクラスの関係や、なぜそうなっているか、派生させる場合の例、気をつけポイント
などをまとめたドキュメントがあって欲しい。

過去に、hrpsysのidl (STやautobalancerなど)の変更で変更が必須になることがあって、
こちら，該当コミットなどはありますでしょうか．

これはこういう流れだね。
idlの変更
fkanehiro/hrpsys-base@0f702be#diff-fdb21e47f442c727ac9cacb88bbb65a9

pythonの修正
5ec611c#diff-7735c73bd331a515e088d009888ef158

pythonの修正はすべてのロボットに必要となるはず（設定していないパラメータがあれば問題は発覚しないが、隠れているだけ）

[kyawawa]

パラメータ設定部分が分かりにくいのはそうだろうと思うのですが、pythonまわりのわかりにくさの根本は、
どういう継承になっていて、どこにファイルがあって、メソッド、パラメータがどこで設定されているか分からない
ということなのだろうと思います。

これは僕もそう思います．
素晴らしく簡潔な継承の構成を見つければ解決する問題なのでしょうか．
それは難しそうなのでまずはPythonのクラスにdocstringを書いてみます．

これはこういう流れだね。

これ僕のcommitですね．失礼しました．
この件は完全に忘れていましたが，パラメータの型や変数名が変わった時はすべての修正が必要だという認識はありました．
ただ，この問題への対応策は思いつきませんでした．．．

[kyawawa]

dbbf0d5 にてdocstringを書いた上で，今回の変更を適用した場合の継承関係の図も書いてみました．

この構成である理由や注意点などは含められませんでしたが，クラスの関係性や各クラスでどのようなことをやっているのかの簡単な説明は理解できる図になったと思います．
今回の主な目的は，図真ん中のURATAHrpsysConfiguratorと下のRobotの設定を分離し，各ロボットを使う人が管理しやすくすることになります．

[YutaKojio]

やっぱりこの分離が必要だと感じてきましたが，どうでしょう．

現在，urata_hrpsys_configに各ロボット固有のパラメータが書かれており，
それを継承した，
(1) choreonoidを使う場合： choreonoid_hrpsys_config
(2) 実機を使う場合: urata_real_hrpsys_config
を使うようになっています．

そのため，urata系ロボット以外(レポジトリ自体も違う)では上記の(1),(2)をそれぞれ継承してさらにロボット固有のパラメータをそれぞれ設定する必要があります．
固有パラメータが(1),(2)の場合の２つのファイルに同じ内容が書かれたものが存在してしまっていて，管理が非常にしづらいです．（某ロボットが実際今そうなっています）

そこで，#557 (comment) の図のような構成にすると簡潔に解決すると思っています．