Shell: Remove unnecessary new lines at startup

[theob-pro]

When the shell start, it will print two lines. The reason for that behavior is not documented. Remove that to make the code simpler and shorter one line at a time.

[henrikbrixandersen]

When the shell start, it will print two lines. The reason for that behavior is not documented. Remove that to make the code simpler and shorter one line at a time.

What makes you think those newlines are unnecessary? The code was added for a reason.

[theob-pro]

When the shell start, it will print two lines. The reason for that behavior is not documented. Remove that to make the code simpler and shorter one line at a time.

What makes you think those newlines are unnecessary? The code was added for a reason.

The fact that there is no documentation nor any tests. I checked the PR that introduced the behavior (#9362) and did not find any arguments.

If you can think of how it could be useful, please let me know. And if possible, add a test to demonstrate the usefulness of it or at least some documentation.

[jakub-uC]

Thank you for your feedback.
The reason for adding these new lines at the start of the shell is to ensure that the prompt begins on a clean line. This is particularly useful in situations where the program may have experienced a reset or if it has hung and subsequently restarted. By starting the prompt on a new line, it visually separates the new shell session from any previous output on the terminal, making it much clearer for the user to see where the new session begins.

I understand the importance of tests and documentation, but in this case, the functionality is straightforward and doesn’t require them. The code’s purpose is clear and adding tests or detailed documentation might not add much value.

However, I do see the benefit of making the intent clearer, so I'm happy to add a comment above the code to explain its purpose. This should help anyone reading the code in the future.

If you have further concerns or suggestions, I’m open to discussing them, but I believe a concise comment should suffice.

[theob-pro]

Hi @jakub-uC, thanks for taking the time to explain things to me, it's much appreciated!

That said, the way it is implemented looks a bit hacky to me.

You said that this functionality is straightforward but is it? At first, I did not understand why there was a condition on the length of the prompt.

From what I saw elsewhere when playing with other shells and terminal programs, the usual way would rather be to move the cursor position for this kind of thing.

So, I investigated a bit more, and I now understand why there is that if condition, but it's definitely not straightforward:

The function state_set in shell_start calls z_shell_print_prompt_and_cmd which calls z_shell_op_cursor_position_synchronize. In that function there is the following code:

zephyr/subsys/shell/shell_ops.c

Lines 75 to 80 in 6b6c433

	/* In case cursor reaches the bottom line of a terminal, it will
	* be moved to the next line.
	*/
	if (full_line_cmd(sh)) {
	z_cursor_next_line_move(sh);
	}

Seems to make sense: the line is full, we move the cursor to the next line. Now let's look at the documentation of full_line_cmd:

Function returns true if command length is equal to multiplicity of terminal width.

Seems odd, the docstring says that the function does a bit more than the comment at the call-site. Let's take a look at the implementation:

zephyr/subsys/shell/shell_ops.c

Lines 41 to 48 in 6b6c433

	/* Function returns true if command length is equal to multiplicity of terminal
	* width.
	*/
	static inline bool full_line_cmd(const struct shell *sh)
	{
	return ((sh->ctx->cmd_buff_len + z_shell_strlen(sh->ctx->prompt))
	% sh->ctx->vt100_ctx.cons.terminal_wid == 0U);
	}

Indeed, the function does more than what the comment in z_shell_op_cursor_position_synchronize states: If the line is empty, the function will return true and the cursor will be moved to a new line.

We now understand why there is that condition on the prompt size in shell_start. It's because when there is no prompt, the shell will have the correct behavior and will put the cursor on a new line. But that behavior is not documented at all and kind of a side effect according to the comment in z_shell_op_cursor_position_synchronize.

My point writing that is to highlight how something that may seem trivial isn't. I don't mind having two lines when I start my shell. But I definitely care about the code complexity and readability. And having code like that is not helpful for future contributors.

Here are my suggestions:

1. Apply my patch, as I demonstrated it, those two lines shouldn't be here
2. If you both agree (@henrikbrixandersen @jakub-uC), I'll make a follow-up PR that will remove the side effect of full_line_cmd and then add a call to z_cursor_next_line_move in shell_start

[jakub-uC]

@theob-pro
Please take a look at the example:

If you remove these two new lines, the result after the reset will be as shown in the picture, and it will have the following consequences until the command is executed (press Enter):

The prompt will be printed on the same line, in the same place where it was before the reset. This does not look neat.
The shell's logic will have an incorrect understanding of the cursor's position, which will negatively affect the correct handling of arrow keys for cursor movement (e.g., when navigating up/down during command editing).
The code you are analyzing further assumes that the cursor started at position x == 0 and it is not always true with your patch.

After the command: kernel reboot cold, the current code is executed. (Code includes \n\n)

In the place marked in the screenshot System Reset, I uploaded a new program with your patch.

[theob-pro]

@jakub-uC, I understand your concern about the reset, and I agree with you. My point here is that the way it's done is not correct and hard to understand.

Was it clear to you why the there was that if condition if the prompt is empty? Does that mean that we don't mind writing on the same line after a reset if we don't have a prompt?

What I am proposing is fixing full_line_cmd and using z_cursor_next_line_move in shell_start. Does that make sense to you?

EDIT: I pushed my proposed change here so it's maybe easier to understand what I am proposing.

[jakub-uC]

I'm not entirely sure if I fully understand how you want to modify the full_line_cmd function. If it's not too much trouble, could I kindly ask you to add a commit to this PR that modifies this function?

Replacing z_shell_raw_fprintf(sh->fprintf_ctx, "\n\n"); with two calls z_cursor_next_line_move is also fine with me. Calling the function twice doesn't add much overhead, especially since this code isn't executed very often. Typically after a reboot or shell suspension-resumption. This will definitely improve the readability of the code.

There is indeed an inconsistency in the comments for the full_line_cmd function. It should return true if the cursor needs to be on a new line, which is when the prompt plus the printed command take up the entire width of the terminal.

Thank you for your patience with the explanations, but each of us have a different context in mind, which leads to misunderstandings. Adding a commit with the proposed change would greatly improve communication.

[jakub-uC]

Ahh you did it already! :)
It looks great! Could you please add 2 new lines instead of 1? I should mention that this requirement was established at Nordic when I was still working there. ;)

[theob-pro]

It looks great! Could you please add 2 new lines instead of 1? I should mention that this requirement was established at Nordic when I was still working there. ;)

Could you please provide more information? Is there a specific reason for that?

As I understand the reason for adding a new line at the shell startup, it's hard for me to find a good reason to add a second one. To be honest, if this is a personal preference I would prefer to tell users that they can configure the prompt however they want using one of the CONFIG_SHELL_PROMPT_* Kconfig symbols and that they can add new line there.

[jakub-uC]

I won't insist on making the change. The reasoning at the time was that with 2 lines, you can see the device reset, and it's possible to distinguish it from the regular execution of the command.

[theob-pro]

Rebased to fix failing test.