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

make pacledt-compose'Node' #9

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

make pacledt-compose'Node' #9

wants to merge 7 commits into from

Conversation

Noir-swim
Copy link

#3

@Noir-swim Noir-swim requested a review from H1rono June 2, 2024 07:49
Copy link
Member

@H1rono H1rono left a comment

Choose a reason for hiding this comment

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

とりあえずこれだけ
あと今入ってる日本語のコメント全部消してください

packet/packet_composed/CMakeLists.txt Outdated Show resolved Hide resolved
packet/packet_composed/CMakeLists.txt Outdated Show resolved Hide resolved
packet/packet_composed/CMakeLists.txt Outdated Show resolved Hide resolved
packet/packet_composed/CMakeLists.txt Outdated Show resolved Hide resolved
packet/packet_composed/src/composed.cpp Outdated Show resolved Hide resolved
packet/packet_composed/src/composed_main.cpp Outdated Show resolved Hide resolved
@Noir-swim Noir-swim requested a review from H1rono June 10, 2024 12:47
packet/packet_composed/package.xml Outdated Show resolved Hide resolved
packet/packet_composed/package.xml Outdated Show resolved Hide resolved
@Noir-swim Noir-swim requested a review from H1rono June 10, 2024 13:18
packet/packet_composed/src/composed.cpp Outdated Show resolved Hide resolved
…とをしている気がします。ご指摘いただきたいです。よろしくお願いいたします。
@Noir-swim
Copy link
Author

