> On Jan 22, 2024, at 12:16 PM, Gedare Bloom <ged...@rtems.org> wrote: > > I have a couple minor notes below. More important, does this change > require updating documentation?
I'd have to look to see if this window size detection is documented, I wanted to fix the problem it caused me. > > I know we have a somewhat aging shell-specific guide: > https://docs.rtems.org/branches/master/shell/index.html > > > On Fri, Jan 19, 2024 at 5:19 AM <dufa...@hda.com> wrote: >> >> From: Peter Dufault <dufa...@hda.com> >> >> - Fix detection of timeout in rtems_shell_term_wait_for(). >> >> - Switch to using "termios" VTIME inter-character timeout. >> The previous implementation is dependent on the BSP clock tick value. >> >> Updates #4763 >> --- >> cpukit/libmisc/shell/shell.c | 101 >> ++++++++++++++++++++++++++----------------- >> 1 file changed, 62 insertions(+), 39 deletions(-) >> >> diff --git a/cpukit/libmisc/shell/shell.c b/cpukit/libmisc/shell/shell.c >> index 9cefc80..60cdb4f 100644 >> --- a/cpukit/libmisc/shell/shell.c >> +++ b/cpukit/libmisc/shell/shell.c >> @@ -805,28 +805,52 @@ void rtems_shell_print_env( >> } >> #endif >> >> +/* Debugging output for the terminal I/O associated with window size >> detection >> + * can be sent with rtems_shell_winsize_db(). >> + */ >> + >> +/* Window size detection knobs. >> + */ >> +static bool rtems_shell_winsize_dbf; /* Debug. "true" to debug. */ > Is this necessary? How does the application set/test this? I don't know. Same with the "vtime" setting, no way to change that either. Rationale: - It puts what you may want to change in one place. - I'm not a fan of "#define". - I didn't want to add a public interface to tweak things (then I *would* need to document). - It was useful. I left it in to make it clear how to compile-time turn it on if you need it. I can make the "dbf" behavior pre-processor enabled at compile-time. Or take it out. I would leave the "vtime" static, there was no way to change the previous timeout either. > >> +static int rtems_shell_winsize_vtime = 1; /* VTIME timeout in .1 seconds */ >> + >> +static void rtems_shell_winsize_vdb(const char *format, va_list ap) { >> + if (rtems_shell_winsize_dbf == false) >> + return; >> + printf("shell_winsize: "); >> + vprintf(format, ap); >> + fflush(stdout); >> +} >> + >> +static void rtems_shell_winsize_db(const char *format, ...) { >> + va_list ap; >> + va_start(ap, format); >> + rtems_shell_winsize_vdb(format, ap); >> + va_end(ap); >> +} >> + >> /* >> * Wait for the string to return or timeout. >> */ >> -static bool rtems_shell_term_wait_for(const int fd, const char* str, const >> int timeout) >> +static bool rtems_shell_term_wait_for(const int fd, const char* str) >> { >> - int msec = timeout; >> int i = 0; >> - while (msec-- > 0 && str[i] != '\0') { >> + while (str[i] != '\0') { >> char ch[2]; >> - if (read(fd, &ch[0], 1) == 1) { >> - fflush(stdout); >> + ssize_t n; >> + if ((n = read(fd, &ch[0], 1)) == 1) { >> if (ch[0] != str[i++]) { >> + rtems_shell_winsize_db("Wrong char at char %d.\n", i); >> return false; >> } >> - msec = timeout; >> } else { >> - usleep(1000); >> + rtems_shell_winsize_db("%s reading char %d.\n", >> + (n == 0) ? "Timeout" : "Error", i); > I'd suggest putting the 'i' on it's own line here. I don't know that > there's a specific style guide for this file, but having trailing > statements after the ternary is a bit harder to read. imo Sure. > >> + return false; >> } >> } >> - if (msec == 0) { >> - return false; >> - } >> + >> + rtems_shell_winsize_db("Matched string.\n"); >> return true; >> } >> >> @@ -836,41 +860,42 @@ static bool rtems_shell_term_wait_for(const int fd, >> const char* str, const int t >> static int rtems_shell_term_buffer_until(const int fd, >> char* buf, >> const int size, >> - const char* end, >> - const int timeout) >> + const char* end) >> { >> - int msec = timeout; >> int i = 0; >> int e = 0; >> memset(&buf[0], 0, size); >> - while (msec-- > 0 && i < size && end[e] != '\0') { >> + while (i < size && end[e] != '\0') { >> char ch[2]; >> - if (read(fd, &ch[0], 1) == 1) { >> - fflush(stdout); >> + ssize_t n; >> + if ( (n = read(fd, &ch[0], 1)) == 1) { >> buf[i++] = ch[0]; >> if (ch[0] == end[e]) { >> e++; >> } else { >> + rtems_shell_winsize_db("Reset search for end string.\n"); >> e = 0; >> } >> - msec = timeout; >> } else { >> - usleep(1000); >> + /* Error or timeout. */ >> + rtems_shell_winsize_db( >> + "%s looking for end string \"%s\". Read \"%s\".\n", >> + (n == 0) ? "Timeout" : "Error", end, buf >> + ); >> + return -1; >> } >> } >> - if (msec == 0 || end[e] != '\0') { >> - return -1; >> - } >> - i -= e; >> + i -= e; /* Toss away the matched end. */ >> if (i < size) { >> buf[i] = '\0'; >> } >> + rtems_shell_winsize_db("Found end string \"%s\".\n", end); >> return i; >> } >> >> /* >> - * Determine if the terminal has the row and column values >> - * swapped >> + * Determine if this is a version of the "tmux" terminal emulator with >> + * the row and column values swapped >> * >> * https://github.com/tmux/tmux/issues/3457 >> * >> @@ -878,19 +903,19 @@ static int rtems_shell_term_buffer_until(const int fd, >> * in the time it takes to get the fix into code so see if tmux is >> * running and which version and work around the bug. >> * >> - * The terminal device needs to have VMIN=0, and VTIME=0 >> + * The terminal device needs to have VMIN=0, and VTIME set to the >> + * desired timeout in .1 seconds. >> */ >> -static bool rtems_shell_term_row_column_swapped(const int fd, const int >> timeout) { >> +static bool rtems_shell_term_row_column_swapped(const int fd, const int >> fout) { >> char buf[64]; >> memset(&buf[0], 0, sizeof(buf)); >> /* >> * CSI > Ps q >> * Ps = 0 => DCS > | text ST >> */ >> - fputs("\033[>0q", stdout); >> - fflush(stdout); >> - if (rtems_shell_term_wait_for(fd, "\033P>|", timeout)) { >> - int len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), "\033\\", >> timeout); >> + write(fout, "\033[>0q", 5); >> + if (rtems_shell_term_wait_for(fd, "\033P>|")) { >> + int len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), "\033\\"); >> if (len > 0) { >> if (memcmp(buf, "tmux ", 5) == 0) { >> static const char* bad_versions[] = { >> @@ -916,8 +941,8 @@ static bool rtems_shell_term_row_column_swapped(const >> int fd, const int timeout) >> static void rtems_shell_winsize( void ) >> { >> const int fd = fileno(stdin); >> + const int fout = fileno(stdout); >> struct winsize ws; >> - const int timeout = 150; >> char buf[64]; >> bool ok = false; >> int lines = 0; >> @@ -933,18 +958,16 @@ static void rtems_shell_winsize( void ) >> if (tcgetattr(fd, &cterm) >= 0) { >> struct termios term = cterm; >> term.c_cc[VMIN] = 0; >> - term.c_cc[VTIME] = 0; >> - if (tcsetattr (fd, TCSADRAIN, &term) >= 0) { >> + term.c_cc[VTIME] = rtems_shell_winsize_vtime; >> + if (tcsetattr (fd, TCSAFLUSH, &term) >= 0) { >> memset(&buf[0], 0, sizeof(buf)); >> /* >> * >> https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Miscellaneous >> * >> * CSI 1 8 t >> */ >> - fputs("\033[18t", stdout); >> - fflush(stdout); >> - if (rtems_shell_term_wait_for(fd, "\033[8;", timeout)) { >> - int len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), >> ";", timeout); >> + if (write(fout, "\033[18t", 5) == 5 && >> rtems_shell_term_wait_for(fd, "\033[8;")) { >> + int len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), >> ";"); >> if (len > 0) { >> int i; >> lines = 0; >> @@ -953,7 +976,7 @@ static void rtems_shell_winsize( void ) >> lines *= 10; >> lines += buf[i++] - '0'; >> } >> - len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), "t", >> timeout); >> + len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), "t"); >> if (len > 0) { >> cols = 0; >> i = 0; >> @@ -966,7 +989,7 @@ static void rtems_shell_winsize( void ) >> } >> } >> } >> - if (rtems_shell_term_row_column_swapped(fd, timeout)) { >> + if (ok && rtems_shell_term_row_column_swapped(fd, fout)) { >> int tmp = lines; >> lines = cols; >> cols = tmp; >> -- >> 1.8.3.1 >> >> _______________________________________________ >> devel mailing list >> devel@rtems.org >> http://lists.rtems.org/mailman/listinfo/devel Peter ----------------- Peter Dufault HD Associates, Inc. Software and System Engineering _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel