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: Remove use of ctx_shell and improve shell prints #70945

Open
Thalley opened this issue Apr 1, 2024 · 0 comments
Open

Bluetooth: Shell: Remove use of ctx_shell and improve shell prints #70945

Thalley opened this issue Apr 1, 2024 · 0 comments
Labels
area: Bluetooth area: Shell Shell subsystem Enhancement Changes/Updates/Additions to existing features

Comments

@Thalley
Copy link
Collaborator

Thalley commented Apr 1, 2024

Is your enhancement proposal related to a problem? Please describe.
The ctx_shell in the Bluetooth shell is a global pointer to some shell instance. This is generally a poor design. The design have the following (and probably additional) issues:

  1. A global pointer that is directly accessed by multiple threads without any locking is generally a bad design choice
  2. Access to the pointer from e.g. callback functions need to be guarded to avoid NULL pointer access if it has not yet been assigned
  3. The shell module is not designed to be a singleton, but is reduce to that via this design
  4. The shell is assigned in a few specific commands, and thus the shell pointer may be updated while in the middle of a different command
  5. Shell prints in callbacks have a significant effect on processing time as they are printed immediately similar to CONFIG_LOG_MODE_IMMEDIATE, nor do they have any timestamps like the logging system. These things may mess up the order of what is being logged by the stack, and what is printed by the shell.
  6. Even if the shell is using a different USB endpoint than audio, the shell print functions cannot be used in USB audio callbacks without messing up the audio

Describe the solution you'd like
The ctx_shell needs to be removed completely. When a command is issued in a shell instance, whatever callbacks there may be called should appear in that shell, e.g. if you start scan on shell A then the scan reports shall be printed on shell A, while other commands could be done on other shell instances in the meanwhile.

Additionally shell prints (or at least the ones in callbacks) should ideally be handled as background tasks (asynchronous printing) where they can be properly interleaved with the logging system and be timestamped similarly.

Describe alternatives you've considered
While still removing the ctx_shell, we could alternatively print to all shell instances in the callbacks by introducing something like a shell_wall_print that does not require a specific pointer, but rather prints to all shell instances.

Additional context
Additional points and discussing can be found in #69736. In this PR an attempt to replace the shell prints in callbacks with the logging system was suggested, but the logging system and the shell may be on different transports, so that is not ideal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth area: Shell Shell subsystem Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

No branches or pull requests

1 participant