topic Re: Unprofessional Shell behavior in MQX Software Solutions

Unprofessional Shell behavior

anguel — Fri, 26 Apr 2013 15:00:42 GMT

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

Re: Unprofessional Shell behavior

Martin_ — Tue, 07 May 2013 10:36:41 GMT

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 !

Re: Unprofessional Shell behavior

anguel — Tue, 07 May 2013 11:46:22 GMT

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

Re: Unprofessional Shell behavior

Martin_ — Tue, 07 May 2013 12:03:33 GMT

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...

Re: Unprofessional Shell behavior

anguel — Tue, 07 May 2013 12:48:04 GMT

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...

Re: Unprofessional Shell behavior

trailman — Sat, 11 May 2013 16:47:13 GMT

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.

Re: Unprofessional Shell behavior

anguel — Mon, 13 May 2013 12:34:30 GMT

Hi Gilles,

Many thanks for the help with the DEL vs Backspace problem! Indeed some terminals seem to send DEL and MQX does not handle that.

As far as I understand this does not fix the prompt deletion or did I overlook something?

Regards,

Anguel

Re: Unprofessional Shell behavior

trailman — Tue, 14 May 2013 20:12:45 GMT

Hi Anguel,

You're right, the code above does not prevent the prompt deletiion; it just makes DEL behave as BS so that you can delete characters properly and be sure of the command that is run when pressing ENTER.

Here is a new code for MQX3.7 disabling the echo of the BS/DEL when the command is empty, so that the BS/DEL does not delete more characters on your console than expected. When IO_SERIAL_TRANSLATION and IO_SERIAL_ECHO are enabled (the case for the prompt), I use the field LOCATION of the file descriptor to store the number of charactes currently in the command. This field was not used by serial driver (seek() not supported).

Here is the new code for _io_serial_int_read() in mqx/source/io/serial/int/serl_int.c in MQX3.7 (for interrupt mode serial driver)

} /* Endif */
_int_enable();

/**** START new code ****/
if (flags & IO_SERIAL_TRANSLATION) {
   if (c == '\n') {
      if (flags & IO_SERIAL_ECHO) {
         _io_serial_int_putc_internal(int_io_dev_ptr, (char)c, 0);
         fd_ptr->LOCATION = 0; // start on a new empty line
      }
   } else if (c == '\r') {
      /* Start CR 387 */
      if (flags & IO_SERIAL_ECHO) {
         _io_serial_int_putc_internal(int_io_dev_ptr, (char)c, 0);
         _io_serial_int_putc_internal(int_io_dev_ptr, '\n', 0);
         fd_ptr->LOCATION = 0; // start on a new empty line
      } /* Endif */
      /* End CR 387 */
      c = '\n';
   } else if ((c == '\b') || (c == 127)) { // BS or DEL
      if ((flags & IO_SERIAL_ECHO) && fd_ptr->LOCATION) {
         _io_serial_int_putc_internal(int_io_dev_ptr, (char)'\b', 0);
         _io_serial_int_putc_internal(int_io_dev_ptr, (char)' ', 0);
         _io_serial_int_putc_internal(int_io_dev_ptr, (char)'\b', 0);
         fd_ptr->LOCATION--; // character removed
      }
      c = '\b'; // DEL is translated to BS
   } else {
      if (flags & IO_SERIAL_ECHO) {
         _io_serial_int_putc_internal(int_io_dev_ptr, (char)c, 0);
         fd_ptr->LOCATION++; // new character added to line
      }
   }
} else {
   /* no IO_SERIAL_TRANSLATION */
   if (flags & IO_SERIAL_ECHO) {
      _io_serial_int_putc_internal(int_io_dev_ptr, (char)c, 0);
   }
}
/**** END new code ****/

*data_ptr++ = c;
--i;

However this is only for the prompt on a serial line. When connecting through telnet, a similar code has to be added to rtcs/source/apps/telnetio.c in the lines under

if (io_ptr->LOCAL_OPTIONS[TELOPT_ECHO]) {

However, I think the best way to do it for both serial line and telnet would be to :

- disable echo at serial line and telnet level

- modify shell/source/shell/shell.c to use a custom shell_getline() instead of fgets()

- manage command line edition and echo in this new function

This would make possible to add advanced edition features such as insertion or use of arrow keys. This is not possible to do it as it is currently implemented with fgets()

Re: Unprofessional Shell behavior

anguel — Wed, 15 May 2013 13:16:15 GMT

Gilles,

Again, many many thanks for your help. I will have to look deeper into MQX Shell to understand especially the last part of your comments and the universal proposed solution. What I hope is that Freescale will get the Shell work properly now that you have pointed them to a possible solution. I also hope for a confirmation from Martin that your solution will be implemented ASAP by the developers.

PS: For some unknown reason I cannot mark your answers as solution or as helpful...

Regards,

Anguel

Re: Unprofessional Shell behavior

c0170 — Wed, 15 May 2013 18:39:18 GMT

Hello,

thank you gilles buloz for sharing your workaround :smileywink: We will look into this.

@anguel

I changed this thread into a discussion (not a question), that's the reason why you can't mark it as helpful/correct answer.

Regards,

c0170

Re: Unprofessional Shell behavior

trailman — Wed, 26 Jun 2013 20:27:17 GMT

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

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.

Re: Unprofessional Shell behavior

anguel — Thu, 27 Jun 2013 08:36:48 GMT

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

Re: Unprofessional Shell behavior

trailman — Fri, 28 Jun 2013 22:03:49 GMT

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

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.

Re: Re: Unprofessional Shell behavior

trailman — Wed, 18 Sep 2013 22:07:21 GMT

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

Re: Re: Re: Unprofessional Shell behavior

trailman — Sun, 22 Sep 2013 20:51:34 GMT

The zip file containing all modifed files for MQX 3.7

Re: Unprofessional Shell behavior

danieldelatorre — Wed, 14 May 2014 08:15:00 GMT

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.

Re: Re: Re: Re: Unprofessional Shell behavior

trailman — Sun, 09 Nov 2014 22:39:02 GMT

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.

Re: Unprofessional Shell behavior

andrewdarrow — Fri, 24 Jul 2015 14:05:20 GMT

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!

Re: Unprofessional Shell behavior

danielhadley — Mon, 27 Jul 2015 08:33:10 GMT

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.

Re: Unprofessional Shell behavior

trailman — Fri, 31 Jul 2015 20:31:54 GMT

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.