ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
124 stars 139 forks source link

StdIn Not Handled in `ExecuteProcess`? #735

Open mwcondino opened 11 months ago

mwcondino commented 11 months ago

Bug report

Required Info:

Steps to reproduce issue

Run this file with ros2 launch ./test.launch.py

test.launch.py:

"""Launch file for testing."""

import asyncio as aio
import contextlib
import sys
import termios

from launch import LaunchDescription
from launch.actions import ExecuteProcess, OpaqueCoroutine

from launch.events.process import ProcessStdin

# From https://stackoverflow.com/questions/35223896/listen-to-keypress-with-asyncio
@contextlib.contextmanager
def raw_mode(file):
    old_attrs = termios.tcgetattr(file.fileno())
    new_attrs = old_attrs[:]
    new_attrs[3] = new_attrs[3] & ~(termios.ECHO | termios.ICANON)
    try:
        termios.tcsetattr(file.fileno(), termios.TCSADRAIN, new_attrs)
        yield
    finally:
        termios.tcsetattr(file.fileno(), termios.TCSADRAIN, old_attrs)

# Function to emit an event when a key is pressed
async def read_char_from_stdin(_launch_context, action=None):
    with raw_mode(sys.stdin):
        reader = aio.StreamReader()
        loop = aio.get_event_loop()
        await loop.connect_read_pipe(lambda: aio.StreamReaderProtocol(reader), sys.stdin)

        while not reader.at_eof():
            ch = await reader.read(1)
            # '' means EOF, chr(4) means EOT (sent by CTRL+D on UNIX terminals)
            if not ch or ord(ch) <= 4:
                break
            print(f'Got: {ch!r}')

            pid = action.process_details['pid']
            await _launch_context.emit_event(
                event=ProcessStdin(
                    text=ch, action=action, name=action.name, cmd=action.cmd,
                    cwd=action.cwd, env=action.env, pid=pid))

def generate_launch_description():
    """Generate the args and items to launch."""
    # Create the launch description and populate
    ld = LaunchDescription()

    cmd_list = ['ros2', 'bag', 'play', 'test_bag']

    bag_runner = ExecuteProcess(
        cmd=cmd_list,
        emulate_tty=True,
    )

    ld.add_action(OpaqueCoroutine(coroutine=read_char_from_stdin, kwargs={'action': bag_runner}))

    ld.add_action(bag_runner)

    return ld

Once that is running, press space - the expected behavior is that the space character would be forwarded to the ExecuteProcess action which is playing back the bag (any bag will work for this test, so long as the name matches up).

However, when I press space (or really any character), I get this warning:

[WARNING] [ros2-1]: in ExecuteProcess('140309050411568').__on_process_stdin_event()

Expected behavior

The ProcessStdin event should forward data to the matching process.

Actual behavior

It looks like there's just a warning message. From looking into the code here, it appears as if the function is not really implemented, unless I'm missing something.

I think that ExecuteProcess may need some updating to correctly pipe data passed into the __on_process_stdin_event function to the underlying subprocess.

clalancette commented 11 months ago

I actually didn't realize we had ProcessStdin. So maybe there is a bug here.

But I will say that this whole thing is an anti-pattern, in my opinion. The point of launch is to run multiple processes, so which process should stdin be forwarded to? It's totally unclear. I guess we could have some kind of 'marker' where one and only one process can process stdin, but at that point why not just open another terminal?

But maybe others have a different opinion.

mwcondino commented 11 months ago

I don't think I made my use case super clear - the example launch is a simplified version of a launch file I'd like to use to start up some nodes while also playing back bag data. I would find it convenient to be able to pause/start the bag process at will.

I suppose I could run that in a separate terminal, but there's some default args I will always want to pass to the bag process, and it's convenient to do that from a launch file. Furthermore, there are certain topics I'd like to conditionally remap based on launch configurations, and this becomes really clunky without a dedicated script to generate the playback command.

On Fri, Sep 15, 2023, 5:26 AM Chris Lalancette @.***> wrote:

I actually didn't realize we had ProcessStdin. So maybe there is a bug here.

But I will say that this whole thing is an anti-pattern, in my opinion. The point of launch is to run multiple processes, so which process should stdin be forwarded to? It's totally unclear. I guess we could have some kind of 'marker' where one and only one process can process stdin, but at that point why not just open another terminal?

But maybe others have a different opinion.

— Reply to this email directly, view it on GitHub https://github.com/ros2/launch/issues/735#issuecomment-1721193196, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIW4W2IO3OCHIIG7AL5SG73X2RCQLANCNFSM6AAAAAA4ZCFETI . You are receiving this because you authored the thread.Message ID: @.***>

alexsmau commented 8 months ago

Hi @mwcondino,

I am in the same situation, having a lunch file containing, among other things, a ros2 bag play with a bunch of remappings and the like. Like you, stdin is disabled so I cannot pause/resume/ at will.

I did find a work-around using rqt. Just type rqt and a GUI will pop up. Then go to the tab Plugins->Services->Service Caller. In the Service drop-down list find /rosbag2_player/pause (or resume or whatever) and then click on the Call button. And presto! You have a Pause/Resume :)

The downside is that it's quite cumbersome since you have to select the action each time from a drop-down list. But it is not that bad if you only have to do it rarely. Perhaps there is a way to make a list of services you want to call and just double-click them... Maybe....

Hope this is useful.