[PATCH rtems6 1/1] libmisc/shell: Fix timeout in getting terminal size
Peter Dufault
dufault at hda.com
Wed Jan 24 08:48:38 UTC 2024
> On Jan 23, 2024, at 7:09 PM, Chris Johns <chrisj at rtems.org> wrote:
>
> On 23/1/2024 9:00 pm, Peter Dufault wrote:
>>> 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
>>>>
>
> The original change is by me so I can take a look at this. Thanks for mentioning
> this. :)
>
>>>>
>>>> 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>.
>
> Thanks for look into this and cleaning up the cruft. It couuld be BPS specific
> and why it the include is an issue for some and not others.
>
>>
>> #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
>
> Compiling out the debug code is a good approach if not enabled.
>
>>
>> - 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 */
>
> Why is this in RAM. Do you see users needing to adjust this value? If so now
> would a user change it?
>
>>>>> +
>>>>> +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. */
>
> Question?
No, it's a comment regarding what I'm about to log. It was less of an obvious comment before I added the logging.
>
>>>>> + 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);
>
> Why the change to use write?
To be really sure it isn't mixed up with anything else. Recall the window size request sequence times out though the GNOME terminal supports it (checked with "echo" from the Linux shell). I want to be 100% sure the sequence goes in a chunk. The input is already low level.
>
>>>>> + 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) {
No one asked about TCSADRAIN. We throw away input until we get the matching sequence, may as well use TCSADRAIN.
>>>>> 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;"))
>
> I prefer these statements to be on separate lines.
I can do that. I'll send a V2.
>
> Chris
>
> {
>>>>> + 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
>>
>>
>>
>> _______________________________________________
>> 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