[PATCH rtems6 1/1] libmisc/shell: Fix timeout in getting terminal size
Peter Dufault
dufault at hda.com
Tue Jan 23 10:00:59 UTC 2024
> On Jan 22, 2024, at 1:51 PM, Peter Dufault <dufault at hda.com> wrote:
>
>
>
>> On Jan 22, 2024, at 12:16 PM, Gedare Bloom <gedare at 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 <dufault at hda.com> wrote:
>>>
>>> From: Peter Dufault <dufault at 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'm embarrassed to say I didn't see that SHELL_WINSIZE_DEBUG is already in the code. I will switch to use that.
I don't like losing the format checking when a flag is undefined. I can avoid that by unconditionally adding <rtems/bspIo.h>. There is already SHELL_DEBUG conditional code using "printk()" without including <rtems/bspIo.h>.
#define SHELL_STD_DEBUG 1 /* Or 0. */
#define SHELL_WINSIZE_DEBUG 1 /* Or 0. */
#if SHELL_STD_DEBUG | SHELL_WINSIZE_DEBUG
#include <rtems/bspIo.h>
#define shell_std_debug_(...) \
do { printk("shell[%08x]: ", rtems_task_self()); printk(__VA_ARGS__); } while (0)
#endif
#if SHELL_STD_DEBUG
#define shell_std_debug(...) do { shell_std_debug_(__VA_ARGS__); } while (0)
#else
#define shell_std_debug(...)
#endif
#if SHELL_WINSIZE_DEBUG
#define shell_winsize_debug(...) do { shell_std_debug_(__VA_ARGS__); } while (0)
#else
#define shell_winsize_debug(...)
#endif
- OR: Polluted (with rtems/bspIo.h) implementation:
#define SHELL_STD_DEBUG 1 /* Or 0. */
#define SHELL_WINSIZE_DEBUG 1 /* Or 0. */
#include <rtems/bspIo.h>
#define shell_std_debug_(ENABLE, ...) \
do { if (ENABLE) printk("shell[%08x]: ", rtems_task_self()); printk(__VA_ARGS__); } while (0)
#define shell_std_debug(...) shell_std_debug_(SHELL_STD_DEBUG, __VA_ARGS__)
#define shell_winsize_debug(...) shell_std_debug_(SHELL_WINSIZE_DEBUG, __VA_ARGS__)
>
> 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 at rtems.org
>>> http://lists.rtems.org/mailman/listinfo/devel
>
> Peter
> -----------------
> Peter Dufault
> HD Associates, Inc. Software and System Engineering
>
>
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
Peter
-----------------
Peter Dufault
HD Associates, Inc. Software and System Engineering
More information about the devel
mailing list