Add option to disable recorder keyboard controls

[berndpfrommer]

Currently only the Player node has an option to disable the keyboard control. This PR adds the same feature to the Recorder node.

When I instantiate the Recorder within C++ as a node and directly run it with e.g. a SingleThreaded executor, it messes up my terminal to the point that I can no longer read the output and need to reset the terminal. Very annoying. Also, I don't want to see output about allowed keystrokes in my log files. This output may have made sense when the recorder was only run from the CLI, but now that the Recorder is a bona-fide composable node, the logging is unwanted.

It turns out that already instantiating (not just using) a KeyboardHandler messes up my terminal, so this PR avoids creating a KeyboardHandler altogether when record_options.disable_keyboard_controls is true.

[MichaelOrlov]

Hi @berndpfrommer I am the author of the KeyboardHandler package.
As regards:

It turns out that already instantiating (not just using) a KeyboardHandler messes up my terminal, so this PR avoids creating a KeyboardHandler altogether when record_options.disable_keyboard_controls is true.

It messes up your terminal because we instantiate KeyboardHandler with a disabled signals handler inside. Because we used to handle SIGTERM and SIGINT signals in the rosbag2 python wrapper layer and gracefully finish the recorder and player.

rosbag2/rosbag2_transport/src/rosbag2_transport/recorder.cpp

Lines 748 to 753 in 1bb7a0e

	#ifndef _WIN32
	auto keyboard_handler = std::make_shared<KeyboardHandler>(false);
	#else
	// We don't have signal handler option in constructor for windows version
	auto keyboard_handler = std::shared_ptr<KeyboardHandler>(new KeyboardHandler());
	#endif

However, in the cases when rosbag2 recorder or player instantiating via composition signals are not handled by the rosbag2 anymore. Therefore need to instantiate the KeyboardHandler class in those cases with install_signal_handlere parameter equal to true

See description for KeyboardHandler constructor here
https://github.com/ros-tooling/keyboard_handler/blob/13072acb5c4c07b3ad220b2bb7bd3f3080824414/keyboard_handler/include/keyboard_handler/keyboard_handler_unix_impl.hpp#L48-L55

/// \note In case if install_signal_handler is false caller code should call static
/// KeyboardHandlerUnixImpl::restore_buffer_mode_for_stdin() in case of process termination
/// caused by signal arrival.

[MichaelOrlov]

@berndpfrommer LGTM after adding a lot more above your original commit.
I've made a rebase on top of the latest Rolling and made the following changes:

• Add CLI option --disable-keyboard-controls for rosbag2 recorder
• Instantiate KeyboardHandler with installed signal handler by default
  in the recorded and player constructors to mitigate issues when CTRL+C
  not handled on upper layers as when player and recorder constructed via
  composition.
• Explicitly construct KeyboardHandler with disabled signal handler from
  rosbag2_py layer since we have our own signals handler on that layer.
• Add test coverage for parameters parser for disable_keyboard_controls
  parameters.
• Regenerate Python stub files (.pyi) for rosbag2_py package.

Please review and test that it works as you expected.
With the latest fixes recorder and player shall not "mess up" terminal output when player or recorder constructed via composition even with disable_keyboard_controls = false

[MichaelOrlov]

Gist: https://gist.githubusercontent.com/MichaelOrlov/43a20975aa8e739bcbce1d49a7a02379/raw/ec8c69bbd8e43a7ed596cd692b447bff7983a981/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_py rosbag2_transport rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_py rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13696

• Linux
• Linux-aarch64
• Windows

[berndpfrommer]

Wow. What happened to my little PR :) ?
Thanks for fixing this so quickly, and fixing the player as well. I don't really know the code base well enough to make a meaningful review, but I will test on Monday (on the command line and by instantiating the nodes in c++ code).

[MichaelOrlov]

@berndpfrommer Thanks for the review.
I am sorry, I don't have time for nitpick. If we want this fix to be in the Jazzy release we need to merge it today.

[berndpfrommer]

Giving up on trying to test this. I cannot build anymore on Ubuntu 22.04/rolling without complete build from source.
The version upgrade to yaml-cpp #1605 also requires updating a whole bunch of other stuff, including eventually rclcpp. Testing must wait until the other dependencies have settled down and I have a stable build environment again that does not involve compiling the entire distro from source. Delaying testing until Noble is released.

[MichaelOrlov]

@berndpfrommer Hmm. I always compile from sources.
However, on my Ubuntu 22.04/rolling setup I found out that need to install liblz4-dev before trying to build code after the recent updates sudo apt install liblz4-dev. Don't know if it specific to my local setup.