slackでお騒がせしてすみません。
今、packet_interfacesのmsgファイルに対する出力関係のノード等をFigma(https://www.figma.com/board/auJaW4Ava0yzcklB8T6eJX/24jam?node-id=0-1&t=zgHRTrbp86YCIbpF-1)
見ながら作ったつもりのコードをpushしました。
しかし、ディレクトリの構造が違ったり、解釈違いで余計なことをしている気がします。
ご指摘いただきたいです。
また、トピック名は良い名前が思いつかずhppファイルの名前をそのまま書きました。改名を希望しております。
よろしくお願いいたしますm(_ )m m( _)m

@Noir-swim Noir-swim requested a review from H1rono June 17, 2024 14:11
Copy link
Member

@H1rono H1rono left a comment

Choose a reason for hiding this comment

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

作成するnodeは一つだけでいいです
欲しいのは"複数のtopicをsubscribeして一つのtopicにまとめるnode"なので, 以下のようなクラス定義が与えられます

class Composed : public rclcpp::Node {
private:
    rclcpp::Publisher<packet_interfaces::msg::Composed>::SharedPtr composed_publisher;

    rclcpp::Subscription<packet_interfaces::msg::Depth>::SharedPtr depth_subscription;
    rclcpp::Subscription<sensor_msgs::msg::Imu>::SharedPtr         imu_subscription;
    rclcpp::Subscription<packet_interfaces::msg::Flex>::SharedPtr  flex1_subscription;
    rclcpp::Subscription<packet_interfaces::msg::Flex>::SharedPtr  flex2_subscription;
    // current, voltageのsubscriptionは略

    void depth_topic_callback(const packet_interfaces::msg::Depth& msg);
    void imu_topic_callback(const sensor_msgs::msg::Imu& msg);
    void flex1_topic_callback(const packet_interfaces::msg::Flex& msg);
    void flex2_topic_callback(const packet_interfaces::msg::Flex& msg);
    // current, voltageのcallbackは略

    // さらにいくつかのメソッド, プロパティが必要になる

public:
    Composed();
};

より具体的には, "subscribeしたtopicの内容を記憶し, それらを組み合わせたtopicを定期的にpublishする"といったnodeになると思います.

@Noir-swim
Copy link
Author

今、コメントを読んでコードを変更しているところです。

私がやる方向性として
複数のセンサーデータを受信し、それをまとめて別のメッセージとして発行するノードのヘッダーファイルを作る。
Composedクラスがセンサーデータ(Depth、IMU、Flexなど)を受信し、それらを1つのComposedメッセージとしてまとめてパブリッシュする。ということまで理解しております。←あってますでしょうか?

このことを理解してpacket_composed/src内にあるcompsed.cppとcomposed_main.cpp、CMakeLists.txt、package.xmlt,
include内のsensorspacket.hppに変更を加えてPR出そうとしています。

ということは、この前私がPRを出したときにdepthなどの複数のノードを作成したパッケージは削除して、一番最初に作成したcomposedのファイルだけ残せばよいのでしょうか?

ちなみにまだ、ファイルを一切消していません。。

よろしくお願いいたします。

@H1rono
Copy link
Member

H1rono commented Jun 23, 2024

複数のセンサーデータを受信し、それをまとめて別のメッセージとして発行するノードのヘッダーファイルを作る。
Composedクラスがセンサーデータ(Depth、IMU、Flexなど)を受信し、それらを1つのComposedメッセージとしてまとめてパブリッシュする。ということまで理解しております。

あってる!Flexは2つあるので注意してください。
あと、センサーデータはそれぞれ別のタイミングで送られてくるので、それらをまとめてpublishするためにタイマーが必要になるかもしれません(何か別の方法でpublishの処理を書いてもいいです)。

ということは、この前私がPRを出したときにdepthなどの複数のノードを作成したパッケージは削除して、一番最初に作成したcomposedのファイルだけ残せばよいのでしょうか?

あってる!!他のやつはバッサリ捨てちゃってください 🔥

@Noir-swim
Copy link
Author

ありがとうございます!!!
了解です。考えてみます!

@Noir-swim
Copy link
Author

Noir-swim commented Jun 25, 2024

#3
お疲れ様です。PR出してみました。

色々と調べて、Composedクラスがセンサーデータ(Depth、IMU、Flexなど)を受信し、それらを1つのComposedメッセージとしてまとめてパブリッシュする。
センサーデータをまとめてpublishするためにタイマーを作る。
つもりのコードを書き足してみました。

[自分がした大まかな手順]
①不要なファイルを削除した。
以下、[composed.cpp、sensorspacket.hpp,CMakeLists.txt,package.xmlt]を変更させた。
②複数のサブスクリプションを通じて、(Depth、IMU、Flex1、Flex2、Current、Voltage)を受信させた。
③各センサーデータに対応するmsgを受信し、それをComposed_msgに追加してパブリッシュした。
④コンストラクタの初期化してメンバ変数を初期化した。
⑤コールバックごとに新しいComposedメッセージを作成するのではなく、1つのメッセージを再利用して更新した。

このノードでやりたいことはわかるのですが、、
ところどころ文法などが滅茶苦茶になっていると思います。。
雑なコードですみません(._.)
添削お願いします。。

@Noir-swim Noir-swim requested a review from H1rono June 25, 2024 15:30
Copy link
Member

@H1rono H1rono left a comment

Choose a reason for hiding this comment

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

やりたいのは「複数topicを受信して、 一つの topicにまとめて 同時に publishする」ということです。今のコードだと「一つのtopicにまとめる」ところはできてますが、「同時にpublishする」というのができていません。これを実現するためには、以下のようにする必要があります。

  • subscriptionで受信したtopicを一度保存する
  • タイマー機能で、保存されているtopicを一つにまとめてpublishする

この「一度保存する」というのをどう実現するかが少し難しいところですが、考えて見てください。(しばらく考えてわかんなかったら教えてください)

packet/packet_composed/.vscode/c_cpp_properties.json Outdated Show resolved Hide resolved
packet/packet_composed/.vscode/settings.json Outdated Show resolved Hide resolved
packet/packet_composed/package.xml Outdated Show resolved Hide resolved
Comment on lines 15 to 16
_publisher = this->create_publisher<std_msgs::msg::String>("/sensors_composed", 10);
composed_publisher = this->create_publisher<packet_interfaces::msg::Composed>("composed_topic", 10);
Copy link
Member

Choose a reason for hiding this comment

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

stringのpublishはしません & topic名はFigmaのやつ(から/packet/のプレフィックスを取り除いたもの)と合わせるようにお願いします

Suggested change
_publisher = this->create_publisher<std_msgs::msg::String>("/sensors_composed", 10);
composed_publisher = this->create_publisher<packet_interfaces::msg::Composed>("composed_topic", 10);
composed_publisher = this->create_publisher<packet_interfaces::msg::Composed>("sensors_composed", 10);

Comment on lines 18 to 34
depth_subscription = this->create_subscription<packet_interfaces::msg::Depth>(
"depth_topic", 10, std::bind(&Composed::depth_topic_callback, this, std::placeholders::_1));

imu_subscription = this->create_subscription<sensor_msgs::msg::Imu>(
"imu_topic", 10, std::bind(&Composed::imu_topic_callback, this, std::placeholders::_1));

flex1_subscription = this->create_subscription<packet_interfaces::msg::Flex>(
"flex1_topic", 10, std::bind(&Composed::flex1_topic_callback, this, std::placeholders::_1));

flex2_subscription = this->create_subscription<packet_interfaces::msg::Flex>(
"flex2_topic", 10, std::bind(&Composed::flex2_topic_callback, this, std::placeholders::_1));

current_subscription = this->create_subscription<packet_interfaces::msg::Current>(
"current_topic", 10, std::bind(&Composed::current_topic_callback, this, std::placeholders::_1));

voltage_subscription = this->create_subscription<packet_interfaces::msg::Voltage>(
"voltage_topic", 10, std::bind(&Composed::voltage_topic_callback, this, std::placeholders::_1));
Copy link
Member

Choose a reason for hiding this comment

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

ここのtopic名も書き換えてください

Copy link
Member

Choose a reason for hiding this comment

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

スクリーンショット 2024-06-28 17 02 07

ここの/packet/プレフィックスを取り除いたものなので、sensors_composed, sensors/imu, sensors/flex/1, ...って感じです

packet/packet_composed/src/composed.cpp Outdated Show resolved Hide resolved
@Noir-swim
Copy link
Author

一旦、上記のコメント読んで直したファイルをcommit→PR出しました。
👀のマークがついているところは直しが怪しいところです。
間違っていたらご指摘お願いします。

「同時にpublishする」という部分については考え中です。
subscriptionで受信したtopicを一度保存するタイマー機能で、保存されているtopicを一つにまとめてpublishする。方法について今考えています。
日曜日まで考えてみます。m(_ _)m

個人的に浮かんだ考え方はcomposed.cppのcomposedのループを利用すればいいと思っています。

void Composed::_loop() {
    std::stringstream ss;
    ss << "Hello, world! " << _count;
    std_msgs::msg::String msg{};
    msg.data = ss.str();
    RCLCPP_INFO(this->get_logger(), "say %s", ss.str().c_str());
    _publisher->publish(msg);
    _count++;
}

この部分にif文を追加して、同時に受け取ったときpublishするというif文を作る。
「一度保存させる」ためにはsensorspacket.hppのprivate メンバにそれぞれのsubscliptionで受信したtopicが適切に入れるような関数を個々につくって保存する。
この2つの方向性で考えています。。

@H1rono
Copy link
Member

H1rono commented Jun 28, 2024

個人的に浮かんだ考え方はcomposed.cppのcomposedのループを利用すればいいと思っています

あってる。_loop関数内でcomposed_publisher->publishを呼ぶことになるはずです

この部分にif文を追加して、同時に受け取ったときpublishするというif文を作る。

subscriptionのcallbackとtimerのcallbackは非同期に呼ばれるので、「同時に受け取った時」を判定するのはとても難しそうです。

「一度保存させる」ためにはsensorspacket.hppのprivate メンバにそれぞれのsubscliptionで受信したtopicが適切に入れるような関数を個々につくって保存する。

良さそうだけど、関数ではなく状態、ないしメンバ変数かな

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.

None yet

2 participants