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

Implement "listen" subcommand (+ code structure) #9

Closed
wants to merge 14 commits into from

Conversation

george-cosma
Copy link
Contributor

This PR is an extension of PR #2 and PR #8 .

I've always felt that the prototype of the listen subcommand shouldn't be merged. While it was mostly functional, it had no room for modifications or expansion later down the road. Overall, it wasn't clean code. This version of "listen" aims to answer that issue. Also, as a small difference between this and the prototype, this version does not:

  • replace '\n' with '\r\n'
  • replace ASCII delete with ANSI Escape Sequence delete
  • replace NULL with UTF8 Null

The issues that caused these "hacks" have been fixed within the kernel.

PR #8 was meant to propose a structure to the project so that features would be simple to add. This PR exemplifies this, and makes use of the aforementioned structure, even though #8 hasn't been merged in yet. I am happy with it as-is, and I would suggest PR #8 be merged first (if everyone is okay with it) before this is.

I know this PR is quite large, but reviews would be greatly appreciated. I've tried to cut down on repetitive and unnecessary code, but there is so much I can do whilst implementing a core function of the app, and I don't know if this could be split up into multiple pull requests (besides #8).

Help Wanted

There is a part of this code I am not entirely happy with:

#[tokio::main]
async fn main() -> Result<(), TockloaderError> {
    let result = run().await;
    if let Err(e) = &result {
        eprintln!("{}", e);
    }

    result
}

async fn run() -> Result<(), TockloaderError> {
    // ...
}

To explain why I did this, I wanted to make sure the user gets a message when an error is encountered, and the default behavior was to use the Debug print. I could copy the code from Display to Debug, but there still needs to be an implementation of Display for us to implement Error. (Do we really need to implement error?). Plus, it might still be useful to have that debug implementation, especially for those nested errors:

# I disconnected my board mid-typing:
$ cargo run -- listen --serial
    Finished dev [unoptimized + debuginfo] target(s) in 0.15s
     Running `target\debug\tockloader.exe listen --serial`
Initialization complete. Entering main loop.
tock$ aaaaaaThe I/O operation has been aborted because of either a thread exit or an application request. (os error 995)
Error: IOError(Os { code: 995, kind: TimedOut, message: "The I/O operation has been aborted because of either a thread exit or an application request." })
error: process didn't exit successfully: `target\debug\tockloader.exe listen --serial` (exit code: 1)

TODO

Compared to the python version, this still doesn't have all the bells and whistles, but I think this can be merged for now, and later on we can worry about those. A list is still present in #2.

A note on testing:

Run cargo run -- listen --serial
The --serial flag is now needed, as I've decided to maintain the priority given to OpenOCD in the original version of tockloader.

@george-cosma
Copy link
Contributor Author

george-cosma commented Sep 30, 2023

I am temporarily not able to test the latest commit with a physical board. By my logic, the code shouldn't fail, but you can never be too sure. I will update this comment when I do test it.

(P.S. Also, there's an argument to be made that this commit isn't needed, as reuniting the streams doesn't actually serve any purpose. Though it can't hurt, right?)

@george-cosma
Copy link
Contributor Author

After some tests for this commit, it turns out that writer_arc still had a strong reference, since read_key() spawns a blocking task and awaits it.

While creating a weak link to the writer fixes the panic, a button still needs to be pressed for the program to exit. I'm unsure if shutdown_background could help us. Maybe there's also the possibility of putting a timeout on the reading - but it may just leak resources.

Looking at term, on windows system we MIGHT be able to send phony data to the console (WriteConsoleInput), but I am unsure if the same behavior could be replicated on unix systems (maybe this?).

I'm unsure on how to proceed with this.

@george-cosma
Copy link
Contributor Author

I have implemented a possible solution to this - writing to stdin. I don't know how cross-platform compatible my solution is - it works under Windows and WSL, but I cannot say anything of MacOS. A proof of concept - where I write a single, predetermined letter to stdin to free up read_key - would be this commit on my local fork.

I'll try to bring up writing to stdin as a possible feature for console-rs - though I don't foresee it being added soon. The whole ordeal on windows is quite difficult as the behind-the-scenes system is quite... complex... intertwined... messy. On linux, it's a bit more straight forward, though no guarantees are made about it being supported in future releases.

We still need to figure out what to do in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant