Merge pull request #13196 from HendrikVE/shell-readline-refactor

sys/shell: refactor readline function

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

sys/shell/shell.c

@@ -39,32 +39,38 @@
 #include "shell_commands.h"
 
 #define ETX '\x03'  /** ASCII "End-of-Text", or ctrl-C */
-#if !defined(SHELL_NO_ECHO) || !defined(SHELL_NO_PROMPT)
+#define BS  '\x08'  /** ASCII "Backspace" */
-#ifdef MODULE_NEWLIB
+#define DEL '\x7f'  /** ASCII "Delete" */
-/* use local copy of putchar, as it seems to be inlined,
+
- * enlarging code by 50% */
+#ifdef MODULE_SHELL_COMMANDS
-static void _putchar(int c) {
+    #define MORE_COMMANDS _shell_command_list
-    putchar(c);
+#else
-}
+    #define MORE_COMMANDS
-#else
+#endif /* MODULE_SHELL_COMMANDS */
-#define _putchar putchar
-#endif
-#endif
 
-static void flush_if_needed(void)
-{
 #ifdef MODULE_NEWLIB
-    fflush(stdout);
+    #define flush_if_needed() fflush(stdout)
-#endif
+#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
+        MORE_COMMANDS
-        _shell_command_list,
-#endif
     };
 
     /* iterating over command_lists */
@@ -95,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
+        MORE_COMMANDS
-        _shell_command_list,
-#endif
     };
 
     /* iterating over command_lists */
@@ -233,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.
  *
@@ -242,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.
@@ -250,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)
 {
@@ -260,82 +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
+        switch (c) {
-         * linebreaks (\r). QEMU transmits only a single '\r' == 13 on hitting
+
-         * enter ("-serial stdio"). DOS newlines are handled like hitting enter
+            case EOF:
-         * twice, but empty lines are ignored. Ctrl-C cancels the current line.
+                return EOF;
-         */
+
-        if (c == '\r' || c == '\n' || c == ETX) {
+            case ETX:
-            if (c == 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();
-#ifndef SHELL_NO_ECHO
-            _putchar('\r');
-            _putchar('\n');
-#endif
 
-            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 == 0x08 || c == 0x7f) {
-            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 */
-#ifndef SHELL_NO_ECHO
-            _putchar('\b');
-            _putchar(' ');
-            _putchar('\b');
-#endif
-        }
-        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;
-            }
-#ifndef SHELL_NO_ECHO
-            _putchar(c);
-#endif
-        }
         flush_if_needed();
     }
 }
 
-static inline void print_prompt(void)
-{
-#ifndef SHELL_NO_PROMPT
-    _putchar('>');
-    _putchar(' ');
-#endif
-
-    flush_if_needed();
-}
-
 void shell_run_once(const shell_command_t *shell_commands,
                     char *line_buf, int len)
 {

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)