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

Process console command history #3381

Merged
merged 55 commits into from
Feb 7, 2023

Conversation

CosminGGeorgescu
Copy link
Contributor

@CosminGGeorgescu CosminGGeorgescu commented Jan 23, 2023

Pull Request Overview

This pull request adds an easy-to-use Bash-like command history for the process console.

Added features

  • User can enable/disable the command history from the board's main.rs.
  • If enabled, user can scroll up or down by pressing up and down arrow keys respectively.
  • Pressing on accident up or down arrow saves the unfinished command and can be fetched back by going to the bottom of command history.
  • Modifying a command from history is also saved (if accidentally pressed up or down arrow).
  • Empty commands (whitespace-filled) don't get registered in the command history.
  • Invalid commands also are saved into the history structure.

Code size for IMIX binary build with enabled/disabled command history:

Build .text .data .bss .dec
Tock master 174468 0 29592 204060
Enabled with default size 175492 0 28944 204436
Disabled 174292 0 28576 202868

If disabled, the code size decreases.

Testing Strategy

The implementation of the command history was tested on the nrf52840dk and esp32-c3-devkitM-1 boards.

Files changed

capsules/process_console.rs
~ implementation for command history

boards/components/process_console.rs
~ added a new branch to process_console_component_static macro for easily enable/disable the command history

Documentation Updated

Updated the documentation in doc/Process_Console.md accordingly :

  • Code support on how to enable/disable the command history
  • How to use command history
  • Support for selecting a custom size for the command history

TODO or Help Wanted

Feedback is much appreciated.

Documentation Updated

  • Updated the relevant files in /docs.

Formatting

  • Ran make prepush.

CosminGGeorgescu and others added 30 commits January 21, 2023 20:09
… to let user decide the size of the history buffer
… the macro to let user decide the size of the history buffer
…he posibility to add a command full of spaces
Co-authored-by: Alexandru Radovici <msg4alex@gmail.com>
Co-authored-by: Alexandru Radovici <msg4alex@gmail.com>
@alexandruradovici
Copy link
Contributor

This feature is very helpful in education, most of our students tried immediately to scroll up and down. I think this can be added, as it does not increase code size.

I curious though why the code size actually decreases with the feature turned off.

Co-authored-by: Alexandru Radovici <msg4alex@gmail.com>
@alexandruradovici
Copy link
Contributor

Kind reminder to review this, we need this the for Rust Nation presentation. :)

lschuermann
lschuermann previously approved these changes Jan 30, 2023
Copy link
Member

@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

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

I'm by no means an expert on the process console code, but from a coarse review this looks good to me. I'm not too worried about the code size or memory usage impact of this (as long as it doesn't grow astronomically), given it's intended solely for debugging and experimentation.

hudson-ayers
hudson-ayers previously approved these changes Jan 30, 2023
Copy link
Contributor

@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

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

The decreased code size when this is disabled is just an indication that LLVM was making some suboptimal inlining decisions regarding the process console before this was added, but that may not apply universally across boards and could be fixed with explicit inline directives anyway.

Since this can be disabled from main I am indifferent to the addition -- I personally would probably advocate for it being off-by-default (i.e. unless you specify a length when creating the component no commands will be saved) but will not block this on that.

@alexandruradovici
Copy link
Contributor

alexandruradovici commented Jan 31, 2023

I think that for educational purposes and for getting started with Tock, the history should be on by default. Most developers do expect this feature from the console, all of our students being frustrated that it was not available.

Instead of checking if the buffer is filled up, try to get the byte from the desired position. If returns Some(byte_buffer) update the byte from `EOL` to `byte`, if None returned do nothing. (P.S: the same functionality is achieved).

Tested on `nrf52840dk` and works as expected.
Just changed the inner method to match the same style as the `insert_byte` method (also tested on the `nrf52840dk` board).
Copy link
Contributor

@phil-levis phil-levis left a comment

Choose a reason for hiding this comment

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

As the original process console author, I agree with this improvement conceptually. I haven't done a code review.

// Catch the Up and Down arrow keys
if read_buf[0] == ESC {
// Signal that a control sequence has started and capture it
self.control_seq_in_progress.set(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to collaborate with Alex on this since we have a downstream version of process console that handles command history.

I want to share the small state machines that we use down stream that you can either choose to use or not (your call). I just wanted it to be an option. Consider this last minute collaboration.

/// Start of terminal escape sequence
const CHAR_ESC_START: u8 = b'\x1B'; 

/// The FSM for processing terminal escape sequences
#[derive(Copy, Clone)]
enum EscState {
    /// Most recent character is not part of an escapee sequence, so process it elsewhere
    Bypass,
    /// The escape sequence completed with a recognized escape key
    Complete(EscKey),
    Started,
    Bracket,
    Bracket1,
    Bracket3,
    Bracket4,
    Unrecognized,
    UnrecognizedDone,
}

impl EscState {
    /// Process the data for this state and returns the next state in the FSM
    fn next_state(self, data: u8) -> Self {
        use self::{EscKey::*, EscState::*};
        match (self, data) {
            (Bypass, CHAR_ESC_START)
            | (UnrecognizedDone, CHAR_ESC_START)
            | (Complete(_), CHAR_ESC_START) => Started,
            (Bypass, _) | (UnrecognizedDone, _) | (Complete(_), _) => Bypass,
            (Started, b'[') => Bracket,
            (Bracket, b'A') => Complete(Up),
            (Bracket, b'B') => Complete(Down),
            (Bracket, b'C') => Complete(Right),
            (Bracket, b'D') => Complete(Left),
            (Bracket, b'1') => Bracket1,
            (Bracket1, b'~') => Complete(Home),
            (Bracket, b'3') => Bracket3,
            (Bracket3, b'~') => Complete(Delete),
            (Bracket, b'4') => Bracket4,
            (Bracket4, b'~') => Complete(End),
            _ => {
                if EscState::terminator_esc_char(data) {
                    UnrecognizedDone
                } else {
                    Unrecognized
                }
            }
        }
    }

    /// Returns true if the processing next state logically consumed the data and it should not
    /// be processed elsewhere
    pub fn data_consumed(&self) -> bool {
        !matches!(self, EscState::Bypass)
    }

    fn terminator_esc_char(data: u8) -> bool {
        data.is_ascii_alphabetic() || data == b'~'
    }
}

And when it is used, we do something like the follow. Note that cursor is the position of the cursor and len is the total pending command length.

        let esc_state = self.esc_state.get().next_state(ch);
        self.esc_state.set(esc_state);

        if let EscState::Complete(key) = esc_state {
            match key {
                EscKey::Right if cursor < len => {
                    Direction::Right.move_cursor(1);
                    cursor += 1;
                }
                EscKey::Left if cursor > 0 => {
                    Direction::Left.move_cursor(1);
                    cursor -= 1;
                }
                EscKey::End if cursor < len => {
                    Direction::Right.move_cursor(len - cursor);
                    cursor = len;
                }
                EscKey::Home if cursor > 0 => {
                    Direction::Left.move_cursor(cursor);
                    cursor = 0;
                }
                EscKey::Delete if cursor < len => {
                    // Remove the character under the cursor
                    try_copy_within(&mut cmd_buf, cursor + 1..len, cursor)?;
                    len -= 1;
                    // Print the updated buffer with a clearing space at end
                    for c in cmd_buf.iter().skip(cursor).take(len - cursor) {
                        console_output_no_eol!("{}", *c as char);
                    }
                    console_output_no_eol!(" ");
                    Direction::Left.move_cursor(len - cursor + 1);
                }
                EscKey::Up | EscKey::Down => {
                    let res: Result<(), ErrorCode> = self.mru.map_or(Ok(()), |mru| {
                        // Clear the current line on console
                        Direction::Left.move_cursor(cursor);
                        for _ in 0..len {
                            console_output_no_eol!(" ");
                        }
                        Direction::Left.move_cursor(len);

                        // Copy in the next data from mru circular buffer slices
                        let (b1, b2) = if matches!(key, EscKey::Up) {
                            mru.get_next_older()
                        } else {
                            mru.get_next_newer()
                        };
                        let total = b1.len() + b2.len();
                        cmd_buf
                            .get_mut(..b1.len())
                            .ok_or(ErrorCode::INVAL)?
                            .copy_from_slice(b1);
                        cmd_buf
                            .get_mut(b1.len()..total)
                            .ok_or(ErrorCode::INVAL)?
                            .copy_from_slice(b2);
                        cursor = total;
                        len = total;

                        // Print all new characters
                        for c in cmd_buf.iter().take(len) {
                            console_output_no_eol!("{}", *c as char);
                        }
                        Ok(())
                    });
                    res?
                }
                _ => {}
            };
        }
        // If the escape sequence consumed the character, we are done processing this character
        if esc_state.data_consumed() {
            return Ok(());
        }

Also here is the Direction helper

/// Direction to move a cursor
enum Direction {
    Left,
    Right,
}

impl Direction {
    fn move_cursor(&self, count: usize) {
        if count != 0 {
            console_output_no_eol!(
                "\x1b[{}{}",
                count,
                match self {
                    Direction::Left => "D",
                    Direction::Right => "C",
                }
            );
        }
    }
}

Lastly, we have a Most Recently Used (mru in code above) circular buffer cache for console history that compactly stores previous commands with an upper bound limit on the totally number of bytes and stored commands, which ever comes first. I can also share this too with its unit tests.

Also don't feel like you have to make these changes before landing this PR. If you want to incorporate this feedback in a follow up PR, I would be happy to help out. I just wanted to start talking about this.

Copy link
Contributor

@alexandruradovici alexandruradovici Feb 4, 2023

Choose a reason for hiding this comment

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

I agree, @MATrix22 @CosminGGeorgescu you can prepare a new pull request as soon as this gets merged. Thank you @jettr for the feedback.

@hudson-ayers
Copy link
Contributor

bors r+

(On the Friday call, there was agreement that we could merge this as-is then seperately incorporate Jett's suggestion in a new PR, so as to ensure this makes it in before RustNation)

@bors
Copy link
Contributor

bors bot commented Feb 7, 2023

@bors bors bot merged commit 6bbfd51 into tock:master Feb 7, 2023
@mihai-negru mihai-negru deleted the process_console_history branch February 20, 2023 06:42
bors bot added a commit that referenced this pull request May 26, 2023
3414: Implementing Escape State Machine for process console and command navigations r=bradjc a=mihai-negru

### Thanks

Thanks to [jettr](https://github.com/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 | 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

- [x] Ran `make prepush`.

Co-authored-by: Matrix22 <determinant289@gmail.com>
Co-authored-by: Mihai Negru <matrix22@voldemort.local>
Co-authored-by: Mihai Negru <86746059+Matrix22@users.noreply.github.com>
Co-authored-by: Mihai Negru <86746059+mihai-negru@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants