sys/shell: add XFA support

[kaspar030]

Contribution description

This PR adds a macro that allows adding shell commands using a cross file array.

It basically allows this to be put in any file:

#include "shell.h"
static int _my_command(int argc, char **argv) {
 // ...
}

SHELL_COMMAND(my_command, "my command help text", _my_command);

Testing procedure

This PR extends the tests/shell test scripts. Running this on some more platforms (AVR, RISC-V, ...) would be helpful!
Otherwise, please give feedback on the integration / usage.

Issues/PRs references

First user of XFA.

[kaspar030]

I merged the IS_USED suggestions (they're in a fixup now).

[kaspar030]

Somehow the IS_USED make this a bit messy, as the top-level definition is either always there or not available for within the IS_USED blocks. @miri64 do you mind if I revert that?

[miri64]

Somehow the IS_USED make this a bit messy, as the top-level definition is either always there or not available for within the IS_USED blocks. @miri64 do you mind if I revert that?

I am not sure what you mean by "top-level definition" or how IS_USED() makes things more messy than using defined() or #ifdef.

[miri64]

Do you mean this?

[miri64]

Alternatively, this would also work, if that is less messy in your opinion but still be within the coding conventions.

[kaspar030]

I am not sure what you mean by "top-level definition" or how IS_USED() makes things more messy than using defined() or #ifdef.

I mean this:

 43 #if IS_USED(MODULE_SHELL_COMMAND_XFA)                                                                                          
 44 /* define shell command cross file array */                                                                                     45 XFA_INIT_CONST(shell_command_t*, shell_commands_xfa);                                                                          
 46 #endif                                                                                                                         

I am not sure what you mean by "top-level definition" or how IS_USED() makes things more messy than using defined() or #ifdef.

I had used C if instead of preprocessor if whithin a function, which failed because it was missing a file-level definition (which is also behind preprocessor #if).

LGTM from my side now. I am a bit wary about an optional feature extension (shell_commands_xfa) being added to the main test for the base feature (shell), but it is not a blocking thing for me. What do @HendrikVE or @fjmolinas (as the people who last worked on that test) think about this?

[miri64]

I am a bit wary about an optional feature extension (shell_commands_xfa) being added to the main test for the base feature (shell), but it is not a blocking thing for me. What do @HendrikVE or @fjmolinas (as the people who last worked on that test) think about this?

To make things perfectly clear, this is just about moving the tests introduced here to their own application.

[kaspar030]

To make things perfectly clear, this is just about moving the tests introduced here to their own application.

"just"

How about we make this un-optional again. This is supposed to be a huge improvement in how we organize code. We don't expect any problems. I tagged this "experimental" because XFA is kinda new and the API might change. I don't want to dance around this by IS_USED and a seperate test application.

@miri64 do you see something wrong with the general approach?

If not, again, I'd rather have this non-optional and get maximum exposure (fail fast), so we quickly see if the approach is viable.

[miri64]

How about we make this un-optional again. This is supposed to be a huge improvement in how we organize code. We don't expect any problems. I tagged this "experimental" because XFA is kinda new and the API might change. I don't want to dance around this by IS_USED and a seperate test application.

Again, I don't mind it really to be in there. It just is my inner alarm bells ringing adding an (optional) experimental feature to what seems to me like a very central test (after all the shell is very important to our testing).

@miri64 do you see something wrong with the general approach?

Nope.

If not, again, I'd rather have this non-optional and get maximum exposure (fail fast), so we quickly see if the approach is viable.

Again, IMHO this can be merged as is, I just want some more input by people who actually worked with the shell more in-depth. If there is no problem from there side or no input, this can go in ASAP.

[kaspar030]

again, I'd rather have this non-optional and get maximum exposure (fail fast), so we quickly see if the approach is viable.

Let's not hide this but expose it to the max, so we can have confidence at the end of the release cycle that the approach is viable.

[chrysn]

While just familiarizing myself with XFA I support this being nonoptional. Fewer pseudomodules, less ifdef, and if XFA does as I remember it being advertised, as long as nothing hooks in it's a no-op (as in absent, not as in the ASM instruction) when built.

[kaspar030]

and if XFA does as I remember it being advertised, as long as nothing hooks in it's a no-op (as in absent, not as in the ASM instruction) when built.

Hm, did we advertize that? I think the loops iterating XFAs will not optimize out, as the array lengths are determined at runtime.

[miri64]

Fewer pseudomodules, less ifdef, and if XFA does as I remember it being advertised, as long as nothing hooks in it's a no-op (as in absent, not as in the ASM instruction) when built.

Mh... but doesn't this make it problematic when a user expects it to be there, but it isn't? With a pseudomodule we have at least a chance to warn the user about missing functionality.

[kaspar030]

I realized I didn't make the shell commands defined by SHELL_COMMANDS() "const", so they ended up in RAM. Latest commit fixes that.

[kaspar030]

• squashed

@fjmolinas would you give this another test run? I added a "const" in the macro which should get some testing...

[fjmolinas]

* squashed

@fjmolinas would you give this another test run? I added a "const" in the macro which should get some testing...

All good, I had a failure on nucleo-l433rc but that failures is in master as well.

[miri64]

Only a minor comment left. Please squash.

[miri64]

ACK, this PR was previously tested by @fjmolinas and retested after the latest rework. If Murdock is happy, this can go in.

[kaspar030]

Thanks everyone!

[chrysn]

Just for better understanding this: Why are the pointers to the command structs kept in XFA, and nit the command structs right away? Seems to me like a useless indirection, but maybe I missed the reason in the discussion.

[kaspar030]

Just for better understanding this: Why are the pointers to the command structs kept in XFA, and nit the command structs right away? Seems to me like a useless indirection, but maybe I missed the reason in the discussion.

hm, I didn't think much about this. I think both are fine. keeping just pointers in XFA makes the size of the shell command thingie show up together with the .data of the module defining it. And, the xfa itself has to be volatile, whereas the data pointed to doesn't.

Would changing this make anything easier?

[chrysn]

hm, I didn't think much about this. I think both are fine.

Yeah, it's a small detail; my main concern was for the increased memory use and indirection in iteration, but that's, like, 1 pointer per command and 1 deref instruction.

keeping just pointers in XFA makes the size of the shell command thingie show up together with the .data of the module defining it.

I think it still would if the full shell command struct were in XFA. (nm & co count symbol sizes, and each entry in the XFA is an own symbol with inside some object file with its proper source attribution).

And, the xfa itself has to be volatile, whereas the data pointed to doesn't.

OK, the volatility makes sense because it's part of the type. (It's different in Rust so I didn't notice).

Would changing this make anything easier?

No, it would just shave, like, 3 lines off the wrappers.