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

Add demux #106

Merged
merged 18 commits into from
Jul 9, 2024
Merged

Add demux #106

merged 18 commits into from
Jul 9, 2024

Conversation

rcywongaa
Copy link
Contributor

@rcywongaa rcywongaa commented May 27, 2024

Completes #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:

  • Callbacks use lambda instead of std::bind to get default arguments working

@rcywongaa rcywongaa mentioned this pull request May 27, 2024
@rcywongaa rcywongaa force-pushed the add_demux branch 3 times, most recently from f649f80 to 7229331 Compare May 28, 2024 15:58
@rcywongaa rcywongaa marked this pull request as ready for review May 28, 2024 16:00
@rcywongaa rcywongaa requested a review from a team as a code owner May 28, 2024 16:00
@rcywongaa rcywongaa requested review from emersonknapp and jhdcs May 28, 2024 16:00
rcywongaa added 10 commits May 30, 2024 14:49
Signed-off-by: Rufus Wong <[email protected]>
Signed-off-by: Rufus Wong <[email protected]>
Signed-off-by: Rufus Wong <[email protected]>
Signed-off-by: Rufus Wong <[email protected]>
Signed-off-by: Rufus Wong <[email protected]>
Signed-off-by: Rufus Wong <[email protected]>
Signed-off-by: Rufus Wong <[email protected]>
@christophebedard christophebedard self-requested a review June 4, 2024 21:14
@rcywongaa
Copy link
Contributor Author

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

@MichaelOrlov
Copy link
Member

@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
Copy link
Contributor Author

rcywongaa commented Jun 8, 2024

@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
Copy link
Member

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.

@rcywongaa
Copy link
Contributor Author

rcywongaa commented Jun 12, 2024

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

topic_tools/test/test_topic_tool.hpp Show resolved Hide resolved
topic_tools/src/demux_node.cpp Show resolved Hide resolved
topic_tools/src/demux_node.cpp Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
topic_tools/src/demux.cpp Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
topic_tools/src/tool_base_node.cpp Outdated Show resolved Hide resolved
rcywongaa added a commit to rcywongaa/topic_tools that referenced this pull request Jun 13, 2024
rcywongaa added a commit to rcywongaa/topic_tools that referenced this pull request Jun 13, 2024
rcywongaa added a commit to rcywongaa/topic_tools that referenced this pull request Jun 13, 2024
rcywongaa added a commit to rcywongaa/topic_tools that referenced this pull request Jun 13, 2024
This addresses comment
ros-tooling#106 (comment)

Signed-off-by: Rufus Wong <[email protected]>
rcywongaa added a commit to rcywongaa/topic_tools that referenced this pull request Jun 13, 2024
@rcywongaa rcywongaa requested a review from MichaelOrlov June 21, 2024 09:44
@MichaelOrlov
Copy link
Member

@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
Copy link
Contributor Author

@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. 🙇

Copy link
Member

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@rcywongaa Thanks for the iterations and your patience.
Overall looks good now.
However, I would like you to ask to drop your Update CI to use noble rolling
commit from this PR, since we already have these changes on master.
See merged PR #107 for details.

@MichaelOrlov MichaelOrlov linked an issue Jul 9, 2024 that may be closed by this pull request
@rcywongaa
Copy link
Contributor Author

@MichaelOrlov Done!

Copy link
Member

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

LGTM.

@MichaelOrlov MichaelOrlov merged commit ad48bc0 into ros-tooling:rolling Jul 9, 2024
12 checks passed
@MichaelOrlov
Copy link
Member

https://github.com/Mergifyio backport jazzy

Copy link

mergify bot commented Jul 9, 2024

backport jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 9, 2024
* Copy, rename and make buildable

Signed-off-by: Rufus Wong <[email protected]>

* Swap input and output topic

Signed-off-by: Rufus Wong <[email protected]>

* Reset publisher if output topic changes

Signed-off-by: Rufus Wong <[email protected]>

* Add tests

Signed-off-by: Rufus Wong <[email protected]>

* Fix test failures

Signed-off-by: Rufus Wong <[email protected]>

* Fix header guard

Signed-off-by: Rufus Wong <[email protected]>

* Make linter happy

Signed-off-by: Rufus Wong <[email protected]>

* Add README entry

Signed-off-by: Rufus Wong <[email protected]>

* Remove lazy parameter

Signed-off-by: Rufus Wong <[email protected]>

* Rename default input topic to ~/input

Signed-off-by: Rufus Wong <[email protected]>

* Fix typo

Signed-off-by: Rufus Wong <[email protected]>

* Remove usage of ament_cmake_auto

Signed-off-by: Rufus Wong <[email protected]>

* Add missing dependency on ament_cmake

Signed-off-by: Rufus Wong <[email protected]>

* Fix uncrustify warnings

Signed-off-by: Rufus Wong <[email protected]>

* Add initial_topic_ variable to avoid confusion

This addresses comment
#106 (comment)

Signed-off-by: Rufus Wong <[email protected]>

* Fix outtopic spelling

This addresses comment
#106 (comment)

Signed-off-by: Rufus Wong <[email protected]>

* Add error message on incorrect number of arguments

Addresses
#106 (comment)

Signed-off-by: Rufus Wong <[email protected]>

* Formatting

Signed-off-by: Rufus Wong <[email protected]>

---------

Signed-off-by: Rufus Wong <[email protected]>
(cherry picked from commit ad48bc0)
MichaelOrlov pushed a commit that referenced this pull request Jul 9, 2024
* Copy, rename and make buildable

Signed-off-by: Rufus Wong <[email protected]>

* Swap input and output topic

Signed-off-by: Rufus Wong <[email protected]>

* Reset publisher if output topic changes

Signed-off-by: Rufus Wong <[email protected]>

* Add tests

Signed-off-by: Rufus Wong <[email protected]>

* Fix test failures

Signed-off-by: Rufus Wong <[email protected]>

* Fix header guard

Signed-off-by: Rufus Wong <[email protected]>

* Make linter happy

Signed-off-by: Rufus Wong <[email protected]>

* Add README entry

Signed-off-by: Rufus Wong <[email protected]>

* Remove lazy parameter

Signed-off-by: Rufus Wong <[email protected]>

* Rename default input topic to ~/input

Signed-off-by: Rufus Wong <[email protected]>

* Fix typo

Signed-off-by: Rufus Wong <[email protected]>

* Remove usage of ament_cmake_auto

Signed-off-by: Rufus Wong <[email protected]>

* Add missing dependency on ament_cmake

Signed-off-by: Rufus Wong <[email protected]>

* Fix uncrustify warnings

Signed-off-by: Rufus Wong <[email protected]>

* Add initial_topic_ variable to avoid confusion

This addresses comment
#106 (comment)

Signed-off-by: Rufus Wong <[email protected]>

* Fix outtopic spelling

This addresses comment
#106 (comment)

Signed-off-by: Rufus Wong <[email protected]>

* Add error message on incorrect number of arguments

Addresses
#106 (comment)

Signed-off-by: Rufus Wong <[email protected]>

* Formatting

Signed-off-by: Rufus Wong <[email protected]>

---------

Signed-off-by: Rufus Wong <[email protected]>
(cherry picked from commit ad48bc0)

Co-authored-by: Rufus Wong <[email protected]>
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.

Port demux
3 participants