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

logging: fixes and enhancements to initialization and shell cmds #84955

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ycsin
Copy link
Member

@ycsin ycsin commented Jan 31, 2025

logging: init backend id regardless of autostart

The id is basically a compile-time constant. Setting it every
time the backend is enabled is unnecessary. Instead, set it on
z_log_init() regardless of whether or not it requires to be
autostarted.

Fixes an issue where the filter_get/filter_set
accessed the wrong index and displayed the wrong log level when
user accesses the status of an uninitialized backend via:
log backend <uninitialized_backend> status.

Also fixes an issue when user tries to list the backends via:
log list_backends, where all uninitialized backends will have
ID = 0.

logging: log_cmds: init uninitialized backend on log_go()

For backends that do not autostart themselves, initialize
& enable them on log backend <log_backend_*> go, so
that they function properly.

Fixes #84952
Fixes #84954

Testing

Console log
Hello World! qemu_riscv64/qemu_virt_riscv64

*** Booting Zephyr OS build v4.0.0-3714-gb6f8eff2f322 ***
uart:~$ log list_backends 
log_backend_uart
        - Status: disabled
        - ID: 1

shell_uart_backend
        - Status: enabled
        - ID: 2

uart:~$ log backend shell_uart_backend status
module_name                              | current | built-in 
----------------------------------------------------------
cbprintf_package                         | inf     | inf
getopt                                   | inf     | inf
log                                      | inf     | inf
log_mgmt                                 | inf     | inf
log_uart                                 | inf     | inf
mpu                                      | inf     | inf
os                                       | inf     | inf
shell.shell_uart                         | inf     | inf
shell_uart                               | inf     | inf
uart_ns16550                             | inf     | inf
uart:~$ log backend log_backend_uart status
Logs are halted!
module_name                              | current | built-in 
----------------------------------------------------------
cbprintf_package                         | none    | inf
getopt                                   | none    | inf
log                                      | none    | inf
log_mgmt                                 | none    | inf
log_uart                                 | none    | inf
mpu                                      | none    | inf
os                                       | none    | inf
shell.shell_uart                         | none    | inf
shell_uart                               | none    | inf
uart_ns16550                             | none    | inf
uart:~$ log backend shell_uart_backend enable wrn
uart:~$ log backend shell_uart_backend status
module_name                              | current | built-in 
----------------------------------------------------------
cbprintf_package                         | wrn     | inf
getopt                                   | wrn     | inf
log                                      | wrn     | inf
log_mgmt                                 | wrn     | inf
log_uart                                 | wrn     | inf
mpu                                      | wrn     | inf
os                                       | wrn     | inf
shell.shell_uart                         | wrn     | inf
shell_uart                               | wrn     | inf
uart_ns16550                             | wrn     | inf
uart:~$ log backend log_backend_uart status
Logs are halted!
module_name                              | current | built-in 
----------------------------------------------------------
cbprintf_package                         | none    | inf
getopt                                   | none    | inf
log                                      | none    | inf
log_mgmt                                 | none    | inf
log_uart                                 | none    | inf
mpu                                      | none    | inf
os                                       | none    | inf
shell.shell_uart                         | none    | inf
shell_uart                               | none    | inf
uart_ns16550                             | none    | inf
uart:~$ log backend log_backend_uart go
uart:~$ log backend log_backend_uart status
module_name                              | current | built-in 
----------------------------------------------------------
cbprintf_package                         | inf     | inf
getopt                                   | inf     | inf
log                                      | inf     | inf
log_mgmt                                 | inf     | inf
log_uart                                 | inf     | inf
mpu                                      | inf     | inf
os                                       | inf     | inf
shell.shell_uart                         | inf     | inf
shell_uart                               | inf     | inf
uart_ns16550                             | inf     | inf
uart:~$ log backend shell_uart_backend status
module_name                              | current | built-in 
----------------------------------------------------------
cbprintf_package                         | wrn     | inf
getopt                                   | wrn     | inf
log                                      | wrn     | inf
log_mgmt                                 | wrn     | inf
log_uart                                 | wrn     | inf
mpu                                      | wrn     | inf
os                                       | wrn     | inf
shell.shell_uart                         | wrn     | inf
shell_uart                               | wrn     | inf
uart_ns16550                             | wrn     | inf
uart:~$ log backend log_backend_uart enable err
uart:~$ log backend log_backend_uart status
module_name                              | current | built-in 
----------------------------------------------------------
cbprintf_package                         | err     | inf
getopt                                   | err     | inf
log                                      | err     | inf
log_mgmt                                 | err     | inf
log_uart                                 | err     | inf
mpu                                      | err     | inf
os                                       | err     | inf
shell.shell_uart                         | err     | inf
shell_uart                               | err     | inf
uart_ns16550                             | err     | inf
uart:~$ log backend shell_uart_backend status
module_name                              | current | built-in 
----------------------------------------------------------
cbprintf_package                         | wrn     | inf
getopt                                   | wrn     | inf
log                                      | wrn     | inf
log_mgmt                                 | wrn     | inf
log_uart                                 | wrn     | inf
mpu                                      | wrn     | inf
os                                       | wrn     | inf
shell.shell_uart                         | wrn     | inf
shell_uart                               | wrn     | inf
uart_ns16550                             | wrn     | inf

ycsin added 2 commits January 31, 2025 16:12
The `id` is basically a compile-time constant. Setting it every
time the backend is enabled is unnecessary. Instead, set it on
`z_log_init()` regardless of whether or not it requires to be
`autostart`ed.

Fixes an issue where the `filter_get`/`filter_set`
accessed the wrong index and displayed the wrong log level when
user accesses the status of an uninitialized backend via:
`log backend <uninitialized_backend> status`.

Also fixes an issue when user tries to list the backends via:
`log list_backends`, where all uninitialized backends will have
ID = 0.

Signed-off-by: Yong Cong Sin <[email protected]>
Signed-off-by: Yong Cong Sin <[email protected]>
For backends that do not autostart themselves, initialize
& enable them on `log backend <log_backend_*> go`, so
that they function properly.

Signed-off-by: Yong Cong Sin <[email protected]>
Signed-off-by: Yong Cong Sin <[email protected]>
Comment on lines +346 to +350
while (log_backend_is_ready(backend) != 0) {
if (IS_ENABLED(CONFIG_MULTITHREADING)) {
k_msleep(10);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is similar to how the logging subsys initializes the backends with autostart set, but potentially livelocking the shell backend thread.

@ycsin ycsin requested a review from luchnikov January 31, 2025 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant