mortbopet / Ripes

A graphical processor simulator and assembly editor for the RISC-V ISA
https://ripes.me/
MIT License
2.49k stars 270 forks source link

How to handle System Input in CLI? #353

Open federicovilla55 opened 4 months ago

federicovilla55 commented 4 months ago

As stated in the current implementation of the clirunner.cpp file: https://github.com/mortbopet/Ripes/blob/0faf41b669a93a1944707cd7d111a5e9241425fe/src/cli/clirunner.cpp#L47 there is no provision for handling system input from the command-line interface (CLI).

I am working on implementing the capability to accept input from STDIN within the CLI.

I'm focusing on the systemio.h file, that contains the method readFromFile, which is called while performing the operation of the ReadSyscall. The method readFromFile extract from a map the stream that needs to be used. There's a special case for the stream 0: STDIN.

My proposal for now consists of:

  1. Adding the following method in the systemio.h file. This method sets the stream at STDIN position to the standard input.
    static void setCLIInput(){
    FileIOData::streams.erase(STDIN);
    FileIOData::streams.emplace(STDIN, stdin);
    }
  2. Modifying the number of bytes read at each step in the systemio.h file:
    static int readFromFile(int fd, QByteArray &myBuffer, int lengthRequested) {
    // ...
    if (fd == STDIN) {
      // ...
      while (myBuffer.size() < lengthRequested) {
         // ...
         auto readData = InputStream.read(1).toUtf8();
         // ...
      }
    }
    }
  3. In the clirunner.cpp file call the new method:
    
    CLIRunner::CLIRunner(const CLIModeOptions &options)
    : QObject(), m_options(options) {
    // to handle system input
    SystemIO::setCLIInput();

}



Those additions should replace the old input stream (which works for the GUI) and create a new one for the CLI.

Your contributions and feedback on the implementation of this feature are appreciated. 
Thanks 😄 
mortbopet commented 4 months ago

Your solution makes sense to me. Ideally, all of SystemIO would be rewritten to something nicer - i could imagine a solution where a SystemIO class exposes hooks to register streams for the various file descriptors, such that we'd have less control flow specific code in Systemio.h/cpp.

Would you mind elaborating on why you'd change from InputStream.read(lengthRequested) to InputStream.read(1)?

federicovilla55 commented 4 months ago

Regarding InputStream.read(1) the change was made because during CLI execution, the program should pause and wait for input to be submitted.

With the GUI console the while loop in the readFromFile(...) method continues until putStdInData inserts something in the QByteArray pointed by the GUI QTextStream.

Keeping the line InputStream.read(lengthRequested) would cause the program to wait for exactly lengthRequested characters to be in the stdin, but we want to read at most that number of characters. This differs from the behavior described in the read() documentation and what happens with a QTextStream that points at a QByteArray, such as the one used to read the input from the console.

It's possible that this difference in behavior is due to the special QTextStream created that points to stdin?

With the method readLine of the QTextStream class there is no more that problem, but there may be other problems with the line separator character (\n) that is automatically removed from the read data.

A possible solution arose from reading one character at a time. The number of characters doesn't exceed lengthRequested due to the while condition: while (myBuffer.size() < lengthRequested) { ... }. However, a problem could arise if there are multiple line separators in the user input; in that case, the input would be truncated at the first line separator.

If you have a better solution, it would be welcome.

mortbopet commented 4 months ago

That analysis sounds reasonable to me; as long as we have the solution which provides an intuitive user experience and doesn't deadlock the program with it trying to wait for user input.