[PATCH rtems6 1/1] libmisc/shell: Fix timeout in getting terminal size
Chris Johns
chrisj at rtems.org
Wed Jan 24 00:09:01 UTC 2024
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?
>>>> + 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?
>>>> + 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;"))
I prefer these statements to be on separate lines.
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
More information about the devel
mailing list