Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys/shell_commands: convert to SHELL_COMMAND() #18152

Merged
merged 5 commits into from
Jun 8, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented May 31, 2022

Contribution description

This drops the manually managed "shell command registry" and uses the XFA based SHELL_COMMAND() mechanism for registration instead.

Open ToDos

Adding a static in front of the handler function should now be possible in most cases, except when the command handler is actually used externally. I'd rather do this in a follow up and get his in as quickly and smoothly as possible to avoid merge conflicts.

Testing procedure

Except for alphabetical ordering, the shell command should still all registered as before.

Issues/PRs references

Same as #16095 but already moved the SHELL_COMMAND() registration next to the implementation.

@github-actions github-actions bot added Area: BLE Area: Bluetooth Low Energy support Area: examples Area: Example Applications Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework labels May 31, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 31, 2022
@github-actions github-actions bot added the Area: doc Area: Documentation label Jun 1, 2022
@maribu
Copy link
Member Author

maribu commented Jun 1, 2022

If you wonder why I dropped the abuse of the ifconfig shell command being called from C without ever running the shell from examples/nanocoap_server: Link time garbage collection doesn't work with XFA and all the shell command registration as well as their implementation are now linked in. I just copy-pasted a few lines from examples/gnrc_minimal to print the obtained addresses instead, which should be even small than it was in master.

Also, it is IMO super confusing to see shell command output without the shell. I assumed the application to be crashed when I was unable to invoke shell commands via serial until I realized that no shell is actually running.

@maribu
Copy link
Member Author

maribu commented Jun 1, 2022

How did this ever work?

#ifdef MODULE_OPENWSN
extern int _openwsn_ifconfig(int argc, char **argv);
extern int _openwsn_handler(int argc, char **argv);
#endif

int _openwsn_ifconfig(char *arg)
{
(void)arg;

@maribu maribu force-pushed the sys/shell_commands branch from e3149fa to f4630a4 Compare June 1, 2022 13:57
@fjmolinas
Copy link
Contributor

How did this ever work?

#ifdef MODULE_OPENWSN
extern int _openwsn_ifconfig(int argc, char **argv);
extern int _openwsn_handler(int argc, char **argv);
#endif

int _openwsn_ifconfig(char *arg)
{
(void)arg;

No idea 😕, sorry about that

@miri64
Copy link
Member

miri64 commented Jun 1, 2022

How did this ever work?

image

@maribu maribu force-pushed the sys/shell_commands branch from f4630a4 to 818ac85 Compare June 2, 2022 11:08
@github-actions github-actions bot added the Area: network Area: Networking label Jun 2, 2022
@maribu maribu added State: waiting for other PR State: The PR requires another PR to be merged first and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 2, 2022
@maribu maribu force-pushed the sys/shell_commands branch from 818ac85 to 6db5836 Compare June 7, 2022 06:33
@github-actions github-actions bot removed the Area: network Area: Networking label Jun 7, 2022
@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jun 7, 2022
@maribu maribu force-pushed the sys/shell_commands branch from 6db5836 to 97f2033 Compare June 7, 2022 07:26
@maribu
Copy link
Member Author

maribu commented Jun 7, 2022

rebased to solve merge conflict

@maribu
Copy link
Member Author

maribu commented Jun 7, 2022

All green now 🎉

@maribu maribu requested review from benpicco and fjmolinas June 7, 2022 20:19
@maribu maribu merged commit 20d3304 into RIOT-OS:master Jun 8, 2022
@maribu maribu deleted the sys/shell_commands branch June 8, 2022 04:40
@maribu
Copy link
Member Author

maribu commented Jun 8, 2022

Thanks for the review!

@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
ecourtois added a commit to cogip/mcu-firmware that referenced this pull request Jan 2, 2023
To fix warnings breaking compilation using GCC 12.

Remove `#include "shell_comands.h"` as in RIOT-OS/RIOT#18152

Signed-off-by: Eric Courtois <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BLE Area: Bluetooth Low Energy support Area: doc Area: Documentation Area: examples Area: Example Applications Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants