Unprofessional Shell behavior

[anguel]

Hi!

I am experimenting with MQX Shell in a terminal and see the following problems:

1. Backspace also deletes the prompt, i.e. the shell>

2. Entered chars are deleted properly but are not removed from the buffer, i.e. the user thinks that he has deleted entered chars but actually they are still there and will be executed.

Is this expected and are there any ways to fix this? Thanks!

Regards,

Anguel

[trailman]

Here is the full featured final patch I tested today on my MCF52259 board, with MQX 3.4 and MQX 3.7.

Features are :

- shell command line edition with history (when connected through serial line or through Telnet)

- for existing applications using fgets or fgetline ..., , support for DEL in additon to BS for BACKSPACE key, and fix to prevent prompt deletion due to over usage of BACKSPACE key (works when connected through serial line or through Telnet)

- support for legacy ! + <ENTER> to run last command again

- fix for IO_IOCTL_SERIAL_SET_FLAGS ioctl that was failing in disabling the echo when connected through Telnet (so now your can disable the echo for example at a password imput.to prevent password from beeing displayed)

This patch is for MQX 3.7 and is probably ok "as is" with newer versions. To apply it on MQX 3.4, edit the patch to remove the ", 0" from the _io_serial_int_putc_internal() calls in mqx/source/io/serial/int/serl_int.c

As said before, the default history size is only one command (the last one), but it can be set to any greater value (if you have enough RAM) by changing shell/source/include/sh_prv.h from :

    char                 HISTORY[SHELL_MAX_CMDLEN]; // one line of history

to

    char                 HISTORY[SHELL_MAX_CMDLEN*10]; // 10 lines of history

WARNING : this patch fixes both interrupt and polled serial drivers; however I only tested this patch using the interrupt serial driver. Polled serial driver has not been tested however the modified code is exactly the same in both cases so the patch should work alos in polled mode.

[trailman]

In addition to the patch above, here is a new version of the shell/source/shell/shell.c file to have a more memory efficient command line history.

To save some memory, each entry of history is no more stored in a fixed size array of 80 bytes per entry (leading to 1600 bytes for 20 history entries).

Instead of that, a single array is used for both command line and history : the command is the string at the beginning of the array and is null terminated; and the history is stored in the remaining place after this null.

The number of entries available in history depends on the size of the last commands typed, and on the size of the array.

For example in shell/source/include/sh_prv.h, replace :

char	CMD_LINE[SHELL_MAX_CMDLEN];
char	HISTORY[SHELL_MAX_CMDLEN];

with

char	CMD_LINE[SHELL_MAX_CMDLEN * 2];

to have a single 160 bytes buffer, and use the new shell.c attached

[trailman]

The zip file containing all modifed files for MQX 3.7

[trailman]

As a patch file is better to see the changes made and apply these changes to some other MQX release, please find the patch file corresponding to the modifed files above.

Nearly all modifications are made to the shell : to shell.c and sh_prv.h. These changes should be easy to port to some newer MQX releases (above 3.7) if needed.

Patching serl_int.c and serl_pol.c was to fix command line input with echo enabled, but as the new Shell_getline() implemented by this patch (in shell.c) runs with echo disabled, this should no more be needed.

Also the echo problem in the RTCS Telnet server seems fixed in newer MQX versions (according to release notes), so patching telnetio.c should also no more be required with these releases.

[andrewdarrow]

Hi gilles,

I'm wondering if you have created a patch for the KSDK 1.2.0 version of MQX? We are looking at porting your patch to KSDK 1.2.0 but wanted to check with you first to see if you've already done so.

Thanks!

[danielhadley]

Hi,

I used Gilles patch on KSDK 1.20 recently. I didnt apply the patch automatically but went through altering the files by hand as I wasn't sure how much the directory structure had changed.

I did have to change a few things, mainly the ioctl calls in the Shell_getline function in shell.c that disable echo, then some minor find & replace work elsewhere. I'd be happy to help too.

[trailman]

Hi Daniel,

I'm happy to see that you used my patch.

This patch is not specific to a platform but to the shell itself so it may be used on any platform supported by MQX; however I only had the opportunity to work with MQX3.7 on a MCF52259 board so I made no other patch than the one I posted here (this answers Andrew's question).

Reading your post, I understand you have succeeded in porting my code to your version of MQX and platform, so I would be very interested if you could tell me which MQX version and post the patch of your changes.

This could also be helpful to some other guys.

[trailman]

To keep up-to-date with MQX, I downloaded the latest MQX 4.2.0 to have a look to the source code, and I also tried to apply my patch made for MQX3.7 on MQX4.2.0, and to do the required changes for MQX4.2.0

As I have no hardware nor development environment anymore, I did not have the opportunity to build or test it. However I have a patch file (updating shell.c and sh_prv.h) and the resulting updated files (for the ones not familiar with patch files).

I POST THEM "AS IS" BUT THEY MAY NOT BUILD NOR WORK.

Patching some other files should not be required anymore with MQX4.2.0, but it's still not clear to me if temporarily disabling the echo works when connected to the shell through Telnet (required by my patch). If you have characters echoed twice, try to make the following change to rtcs/source/apps/telnetio.c (in  _io_telnet_echo()) to replace :

    if (io_ptr->LOCAL_OPTIONS[TELOPT_ECHO])

with

    if ((io_ptr->LOCAL_OPTIONS[TELOPT_ECHO]) && (io_ptr->FLAGS & IO_SERIAL_ECHO))

[danielhadley]

In Shell.c I've replaced the code to temporarily disable echo as you've said. The original from your patch is this:

    // disable echo on terminal
    ioctl(stdin, IO_IOCTL_SERIAL_GET_FLAGS, (void *)&flags);
    flags_orig = flags;
    flags &= ~(IO_SERIAL_ECHO /*| IO_SERIAL_TRANSLATION*/); // keep translation to use \n instead of \r\n in printf()
    ioctl(stdin, IO_IOCTL_SERIAL_SET_FLAGS, &flags);

The ioctl function seems to have changed in MQX4.2.0 with KSDK, so I've changed it to suit. Im pretty sure this is only for the KSDK version:

// disable echo on terminal
_nio_ioctl(fp->_FD,error,IO_IOCTL_SERIAL_GET_FLAGS,&flags);
flags_orig = flags;
flags &= ~(NIO_TTY_FLAGS_ECHO);
_nio_ioctl(fp->_FD ,error,IO_IOCTL_SERIAL_SET_FLAGS,&flags);

It now requires an int to be passed for error handling.

When when the flags_orig is restored later on i use the same format.

[trailman]

Hi Daniel,

Having a look to the MQX source code it seems that the code to use depends on the build flags MQX_USE_IO_OLD and PLATFORM_SDK_ENABLED. My code original code/patch was for the MQX_USE_IO_OLD case.(older MQX style, also without SDK). Thanks for pointing this change out.

Did you have to change something for  _io_fgetc() /  _io_fputc ?

Did you have the opportunity to test the shell when connected through Telnet ?

[danielhadley]

I've used fgetc() and fputc() instead of their _io_ equivalents. That must be what i changed first with a find and replace. I have having massive issues getting the character echo turned off before I realised they'd changed how ioctl worked. After i made the changes above everything started working great over telnet.

[danielhadley]

Hi,

I'll take a look at what I did today and will try to cobble it into something presentable haha. Its for MQX with KSDK 4.2

[trailman]

Hi all,

I've made a patch to add command line edition (including arrow keys) and history to the shell prompt :

  ENTER ; validate edition

  CTRL-C or ESCAPE+ESCAPE : abort edition

  BACKSPACE (BS or DEL) : delete character on the left of cursor

  CTRL-A / CTRL-E : move cursor to beginning / end of line

  CTRL-B / ARROW-LEFT : move cursor back one character

  CTRL-F / ARROW-RIGHT : move cursor forward one character

  CTRL-P / ARROW-UP : older history entry

  CTRL-N / ARROW-DOWN : newer history entry

This works with shell on serial console but also when connected through Telnet.

I've tested this patch with MQX 3.4 and MQX 3.7 but it should work with other versions as nothing seems to have changed in the impacted code.

This support is added in a new Shell_getline() function called at prompt that temporarily disables echo on stdin and manages the echo by itself (as does bash shell under Linux). This function is called by the shell instead of fgets() for prompt.

I've also fixed the telnet server to make it work also when connected to the MQX shell through Telnet.

This also fixes the IO_IOCTL_SERIAL_SET_FLAGS ioctl that was failing in disabling the echo when connected through Telnet; so now your can disable the echo for example at a password imput.to prevent password from beeing displayed.

By default the history size is only one command (the last one), but it can be set to any greater value (if you have enough RAM) by changing shell/source/include/sh_prv.h from :

    char                 HISTORY[SHELL_MAX_CMDLEN]; // one line of history

to

    char                 HISTORY[SHELL_MAX_CMDLEN*10]; // 10 lines of history

If you want to call Shell_getline() from an application program, you can declare it as global and use some history or not depending on requirements.

In addition you can also use the last patch I posted if you want to fix non working BACKSPACE key or prevent over deletion when using standard fgets(), fgetline() .... functions on serial console, in existing applications other than shell prompt.

Also please note that "!" no more works to recall last command (use ARROW-UP or CTRL-B), so in addition to what the patch does, you can also delete the whole "if" block containing the deleted line :

// strncpy(shell_ptr->HISTORY,shell_ptr->CMD_LINE,sizeof(shell_ptr->HISTORY));

If you want to keep support for "!", add it to the Shell_getline_internal() function where ENTER or \n are processed to return the last command from history when "!" has been typed.

[anguel]

Gilles,

Many many thanks for your enhancement. This is really great and Freescale should reward you in some way.

I am currently working on a different non-MQX project but will certainly use your patch as soon as I get back to MQX Shell!

Many thanks again!

Best regards,

Anguel

[Martin_]

Hi Anguel,

yes, that is the shell behavior. Indeed deleted chars should be deleted from the buffer. I will notify MQX team to look on this,

If you decide to make the improvement on your own, please share your code with community ! Thanks !

[anguel]

Martin,

Thank you for the answer. I had looked through the demos and thought that maybe some of the demos are unfinished and therefore show this incomplete shell behavior...

Actually, I am really disappointed. The shell is presented as part of the "mature" and "proven" MQX and I expected something that one can use "as is" and immediately give it to the customers, not something that has to be developed first.

I hope that Freescale will implement the shell properly as soon as possible. I have enough work to do with other problems of MQX and non-working board demos that normally "should" work out of the box according to the advertisements. I simply don't have the time and resources to rewrite the shell component so I hope that your developers will be able to do this. Sorry, but I am tired of all the marketing to see that nothing is actually done properly...

Regards,

Anguel

[Martin_]

Anguel, although I can understand some of your comments, I also think that "shell" is working out of the box, it just doesn't have features that your application requires. For this purposes, we give you all source codes. You don't need to start programming from scratch. Personally I see this as advantage.

Thank you for your feedback ! Forwarding to responsible internal team...

[anguel]

I am not aware of your software quality assurance if you have any, but in my small company if a programmer gives such a shell to a customer I would immediately fire him. Such things don't need experienced programmers, some students that have ever used a command prompt should be able to see such problems. Source code is nice but Freescale is not distributing the shell as some unfinished demo code but as a component of a proven industrial-grade OS that is in version 4. An customers expect to be able to use such components without modifications. Again, no shell should delete its own prompt and commands deleted by backspace should not execute on enter just because they are still in some internal buffer for some unknown reason. There is IMHO no excuse for such things, I have not seen a shell that behaves like this. This is not a shell, this is blind typing where you don't execute what you see on the screen. Unfortunately, many developers do not even test such things, rely on your shell and then the customer accidentally formats his SD card because he thought that he had deleted the format command he had entered before...

[danieldelatorre]

Go BUY an RTOS if you don't like what is free.  There is a saying that might teach you quite a bit, you get what you pay for.

[trailman]

Hi Anguel,

I had the same problem. This should be due to your console that sends a DEL (ASCII 127) instead of a BACKSPACE (ASCII when you press the BACKSPACE key (depending on console configuration).

Your can oress CTRL-H instead and it should delete characters properly.

In fact the DEL is echo-ed back to your console and visually deletes a character on the line, but is not understood by MQX as it should be. That's why MQX lets you also delete the prompt on your console.

I fixed it for MQX34 by adding support for DEL in the BSP sources :

    mqx/source/io/serial/int/serl_int.c

    mqx/source/io/serial/polled/serl_pol.c

(only one file is really mandatory : the one you use, depending if you use the polled or int serial driver)

and rebuild the MQX BSP

The fix is easy; for example in serl_int.c I've added the code between line 13 and 19 below :

if (flags & IO_SERIAL_TRANSLATION) {
  if (c == '\r') {
    /* Start CR 387 */
    if (flags & IO_SERIAL_ECHO) {
      _io_serial_int_putc_internal(int_io_dev_ptr, (char)c);
    } /* Endif */
    /* End CR 387 */
    c = '\n';
  } else if ((c == '\b') && (flags & IO_SERIAL_ECHO)) {
    _io_serial_int_putc_internal(int_io_dev_ptr, (char)'\b');
    _io_serial_int_putc_internal(int_io_dev_ptr, (char)' ');
  } /* Endif */
  else if (c == 127) { // DEL is translated to BS
    if (flags & IO_SERIAL_ECHO) {
      _io_serial_int_putc_internal(int_io_dev_ptr, (char)'\b');
      _io_serial_int_putc_internal(int_io_dev_ptr, (char)' ');
    }
    c = '\b';
  }
}

You should find nearly the same code in serl_pol.c where you can apply the same fix if needed.