monitor: make Ctrl-] work when running on WSL (v2) #1209
Conversation
|
Please note that I'm a python beginner. If there are better ways e.g. to 'unbuffer' stdin etc., please let me know. |
Because the TIOCSTI termios ioctl is not supported on WSL (bash on windows) at this time, another way for cancelling a blocking read on stdin is needed. Also, setting a timeout using termios.c_cc[VMIN] = 0 with termios.c_cc[VTIME] = 1 does not currently work for WSL. A first approach to mimic the Windows kludge based on kbhit() by using ioctl(TIOCINQ) for peeking at the input queue before calling read() basically worked but failed badly for fast multi character input like escape sequences sent back by the terminal. This can be easily reproduced using the console example (examples/system/console). The root cause was identified to be the buffering involved reading and decoding stdin in python. The current solution uses pthread_kill() via ctypes (python 2) to unblock getkey() in ConsoleReader which seems to be working well. However, even in this case stdin must have been re-opened without buffering. Otherwise, the read() may be restarted internally after seeing EINTR. This change has been successfully tested on WSL with the console example, escape sequences (UP, DOWN keys), Ctrl-] to leave the monitor and Ctrl-T/Ctrl-A for re-flashing. Unfortunately, testing on other systems was not possible at this time.
There was a problem hiding this comment.
Nice work finding a solution for this. Sorry it took me a while to test it. I finally got WSL installed myself and had a poke around.
I don't really like this as a bunch of hacks to get around WSL limitations, but at the same time it seems to work and I don't know a better way to unblock stdin (except possibly to poll using select() and have a timeout, which seems icky in its own way).
I was a bit skeptical about killing a pthread inside of the Python interpreter, but in my tests I couldn't break anything and could run multiple gdb sessions in a row, etc, so it seems harmless enough... and the existing code is going to crash on WSL anyhow, so anything that's less crashy is probably an improvement. ;)
I left a couple of minor comments about code style.
tools/idf_monitor.py
Outdated
|
|
||
| def run(self): | ||
| if self.pthread_self != None: |
There was a problem hiding this comment.
Typical Python style is to use "is not None" instead of "!= None".
In this case (and in the same check below) I'd be more comfortable using if running_on_wsl everywhere as this makes it easier to trace all the of the WSL workarounds (which can hopefully some day be removed!)
There was a problem hiding this comment.
(Also if you don't need to test whether self.pthread_self is None, you can remove the self.pthread_self = None default value settings from the constructor)
| except IOError: | ||
| if not self.alive: | ||
| # expected during _cancel when killed | ||
| break |
There was a problem hiding this comment.
This code swallows the IOError if self.alive is true. Does it make more sense to re-raise it?
tools/idf_monitor.py
Outdated
| @@ -220,6 +245,20 @@ class Monitor(object): | |||
| """ | |||
| def __init__(self, serial_instance, elf_file, make="make", toolchain_prefix=DEFAULT_TOOLCHAIN_PREFIX, eol="CRLF"): | |||
| super(Monitor, self).__init__() | |||
| posix_is_wsl = False | |||
There was a problem hiding this comment.
What do you think about naming this running_on_wsl to match the field used in the Console instance? Another change that makes it easier to search for all of the WSL-related hacks at once. :)
|
Thank you very much for the valuable feedback! I will take care ASAP, but not before 2017-11-26 as I will be travelling till Sunday. |
Instead of unconditionally swallowing IOError on stdin, re-raise the exception when monitor is still active. Only suppress it during unblocking of stdin, when it is expected. (Suggested by projectgus.)
- rename posix_is_wsl to running_on_wsl - conditions for WSL specific code depend on 'running_on_wsl' (Suggested by projectgus.)
|
Hi, I tried to implemented the requested changes. |
|
Anyway, I will try to find a select() based solution, now that it is clear that `unbuffered stdin' seems to be key. I would also be more comfortable without having to use pthread_kill() ;-) |
This seems like a good idea. You can probalby make the
OK, that sounds great. I will hold off on doing anything more with this PR until I hear how you get on. :) |
|
Closing in favour of new approach in #1324. |
Because the TIOCSTI termios ioctl is not supported on WSL (bash on
windows) at this time, another way for cancelling a blocking read on
stdin is needed. Also, setting a timeout using termios.c_cc[VMIN] = 0
with termios.c_cc[VTIME] = 1 does not currently work for WSL.
A first approach to mimic the Windows kludge based on kbhit() by using
ioctl(TIOCINQ) for peeking at the input queue before calling read()
basically worked but failed badly for fast multi character input like
escape sequences sent back by the terminal. This can be easily
reproduced using the console example (examples/system/console). The root
cause was identified to be the buffering involved reading and decoding
stdin in python.
The current solution uses pthread_kill() via ctypes (python 2) to
unblock getkey() in ConsoleReader which seems to be working well.
However, even in this case stdin must have been re-opened without
buffering. Otherwise, the read() may be restarted internally after
seeing EINTR.
This change has been successfully tested on WSL with the console
example, escape sequences (UP, DOWN keys), Ctrl-] to leave the monitor
and Ctrl-T/Ctrl-A for re-flashing. Unfortunately, testing on other
systems was not possible at this time.