bluetooth: shell: eliminate ctx_shell usage

[ndrs-pst]

This PR aims to eliminate the dependency on ctx_shell in the Bluetooth audio/shell/*, mesh/shell/*, and classic/shell/* directories, making the code more maintainable.
The shell_* functions that depended on ctx_shell have been replaced with the appropriate bt_shell_* functions.

[ndrs-pst]

@Thalley Hi, the follow-up PR after #74652 get merged. 🚀

[jhedberg]

I had no idea we had this much shell code. Especially on the audio side.

[PavelVPV]

Putting DNM to review mesh changes.

[PavelVPV]

If this API is being used not just by host, then I think it should be moved to common/ folder.

This zephyr_library_sources_ifndef part can be fixed by a hidden Kconfig option (added to common/Kconfig) which is selected by BT_MESH_SHELL or BT_SHELL Kconfig options. If I'm not missing anything, this will also solve the _ifndef part.

[ndrs-pst]

Thank you for the suggestion. Here's the updated approach:

common/Kconfig

config BT_PRIVATE_SHELL
    bool
    default y if BT_MESH_SHELL || BT_SHELL

common/CMakeList

zephyr_library_sources_ifdef(CONFIG_BT_PRIVATE_SHELL bt_shell_private.c)

Then move bt_shell_private.c and bt_shell_private.h into the common folder instead.

WDYT?

[PavelVPV]

Looks good. But I'd add select BT_PRIVATE_SHELL to BT_MESH_SHELL and BT_SHELL so it is unconditionally selected when one of these options is enabled. This also makes visible dependency of host and mesh shell on that API.

[Thalley]

What is the use case where BT_MESH_SHELL=y and BT_SHELL=n?

Shouldn't BT_MESH_SHELL depend on BT_SHELL, so that bt_shell_private is always compiled for BT_MESH_SHELL?

Do we need BT_MESH_SHELL at all, or should it just be similar to LE, LL and Classic and enable the mesh shell if BT_SHELL && BT_MESH?

[PavelVPV]

BT_MESH_SHELL is completely independent of BT_SHELL:

zephyr/subsys/bluetooth/mesh/shell/Kconfig

Lines 6 to 11 in ef6f804

	menuconfig BT_MESH_SHELL
	bool "Bluetooth Mesh shell"
	select SHELL
	help
	Activate shell module that provides Bluetooth Mesh commands to
	the console.

And currently they can't work together. Mesh must be initialized before settings are loaded. The the shell's bt init function doesn't do this.

[Thalley]

And currently they can't work together. Mesh must be initialized before settings are loaded. The the shell's bt init function doesn't do this.

That sounds fairly trivial to implement.

Would it make sense to get rid of BT_MESH_SHELL (not in this PR of course)?

[PavelVPV]

No, it is not trivial as it depends on composition data and provisioning context.

There is no such request from users/customers/PMT. Also, 2 shells seem to have different purpose. For mesh most of the code is embedded into an application and the application can enable the mesh shell only for a certain model. For example, you can enable shell commands for client models without changing code in your application by toggling few Kconfig options. The host's shell adds a lot of code which is not needed in most (if not all) mesh applications.

[ndrs-pst]

Hi @Thalley, according to @PavelVPV's response, can I move forward with the changes as per his suggestion?
Also, I need to resolve the merge conflict too. 😃

[Thalley]

Yeah, sure :)

[Thalley]

May I suggest an alternative here?

Suggested change

	config BT_PRIVATE_SHELL
	bool
	default n
	config BT_PRIVATE_SHELL
	def_bool BT_SHELL || BT_MESH_SHELL

select should be used sparingly: https://docs.zephyrproject.org/latest/build/kconfig/tips.html#select-pitfalls, and while this specific use is one of the "safe" uses of select, defaulting to the values that would otherwise select it is more flexible (works with ints too), and avoids any future issues with select, while achieving the same.

That being said, it could also be argued that BT_PRIVATE_SHELL is a bit overkill, and that

zephyr_library_sources_ifdef(CONFIG_BT_PRIVATE_SHELL bt_shell_private.c)

could just be modified to

if (CONFIG_BT_SHELL OR CONFIG_BT_MESH_SHELL)
	zephyr_library_sources(bt_shell_private.c)
endif()

If you want to keep it as is, then I my only suggestion would be to

Suggested change

	config BT_PRIVATE_SHELL
	bool
	default n
	config BT_PRIVATE_SHELL
	bool

as bool Kconfigs always default to false.

[ndrs-pst]

Thanks for your comment. May I also get a suggestion from @PavelVPV?
Additionally, I'm waiting for the CI to turn green first.

[PavelVPV]

If you want to keep Kconfig option, then I think it is exactly the case for select. It won't cause issues described in the doc as these functions don't bring new dependencies.

It can be a bit more clear to have a Kconfig option rather than including bt_shell_private.c by explicitly checking bt shell o rmesh shell Kconfig options state as it won't require CMakeLists.txt change if these functions need to be enabled in some other cases. But I leave it to you here.