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 #15

Conversation

CosminGGeorgescu
Copy link

@CosminGGeorgescu CosminGGeorgescu commented Jan 9, 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

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

Formatting

  • Ran make prepush

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
@valexandru
Copy link

Also, a recommendation is to run make prepush before pushing the code.

@alexandruradovici
Copy link

Our suggestion is to do one of the following:

  • as this pull request is more complete, @MATrix22 clones this pull request branch from @CosminGGeorgescu, makes the changes to the data types and applies the const generics (<const ...>) to it, and resubmits the pull reqeuest
  • @CosminGGeorgescu gives access to @MATrix22 to his repository so that he can push the changes directly to this pull request

@CosminGGeorgescu
Copy link
Author

I would go with the second bullet-point, if it's ok with you @MATrix22.

@mihai-negru
Copy link

I would go with the second bullet-point, if it's ok with you @MATrix22.

Yes, it is ok, i will try to make the modifications on Sunday if it's alright?

…ate examples, include "Copyright Tock Contributors" in the examples.
License policy 2023-01-23 core call updates
…ividual contributor copyright from the main example.
@mihai-negru
Copy link

I would go with the second bullet-point, if it's ok with you @MATrix22.

@CosminGGeorgescu I created this char room to discuss futher implementations

@mihai-negru
Copy link

Testing the implementation on the nrf52840dk chip I found that typing the same command multiple times when pressing the up arrow promts all the inserted commands multiple times even though they are the same, I think it would be better if the command equals to the previous command we should not to insert into the structure. @CosminGGeorgescu what do you thing about this?

@CosminGGeorgescu
Copy link
Author

Intended as to mimic an unconfigured Bash. However, if @alexandruradovici or @valexandru object, a ignoredups / erasedups behaviour can be implemented.

@alexandruradovici
Copy link

@CosminGGeorgescu do you have any branch protection? @MATrix22 gers an error when trying to push to.

@CosminGGeorgescu
Copy link
Author

CosminGGeorgescu commented Jan 14, 2023

@CosminGGeorgescu do you have any branch protection? @MATrix22 gers an error when trying to push to.

Should be good by now.

@alexandruradovici alexandruradovici added the needs-rebase The pull request needs to be rebased due to conflicts with the master. label Jan 14, 2023
mihai-negru and others added 18 commits January 21, 2023 20:09
…ion for the ProcessConsole using custom COMMAND_HISTORY_LEN
…d, just the modifications were registred, not all command
…ces and remove unnecessary white spaces between command words
…ing + adding new bytes does not accumulate test between unfinished commands
@alexandruradovici alexandruradovici removed the needs-rebase The pull request needs to be rebased due to conflicts with the master. label Jan 23, 2023
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.

Let's send this to upstream. When writing the pull request, please fill out the form from the pull request template and actually run the make prepush command.

@CosminGGeorgescu
Copy link
Author

PR sent tock#3381.

@alexandruradovici
Copy link

Please write the full pull request message from here to upstream, including the code sizes and the fact that this changes does actually decrease code size if disabled.

@alexandruradovici alexandruradovici added the upstream This pull request was opened in the upstream reposiotry label Jan 23, 2023
Co-authored-by: Alexandru Radovici <msg4alex@gmail.com>
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).
@mihai-negru mihai-negru deleted the process_console_history branch February 20, 2023 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component upstream This pull request was opened in the upstream reposiotry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants