[feature] Portable Keyboard source

ubgk commented 1 year ago

This PR introduces a portable keyboard observer, inspired by this blog post.

To the best of my knowledge, there exists no portable way of reading keyboard device events (like we couldn't find one for Joystick, either). This observer therefore reads from the standard input to map incoming characters to respective keys (e.g. 'a' -> A,'A' -> a).

ASCII characters are encoded by a single byte, and their byte values are platform independent.

Other non-ASCII keys, such as arrow keys, write to the standard input, however, the specific sequence is platform specific. I tried decoding them on my macOS Ventura and on an Ubuntu 22.04 VM.

The implementation should also work over SSH, so it should be possible to run the observer on Upkie and control over SSH.

Features:

Portable implementation (ASCII alphanumericals are guaranteed to work everywhere AFAIK)
Support for non-ASCII keys such as arrow keys (we should discuss if we want to keep this or not)
SSH support as a by-product of the followed approach

Limitations:

Arrow key byte sequences tested on only two machines
No support for simultaneous key presses

ubgk commented 1 year ago

@stephane-caron Does clang-format fail on your machine too? I use version 16.0.1 installed with brew whereas the CI linter has version 14.0.0-1ubuntu1.1?

stephane-caron commented 1 year ago

I tested on my machine with

clang-format -style=file -i observation/sources/Keyboard.cpp

And it yields some changes to Keyboard.h as well. My local version is:

$ clang-format --version
clang-format version 16.0.6 (https://github.com/ssciwr/clang-format-wheel 6ec821d5d3ffdd226d5475a0b5e1da6317e4d9a1)

stephane-caron commented 1 year ago

Can we add unit tests for this new source?

You can take inspiration from e.g. the Joystick one https://github.com/tasts-robots/vulp/pull/47. It's OK if we don't get 100% coverage here, just covering the essentials would be fine.

stephane-caron commented 1 year ago

(On a side note, the difference between "sources" and "observers" in Vulp is that "sources" do not take inputs from other observers/sources. I'm thinking we should actually rename them to "sensors" which makes more sense in the robotics context.)

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 5831214847

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

225 of 237 (94.94%) changed or added relevant lines in 5 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.3%) to 87.935%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
observation/sources/Joystick.cpp	2	3	66.67%
observation/sources/Keyboard.cpp	54	65	83.08%
<!--	Total:	225	237	94.94%	-->

Files with Coverage Reduction	New Missed Lines	%
observation/sources/Joystick.cpp	1	98.06%
<!--	Total:	1		-->

Totals
Change from base Build 5819417842:	0.3%
Covered Lines:	1618
Relevant Lines:	1840

💛 - Coveralls

ubgk commented 1 year ago

@stephane-caron I now added some tests for the Keyboard source.

stephane-caron commented 1 year ago

Sounds good! I'll review this PR today.

ubgk commented 1 year ago

Some comments on the code.

I tried the observer locally, pressing the up/down/left/right keys in the terminal where the spine is run:

[2023-08-11 14:40:11.392] [info] Stop cycle 5 / 5
[2023-08-11 14:40:47.940] [warning] Skipped 2 clock cycles
[2023-08-11 14:40:47.958] [warning] Skipped 3 clock cycles
[2023-08-11 14:40:47.961] [warning] Skipped 1 clock cycles
[2023-08-11 14:40:47.970] [warning] Skipped 3 clock cycles
[2023-08-11 14:40:48.134] [warning] Skipped 2 clock cycles
[2023-08-11 14:40:48.342] [warning] Skipped 1 clock cycles
[2023-08-11 14:40:52.603] [info] Applying "bullet" runtime configuration...
[2023-08-11 14:40:52.604] [info] Applying "wheel_contact" runtime configuration
[2023-08-11 14:40:52.604] [info] Applying "floor_contact" runtime configuration
[2023-08-11 14:40:52.604] [info] Applying "wheel_odometry" runtime configuration
[2023-08-11 14:40:52.604] [info] Start requested by agent
^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[A^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[B^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[C^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^C[2023-08-11 14:41:10.818] [info] Shutdown cycle 1 / 5
[2023-08-11 14:41:10.819] [info] Shutdown cycle 2 / 5
[2023-08-11 14:41:10.820] [info] Shutdown cycle 3 / 5
[2023-08-11 14:41:10.821] [info] Shutdown cycle 4 / 5
[2023-08-11 14:41:10.821] [info] Wrapping up last communication cycle
[2023-08-11 14:41:10.821] [info] Shutdown cycle 5 / 5

But my log foxplot /tmp/2023-08-11_143853_bullet_spine.mpack -l /observation/keyboard/UP does not show any activity:

Could you please try this again? I removed the "Linux bytes" and ran upkie remotely on an AWS EC2. It seemed to work fine there.

I think I also addressed all your comments too.

stephane-caron commented 1 year ago

Thanks for following up :smiley: I tried again with the same procedure:

Terminal 1: ./start_simulation.sh
Terminal 2: run an agent (the MPC agent, but not relevant here)
Go back to Terminal 1 and press the UP or DOWN keys

I get the following warnings though:

^[[A[2023-08-16 11:24:48.354] [warning] Skipped 8 clock cycles
[2023-08-16 11:24:48.357] [warning] All bytes could not be read from the standard input!
^[[A^[[A^[[A[2023-08-16 11:24:48.458] [warning] All bytes could not be read from the standard input!
^[[A^[[A[2023-08-16 11:24:48.509] [warning] All bytes could not be read from the standard input!
^[[A^[[A^[[A[2023-08-16 11:24:48.610] [warning] All bytes could not be read from the standard input!
^[[A^[[A[2023-08-16 11:24:48.661] [warning] All bytes could not be read from the standard input!
^[[A^[[A^[[A[2023-08-16 11:24:48.763] [warning] All bytes could not be read from the standard input!
^[[A^[[A[2023-08-16 11:24:48.814] [warning] All bytes could not be read from the standard input!
^[[A^[[A^[[A[2023-08-16 11:24:48.916] [warning] All bytes could not be read from the standard input!
^[[A^[[A[2023-08-16 11:24:48.967] [warning] All bytes could not be read from the standard input!
^[[A^[[A^[[A[2023-08-16 11:24:49.069] [warning] All bytes could not be read from the standard input!
^[[A^[[A[2023-08-16 11:24:49.120] [warning] All bytes could not be read from the standard input!
^[[A^[[A[2023-08-16 11:24:49.171] [warning] All bytes could not be read from the standard input!
^[[A^[[A^[[A[2023-08-16 11:24:49.273] [warning] All bytes could not be read from the standard input!
^[[A^[[A[2023-08-16 11:24:49.324] [warning] All bytes could not be read from the standard input!
^[[A^[[A^[[A[2023-08-16 11:24:49.425] [warning] All bytes could not be read from the standard input!
^[[A^[[A[2023-08-16 11:24:49.476] [warning] All bytes could not be read from the standard input!
^[[A^[[A^[[A[2023-08-16 11:24:49.577] [warning] All bytes could not be read from the standard input!
^[[A^[[A[2023-08-16 11:24:49.628] [warning] All bytes could not be read from the standard input!
^[[B[2023-08-16 11:24:50.153] [warning] Skipped 12 clock cycles
[2023-08-16 11:24:50.178] [warning] Skipped 12 clock cycles

But the output seems correct :+1:

Is the All bytes could not be read from the standard input! situation a concern?

ubgk commented 1 year ago

But the output seems correct 👍

Thanks for trying it out.

Is the All bytes could not be read from the standard input! situation a concern? We get that because the input is faster than the polling interval.

This was useful for debugging, but now that I think of it, you'll almost always find more than 3 bytes in STDIN if you use arrow keys (unless your keyboard inputs exactly once per polling, which unlikely to be the case), so I'll just disable the warning.

stephane-caron commented 1 year ago

This was useful for debugging, but now that I think of it, you'll almost always find more than 3 bytes in STDIN if you use arrow keys (unless your keyboard inputs exactly once per polling, which unlikely to be the case), so I'll just disable the warning.

Okay, I haven't checked this fully, but the keyboard is working and we can always revisit this point if it turns out to be itchy in the future. Let's merge!

I also have a follow-up question https://github.com/tasts-robots/vulp/issues/49 :wink:

upkie / vulp