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

[ndrs-pst]

This commit aims to reduce the Bluetooth shell code footprint with the following changes:

• Introduced bt_shell_private.c and bt_shell_private.h to provide common functions for the Bluetooth shell.
• For now, equivalent function to shell_info, shell_print, shell_warn and shell_error, but without sh as Bluetooth shell relies on ctx_shell when there is no sh from outside.

The cost of newly added bt_shell_info_impl ... bt_shell_error_impl will be insignificant if we have many individual calls that need to pass both sh and color parameter each time.

[Thalley]

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]

@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

[ndrs-pst]

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 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.

Not sure yet

[ndrs-pst]

@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]

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. 👀

[github-actions]

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.