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

Made zenoh router runnable in launch file #22

Closed

Conversation

CihatAltiparmak
Copy link
Member

@CihatAltiparmak CihatAltiparmak commented Jul 30, 2024

With this PR, we will be able to run zenoh router in launch file. This PR provides us to work on automatizing to benchmark all middleware stuffs until zenoh router can be started implicitly inside rmw layer. I used SIGTERM to close zenoh router process, but rmw_zenoh doesn't handle SIGTERM whereas handles SIGINT. I have sent a PR for it. If it's merged, this PR will work well. Maybe it can be used SIGINT however i'm besider of SIGTERM.

@CihatAltiparmak CihatAltiparmak changed the title Made runnable zenoh router in launch file Made zenoh router runnable in launch file Jul 30, 2024
@CihatAltiparmak
Copy link
Member Author

@sjahr it can be better to review this in the your available time. Even if we don't merge this stuffs before other PRs, i t would be better to review this in the frontline before merging.

@henningkayser
Copy link
Member

I'm sure we discussed this previously, but what is the main reason to directly call the router instead of using ExecuteProcess? https://github.com/ros2/launch/blob/rolling/launch/launch/actions/execute_process.py

@CihatAltiparmak
Copy link
Member Author

CihatAltiparmak commented Aug 5, 2024

Because zenoh router needs to use /dev/tty as stdin. We cannot set stdin in ExecuteProcess. the functionality of launch tool doesn't allow to set stdin, Even if we use shell=True parameter and set stdin using ros2 run rmw_zenoh_cpp rmw_zenoh < /dev/tty, launch closes process unsoundly, it somehow trigger neither the SIGTERM nor SIGINT. I could solve this using separate subprocess mechanism, which allows us to modify stdin of process directly. See my comment in this issue. ( ros2/rmw_zenoh#206 (comment) )

But i'm okay with use ros2 run rmw_zenoh_cpp rmw_zenohd in this PR. This time perror gives rise to ugly ending, but all of the process finishes gracefully. See my comment ( ros2/rmw_zenoh#252 (comment) )

@henningkayser
Copy link
Member

Because zenoh router needs to use /dev/tty as stdin. We cannot set stdin in ExecuteProcess. the functionality of launch tool doesn't allow to set stdin, Even if we use shell=True parameter and set stdin using ros2 run rmw_zenoh_cpp rmw_zenoh < /dev/tty, launch closes process unsoundly, it somehow trigger neither the SIGTERM nor SIGINT. I could solve this using separate subprocess mechanism, which allows us to modify stdin of process directly. See my comment in this issue. ( ros2/rmw_zenoh#206 (comment) )

But i'm okay with use ros2 run rmw_zenoh_cpp rmw_zenohd in this PR. This time perror gives rise to ugly ending, but all of the process finishes gracefully. See my comment ( ros2/rmw_zenoh#252 (comment) )

Thank you! IMO, this explanation should be documented in the launch file. Otherwise 👍 from me

@CihatAltiparmak CihatAltiparmak linked an issue Aug 18, 2024 that may be closed by this pull request
@CihatAltiparmak
Copy link
Member Author

CihatAltiparmak commented Aug 19, 2024

Let's merge after #26 and #30 are merged.

But i'm okay with use ros2 run rmw_zenoh_cpp rmw_zenohd in this PR.

Now seems not okay with use of it 🫤

@CihatAltiparmak
Copy link
Member Author

I'm closing this PR because some PRs about zenoh_router is solved so that tty is removed and SIGTERM handling is added. I have decided to add zenoh_router to other PRs which is addition of other scenarios.

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.

Document how to start zenoh router in launch file
2 participants