Shell Enhancements Topic is solved

[jstruebel]

I added some features to the basic shell. They are a command history and command completion when you press the "tab" key. I also removed some extra code for exit that seemed redundant and I made the usage function in shell.c and shell_cmd.c public. The patches for each change are attached. They are:

01-shellExit.patch - Remove the explicit exit code from the command processing section of the shell thread since it is now part of the shell_local_commands. Also removed the extra printing of exit in the help message.

02-commandHistory.patch - Added command history functionality. It saves the current command line to a buffer when you press "enter". To navigate the history use ^P and ^N.

03-commandCompletions.patch - Added command completion based on the shell_local_commands and the user-specified commands when the "tab" key is pressed.

04-escapeSequence.patch - Added escape sequence processing to shellGetLine. This is required to use the up and down arrow keys for the history patch. Those are the only escape sequences that it handles at the moment.

05-shellUsage.patch - Moved the usage function to the shellUsage macro in shell.h so that it can be used by user-specified commands. Made all of the commands defined in shell.c and shell_cmd.c use the new macro for their usage message.

Also, given the configurations that can be specified for the shell, would it make sense to add a shellconf.h file to projects and include it from shell.h to allow the user to easily change the default defines? Right now I've just added them to my chconf.h when I needed to use a different value, but I think a separate file is better to avoid cluttering the chconf.h.

I used the Github mirror and git to generate the patches, so if you need them in a different format I can see about how to create them against the SVN repo. Just let me know.

[Giovanni]

Hi,

Great, I will give it a try tomorrow.

Giovanni

[Giovanni]

Hi,

I committed all patches with few changes adding a switch that enables a shellconf.h file. All defaults now are FALSE for compatibility with existing code or I would have to modify all demos and there would be impacts on user code too.

The only thing I don't like much is that extra structure to be passed to configuration, it has to be initialized but cannot be const. Would be possible to just have a pointer to a buffer and a size field directly in the configuration structure? the indexes would be the first bytes in the buffer itself, the buffer would be initialized by the thread on entry.

Everything is very useful anyway, thanks.

Giovanni

[electronic_eel]

Hi,

thank you for publishing this. It will make the shell much nicer to work with.

There is just one point I'm not sure about:

To navigate the history use ^P and ^N.

Won't it make more sense to use the common VT100 sequences for that? That way you can just use a regular terminal program and press the cursor up and down keys on your keyboard and the shell will act accordingly. Using ^P and ^N instead isn't intuitive to the user.

Here is a link to the VT100 manual where all the sequences are explained:
http://www.vt100.net/docs/vt100-ug/contents.html

[jstruebel]

That makes sense to have all the defaults FALSE.

I had created the extra structure so that I didn't clutter up the config structure, but if you think it's better to have the buffer and size fields in the main config structure I don't have a problem with that.

I'm not quite certain I understand how you are intending the indexes to work. Since the buffer is an array of char all the elements are 8-bit, but the indexes are ints, so they may be 16-bit or 32-bit. Is that just taken care of via casting? The indexing logic would need to be changed to not use that portion of the buffer, and we would need the user to allocate enough space to hold them plus the history data. Can we specify that the beginning of the buffer follows the structure for the indexes so that the code remains readable?

Forgive me if I suggest to do something that doesn't make sense or ask dumb questions. I'm a hardware engineer and I only have limited experience with software. I want to learn and understand what I can so that my contributions are helpful.

Jonathan

[jstruebel]

@electronic_eel, I agree that the ^P and ^N codes aren't intuitive, but I had to add escape code handling to support the up and down arrows. I used them to test just the history buffer changes since I saw that is what the microrl project used if the arrow keys weren't enabled.

The later 04-escapeSequence.patch added the escape code capability, but I made it as a feature that can be disabled in case the user didn't want the extra code. Perhaps we require to use the escape sequences for the command history? It probably doesn't add too much code though and would be more intuitive.

Thank you for the link.

Jonathan

[Giovanni]

I'm not quite certain I understand how you are intending the indexes to work. Since the buffer is an array of char all the elements are 8-bit, but the indexes are ints, so they may be 16-bit or 32-bit.

You could cast the buffer pointer to a struct the use the struct fields. The real buffer would start at +sizeof(struct).

Alternatively those indexes could be variables in the thread (cleaner).

Giovanni

[jstruebel]

I like the idea of making them variables in the thread. They really aren't needed anywhere else and that way the user doesn't have to worry about them either. I'll see about making those changes this week when I have time and post back.

Jonathan

[Giovanni]

You could make shellGetLine() return control codes into a msg_t and handle things inside the thread so there is no need to pass vars to the function.

Giovanni

[jstruebel]

I moved the history buffer indexes into the thread so that only the buffer and buffer size are contained in the shell config structure. I kept the ShellHistory type and used that within the thread so that I only had to pass one additional variable to shellGetLine(). Because the command completion needs to know the value of p and it also changes it, I think it is cleaner to just pass the shp variable through it rather than trying to handle the history buffer and command completion with msg_t return codes. The patch is attached.

Jonathan