Skip to content
This repository has been archived by the owner on Jan 28, 2023. It is now read-only.

sender with yaml config #62

Merged
merged 7 commits into from
Jan 3, 2020

Conversation

MiraiHattori
Copy link
Contributor

@MiraiHattori MiraiHattori commented Jan 1, 2020

#3

いつかsim_sender.cppをsim_sender_node.cppに,sim_senderをsim_sender_nodeに改名したい気持ちがあるのですが,diffが見づらいのでこれだけPRを出しておきます.

@MiraiHattori
Copy link
Contributor Author

#8
についても,READMEを追加しました.
consai2からの移植という意味で,sim_sender.cppにはコメントを追加していません.

@ShotaAk
Copy link
Contributor

ShotaAk commented Jan 3, 2020

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

末尾に_nodeを付けないとNGなんでしたっけ?(素朴な疑問)

@@ -0,0 +1,53 @@
# consai2_sender
Copy link
Contributor

Choose a reason for hiding this comment

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

ついでにconsai2r2_senderにしてくれるとありがたいです。

-->

## 参考ページ
### Google proto files -->
Copy link
Contributor

Choose a reason for hiding this comment

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

末尾の-->の削除をお願いします

@@ -8,13 +8,16 @@
<license>MIT</license>

<buildtool_depend>ament_cmake</buildtool_depend>
<buildtool_depend>python_cmake_module</buildtool_depend>
Copy link
Contributor

Choose a reason for hiding this comment

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

python_cmake_moduleは必要ですか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不要なので,削除する方向で修正します.ありがとうございます.

@@ -32,6 +32,7 @@
#include "grSim_Replacement.pb.h"

using std::placeholders::_1;
using namespace std::chrono_literals;
Copy link
Contributor

Choose a reason for hiding this comment

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

chrono_literalsは将来的に使うから追加したのでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらに関しては,同期パラメータクライアントが必要だと思いこんでいた際にパラメータ取得待ちのため追加したものでした.
現在は不要なので削除しようと思います.

@ShotaAk
Copy link
Contributor

ShotaAk commented Jan 3, 2020

動作チェックしました。
コメントの確認をお願いします。

@MiraiHattori
Copy link
Contributor Author

MiraiHattori commented Jan 3, 2020

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

末尾に_nodeを付けないとNGなんでしたっけ?(素朴な疑問)

これに関しては,規定はないと思っています.
ただ,「rclcpp::Nodeを継承したクラスを定義した.hppに対応する,実装を書いたcppファイル」と「.hppに書かれたクラスを使用してmain関数でループを回すcppファイル」がファイル名レベルで分かれていたほうが可読性が高いのではないかと思っています.
https://github.com/ros2/examples/tree/master/rclcpp/minimal_composition/src
がそのような運用になっていそうです.

@MiraiHattori
Copy link
Contributor Author

MiraiHattori commented Jan 3, 2020

[追記]よくみたところ,実際のところは逆で,main関数がある方は_nodeではないようです...よって,_nodeはいらないというのが正しそうです

…sender/package.xml] remove python_cmake_module, [consai2r2_sender] remove using chrono literals
@MiraiHattori
Copy link
Contributor Author

修正してpushしました.

@ShotaAk
Copy link
Contributor

ShotaAk commented Jan 3, 2020

確認しました。マージします。
ノード名に関しては、また気づきがあったら適宜修正しましょう。

@ShotaAk ShotaAk merged commit 72eeff9 into SSL-Roots:master Jan 3, 2020
@MiraiHattori MiraiHattori deleted the dev/sender-with-yaml-config branch January 3, 2020 14:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants