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

shell: Refactor command execution to enable raw arguments #24329

Merged
merged 2 commits into from
May 8, 2020

Conversation

nordic-krch
Copy link
Contributor

Added special flag that can be used to indicate that optional arguments are passed without any parsing (e.g. quotation marks removal). Modified execute command to parse command line buffer argument by argument.

After this change it is possible to forward whole command to command handler (using select).

It is now possible to specify command like:

SHELL_CMD_ARG_REGISTER(python, NULL, "Python", cmd_python, 1, SHELL_OPT_ARG_RAW);

Then call

select python

and whole content from the prompt will be passed directly to the handler without splitting into arguments, removing quotation marks, etc.

Fixes #24216.
Fixes #23907.

Signed-off-by: Krzysztof Chruscinski krzysztof.chruscinski@nordicsemi.no

@nordic-krch nordic-krch added the area: Shell Shell subsystem label Apr 14, 2020
@nordic-krch nordic-krch requested review from Vudentz and jakub-uC April 14, 2020 07:59
@nordic-krch nordic-krch requested a review from nashif as a code owner April 14, 2020 07:59
@zephyrbot zephyrbot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test labels Apr 14, 2020
@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 14, 2020

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@@ -12,7 +12,7 @@ extern const struct shell_cmd_entry __shell_root_cmds_end[0];

static inline const struct shell_cmd_entry *shell_root_cmd_get(u32_t id)
{
return &__shell_root_cmds_start[id];
return &__shell_root_cmds_start[id];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can benefit from this extra space in the future ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will remove it. For now. This PR already have enough of improvements :)

@nordic-krch nordic-krch force-pushed the shell_raw_args branch 2 times, most recently from a565d2a to fe18b04 Compare April 14, 2020 10:15
@nordic-krch
Copy link
Contributor Author

@stig-bjorlykke this should work with AT commands. once AT command is selected, it will pass whole command content to the handler.

You have to define at command like the one in PR description:

SHELL_CMD_ARG_REGISTER(at, NULL, "AT", cmd_at, 1, SHELL_OPT_ARG_RAW);

* that only mandatory arguments shall be parsed and remaining command string is
* passed as a raw string.
*/
#define SHELL_OPT_ARG_RAW (0xFE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that information about SHELL_OPT_ARG_RAW and SHELL_ARG_MAX values shall be added in KCONFIG help for SHELL_ARGC_MAX. Or maybe we could limit SHELL_ARGC_MAX in KCONFIG ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think there is no relation between those 2. This is limit for optional arguments, there are also mandatory arguments so in theory SHELL_ARGC_MAX could be more than 255, even more than 510. Of course, this is rather unrealistic values so i don't want to put any relation there.

#define SHELL_OPT_ARG_RAW (0xFE)

/** @brief Flag indicating maximum number of arguments. */
#define SHELL_ARG_MAX (0xFD)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is quite similar to SHELL_ARGC_MAX in Kconfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to SHELL_OPT_ARG_MAX.

@nordic-krch nordic-krch force-pushed the shell_raw_args branch 2 times, most recently from 1cf3158 to 2bbe34e Compare April 27, 2020 12:51
@nordic-krch
Copy link
Contributor Author

@jakub-uC ping. Merge window is closing soon.

@nordic-krch
Copy link
Contributor Author

@Vudentz @nashif will anyone take a look? I am aware that there are not many experts in shell guts still I would like to get that in for 2.3 if possible.

@carlescufi
Copy link
Member

@Vudentz and @nashif any chance to get additional review on this one?

@carlescufi carlescufi added this to the v2.3.0 milestone May 6, 2020
@carlescufi
Copy link
Member

@nordic-krch please rebase

Added special flag that can be used to indicate that optional
arguments are passed without any parsing (e.g. quotation marks
removal). Modified execute command to parse command line buffer
argument by argument.

After this change it is possible to forward whole command to
command handler (using select).

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
Added test for commands with SHELL_OPT_ARG_RAW flag set.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@galak galak merged commit 2e6e818 into zephyrproject-rtos:master May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Shell Shell subsystem area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shell: Allow selecting command without subcommands Shell overdo argument parsing in some cases
5 participants