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

"tockloader listen" proof of concept #2

Closed
wants to merge 11 commits into from

Conversation

george-cosma
Copy link
Contributor

@george-cosma george-cosma commented May 3, 2023

This pull request is a WIP proof of concept for the "tockloader listen" subcommand.

Currently, I am able to listen on a given port for output (tested on a micro:bit v2). It works both on Windows and under WSL.

Notes regarding implementation

This PR adds the following libraries:

  • tokio - for creating async code
  • tokio-serial - for reading and writing to serial
  • tokio-util
  • bytes

Notes regarding the python version

The python version of 'tockloader listen' has a lot of functionalities and I'd like note down some of them here.

  • Automatic port detection
  • Stops and Restarts automatically if another tockloader session starts
  • Optionally timestamps and counts recieved messages
  • Hijacks CTRL+C to close gracefully
  • Attempts to auto-reconnect

While creating the proof of concept I'll attempt to implement as many of these functionalities as it is reasonable.

@hudson-ayers
Copy link

Can you convert this to draft if it is still WIP? Or do you want this to be merged in its current state and then keep working from there?

@george-cosma george-cosma marked this pull request as draft May 4, 2023 19:09
@george-cosma
Copy link
Contributor Author

Can you convert this to draft if it is still WIP? Or do you want this to be merged in its current state and then keep working from there?

Oops! Yep, I wanted to make it a draft, but I forgot :D
I changed it now.

@alexandruradovici alexandruradovici added the needs-rebase This pull request needs a rebase label May 5, 2023
@george-cosma
Copy link
Contributor Author

Here's a small update:

In my last commit I added the ability for async read/write to the serial port with the help of a new crate "console_rs" . This crate helps with reading from console without echo-ing output to the user. As things stand now, when a letter is received by the board, it automatically re-sends it back to the host PC.

Currently, there is one issue: on WSL and under an Ubuntu Virtual Machine, when waiting for input, if the board sends output the final output is a bit wonky. The error seems to be coming from the "console" crate.
image

versus on Windows:
image

An issue and a PR are already opened, so as things stand there are good chances this will be fixed in the near future.
console-rs/console#136
console-rs/console#165

CTRL+C is captured by the "console" crate, but it decides to still send the signal rather then passing the key-press. I'll try in my next commit to capture the interrupt signal to exit gracefully.

@alexandruradovici
Copy link
Collaborator

I think the output problem might be due to the way the console interprets \n. It might be that WSL just displays a the received characters as is (\n is go to a new line without line feed) while Windows display a newline (adds a \r in front)?

@george-cosma
Copy link
Contributor Author

Oh, that might be it! I'll try to experiment with it.

@alexandruradovici
Copy link
Collaborator

I think you should always display \r\n when receiving \n.

@george-cosma
Copy link
Contributor Author

george-cosma commented May 12, 2023

I've implemented your suggestion and now it seems to work as expected. Thank you!

@george-cosma
Copy link
Contributor Author

In this last commit I've implemented port auto-detection with the help of tokio-serial, and I've played around a bit more with how I send data to the board.

Previously, I was using the read_char() function which would give me a singular character without echoing it to the screen. I would then send it to the board and the board would echo back the sent character, just as intended. However, with this function all non-ascii input (backspace, arrow keys, etc) wouldn't be send to the board.

In this commit I've switched over to the read_key() function which is a bit more involved. While now the arrow keys and backspace work as intended, when I tried testing this on my Linux Virtual Machine I ran into an issue.

After starting to type I wouldn't receive any more data from the board. The program keeps sending key presses over, but the program wouldn't receive anything back. (I printed to screen a debug message saying what it was sending over, and it kept sending the correct characters. So it was not a printing-to-screen issue.)

I'll try to investigate this strange behavior some more, but I have to say I'm not quite sure why this is happening.

@george-cosma
Copy link
Contributor Author

I have figured out why on my Ubuntu VM the board wasn't working as expected.

The Unexpected Behaviour

After starting to put input into the console, there was a non-zero chance the program would go into a state where the console could still read input but no output from console would be displayed. This usually happens after 2 or 3 key presses.

The Actual Problem

After some deliberation, I had started to suspect that tokio couldn't start the read task because the key task would nearly never yield. The issue is also compounded by the fact that the key press is thread-blocking. After some digging around my hunch might be correct. In this stack overflow post a user reports that Console.ReadKey() in C# blocks the console. I suspect that something similar is happening here.

The Solution

The solution that seems to work on all three of the platforms i can test on (Windows 10 Native, WSL, Linux VM) is to put the "read_key" call under a blocking task (thread?). This seems to solve the issue.

@george-cosma
Copy link
Contributor Author

george-cosma commented May 24, 2023

I've cleaned up the code a bit, and I think it's time I made this PR open for review. While it still doesn't implement all of the functionalities the original "listen" had, I think it's in a good shape for a prototype, something to work off of. So far, I've tested this code on Windows 10, WSL running under Windows 10 and an Ubuntu 22.04 Virtual Machine with a BBC Microbit v2.

@george-cosma george-cosma marked this pull request as ready for review May 24, 2023 21:54
@mateibarbu19
Copy link

Hello, I have found that your implementation works, but has problems when typing the "backspace" after a uncompleted word. It starts removing the caracters from the promt, such as $.

@george-cosma
Copy link
Contributor Author

I can't replicate your issue, could you tell me what OS you used, the terminal (bash/powershell/...), the board you used and the kernel version?

Before, if an invalid UTF-8 sequence was decoded, the program would error out. Now, it will decode as much as it and can keep the remaining uninterpreted bytes in its buffer.
Don't be like me, add the checks to a git hook
@mateibarbu19
Copy link

