SSL-Roots / consai2r2

https://github.com/SSL-Roots/consai_ros2 へ開発を移行しています。
MIT License
8 stars 7 forks source link

Impremented RefereeReceiver #21

Closed spiralray closed 4 years ago

spiralray commented 4 years ago

boostだぁいすき❤️(きらい)(わからん) #11

ShotaAk commented 4 years ago

とりあえず boostだぁいすき:heart:(きらい)(わからん) を確認しました。

HansRobo commented 4 years ago

他のノードを見ている感じ,C++の機能を使っているヘッダファイルは.hpp拡張子になっているのでそちらのほうが良さ気? 参考 : consai2r2_teleop cc : @ShotaAk

ShotaAk commented 4 years ago

C/C++の双方で扱えるコードの場合は.hでいいと思います。 C++のみであれば.hppにしましょう。(自分にも言い聞かせます)

https://index.ros.org/doc/ros2/Contributing/Developer-Guide/ コーディングスタイルガイドでは特に言及されてないようです。

ShotaAk commented 4 years ago

@future731 さんがおっしゃるとおり、CIがmasterをcloneしてたので修正しました。

直したことにより、boostでビルド失敗が発覚しました。

ShotaAk commented 4 years ago

@spiralray さん ~#34 でいろいろ整えられてますが~(#34で整えてるのはvision_receiverの方でした) 、まずはこちらのPR単体でチェックしてマージしましょう。

https://github.com/SSL-Roots/consai2r2/pull/21#discussion_r355128850 にかかれているとおり、 systemを追記することでビルドが通り、動作確認できました。

OSによって変わるあれば、CMakeLists.txtにOS判定を追加しないといけないですね・・

ShotaAk commented 4 years ago

multicast.hpp の場所について調べてみました。

https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#package-layout よると

src: contains all C and C++ code
    Also contains C/C++ headers which are not installed
include: contains all C and C++ headers which are installed
    <package name>: for all C and C++ installed headers they should be folder namespaced by the 
    package name

と書かれているので、installする場合はinclude/consai2r2_receiverに、そうでない場合はsrcに入れるようです。 なので、multicast.hppをinstallする前提でinclude/consai2r2_receiver以下に移動しましょう。

ShotaAk commented 4 years ago

他のconsai2r2_パッケージもヘッダーファイルをinstallしてないので修正が必要ですね。(別issueで後ほどやります)

こんな感じでヘッダーファイルのinstallが必要そうです。

install(
  DIRECTORY include/
  DESTINATION include)

参考:https://github.com/ros2/rosbag2/blob/master/rosbag2/CMakeLists.txt

ShotaAk commented 4 years ago

動作確認中なのでもう少しお待ちを

spiralray commented 4 years ago

CI通すためにrebaseしました。

22 についても現在のbranchから生えるようにrebaseします。

ShotaAk commented 4 years ago

わかりました。チェックします。

ShotaAk commented 4 years ago

@spiralray Referereちゃんと動いてて感動

とても些細なことなのですが・・・ Referee.msg、RefereeGameEnvet.msg、RefereeTeamInfo.msg game_event.proto、referee.proto が実行ファイルになっています。 この原因は何でしょうか・・・? (エラーは起きないけど、気なる案件)

自分の環境だけかもしれないので、確認お願いします。

ShotaAk commented 4 years ago

動作確認しました。ありがとうございます。 ↑のファイル形式の問題が解決できしだい、マージします。

1点、 ssl-game-controllerのリポジトリを確認すると、refereeのプロトコルが更新されているようでした。 https://github.com/RoboCup-SSL/ssl-game-controller/blob/master/pkg/refproto/ssl_referee.proto こちらは、このPRをマージ後に、別issueとして立てます。 (これ以上このPRが肥大化すると\(^o^)/)

spiralray commented 4 years ago

レビューありがとうございます。

install/consai2r2_receiver 内を確認しましたが、私の環境では仰っている実行ファイルは見つけられませんでした。 (install/consai2r2_receiver/include/consai2r2_receiverディレクトリ内に.protoのシンボリックリンクが貼られてしまう問題はありますが…)

ShotaAk commented 4 years ago

installではなく、includeディレクトリでした。 こちらはどうでしょうか?

spiralray commented 4 years ago

特に実行ファイルは見当たりません。

そもそも.protoファイルはincludeディレクトリに置くべきではないのでしょうね… cmakeでincludeディレクトリ以下はinstall対象としていますし、protoファイルはreceiver node以外で必要とはならないので。

spiralray commented 4 years ago

36 を確認したところ、srcもおかしいですね…

パッケージ直下にprotoディレクトリ配置するのが正しい?

ShotaAk commented 4 years ago

僕の説明が下手だったかもしれません。 実行権限 +x が与えられてるのでなんでかな〜っと思っただけです。 もしかしたら流用元からなってるかもしれないので、深入りしなくて良いですね。

➜  proto git:(spiralray-dev/referee-receiver) ls -la
合計 40
drwxrwxr-x 2 shotaak-rt shotaak-rt 4096 12月 12 21:33 .
drwxrwxr-x 3 shotaak-rt shotaak-rt 4096 12月 12 21:33 ..
-rwxrwxr-x 1 shotaak-rt shotaak-rt 2833 12月 12 21:33 game_event.proto
-rw-rw-r-- 1 shotaak-rt shotaak-rt  983 12月 12 21:33 messages_robocup_ssl_detection.proto
-rw-rw-r-- 1 shotaak-rt shotaak-rt 2205 12月 12 21:33 messages_robocup_ssl_geometry.proto
-rw-rw-r-- 1 shotaak-rt shotaak-rt  225 12月 12 21:33 messages_robocup_ssl_refbox_log.proto
-rw-rw-r-- 1 shotaak-rt shotaak-rt  241 12月 12 21:33 messages_robocup_ssl_robot_status.proto
-rw-rw-r-- 1 shotaak-rt shotaak-rt  230 12月 12 21:33 messages_robocup_ssl_wrapper.proto
-rwxrwxr-x 1 shotaak-rt shotaak-rt 5931 12月 12 21:33 referee.proto

protoファイルはinstall対象で良いと思います。 consai2では(美しくないですが)referee_wrapperでprotoファイルを使ってるので、外部パッケージで使用される可能性はあると思います。 https://github.com/SSL-Roots/consai2/blob/master/consai2_game/scripts/example/referee_wrapper.py

spiralray commented 4 years ago

https://github.com/SSL-Roots/consai2r2/issues/36#issuecomment-565445722

ShotaAk commented 4 years ago

protoの配置に着いては #36 に書きます。 こちらは動作確認できたのでマージします。