Creating shell with current SHELL_DEFINE() is not practical

[jukkar]

I have been porting multiple network related shells to use new shell and noticed some issues how the shells are defined.
Currently one needs to do something like this in order to define a shell.

SHELL_CMD_REGISTER(net, &net_commands, "Networking commands", NULL);
SHELL_UART_DEFINE(shell_transport_uart);
SHELL_DEFINE(net_shell, "net:~$ ", &shell_transport_uart, 10, SHELL_FLAG_OLF_CRLF);

Currently this code is called by subsys/net/ip/net_shell.c when CONFIG_NET_SHELL=y is set.

There are other network related shells that can co-exists with this network shell. For example if one defines CONFIG_NET_L2_IEEE802154_SHELL=y then IEEE 802.15.4 specific shell is created. It is also created similar way as generic network shell.

SHELL_CMD_REGISTER(ieee802154, &ieee802154_commands, "IEEE 802.15.4 commands", NULL);
SHELL_UART_DEFINE(ieee802154_shell_transport_uart);
SHELL_DEFINE(ieee802154_shell, "ieee802154:~$ ", &ieee802154_shell_transport_uart, 10, SHELL_FLAG_OLF_CRLF);

It really looks like that the backend configuration should not be done like this for a shell. In these examples, the actual shell implementation is in proper place but how this shell is used should be placed in some shell specific directory and controlled by generic config options. The shells in question do not really care about the backend, they are just generic shells that can be used with any backend.

It looks like that the SHELL_DEFINE() could easily just have the two parameters like this SHELL_DEFINE(net_shell, SHELL_FLAG_OLF_CRLF);. The other parameters should be defined in some other file.

Also noticed that if I enable various shells at the same time, and this is quite common case with networking, the history of the shell stops working.

[jakub-uC]

@jukkar : But do you need SHELL_DEFINE in these files at all?
In my opinion all you need is to register commands which will be available for all created shells - somewhere else.

[jukkar]

If I leave the SHELL_DEFINE() away, which means that I cannot call shell_init(). Then also the shell functionality is missing (= I do not see any shell after this). Regardless how this is is implemented in shell library and support, the expectation (at least from networking shell point of view) is that everything should work out of the box without any extra coding regarding shell configuration.

[jakub-uC]

Possibly I am missing full pictrue here. I thought that: CONFIG_NET_SHELL or CONFIG_NET_L2_IEEE802154_SHELL are in fact only resposible for adding commands to the shell. Shell instance needs to be created separately.

Otherwise you would force one transport medium for shell - UART.

[jukkar]

Shell instance needs to be created separately.

I am fine with this, but which component in zephyr would do this? We can have multiple shells that are activated by just a config option, only common element here is the core shell code so I suppose it should call the shell_init() then?

[jukkar]

Proposal:

• keep SHELL_DEFINE() as we just introduced the flag variable there which is useful, perhaps the shell commands variable could be placed as one parameter there
• place the shell defined by SHELL_DEFINE() to a separate linker section
• make core shell code traverse this section and initialize things like calling shell_init() if it needs to do so

[jakub-uC]

@jukkar : I see that in old implementation shell init was in file: shell_service.c
Can't we reuse this approach? You could also add there macro SHELL_DEFINE as it is inside shell_service.c.
I am thinking about short term solution now.
Your proposal will require some more work.

[jukkar]

I am not sure that we will need any short term solution here. Currently most of the things work ok if I have multiple shells defined, except that history does not work but personally I can live with that. My issue is more like an enhancement request than a bug report anyway.

[jakub-uC]

@jukkar : it works with legacy shell. Plan is to get rid of it asap.

[jakub-uC]

@jukkar : New Shell have different approach comparing to the old one.
I think that Kconfig shall be a bit changed. Instead of activating CONFIG_NET_SHELL we shall activate CONFIG_NET_SHELL_COMMANDS. Shell itself shall be enabled as a separate module.