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

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

Merged
merged 23 commits into from
Nov 29, 2023
Merged

Conversation

mihai-negru
Copy link

@mihai-negru mihai-negru commented Feb 20, 2023

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 (tock#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 data bss dec
Tock Master 175592 0 30108 205700
New Code Size 176232 0 30116 206348
Differences 640 0 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.

@mihai-negru
Copy link
Author

Pull Request Overview

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

The state machine proposed by jettr, has many functionalities, however for this pull request were implemented scrolling in command_history maintaining the same behavior as the last merged PR (tock#3381)

Scketch pull request

The pull request is not finished according to jettr's suggestion, because the functionalities of home, delete, end, left and right are not implemented.

Testing Strategy

This pull request was tested on nrf52840dk board

TODO or Help Wanted

Suggestions in order to make a better command_history

Documentation Updated

  • N/A

Formatting

  • Ran make prepush.

About the suggestions, I would like to modify the command_history:

  • Translate the array in a structure containing an array of Commands
  • Encapsulate any functionality in the history structure
  • Some functionalities require code duplication (at up and down match branch), which can be solved by creating a history structure, that will keep track of modified history and last modified byte (the byte before escape machine started)
  • After encapsulating the array into a structure, we may focus on implementing the left and right functionalities
  • To many command history functionalities are maintained by process console and not by a different structure

I would like to know what do you think about this perspective?

@mihai-negru mihai-negru marked this pull request as ready for review February 22, 2023 18:22
@mihai-negru
Copy link
Author

I have changed the code a bit in order to encapsulate the history functionality in another structure, however I have not put the existance of the escape machine under the COMMAND_HISTORY_LEN > 1 condition, because for futher implementations for Left and Right functionalities we will not be needed the COMMANS_HISTORY_LEN. Also if you disagree with the encapsulation we can forget about this and go back to the commit #765cc43.

When we will implement the Left, Right, Home and End, keys the code will be changed a little bit more because we have to get rid of the if let Some(index) = ....., so by encapsulating, we somehow detached the ProcessConsole from the CommandHistory, so it will be easier to make the futher implementations.

@mihai-negru
Copy link
Author

Also in jettr's message he metioned something about a mru, could you redirect me to a website or some papers in order to understand how to use this and how to implement it in our PR, thanks in advice

@mihai-negru
Copy link
Author

For this implementation the size has increased around 200 bytes in both enabled and disabled states, because the escape machine is not designed in general just for the command history

@mihai-negru
Copy link
Author

Kind reminder to review this, I need this in order to continue with further implementations. :)

Copy link

@alexandruradovici alexandruradovici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really nice.

capsules/src/process_console.rs Outdated Show resolved Hide resolved
capsules/src/process_console.rs Outdated Show resolved Hide resolved
capsules/src/process_console.rs Outdated Show resolved Hide resolved
capsules/src/process_console.rs Outdated Show resolved Hide resolved
capsules/src/process_console.rs Outdated Show resolved Hide resolved
@mihai-negru
Copy link
Author

With this new commit, I have solved the review problems.

As usually, I have run make prepush and tested the modification on the boards in order to see if the program keeps working as expected.

With your confirmation I would like to move on the next phase, in order to implement Left and Right functionalities.

So the next phases that I have in mind are:

  • Left and Right implementation (phase 1)
  • Home and End implementation (phase 2)
  • Delete and Insert implementation (phase 3)
  • Clear screen command (phase 4)

Please, I would like to know if it is alright for the next steps.

Thanks for the review.

@alexandruradovici
Copy link

Yes, the PR looks very nice, start implementing the rest.

@mihai-negru
Copy link
Author

With these commits, I have implemented the left, right, home, end and delete functionalities.

As usual I have tested the implementation on a nrf52840dk board.

I have also signed some constants to \r and \n characters.

@mihai-negru
Copy link
Author

Moving to implement the clear function for process console in order to clear the screen

@mihai-negru
Copy link
Author

As I have seen so far the ProcessConsole does not support escape sequences, because there are several ways to clear the screen using some escape sequences for example printf '\033c', is there a way in order to send the following bytes \033c to the host terminal in order to handle them.

Another way of implementing the clear functionality, would be to print a bunch of \r\n, however it lefts a bunch of space between the top of the terminal and the command line.

Is there a way to achieve this, by using escape sequences?

Thanks for the answer.

@mihai-negru mihai-negru changed the title Implementing Escape State Machine for process console Implementing Escape State Machine for process console and command navigations Mar 14, 2023
@mihai-negru
Copy link
Author

This PR was send successfully to the upstream: tock#3414

@alexandruradovici
Copy link

@mihai-negru any updates here?

@mihai-negru
Copy link
Author

@mihai-negru any updates here?

Hi, sorry, I was a bit busy with math olympics, I am getting back to work this week in order to create the test script file for the escape state macilhine

@mihai-negru
Copy link
Author

mihai-negru commented May 26, 2023

Hi, this pull request was merged into tock master at tock#3414

@alexandruradovici alexandruradovici merged commit fc6256c into UPB-CS-OpenSourceUpstream:master Nov 29, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants