Implementing Escape State Machine for process console and command navigations #3414

mihai-negru · 2023-03-15T09:36:05Z

Thanks

Thanks to jettr for the escape state machine code snapshot and feedback

Pull Request Overview

This pull request implements the jettr's suggestion of implementing an escape state machine (#3381 (comment)).

Also this pull request implements new features over typing experience in the ProcessConsole, in order to make typing more dynamic.

Added features

Use Left and Right arrows in order to navigate through a typing command.
Use Home and End commands to move at the beginning or end of a command.
Press Delete key to remove the character under the cursor.
Inserting or deleting(using backspace) a character inside a command is now possible.
Pressing enter inside a command is also possible.

Flash size for IMIX binary build:

Builds	text	bss	dec
Tock Master	175592	30108	205700
New Code Size	176232	30116	206348
Differences	640	8	648

CommandHistory is enabled by default on Tock Master, so the size compiled refers to new_code + history_size

Testing Strategy

This pull request was tested on a nrf52840dk board

Changed Files

capsules/core/src/process_console.rs

Implementation for new features.

Documentation Updated

Updated the documentation in doc/Process_Console.md accordingly:

Support on how to use the new features.
Explaining that whitespaces do not affect the resulting command.

TODO or Help Wanted

Feedback relating to optimize the new features and how to make a better command_history.

Formatting

Ran make prepush.

…wn Arrows) according to jettr's feedback

…field, working on it

…ight variants

…ant for most used ANSI characters

… modified insertion and deletion inside a command

…ursor)

… in history

.vscode/settings.json

mihai-negru · 2023-03-20T09:03:53Z

Kind reminder to review this, thanks :)

phil-levis · 2023-03-24T18:06:31Z

I think this needs some more testing/debugging. Try this:

type help and return
Hit up-arrow, then hit backspace several times

Initialization complete. Entering main loop
tock$ help
Welcome to the process console.
Valid commands are: help status list stop start fault boot terminate process kernel reset panic
toc    
panicked at 'index out of bounds: the len is 32 but the index is 32', capsules/core/src/process_console.rs:1275:50
        Kernel version a06b0daed

---| No debug queue found. You can set it with the DebugQueue component.

---| Cortex-M Fault Status |---
No Cortex-M faults detected.

---| App Status |---

mihai-negru · 2023-03-24T19:25:06Z

I think this needs some more testing/debugging. Try this:

type help and return
Hit up-arrow, then hit backspace several times

Initialization complete. Entering main loop
tock$ help
Welcome to the process console.
Valid commands are: help status list stop start fault boot terminate process kernel reset panic
toc    
panicked at 'index out of bounds: the len is 32 but the index is 32', capsules/core/src/process_console.rs:1275:50
        Kernel version a06b0daed

---| No debug queue found. You can set it with the DebugQueue component.

---| Cortex-M Fault Status |---
No Cortex-M faults detected.

---| App Status |---

I tried to recreate the behaviour, but could not get some errors.
Regarding to backspace or deletion the cursor is checked every time you press backspace or delete, so I am 100% sure that it cannot get an underflow, also when inserting a new byte the current index is checked via the following condition index < command.len() - 1 so it also could not get an overflow.

Are you sure that you have tested the better-process-console branch? , perhaps you could have tested the last version for command_history.

As said above, I tried to recreate the error, but could not get any messages or panics, maybe if you could to check that you have tested the right branch and also if you could to write some more details about the error, because I could not recreate it, thanks for feedback.

phil-levis · 2023-03-24T22:44:23Z

I am testing the better-process-console branch.

I am testing on an imix board. What board are you using?

mihai-negru · 2023-03-24T22:49:46Z

I am testing the better-process-console branch.

I am testing on an imix board. What board are you using?

A nrf52840dk board

phil-levis · 2023-03-24T22:56:39Z

What sort of machine are you using? I think this is a \r\n problem. Under Linux, an enter is just a \n. This means that

tock/capsules/core/src/process_console.rs

Line 1252 in a06b0da

self.cursor.set(0);

doesn't execute.

phil-levis · 2023-03-24T22:58:31Z

I think you just want to check for a newline, and then see if a \r precedes it, and if so delete that too. You don't need the either \r\n or \n\r logic.

mihai-negru · 2023-03-24T23:00:14Z

What sort of machine are you using? I think this is a \r\n problem. Under Linux, an enter is just a \n. This means that

tock/capsules/core/src/process_console.rs

Line 1252 in a06b0da

self.cursor.set(0);

doesn't execute.

I am using a MacOs system, let me try to check the error on a linux virtual machine and to see if the error happens.

phil-levis · 2023-03-24T23:07:08Z

Regardless, the logic at

tock/capsules/core/src/process_console.rs

Line 1247 in a06b0da

if (previous_byte == NLINE || previous_byte == CR)

is incorrect. Windows/Mac send \r\n, while Linux/BSD/UNIX send \n (terminals often emulate to \r\n). A \n\r is not a valid sequence and implies something is wrong.

mihai-negru · 2023-03-24T23:49:06Z

Checking the \r\n or \n\r was added 16 months ago as a functionality and it was not causing any problems. However, I tried to recreate the error on a linux machine on a nrf52840dk and could not recreate the problem, I will try to test the branch on some other boards and will come with a status report

mihai-negru · 2023-03-25T09:42:30Z

Regardless, the logic at

tock/capsules/core/src/process_console.rs

Line 1247 in a06b0da

if (previous_byte == NLINE || previous_byte == CR)

is incorrect. Windows/Mac send \r\n, while Linux/BSD/UNIX send \n (terminals often emulate to \r\n). A \n\r is not a valid sequence and implies something is wrong.

Here is the merged pull request that discusses about the \r\n and \n\r #2894.

mihai-negru · 2023-03-28T20:21:08Z

So I have tested on MacOs and on Linux on following boards:

microbit_v2
stm32f429idiscovery
nrf52840dk

I could not reproduce the error presented above.

Regarding to handling \r\n and \n\r as discussed in the pull request from #2894:

Tock's master for process_console.rs

if read_buf[0] == ('\n' as u8) || read_buf[0] == ('\r' as u8) {
    if (previous_byte == ('\n' as u8) || previous_byte == ('\r' as u8)) && previous_byte != read_buf[0] {
        // Reset the sequence, when \r\n is received
        self.previous_byte.set(0);

        if COMMAND_HISTORY_LEN > 1 {
            self.command_history_index.insert(None);
            self.modified_in_history.set(false);

             // Clear the unfinished command if the \r\n is received
             self.command_history.map(|cmd_arr| {
                 cmd_arr[0].clear();
             });
         }
     } else {
         self.execute.set(true);
         et _ = self.write_bytes(&['\r' as u8, '\n' as u8]);
     }
}

Now in the second if the previous byte is reseted to EOL, this means that there the cursor should be reseted.

Now, by pressing up and down arrows, it does not echo a \r\n or \n it just removes the current command and prints the next one. In the Escape State machine when Up or Down is inserted the cursor changes it's value to:

self.cursor.set(next_command_len);

Talking about backspace the code is checked via:

if cursor > 0 {
   ...
   self.command_index.set(index - 1);
   self.cursor.set(cursor - 1);
   ...
}

Index and cursor are both decremented and user cannot delete any character if cursor is 0, so it cannot pass to tock$ characters

When inserting a character the cursor and index is also incremented by one.

Left and Right decrement and increment the cursor and are checked via:

EscKey::Left if cursor > 0
EscKey::Right if cursor < index
EscKey::Home if cursor > 0
EscKey::End if cursor < index
EscKey::Delete if cursor < index

So I can't see a glitch in the code as I have tested on three different boards and also on two different Operating Systems, also I have used on Linux minicom to map a \n to \r\n and also it executed without any errors. (also tested without minicom)

Perhaps could you try to test the code on a different board, if it is possible, and also to give me some more details about the error, because I do not know what to change, because I cannot reproduce the error.

mihai-negru · 2023-04-01T23:19:21Z

Update:
Today we had a Rust workshop and used TockOS for trainings, 22 people tested on 11 microbit_v2 this pull request and no errors were found, please I am waiting for more details about the error, thanks.

mihai-negru · 2023-04-07T19:42:12Z

I think the bug is simple. What happens if only a \r or only \n is sent?

It fails for me after hitting enter. The first command works, after that it doesn't. The issue is the call to self.cursor.set() as I noted above. If the console sends only a \r or only a \n, then the code to reset the cursor position never executes, as it requires either a \r\n or a \n\r.

tock/capsules/core/src/process_console.rs

Line 1252 in a06b0da

self.cursor.set(0);

I think the fix is pretty simple. All of the code in that block should be moved into the lower block. That is, the only thing you do when you discover a \r\n sequence is set previous to EOL. The cursor should be reset and and history modified when you process the command.

Hi, I have moved the block code that was reseting the cursor and the history to the lower block, and tested it, and for me everything works perfectly, could you please try to reproduce the error, in order to see if everything is alright?

phil-levis · 2023-04-07T23:55:25Z

It works for me now! But I would recommend creating a test case of different control sequences, \r, \n, etc., and running it through the console to make sure it executes correctly.

mihai-negru · 2023-04-08T09:36:18Z

It works for me now! But I would recommend creating a test case of different control sequences, \r, \n, etc., and running it through the console to make sure it executes correctly.

Do you mean unit tests, or just to run a set of commands into the console?

phil-levis · 2023-04-08T23:31:31Z

I don't think a lot of unit tests is necessary. But it's clear that individual consoles don't cover every input permutation. So it would be good to have a test string that exercises a lot of cases and make sure it passes. Something as simple as a shell script that pipes a defined input (with \r, \n, \r\n, \n\r, deletes, up, left, etc.) into the process console and checks that it doesn't crash. "Works for me" or "I typed in the console" pretty clearly isn't enough coverage.

mihai-negru · 2023-04-09T12:41:32Z

I don't think a lot of unit tests is necessary. But it's clear that individual consoles don't cover every input permutation. So it would be good to have a test string that exercises a lot of cases and make sure it passes. Something as simple as a shell script that pipes a defined input (with \r, \n, \r\n, \n\r, deletes, up, left, etc.) into the process console and checks that it doesn't crash. "Works for me" or "I typed in the console" pretty clearly isn't enough coverage.

As there are no capsules (at least that I have seen) that implement unit testing, how do you see this implemented?

I looked upon the mini tutorial on implementing kernel tests here(https://book.tockos.org/development/tests.html)
Is this the desired type of tests ?

I also though on creating a tests module and to instantiate a ProcessConsole structure in order to test it, however it would test the behaviour on the local machine, not on the board.

Are there any links or websites in order to lookup on implementing the test cases in order to match with the tock style, or the link from Introduction to tock is the best way to go, thanks

alistair23 · 2023-04-12T01:06:29Z

If you have a look at virtual_alarm.rs it has some unit tests.

We also have an automated test runner which you could use, see https://github.com/tock/tock/tree/master/tools/board-runner

bradjc

Just tried this, pretty awesome!

phil-levis · 2023-04-14T03:31:31Z

I don't think a lot of unit tests is necessary. But it's clear that individual consoles don't cover every input permutation. So it would be good to have a test string that exercises a lot of cases and make sure it passes. Something as simple as a shell script that pipes a defined input (with \r, \n, \r\n, \n\r, deletes, up, left, etc.) into the process console and checks that it doesn't crash. "Works for me" or "I typed in the console" pretty clearly isn't enough coverage.

As there are no capsules (at least that I have seen) that implement unit testing, how do you see this implemented?

I looked upon the mini tutorial on implementing kernel tests here(https://book.tockos.org/development/tests.html) Is this the desired type of tests ?

I also though on creating a tests module and to instantiate a ProcessConsole structure in order to test it, however it would test the behaviour on the local machine, not on the board.

Are there any links or websites in order to lookup on implementing the test cases in order to match with the tock style, or the link from Introduction to tock is the best way to go, thanks

For this particular one, I think the test case could be a script you run from your PC. E.g., erase all apps, push some input to tockloader listen, and check that the output is as expected. This means it's not a test you can run through the build system, but the machinery to do that will be significant.

bradjc · 2023-05-17T18:30:01Z

I think all we need to merge this is a simple test in the tools/ directory which when executed sends some commands to the attached board over serial and verifies it receives the correct output.

mihai-negru · 2023-05-17T19:02:03Z

I think all we need to merge this is a simple test in the tools/ directory which when executed sends some commands to the attached board over serial and verifies it receives the correct output.

Hi, sorry for being so late about the test, I was a bit busy with university, I will send a test this week with the script. Sorry once again

mihai-negru · 2023-05-18T21:38:39Z

I don't think a lot of unit tests is necessary. But it's clear that individual consoles don't cover every input permutation. So it would be good to have a test string that exercises a lot of cases and make sure it passes. Something as simple as a shell script that pipes a defined input (with \r, \n, \r\n, \n\r, deletes, up, left, etc.) into the process console and checks that it doesn't crash. "Works for me" or "I typed in the console" pretty clearly isn't enough coverage.

As there are no capsules (at least that I have seen) that implement unit testing, how do you see this implemented?
I looked upon the mini tutorial on implementing kernel tests here(https://book.tockos.org/development/tests.html) Is this the desired type of tests ?
I also though on creating a tests module and to instantiate a ProcessConsole structure in order to test it, however it would test the behaviour on the local machine, not on the board.
Are there any links or websites in order to lookup on implementing the test cases in order to match with the tock style, or the link from Introduction to tock is the best way to go, thanks

For this particular one, I think the test case could be a script you run from your PC. E.g., erase all apps, push some input to tockloader listen, and check that the output is as expected. This means it's not a test you can run through the build system, but the machinery to do that will be significant.

Hi, I made a small prototype of the test using python, that opened the tockloader and then tried to push some input to stdin, however everytime I get the error "(termios.error: (25, 'Inappropriate ioctl for device'))".

I tried to open a serial port directly to the board and seemed to work properly.

My question is: Is it necessary to send input via the tockloader or can I develop futher the test cases via a direct serial port?

…g output

mihai-negru · 2023-05-19T18:51:20Z

Hi, the test script is done, written in python

It opens a serial port with the board and send some input and waits for some output, then the output is compared with a predefined string.

The script tests everything we discussed in this thread and adds some more testing for left, right, delete, backspace and insert in any place of the command that is currently typed.

mihai-negru · 2023-05-20T11:13:48Z

I've added a licence header in the file, and run the make ci-job-format and passed, however the tock-ci/ci-format fails, do you know what can cause the failing?, thanks

jrvanwhy · 2023-05-22T17:59:26Z

The error message in the tock-ci / ci-format logs (IDK if you have access to those) is:

     Running `tools/target/release/license-checker`
./tools/check_process_console.py:3: license header missing
make: *** [Makefile:166: licensecheck] Error 1

I've added a licence header in the file, and run the make ci-job-format and passed, however the tock-ci/ci-format fails, do you know what can cause the failing?, thanks

Here's my guess, which I'm fairly confident is correct:

(Guess): This PR is built off a version of master that predates the merging of PR #3420. PR #3420 enabled the license checker on all *.py files.
Because you branched off an older version of master, when you run make ci-job-format, it uses the older .lcignore, which passes.
GitHub Actions merges master into your PR before running CI on it. This means that tock-ci / ci-format runs with the latest .lcignore, which fails.

I recommend merging master into this PR's branch locally, which I expect will merge cleanly, then re-running make prepush. That should allow you to see the error on your machine and verified you've fixed it.

…onsole

mihai-negru · 2023-05-22T19:45:21Z

The error message in the tock-ci / ci-format logs (IDK if you have access to those) is:
     Running `tools/target/release/license-checker`
./tools/check_process_console.py:3: license header missing
make: *** [Makefile:166: licensecheck] Error 1
I've added a licence header in the file, and run the make ci-job-format and passed, however the tock-ci/ci-format fails, do you know what can cause the failing?, thanks

Here's my guess, which I'm fairly confident is correct:

(Guess): This PR is built off a version of master that predates the merging of PR #3420. PR #3420 enabled the license checker on all *.py files.

Because you branched off an older version of master, when you run make ci-job-format, it uses the older .lcignore, which passes.

GitHub Actions merges master into your PR before running CI on it. This means that tock-ci / ci-format runs with the latest .lcignore, which fails.

I recommend merging master into this PR's branch locally, which I expect will merge cleanly, then re-running make prepush. That should allow you to see the error on your machine and verified you've fixed it.

Thanks for advice!

bradjc · 2023-05-26T22:15:40Z

bors r+

bors · 2023-05-26T23:17:38Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

mihai-negru and others added 14 commits March 15, 2023 11:11

Implemented the Escape State machine (minimalistic just for Up and Do…

1261b70

…wn Arrows) according to jettr's feedback

Wrap the code into the escape state machine code and byte processor code

4c47862

Encapsulated every field in a new structure except the command index …

007545f

…field, working on it

Encapsulated all history functionalities in CommandHistory Structure

fc3c89e

Made final adjustments for implementing the CommandHistory structure

ae02f23

Removed dummy methods from CommandHistory structure

4d2863f

Modified the Escape state machine skeleton in order to add Left and R…

ffee665

…ight variants

Removed unused allow dead code and added some comments

4e51397

Added explanation comments for Escape State machine and defined const…

9ea5efe

…ant for most used ANSI characters

Added implementation for Left and Right movement throug a command and…

d4580f6

… modified insertion and deletion inside a command

Finished implemented Home and End movement in a command

4a3d6e8

Implemented the delete key functionality (delete the byte after the c…

e443ec4

…ursor)

Solved a small issue that the command was not saved upon modification…

9dc8d64

… in history

Added documentation for command navigation in Process_Console.md

9619af7

mihai-negru mentioned this pull request Mar 15, 2023

Implementing Escape State Machine for process console and command navigations UPB-CS-OpenSourceUpstream/tock#19

Merged

1 task

twilfredo reviewed Mar 16, 2023

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

Added an ending newline

a06b0da

Fixed little bug on reseting the cursor upon pressing enter

78995d9

Updated the the comment for reseting the command

628C

1e41a26

bradjc previously approved these changes Apr 13, 2023

View reviewed changes

Implemented a basic checker for boards, by sending input and receivin…

f151b9f

…g output

mihai-negru dismissed bradjc’s stale review via f151b9f May 19, 2023 18:49

mihai-negru added 3 commits May 19, 2023 21:57

Added header for the script

66ad0e9

Added licence header in order to pass the ci-format job

dee0636

Moved the checker in the root of tools directory

5532861

mihai-negru requested a review from bradjc May 20, 2023 11:27

mihai-negru and others added 2 commits May 22, 2023 22:21

Merge branch 'UPB-CS-OpenSourceUpstream:master' into better-process-c…

0021b89

…onsole

Added licence header

49b0b50

bradjc approved these changes May 24, 2023

View reviewed changes

mihai-negru requested a review from hudson-ayers May 25, 2023 09:32

bors bot merged commit fc6256c into tock:master May 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implementing Escape State Machine for process console and command navigations #3414

Implementing Escape State Machine for process console and command navigations #3414

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Implementing Escape State Machine for process console and command navigations #3414

Implementing Escape State Machine for process console and command navigations #3414

Uh oh!

Conversation

Thanks

Pull Request Overview

Added features

Flash size for IMIX binary build:

Testing Strategy

Changed Files

Documentation Updated

TODO or Help Wanted

Formatting

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!