From 268d1b230571fd70ac3034ffa4386403a4525048 Mon Sep 17 00:00:00 2001 From: Hendrik van Essen Date: Sat, 8 Feb 2020 12:34:44 +0100 Subject: [PATCH 1/5] sys/shell: remove useless "inline prevention" There was some code added to "prevent putchar from being inlined", which supposedly enlarged the code size. Co-authored-by: Juan Carrano --- sys/shell/shell.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/sys/shell/shell.c b/sys/shell/shell.c index b60338cd22..e757207028 100644 --- a/sys/shell/shell.c +++ b/sys/shell/shell.c @@ -39,17 +39,6 @@ #include "shell_commands.h" #define ETX '\x03' /** ASCII "End-of-Text", or ctrl-C */ -#if !defined(SHELL_NO_ECHO) || !defined(SHELL_NO_PROMPT) -#ifdef MODULE_NEWLIB -/* use local copy of putchar, as it seems to be inlined, - * enlarging code by 50% */ -static void _putchar(int c) { - putchar(c); -} -#else -#define _putchar putchar -#endif -#endif static void flush_if_needed(void) { @@ -282,8 +271,8 @@ static int readline(char *buf, size_t size) buf[curr_pos] = '\0'; #ifndef SHELL_NO_ECHO - _putchar('\r'); - _putchar('\n'); + putchar('\r'); + putchar('\n'); #endif return (length_exceeded) ? -ENOBUFS : curr_pos; @@ -305,9 +294,9 @@ static int readline(char *buf, size_t size) } /* white-tape the character */ #ifndef SHELL_NO_ECHO - _putchar('\b'); - _putchar(' '); - _putchar('\b'); + putchar('\b'); + putchar(' '); + putchar('\b'); #endif } else { @@ -319,7 +308,7 @@ static int readline(char *buf, size_t size) length_exceeded = true; } #ifndef SHELL_NO_ECHO - _putchar(c); + putchar(c); #endif } flush_if_needed(); @@ -329,8 +318,8 @@ static int readline(char *buf, size_t size) static inline void print_prompt(void) { #ifndef SHELL_NO_PROMPT - _putchar('>'); - _putchar(' '); + putchar('>'); + putchar(' '); #endif flush_if_needed(); From 45f898563a60c583243260c1bb5c7107b215c074 Mon Sep 17 00:00:00 2001 From: Hendrik van Essen Date: Sat, 8 Feb 2020 12:52:46 +0100 Subject: [PATCH 2/5] sys/shell: remove ifdefs in the middle of code ifdefs in the middle of code reduce the readability and debugability and should be avoided. Co-authored-by: Juan Carrano --- sys/shell/shell.c | 74 +++++++++++++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 28 deletions(-) diff --git a/sys/shell/shell.c b/sys/shell/shell.c index e757207028..c50df40935 100644 --- a/sys/shell/shell.c +++ b/sys/shell/shell.c @@ -39,21 +39,38 @@ #include "shell_commands.h" #define ETX '\x03' /** ASCII "End-of-Text", or ctrl-C */ +#define BS '\x08' /** ASCII "Backspace" */ +#define DEL '\x7f' /** ASCII "Delete" */ + +#ifdef MODULE_SHELL_COMMANDS + #define MORE_COMMANDS _shell_command_list +#else + #define MORE_COMMANDS +#endif /* MODULE_SHELL_COMMANDS */ -static void flush_if_needed(void) -{ #ifdef MODULE_NEWLIB - fflush(stdout); -#endif -} + #define flush_if_needed() fflush(stdout) +#else + #define flush_if_needed() +#endif /* MODULE_NEWLIB */ + +#ifndef SHELL_NO_ECHO + #define ECHO_ON 1 +#else + #define ECHO_ON 0 +#endif /* SHELL_NO_ECHO */ + +#ifndef SHELL_NO_PROMPT + #define PROMPT_ON 1 +#else + #define PROMPT_ON 0 +#endif /* SHELL_NO_PROMPT */ static shell_command_handler_t find_handler(const shell_command_t *command_list, char *command) { const shell_command_t *command_lists[] = { command_list, -#ifdef MODULE_SHELL_COMMANDS - _shell_command_list, -#endif + MORE_COMMANDS }; /* iterating over command_lists */ @@ -84,9 +101,7 @@ static void print_help(const shell_command_t *command_list) const shell_command_t *command_lists[] = { command_list, -#ifdef MODULE_SHELL_COMMANDS - _shell_command_list, -#endif + MORE_COMMANDS }; /* iterating over command_lists */ @@ -270,10 +285,11 @@ static int readline(char *buf, size_t size) } buf[curr_pos] = '\0'; -#ifndef SHELL_NO_ECHO - putchar('\r'); - putchar('\n'); -#endif + + if (ECHO_ON) { + putchar('\r'); + putchar('\n'); + } return (length_exceeded) ? -ENOBUFS : curr_pos; } @@ -281,7 +297,7 @@ static int readline(char *buf, size_t size) /* check for backspace: * 0x7f (DEL) when using QEMU * 0x08 (BS) for most terminals */ - if (c == 0x08 || c == 0x7f) { + if (c == BS || c == DEL) { if (curr_pos == 0) { /* ignore empty line */ continue; @@ -292,12 +308,13 @@ static int readline(char *buf, size_t size) if (!length_exceeded) { buf[--curr_pos] = '\0'; } + /* white-tape the character */ -#ifndef SHELL_NO_ECHO - putchar('\b'); - putchar(' '); - putchar('\b'); -#endif + if (ECHO_ON) { + putchar('\b'); + putchar(' '); + putchar('\b'); + } } else { /* Always consume characters, but do not not always store them */ @@ -307,9 +324,10 @@ static int readline(char *buf, size_t size) else { length_exceeded = true; } -#ifndef SHELL_NO_ECHO - putchar(c); -#endif + + if (ECHO_ON) { + putchar(c); + } } flush_if_needed(); } @@ -317,10 +335,10 @@ static int readline(char *buf, size_t size) static inline void print_prompt(void) { -#ifndef SHELL_NO_PROMPT - putchar('>'); - putchar(' '); -#endif + if (PROMPT_ON) { + putchar('>'); + putchar(' '); + } flush_if_needed(); } From a0a3e6c3a1654b61336e8128f6e4cc58919e6967 Mon Sep 17 00:00:00 2001 From: Hendrik van Essen Date: Sat, 8 Feb 2020 13:42:14 +0100 Subject: [PATCH 3/5] sys/shell: refactor readline function This makes the code of `readline()` clearer and shorter. It also fixes a minor artifact of the long line handling. Previously it was not possible to recover from a long line. That is, if too many characters were sent, the line would be invalidated and pressing backspace would not fix it- the only option was to discard the line. It is now possible to bring the line back to size. Note that visual effects when deleting characters will still depend on the host's terminal. The new code is written in a way that all writes to memory are guarded by bounds check, so an assertion was removed. Co-authored-by: Juan Carrano --- sys/shell/shell.c | 142 +++++++++++++++++++++++++--------------------- 1 file changed, 77 insertions(+), 65 deletions(-) diff --git a/sys/shell/shell.c b/sys/shell/shell.c index c50df40935..4c851286b5 100644 --- a/sys/shell/shell.c +++ b/sys/shell/shell.c @@ -237,6 +237,40 @@ static void handle_input_line(const shell_command_t *command_list, char *line) } } +static inline void print_prompt(void) +{ + if (PROMPT_ON) { + putchar('>'); + putchar(' '); + } + + flush_if_needed(); +} + +static inline void echo_char(char c) +{ + if (ECHO_ON) { + putchar(c); + } +} + +static inline void white_tape(void) +{ + if (ECHO_ON) { + putchar('\b'); + putchar(' '); + putchar('\b'); + } +} + +static inline void new_line(void) +{ + if (ECHO_ON) { + putchar('\r'); + putchar('\n'); + } +} + /** * @brief Read a single line from standard input into a buffer. * @@ -246,6 +280,10 @@ static void handle_input_line(const shell_command_t *command_list, char *line) * If the input line is too long, the input will still be consumed until the end * to prevent the next line from containing garbage. * + * We allow Unix (\n), DOS (\r\n), and Mac linebreaks (\r). + * QEMU transmits only a single '\r' == 13 on hitting enter ("-serial stdio"). + * DOS newlines are handled like hitting enter twice. + * * @param buf Buffer where the input will be placed. * @param size Size of the buffer. The maximum line length will be one less * than size, to accommodate for the null terminator. @@ -254,7 +292,7 @@ static void handle_input_line(const shell_command_t *command_list, char *line) * @return length of the read line, excluding the terminator, if reading was * successful. * @return EOF, if the end of the input stream was reached. - * @return ENOBUFS if the buffer size was exceeded. + * @return -ENOBUFS if the buffer size was exceeded. */ static int readline(char *buf, size_t size) { @@ -264,85 +302,59 @@ static int readline(char *buf, size_t size) assert((size_t) size > 0); while (1) { - /* At the start of the loop, cur_pos should point inside of - * buf. This ensures the terminator can always fit. */ assert((size_t) curr_pos < size); int c = getchar(); - if (c < 0) { - return EOF; - } - /* We allow Unix linebreaks (\n), DOS linebreaks (\r\n), and Mac - * linebreaks (\r). QEMU transmits only a single '\r' == 13 on hitting - * enter ("-serial stdio"). DOS newlines are handled like hitting enter - * twice, but empty lines are ignored. Ctrl-C cancels the current line. - */ - if (c == '\r' || c == '\n' || c == ETX) { - if (c == ETX) { + switch (c) { + + case EOF: + return EOF; + + case ETX: + /* Ctrl-C cancels the current line. */ curr_pos = 0; length_exceeded = false; - } + /* fall-thru */ + case '\r': + /* fall-thru */ + case '\n': + buf[curr_pos] = '\0'; - buf[curr_pos] = '\0'; + new_line(); - if (ECHO_ON) { - putchar('\r'); - putchar('\n'); - } + return (length_exceeded) ? -ENOBUFS : curr_pos; - return (length_exceeded) ? -ENOBUFS : curr_pos; + /* check for backspace: */ + case BS: /* 0x08 (BS) for most terminals */ + /* fall-thru */ + case DEL: /* 0x7f (DEL) when using QEMU */ + if (curr_pos > 0) { + curr_pos--; + if ((size_t) curr_pos < size) { + buf[curr_pos] = '\0'; + length_exceeded = false; + } + white_tape(); + } + break; + + default: + /* Always consume characters, but do not not always store them */ + if ((size_t) curr_pos < size - 1) { + buf[curr_pos++] = c; + } + else { + length_exceeded = true; + } + echo_char(c); + break; } - /* check for backspace: - * 0x7f (DEL) when using QEMU - * 0x08 (BS) for most terminals */ - if (c == BS || c == DEL) { - if (curr_pos == 0) { - /* ignore empty line */ - continue; - } - - /* after we dropped characters don't edit the line, yet keep the - * visual effects */ - if (!length_exceeded) { - buf[--curr_pos] = '\0'; - } - - /* white-tape the character */ - if (ECHO_ON) { - putchar('\b'); - putchar(' '); - putchar('\b'); - } - } - else { - /* Always consume characters, but do not not always store them */ - if ((size_t) curr_pos < size - 1) { - buf[curr_pos++] = c; - } - else { - length_exceeded = true; - } - - if (ECHO_ON) { - putchar(c); - } - } flush_if_needed(); } } -static inline void print_prompt(void) -{ - if (PROMPT_ON) { - putchar('>'); - putchar(' '); - } - - flush_if_needed(); -} - void shell_run_once(const shell_command_t *shell_commands, char *line_buf, int len) { From 013af64a8034373dc92f95c2473a634f2719ee9f Mon Sep 17 00:00:00 2001 From: Hendrik van Essen Date: Sat, 8 Feb 2020 13:59:18 +0100 Subject: [PATCH 4/5] tests/shell: add test case for line editing Test erasing characters using backspace. The test is not really testing a lot right now, because the host is still line buffering. Co-authored-by: Juan Carrano --- tests/shell/tests/01-run.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/tests/shell/tests/01-run.py b/tests/shell/tests/01-run.py index 0bb6ef0936..c292893c5f 100755 --- a/tests/shell/tests/01-run.py +++ b/tests/shell/tests/01-run.py @@ -50,6 +50,8 @@ CMDS = ( 'shell: command not found: ' '123456789012345678901234567890123456789012345678901234567890'), ('unknown_command', 'shell: command not found: unknown_command'), + ('hello-willy\b\b\b\borld', 'shell: command not found: hello-world'), + ('\b\b\b\becho', '\"echo\"'), ('help', EXPECTED_HELP), ('echo a string', '\"echo\"\"a\"\"string\"'), ('ps', EXPECTED_PS), @@ -87,7 +89,7 @@ def check_and_get_bufsize(child): return bufsize -def check_line_exceeded(child, bufsize): +def check_line_exceeded(child, longline): if BOARD == 'nrf52dk': # There is an issue with nrf52dk when the Virtual COM port is connected @@ -97,8 +99,6 @@ def check_line_exceeded(child, bufsize): print_error('test case "check_line_exceeded" broken for nrf52dk. SKIP') return - longline = "_"*bufsize + "verylong" - child.sendline(longline) child.expect('shell: maximum line length exceeded') @@ -112,6 +112,16 @@ def check_line_canceling(child): assert garbage_expected == garbage_received +def check_erase_long_line(child, longline): + # FIXME: this only works on native, due to #10634 combined with socat + # insisting in line-buffering the terminal. + + if BOARD == 'native': + longline_erased = longline + "\b"*len(longline) + "echo" + child.sendline(longline_erased) + child.expect_exact('"echo"') + + def testfunc(child): # avoid sending an extra empty line on native. if BOARD == 'native': @@ -120,11 +130,14 @@ def testfunc(child): check_startup(child) bufsize = check_and_get_bufsize(child) + longline = "_"*bufsize + "verylong" - check_line_exceeded(child, bufsize) + check_line_exceeded(child, longline) check_line_canceling(child) + check_erase_long_line(child, longline) + # loop other defined commands and expected output for cmd, expected in CMDS: check_cmd(child, cmd, expected) From bd3eff3537166c587b8ca1024091f90f91122cdf Mon Sep 17 00:00:00 2001 From: Francisco Molina Date: Tue, 5 May 2020 16:33:15 +0200 Subject: [PATCH 5/5] cpu/esp32: switch from O2 to Os In #12955 optimization was switched to O2 because with the '-Os' option, the ESP32 hangs sporadically in 'tests/bench*' if interrupts where disabled too early by benchmark tests. Since it hasn't been reproduced since and in #13196 O2 was causing un-explained hardfaults, since the aforementioned issue could not be reproduced we switch back to Os by removing O2, as Os will be used by default. --- cpu/esp32/Makefile.include | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cpu/esp32/Makefile.include b/cpu/esp32/Makefile.include index ff8bb5cc6c..869ad55aed 100644 --- a/cpu/esp32/Makefile.include +++ b/cpu/esp32/Makefile.include @@ -7,10 +7,6 @@ endif ESP_SDK_DIR = $(ESP32_SDK_DIR) -# With the '-Os' option, the ESP32 hangs sporadically in 'tests/bench*' if -# interrupts are disabled too early by benchmark tests. -CFLAGS_OPT ?= -O2 - # ESP32 specific flashing options FLASH_CHIP = esp32 FLASH_MODE ?= dout