search

ros-tooling / topic_tools

Tools for directing, throttling, selecting, and otherwise manipulating ROS 2 topics at a meta-level.
Apache License 2.0
71 stars 33 forks source link

Add demux #106

Closed rcywongaa closed 2 months ago

rcywongaa commented 3 months ago

Completes https://github.com/ros-tooling/topic_tools/issues/105

This PR porting https://github.com/ros/ros_comm/blob/noetic-devel/tools/topic_tools/src/demux.cpp to the ROS 2.

demux is a generic ROS topic demultiplexer: one input topic is fanned out to 1 of N output topics. A service is provided to select between the outputs

Design choices:

rcywongaa commented 3 months ago

The test fail doesn't seem related to my changes so this PR is considered done. Let me know otherwise. Thanks!

MichaelOrlov commented 3 months ago

@rcywongaa Actually Build_and_test job failure relates to the usage of the ament_auto_add_library and ament_auto_add_executable in your PR. In general need to avoid those "magic" macros with amen_auto. At least we are not using them in the ROS 2 core packages.

rcywongaa commented 3 months ago

@MichaelOrlov Ah ok, I've fixed that and changed all the other stuff to not use ament_cmake_auto.

Any reason why ament_cmake_auto is discouraged? Seems quite nifty

MichaelOrlov commented 3 months ago

Any reason why ament_cmake_auto is discouraged? Seems quite nifty

It is an evil at the end and creates a lot of mess since we don't control what it includes as dependencies and what it exports as dependencies. Originally, it was introduced for quick prototyping but not for production-ready code.

MichaelOrlov commented 3 months ago

@rcywongaa Could you please address Uncrustify warnings?

Code style divergence in file 'src/demux_node.cpp':
2024-06-11T21:44:52.5204969Z     
2024-06-11T21:44:52.5205315Z     --- src/demux_node.cpp
2024-06-11T21:44:52.5205743Z     +++ src/demux_node.cpp.uncrustify
2024-06-11T21:44:52.5206238Z     @@ -116 +116 @@
2024-06-11T21:44:52.5206846Z     -      get_node_topics_interface()->resolve_topic_name(request->topic);
2024-06-11T21:44:52.5207776Z     +             get_node_topics_interface()->resolve_topic_name(request->topic);
2024-06-11T21:44:52.5208459Z     @@ -155 +155 @@
2024-06-11T21:44:52.5209056Z     -      get_node_topics_interface()->resolve_topic_name(output_topic_);
2024-06-11T21:44:52.5209947Z     +             get_node_topics_interface()->resolve_topic_name(output_topic_);
2024-06-11T21:44:52.5210980Z     @@ -172 +172 @@
2024-06-11T21:44:52.5211849Z     -        get_node_topics_interface()->resolve_topic_name(request->topic);
2024-06-11T21:44:52.5212766Z     +               get_node_topics_interface()->resolve_topic_name(request->topic);
2024-06-11T21:44:52.5213401Z
rcywongaa commented 3 months ago

Fixed, strangely, it doesn't complain locally or on my fork...

MichaelOrlov commented 2 months ago

@rcywongaa I didn't forget about your PR. Just have a busy week, and we need to clean up the mess on a branches for topic_tools since we incorrectly released from rolling to the Jazzy.

Sorry for the delay, and thank you for your patience.

rcywongaa commented 2 months ago

@MichaelOrlov No worries, sorry didn't mean to rush you. Just not too sure what the standard procedure is here. I'll avoid re-requesting a review in the future. :bow:

rcywongaa commented 2 months ago

@MichaelOrlov Done!

MichaelOrlov commented 2 months ago

https://github.com/Mergifyio backport jazzy

mergify[bot] commented 2 months ago

backport jazzy

✅ Backports have been created

* [#109 Add demux (backport #106)](https://github.com/ros-tooling/topic_tools/pull/109) has been created for branch `jazzy`