I can't replicate your issue, could you tell me what OS you used, the terminal (bash/powershell/...), the board you used and the kernel version?

I have tested the latest version of your branch (8930527) on a native Debian 12 machine for a user whose shell is Bash. Two terminal emulators (Konsole and WezTerm) yielded the same results for two boards: microbit_v2 and stm32f412gdiscovery.

@george-cosma
Copy link
Contributor Author

george-cosma commented Aug 24, 2023

Hmm, I have running both Konsole and WezTerm on my Ubuntu 22 virtual machine and on a Debian 12 virtual machine I have am still not experiencing what you describe. I am using a microbit_v2 using kernel version 2.1

Kernel version: 2.1 (build release-2.1-988-g0503b8dc1)

If you are using the same kernel version? If yes, I fear that this might actually happen only on native linux machines. I can try to work on a fix, but I'll be doing it blindly.

I'm not 100% sure how Tock is handling user-input behind the scenes, but it seems to just echo it back on the serial interface, which makes the issue even stranger. I don't remember if I encountered this at some point also whilst figuring out the console crate, but then why can't I replicate it on a VM?

Possibly a related issue is when pressing tab. For me, when I press tab and then backspace, only 1 space is deleted. After pressing backspace again, no more characters are deleted. I'll try to investigate this issue some more...

Edit

I've dug a bit in the tock source code, and the two issues might be very well connected. I've also noticed that left/right arrows don't work for me, neither for python tockloader, nor tockloader-rs. This is quite strange. I'll try to update my kernel - maybe there's something I'm missing. In the meantime, could you please test if left/right arrows work for you (on python version preferably, it's more stable than this)? Nevermind, it was just that I didn't update my kernel. Though I still don't have issues with backspace

@george-cosma
Copy link
Contributor Author

Hopefully that bug should be solved by this newest commit. The issue appears only in newer versions of the kernel, specifically after the code that handled "backspace" was updated in the kernel. Instead of the usual

<backspace><space><backspace>

to delete the last character, an additional

<null/EOF><backspace><space><backspace>

was snuck in. The reason python tockloader worked was because minterm replaces this sequence of bytes with unicode code point U+2400. Compared to a null byte, this is actually can be deleted and interacted with.

In my testing another issue has raised up: I can't pipe input or output from tockloader-rs because of the way the console crate works. It works on stdout rather than stdin. I don't know how much of an issue this is, though possibly raw_tty could help us. Alternatively, while talking to @mateibarbu19 , he pointed out the existence of rustcom a project that has to deal with serial input.

To note, I'd like to also refactor this code once a code structure is established ( see #8 )

@lschuermann
Copy link
Member

I think you should always display \r\n when receiving \n.

Actually, arguably, we shouldn't do that -- Tock usually takes care of always using a full CRLF for newlines, as just a CR or LF alone are very useful control characters when drawing to a terminal emulator over a serial port. It's unfortunate that we still have places in Tock where we don't do this, and we should fix this upstream instead, even just for the sake of working with more full-featured serial monitors in their default configuration.

I'm personally not invested in tockloader listen, neither in the Python version or this reimplementation, so intepreting an LF as a full line-break is fine with me here.

@george-cosma
Copy link
Contributor Author

george-cosma commented Sep 4, 2023

[...] we should fix this upstream instead [...]

I agree with you here. If the issue is a simple one, I might tackle it myself.

Edit: The \n thing is on me: The _heart program only printed \n. Though I've found some backspace/delete issues in the kernel code

Yet another issue

I've found yet another issue: holding down the arrow keys. This one seems to originate from the console crate I've found. When holding down the left arrow key, here's what happens if I write 'abcd' and hold down the left arrow key:

Note:
[src/serial_interface.rs:45] &source = b" ... " <--- this is what the computer receives
[src/serial_interface.rs:214] &buffer = " ... " <---- this is what the computer sends ( / board receives)
--------------------------------------

[src/serial_interface.rs:45] &source = b"Initialization complete"
Initialization complete[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:45] &source = b". Entering main loop.\r\n"
. Entering main loop.
[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:45] &source = b"t"
t[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:45] &source = b"ock$ "
ock$ [src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:214] &buffer = "a"
[src/serial_interface.rs:45] &source = b"a"
a[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:214] &buffer = "b"
[src/serial_interface.rs:45] &source = b"b"
b[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:214] &buffer = "c"
[src/serial_interface.rs:45] &source = b"c"
c[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:214] &buffer = "d"
[src/serial_interface.rs:45] &source = b"d"
d[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:45] &source = b"\x08"
[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:45] &source = b"\x08"
[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:45] &source = b"\x08"
[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:45] &source = b"\x08"
[src/serial_interface.rs:45] &source = b""
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
^[[D[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
^[[D[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
^[[D[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"
[src/serial_interface.rs:214] &buffer = "\u{1b}[D"

After the cursor ends up at the end, the terminal outputs "^[[D", which is weird, as it isn't being sent by the board. That is what leads me to believe it is a consoles issues. Furthermore, this can happen in the middle of a longer string of chars too. When an OS(?) gets that the cursor key is being held, it just fires the event a whole lot more often, and I think that is what is bugging it out.

At this point, I think it would be worthwhile to investigate other crates for direct input.

EDIT: This seems to only happen in my Ubuntu VM, on the VSCode terminal 😵
EDIT2: I think this issue is already know about in console-rs#152

@george-cosma
Copy link
Contributor Author

I think we can close this PR in favour of #9, thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase This pull request needs a rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants