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

bluetooth: shell: refactor shell print for Bluetooth-specific context #74652

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ndrs-pst
Copy link
Contributor

@ndrs-pst ndrs-pst commented Jun 21, 2024

This PR aims to eliminate the dependency on ctx_shell in the Bluetooth host/shell/*, making the code more maintainable.

  • Introduced bt_shell_private.c and bt_shell_private.h to provide common functions for the Bluetooth shell.
  • Limit the usage of ctx_shell to cases where printing requires it and sh is not available.
    (Interim commit before switch to bt_shell_*)
  • Refactor shell print to eliminate ctx_shell usage in the Bluetooth host/shell/*.

Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

I don't think this is the right direction (but please do convince me if you believe so).

Some of the changes are welcome, but the idea of a bt_shell is IMO not.

In your commit message you mention that you want to remove the code footprint, but you also mention an increase cost of this new bt_shell. Aren't those two sentences conflicting?

The BT shell definitely needs improvement. I created a issue #70945 related to this, that requests the removed of the ctx_shell. The removal of the ctx_shell would directly conflict with the majority of this PR, as all the bt_shell functions you've defined will simply call the shell functions directly (as they would need the shell pointer again).

Could you provide some data on the codesize that you've increase/decreased?

@ndrs-pst
Copy link
Contributor Author

@Thalley Thank you for your feedback. It's good to hear that you're addressing ctx_shell. 😃

Aren't those two sentences conflicting?

At first glance, this might seem controversial.
Currently, many shell print require passing parameters like sh and color repeatedly.
By centralizing these interactions within specific bt_shell functions, we streamline the code.

Could you provide some data on the codesize that you've increase/decreased?

By picking up periodic_adv_conn sample with nrf52840dk boards and adding CONFIG_BT_SHELL=y
the footprint of shell\* which get from rom_report shown that

MODULE FLASH SIZE (BEFORE) FLASH SIZE (AFTER) INCREASE/DECREASE
bluetooth\shell\* 15,795 B 14,963 B -832 B
bt.c 13,251 B 12,275 B -976 B
bt_shell_private.c 0 B 144 B +144 B

Here is the footprint from each wrapper
bt_shell_private

@ndrs-pst
Copy link
Contributor Author

To stretch further, I think the functions that should be improved are
shell_fprintf_impl(sh, color, fmt, ##__VA_ARGS__) itself to have a wrapper for the color parameter.

void __printf_like(2, 3) shell_fprintf_error_impl(const struct shell *sh, const char *fmt, ...);
void __printf_like(2, 3) shell_fprintf_info_impl(const struct shell *sh, const char *fmt, ...);
void __printf_like(2, 3) shell_fprintf_print_impl(const struct shell *sh, const char *fmt, ...);
void __printf_like(2, 3) shell_fprintf_warn_impl(const struct shell *sh, const char *fmt, ...);

Since in reality the same message printed out inside the shell is always fixed to a specific color, and if we prioritize the code size, this should make sense.

Otherwise, every time, for example in Cortex-M, r1 needs to be allocated to pass the color value (while r0 is for sh), which causes the caller code to save the previously used r1.

@Thalley
Copy link
Collaborator

Thalley commented Jun 28, 2024

Just tried your PR with the BT shell test application for the nrf5340dk_nrf5340_cpuapp board and got the following results:

With PR

Memory region         Used Size  Region Size  %age Used
           FLASH:      317356 B         1 MB     30.27%
             RAM:       52992 B       448 KB     11.55%
        IDT_LIST:          0 GB        32 KB      0.00%

Without PR

Memory region         Used Size  Region Size  %age Used
           FLASH:      318668 B         1 MB     30.39%
             RAM:       52992 B       448 KB     11.55%
        IDT_LIST:          0 GB        32 KB      0.00%

So while I do not agree with the direction of this PR, as we really need to get rid of ctx_shell, I can't disagree with the 1312 Bytes saved.

I'm not at a point where I'll approve, but I'm also not going to reject this change. I'd like to get input from the other BT collaborators on this.

@Thalley Thalley self-requested a review June 28, 2024 09:18
@Thalley Thalley dismissed their stale review June 28, 2024 09:18

Not sure yet

@ndrs-pst
Copy link
Contributor Author

@Thalley, thank you for the additional information. It seems we should start by getting rid of ctx_shell and then find a way to reduce the footprint.

@ndrs-pst ndrs-pst marked this pull request as draft June 28, 2024 09:32
@ndrs-pst
Copy link
Contributor Author

To stretch further, I think the functions that should be improved are shell_fprintf_impl(sh, color, fmt, ##__VA_ARGS__) itself to have a wrapper for the color parameter.

void __printf_like(2, 3) shell_fprintf_error_impl(const struct shell *sh, const char *fmt, ...);
void __printf_like(2, 3) shell_fprintf_info_impl(const struct shell *sh, const char *fmt, ...);
void __printf_like(2, 3) shell_fprintf_print_impl(const struct shell *sh, const char *fmt, ...);
void __printf_like(2, 3) shell_fprintf_warn_impl(const struct shell *sh, const char *fmt, ...);

Since in reality the same message printed out inside the shell is always fixed to a specific color, and if we prioritize the code size, this should make sense.

Otherwise, every time, for example in Cortex-M, r1 needs to be allocated to pass the color value (while r0 is for sh), which causes the caller code to save the previously used r1.

@Thalley Just a reminder that this stretch was merged in this PR: #75340. 👀

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@ndrs-pst
Copy link
Contributor Author

@Thalley I just added another commit that introduces CONFIG_BT_SHELL_BACKEND for Bluetooth shell output when the shell context is unavailable (e.g., in callbacks).

While exploring ways to eliminate ctx_shell, could we initialize it during startup and hide it from other shell commands?
Since ctx_shell is currently used in multiple places like audio/shell, mesh/shell, and services/ias/shell.

@Thalley
Copy link
Collaborator

Thalley commented Oct 14, 2024

While exploring ways to eliminate ctx_shell, could we initialize it during startup and hide it from other shell commands?

Question is, initialize it to what?
The challenge (and main issue) with the ctx_shell is that it's not a constant. It's just a variable assigned to some shell, of which there can be more 1 or more. That's one of the issue mentioned in #70945

@ndrs-pst
Copy link
Contributor Author

Thanks for the feedback! Please correct me if I'm wrong, the usage of BT_SHELL mostly resides with the SERIAL or RTT backend. Even though it's possible, I'm not sure about the use case for using MQTT or TELNET to remotely access BT_SHELL.


As for the previous question, I believe we can initialize it to backend_name, which is in CONFIG_BT_SHELL_BACKEND, by utilizing shell_backend_get_by_name("backend_name"). Even though we can have one or more shells, why can't we specify unsolicited output to just one specific backend?" 🤔


#include "bt_shell_private.h"

extern const struct shell *ctx_shell;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't bt_shell be more appropriate, since this is now basically an internal API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's also fine!
FWIW, I borrowed the name from #include "net_shell_private.h", so I was following that pattern. 😄

@Thalley
Copy link
Collaborator

Thalley commented Oct 16, 2024

As for the previous question, I believe we can initialize it to backend_name, which is in CONFIG_BT_SHELL_BACKEND, by utilizing shell_backend_get_by_name("backend_name"). Even though we can have one or more shells, why can't we specify unsolicited output to just one specific backend?"

I'm not an expert in the shells, but the current ctx_shell is not a backend, but just a reference to a struct shell. Multiple struct shells can use the same backend as far as I can tell, so not sure how you are expecting to use a specific backend.

If you have 2 or more shells that each may call the BT shell command handler, then currently the ctx_shell only refers to one at them at any one time and may chance between them whenever ctx_shell is reassigned. This is clearly not ideal.
Question is whether, and how, doing command X on shell S1 that trigger callback Y should send the string from the callback to just S1 or also S2 (another shell instance)?

@ndrs-pst
Copy link
Contributor Author

If you have 2 or more shells that each may call the BT shell command handler, then currently the ctx_shell only refers to one at them at any one time and may chance between them whenever ctx_shell is reassigned. This is clearly not ideal.

Yeah, that's why I'm bringing up the real use case. Even if we have two or more shells, do we really need to support BT shell responses for every situation? It feels like we're trying to cover cases that might not happen often. 😃

just S1 or also S2 (another shell instance)?

As you mentioned as an alternative in #70945. A kind of shell_wall_print may look like this


void bt_shell_wall_print_impl(const char *fmt, ...)
{
	va_list args;
	int count;
	const struct shell *sh;

	va_start(args, fmt);
	count = shell_backend_count_get();
	for (int i = 0; i < count; i++) {
	    va_list args_copy;

	    va_copy(args_copy, args);   /* Create a copy of 'args' to safely reuse */
	    sh = shell_backend_get(i);
	    shell_vfprintf(sh, SHELL_NORMAL, fmt, args_copy);

	    va_end(args_copy);          /* Clean up to prevent resource leaks */
	}
	va_end(args);
}

I'm OK if that's the way we can move forward.
Since the priority of this PR is to reduce the footprint and get closer to getting rid of ctx_shell.

WDYT?

@Thalley
Copy link
Collaborator

Thalley commented Oct 17, 2024

If you have 2 or more shells that each may call the BT shell command handler, then currently the ctx_shell only refers to one at them at any one time and may chance between them whenever ctx_shell is reassigned. This is clearly not ideal.

Yeah, that's why I'm bringing up the real use case. Even if we have two or more shells, do we really need to support BT shell responses for every situation? It feels like we're trying to cover cases that might not happen often. 😃

It's hard to tell how people use these APIs, but the API does allow for such behavior, and thus any solution to our "problems" should consider what's possible, even if it will only happen with madmen :D

As you mentioned as an alternative in #70945. A kind of shell_wall_print may look like this

void bt_shell_wall_print_impl(const char *fmt, ...)
{
	va_list args;
	int count;
	const struct shell *sh;

	va_start(args, fmt);
	count = shell_backend_count_get();
	for (int i = 0; i < count; i++) {
	    va_list args_copy;

	    va_copy(args_copy, args);   /* Create a copy of 'args' to safely reuse */
	    sh = shell_backend_get(i);
	    shell_vfprintf(sh, SHELL_NORMAL, fmt, args_copy);

	    va_end(args_copy);          /* Clean up to prevent resource leaks */
	}
	va_end(args);
}

I'm OK if that's the way we can move forward. Since the priority of this PR is to reduce the footprint and get closer to getting rid of ctx_shell.

WDYT?

I think the wall implementation looks fine :) @alwa-nordic was the one who originally suggested that idea, so maybe he has some input here too

@alwa-nordic
Copy link
Collaborator

The wall implementation looks good to me. I'm a bit confused about why it's called a shell "back end", but no matter.

BTW, I think in the future, we should make the shell commands more like CLI programs on a TTY, where the program exits before the shell returns to a prompt. For that, we could block and wait in command functions. For that duration, we have a tread and its stack as working memory.

@ndrs-pst
Copy link
Contributor Author

ndrs-pst commented Nov 9, 2024

@alwa-nordic Thanks for the comment.
I'll address host/shell first and leave the others, e.g. mesh/shell and audio/shell, as they are.

Introduced `bt_shell_private.c` and `bt_shell_private.h` to provide
common functions for the Bluetooth `shell_wall_print`.
These functions are equivalent to `shell_fprintf`, `shell_info`,
`shell_print`, `shell_warn`, `shell_error`, `shell_hexdump` and
`shell_help` but without requiring the `sh` parameter.

The cost of the newly added `bt_shell_fprintf_info` ... `_error` functions
will be negligible when there are many individual calls that need to pass
both the `sh` and `color` parameters each time.

Signed-off-by: Pisit Sawangvonganan <[email protected]>
Limit the usage of `ctx_shell` to cases where printing requires it
and `sh` is not available.

Signed-off-by: Pisit Sawangvonganan <[email protected]>
This change aims to eliminate the dependency on `ctx_shell` in
the Bluetooth `host/shell/*`, making the code more maintainable.
Replaced `shell_*` functions that depended on `ctx_shell` with
the appropriate `bt_shell_*` functions.

The special handling was added to `cmd_scan_off` and `cmd_connect_le`
because these commands both used the `sh` context and previously
depended on `ctx_shell`.

Signed-off-by: Pisit Sawangvonganan <[email protected]>
@ndrs-pst ndrs-pst marked this pull request as ready for review November 11, 2024 17:32
@zephyrbot zephyrbot added the area: Bluetooth ISO Bluetooth LE Isochronous Channels label Nov 11, 2024
@ndrs-pst
Copy link
Contributor Author

Here we go! Finally, this PR is re-opened for review. 😃
Ping @Thalley, @jhedberg, and @alwa-nordic.

Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

Looks good. Since BT is pretty much the only shell to do this, I think it makes sense to keep it in BT. One comment/question

Comment on lines +33 to +55
void bt_shell_hexdump(const uint8_t *data, size_t len)
{
int count;
const struct shell *sh;

count = shell_backend_count_get();
for (int i = 0; i < count; i++) {
sh = shell_backend_get(i);
shell_hexdump(sh, data, len);
}
}

void bt_shell_help(void)
{
int count;
const struct shell *sh;

count = shell_backend_count_get();
for (int i = 0; i < count; i++) {
sh = shell_backend_get(i);
shell_help(sh);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these 2 prints on all shells, should they have wall in the name too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the intention was to replace the shell_* functions that depended on ctx_shell with the appropriate bt_shell_* functions (in-short just adding bt_ prefix) so I think it's no need to have wall in the name. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems to be inconsistent with bt_shell_wall_vfprintf which servers the same purpose, but has wall in the name, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shell (variant) bt_shell (variant) (before) bt_shell (variant) (opt 1) bt_shell (variant) (opt 2)
shell_fprintf bt_shell_fprintf bt_shell_fprintf bt_shell_wall_fprintf
shell_fprintf_info bt_shell_fprintf_info bt_shell_fprintf_info bt_shell_wall_fprintf_info
shell_fprintf_print bt_shell_fprintf_print bt_shell_fprintf_print bt_shell_wall_fprintf_print
shell_fprintf_warn bt_shell_fprintf_warn bt_shell_fprintf_warn bt_shell_wall_fprintf_warn
shell_fprintf_error bt_shell_fprintf_error bt_shell_fprintf_error bt_shell_wall_fprintf_error
shell_hexdump bt_shell_hexdump bt_shell_wall_hexdump bt_shell_wall_hexdump
shell_help bt_shell_help bt_shell_wall_help bt_shell_wall_help
shell_info bt_shell_info bt_shell_info bt_shell_wall_info
shell_print bt_shell_print bt_shell_print bt_shell_wall_print
shell_warn bt_shell_warn bt_shell_warn bt_shell_wall_warn
shell_error bt_shell_error bt_shell_error bt_shell_wall_error

Just bring out the table of print functions provided by bt_shell_private.h.
Which option are we going to choose: opt1, opt2, or something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since they are all wall in the end, then either the current, or opt 2.

The main issue I had is that we had a single function using the wall name which seemed inconsistent as they are all wall variants.

Of course since bt_shell_wall_vfprintf was an internal/static function not exposed by the header, it is less of any issue.

So I'm OK with the current version or opt 2

@Thalley
Copy link
Collaborator

Thalley commented Nov 14, 2024

Found one other place where "ctx_shell" was used outside of BT, but that was easy to just remove: #81395

Comment on lines +33 to +55
void bt_shell_hexdump(const uint8_t *data, size_t len)
{
int count;
const struct shell *sh;

count = shell_backend_count_get();
for (int i = 0; i < count; i++) {
sh = shell_backend_get(i);
shell_hexdump(sh, data, len);
}
}

void bt_shell_help(void)
{
int count;
const struct shell *sh;

count = shell_backend_count_get();
for (int i = 0; i < count; i++) {
sh = shell_backend_get(i);
shell_help(sh);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems to be inconsistent with bt_shell_wall_vfprintf which servers the same purpose, but has wall in the name, right?

Comment on lines +50 to +57
/**
* @brief Prints the current command help.
* (Bluetooth context specific)
*
* Function will print a help string with: the currently entered command
* and subcommands (if they exist).
*/
void bt_shell_help(void);
Copy link
Collaborator

Choose a reason for hiding this comment

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

bt_shell_help should only ever use the shell that tried to do something. I see that we have a case where cmd_connect_le is called from a callback.

In that case (where sh == NULL), I would just not print this at all :) It's pretty useless in a callback (and with wall), since it related to a specific command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, bt_shell_help() shouldn't be implemented, right? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say so, yeah. It's not useful to wall user errors :)

#include <zephyr/shell/shell_backend.h>
#include "bt_shell_private.h"

void bt_shell_wall_vfprintf(enum shell_vt100_color color, const char *fmt, va_list args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
void bt_shell_wall_vfprintf(enum shell_vt100_color color, const char *fmt, va_list args)
static void wall_vfprintf(enum shell_vt100_color color, const char *fmt, va_list args)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it will address on the next push


/* "connect" is what would be in argv[0] normally */
cmd_connect_le(ctx_shell, 1, (char *[]){"connect"});
cmd_connect_le(NULL, 1, (char *[]){"connect"});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's troublesome that we call these shell command handlers with a NULL shell.

I fully understand that without the ctx_shell, we do not have a shell to use in those cases, but I think that we may need to refactor these calls in a way that they do not require a shell then, i.e. move the functionality from cmd_connect_le in a new function that is shell-less, and then call that function both here and in cmd_connect_le.

Otherwise we end up in a situation where we cannot always assume that sh != NULL, and then we need to add sh == NULL checks everywhere.

This will also remove the need for the bt_shell_help function

Copy link
Contributor Author

@ndrs-pst ndrs-pst Nov 14, 2024

Choose a reason for hiding this comment

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

static int bt_do_connect_le(int *ercd, size_t argc, char *argv[])
{
	int err;
	bt_addr_le_t addr;
	struct bt_conn *conn;
	uint32_t options = 0;

	*ercd = 0;

	/* When no arguments are specified, connect to the last scanned device. */
	if (argc == 1) {
		if (auto_connect.addr_set) {
			bt_addr_le_copy(&addr, &auto_connect.addr);
		} else {
			return -ENOENT;
		}
	} else {
		err = bt_addr_le_from_str(argv[1], argv[2], &addr);
		if (err) {
			*ercd = err;
			return -EINVAL;
		}
	}

#if defined(CONFIG_BT_EXT_ADV)
	for (size_t argn = 3; argn < argc; argn++) {
		const char *arg = argv[argn];

		if (!strcmp(arg, "coded")) {
			options |= BT_CONN_LE_OPT_CODED;
		} else if (!strcmp(arg, "no-1m")) {
			options |= BT_CONN_LE_OPT_NO_1M;
		} else {
			return SHELL_CMD_HELP_PRINTED;
		}
	}
#endif /* defined(CONFIG_BT_EXT_ADV) */

	struct bt_conn_le_create_param *create_params =
		BT_CONN_LE_CREATE_PARAM(options,
					BT_GAP_SCAN_FAST_INTERVAL,
					BT_GAP_SCAN_FAST_INTERVAL);

	err = bt_conn_le_create(&addr, create_params, BT_LE_CONN_PARAM_DEFAULT,
				&conn);
	if (err) {
		*ercd = err;
		return -ENOEXEC;
	} else {
		/* unref connection obj in advance as app user */
		bt_conn_unref(conn);
	}

	return 0;
}
static int cmd_connect_le(const struct shell *sh, size_t argc, char *argv[])
{
	int err;
	int ercd;

	err = bt_do_connect_le(&ercd, argc, argv);
	switch (err) {
	case -ENOENT:
		shell_error(sh, "No connectable adv stored. Please trigger a scan first.");
		shell_help(sh);
		return SHELL_CMD_HELP_PRINTED;
	case -EINVAL:
		shell_error(sh, "Invalid peer address (err %d)", ercd);
		return ercd;
	case SHELL_CMD_HELP_PRINTED:
		shell_help(sh);
		return SHELL_CMD_HELP_PRINTED;
	case -ENOEXEC:
		shell_error(sh, "Connection failed (%d)", ercd);
		return -ENOEXEC;
	default:
		shell_print(sh, "Connection pending");
		return 0;
	}
}

WDYT? This brings out a shell-less function like bt_do_connect_le, and it would be great if we don't need to print in the callback. :)

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

Successfully merging this pull request may close these issues.

5 participants