Improve shell_commands

[astralien3000]

I tried to improve the shell_command_list management with the help of a linker script.
The main problem of this technique is that linker scripts are not compatible between boards/cpus.

Is the idea interesting ?

[smlng]

Is the idea interesting ?

Not sure ... please elaborate on the benefits and (future) use cases of this restructuring. Its not obvious (at least to me), so currently I'm just (wild) guessing how this can be useful.

[vincent-d]

Is the idea interesting ?

I would say it is!

I didn't look into it, but I believe this look quite similar (at least in the concept) to #7523. Did you see this one?

[astralien3000]

@smlng

please elaborate on the benefits and (future) use cases of this restructuring.

The first and simple improvement is that instead of maintaining 3 conditional compilation lists (1, 2, 3), we only need to maintain 2 or even 1 (I think I can get rid of shell_commands.c, actually).

Also, this way of doing enable to "register" commands anywhere in the code, making the shell less "centralized". This has some benefits and some drawbacks, but it's still interesting to me.

And the main benefit I have in mind is the possibility to transform a "normal" application (in examples or tests) into a shell command, without needing to put it in the shell_commands module. For example, theses applications could be nice shell commands : periph_uart, driver_dynamixel, etc...

@vincent-d, yes indeed ! I still haven't splitted the commands declaration into several files, but it would be the next step.

[smlng]

@astralien3000 thanks for the explanation ... which matches with what I (wildly) guessed might be the benefits here 😉 And I agree this will ease the shell command handling a lot.

[astralien3000]

Also, this modification could enable to modify the shell interface, while this enable an other way to register user-defined shell commands withour passing an array to shell_run.

[jnohlgard]

The idea is good in my opinion, but I think the implementation has some flaws.
The shell command list should be const and placed in .rodata or .text to avoid taking up ram at runtime. It would also be beneficial if it was possible to only use one single ldscript for all platforms, to make it easier to maintain in the future.
Can this make use of the proposed cross file arrays in the xfa PR?

[astralien3000]

@gebart, I agree for the .text and .rodata, but doesn't some platforms have problems with retrieving data from the flash (for harvard-like architectures) ? I'm thinking of AVRs for examples.
And yes, I will use the xfa.

[PeterKietzmann]

@jcarrano you cared a lot about the shell recently. Would you mind to take this PR over?

[jcarrano]

I think this makes the shell module much more cleaner and it is an improvement.

In my view, with full XFA support, the ideal solution will be for the shell commands to be defined by the module to which they relate (eg. the "can" command would be defined in the "can" module, not in "shell"). Still, this is a good stepping stone in that direction.

The hardest blocker for this at the moment is that the array is placed in RAM, thus wasting both flash and RAM.

[jcarrano]

What does this do? Can this renaming of the section affect something else?

[astralien3000]

Sorry, I don't remember why this was done.

[jcarrano]

Mmm, interesting. I was wondering how you avoided the linker discarding the symbols, which is the reason why #9105 is still unmerged: it needs either #8711 (ld -r) or "--whole-archive" (which I think is the preferred option in conjuction with #10195 )

I see that you avoided much of the problems that have been holding #8711 by limiting the "--whole-archive" to just the shell module.

@cladmi , @gebart Wouldn't this be a good approach "unlock" #8711, or its equivalent with "--whole-archive"? As far as I understand, most of the problems come from a minority of files which define things like interrupt handlers. The "--whole-archive" could be enabled only for select modules (or, if most things work, enabled by default and disabled for select modules.)

[cladmi]

For #8711 I was thinking about rebasing/cleaning it, changing it to whole-archive and going for trying to enable per architecture. It managed to go far enough so that it is only a few PRs away for being able to enable some boards.

Not to say it should be supported only on some boards, but simplify the review.

We now have a server to build the two versions and compare the elf files, just need to take the time to analyze and solve all the issues.

[jcarrano]

trying to enable per architecture.

Agree. I insist, if there is some problematic module (if I recall correcyly we were having too few symbols discarded sometimes?) we can, as a quirk, disable it just for that module.

[jcarrano]

Why is it necessary to put it in RAM in cortex-m?

[astralien3000]

Hello, I don't remember exactly why, but it was something about the main cortexm linker script. Basically, I have tested the code on native and cortexm(3?) targets. Native target puts everything in RAM, so that it is not needed to specify it, while it would make sense to do so for microcontrollers in general, where we need to choose between RAM, EEPROM, Flash.

[kaspar030]

In my view, with full XFA support, the ideal solution will be for the shell commands to be defined by the module to which they relate (eg. the "can" command would be defined in the "can" module, not in "shell").

+100

I'd rather work to finally get XFA merged.

Much of the work needed here (seems like per-platform linker script changes?) would be the same for XFA, but generally usable.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.