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: Add a configuration for not printing shell errors #69002

Merged
merged 1 commit into from Feb 26, 2024

Conversation

Brianmm94
Copy link
Contributor

@Brianmm94 Brianmm94 commented Feb 14, 2024

The shell currently prints out : command not found and
Please specify a subcommand. when invalid commands are
sent.

This can cause issues in certain situations, such as if a U-Boot
console in interactive mode is on the other end of the shell's
UART, where the U-Boot console will stop booting the device
if it receives any characters, and it will print out strings that
the shell then sees as commands. This causes the shell to send
out the messages above, preventing the device running
U-Boot on the other end from booting up.

I added the configurations
CONFIG_SHELL_MSG_SPECIFY_SUBCOMMAND and
CONFIG_SHELL_MSG_CMD_NOT_FOUND to allow disabling
these messages.

Signed-off-by: Brian Moran brian.moran@sabantoag.com

@zephyrbot zephyrbot added the area: Shell Shell subsystem label Feb 14, 2024
Copy link

Hello @Brianmm94, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@jakub-uC
Copy link
Contributor

jakub-uC commented Feb 14, 2024

@Brianmm94 Depending on the context, might turning off shell echo address this issue?
I'm wary that the approach you've suggested might be overly drastic. If you could share more details about the specific scenarios where shell message output is a concern, perhaps we could identify a more refined solution. If possible, I would like to explore less invasive alternatives

@Brianmm94
Copy link
Contributor Author

@jakub-uC Hello, setting SHELL_ECHO_STATUS=n and CONFIG_SHELL_PROMPT_UART="" partially address the issue by no longer printing the echo and prompt, but the shell will still print out the SHELL_ERROR messages I mentioned above when it receives messages from U-Boot, such as Loading or >. If preferred, I could move the configuration checks to shell.c lines 531, 695, and 742 seeing as printing SHELL_MSG_CMD_NOT_FOUND and SHELL_MSG_SPECIFY_SUBCOMMAND are the messages that cause the issue. I could also create a separate configuration for each of them if you think it'd be beneficial.

@jakub-uC
Copy link
Contributor

@Brianmm94
In my opinion, your new suggestion is very good and aligns more closely with our current implementation, for example, the shell's CONFIG_SHELL_HELP printing functionality. I think it will be more coherent and potentially less disruptive.
Thank you for the alternative!

The shell currently prints out `: command not found` and
`Please specify a subcommand.` when invalid commands are
sent.

This can cause issues in certain situations, such as if a U-Boot
console in interactive mode is on the other end of the shell's
UART, where the U-Boot console will stop booting the device
if it receives any characters, and it will print out strings that
the shell then sees as commands. This causes the shell to send
out the messages above, preventing the device running
U-Boot on the other end from booting up.

I added the configurations
`CONFIG_SHELL_MSG_SPECIFY_SUBCOMMAND` and
`CONFIG_SHELL_MSG_CMD_NOT_FOUND` to allow disabling
these messages.

Signed-off-by: Brian Moran <brian.moran@sabantoag.com>
@Brianmm94
Copy link
Contributor Author

Brianmm94 commented Feb 14, 2024

Updated! I used the names CONFIG_SHELL_MSG_CMD_NOT_FOUND and CONFIG_SHELL_MSG_SPECIFY_SUBCOMMAND to match the existing SHELL_MSG_CMD_NOT_FOUND and SHELL_MSG_SPECIFY_SUBCOMMAND #defines.

Edit: I tested all 4 scenarios, as well as testing all 4 with CONFIG_SHELL_HELP enabled. I was debating removing CONFIG_SHELL_MSG_SPECIFY_SUBCOMMAND seeing as it doesn't always print when CONFIG_SHELL_HELP is enabled, but I noticed that it does still print it if -h or --help follows a non-command, or a command without a help string, so I figured it might be nice to keep it still (for example, a -h, a a -h and a a a -h would all print the CONFIG_SHELL_MSG_SPECIFY_SUBCOMMAND string still.

@jakub-uC
Copy link
Contributor

Hi @Brianmm94, I had another thought that I wanted to share with you. Would you mind checking if the CONFIG_SHELL_AUTOSTART=n config meets your needs for your use case? You can call shell_start after booting up. Just a suggestion, and I'd love to hear your thoughts on this!

@Brianmm94
Copy link
Contributor Author

Brianmm94 commented Feb 16, 2024

Hello @jakub-uC, that's actually what we started doing at first before this, but that solution only works if the U-Boot device on the other end just boots up once when the product is first powered on. If the U-Boot device reboots for any reason after that, then the issue still occurs. For example, in production we power off the U-Boot device to reduce power draw when it's not needed and we have APIs for rebooting it. We currently have no way of detecting that the U-Boot device is rebooting, and even if we could detect that accurately, calling shell_stop and then shell_start again anytime it reboots would still be a race condition between calling shell_stop and the shell printing out those messages to stop it that we'd like to avoid if possible.

@henrikbrixandersen
Copy link
Member

Just out of curiosity, why is the Zephyr console and the U-boot console connected together in the first place?

@Brianmm94
Copy link
Contributor Author

Hello @henrikbrixandersen, by default, our other device had U-Boot console on the serial port connected to this device when it was released before I started at the company. This wasn't an issue previously, but it is now that we're updating this device's firmware to use Zephyr. I believe that we may have another serial port that we can move the U-Boot console to in a future firmware update where it won't interact with Zephyr anymore, but that likely won't be for a while. Regardless, I still think these new configurations are beneficial to those who don't have another port to move U-Boot console to, or those who don't want anything printed unless a valid command is sent.

@fabiobaltieri fabiobaltieri merged commit b411094 into zephyrproject-rtos:main Feb 26, 2024
18 checks passed
Copy link

Hi @Brianmm94!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Shell Shell subsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants