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: uart: support multiple instances #64917

Merged
merged 3 commits into from Nov 13, 2023

Conversation

ycsin
Copy link
Collaborator

@ycsin ycsin commented Nov 7, 2023

Extends the UART log backend to support multiple UART instances.

Building samples/hello_world with qemu_x86 using additional Kconfigs:

CONFIG_LOG=y
CONFIG_LOG_BACKEND_UART=y

ROM reports:

main branch

Path                              Size
=======================================
log_backend_uart.c (ROM)           208
├── char_out                        49
├── dropped                         16
├── format_set                      15
├── log_backend_uart                16
├── log_backend_uart_api            28
├── log_backend_uart_init            1
├── log_const_log_uart               8
├── log_output_uart                 16
├── panic                           23
└── process                         36
=======================================
log_backend_uart.c (RAM)            26
├── backend_cb_log_backend_uart      8
├── in_panic                         1
├── log_format_current               4
├── log_output_uart_control_block   12
└── uart_output_buf                  1

This PR, using zephyr,console

Path                              Size
=======================================
log_backend_uart.c (ROM)           280
├── backend_cb_log_backend_uart0     8
├── char_out                        51
├── dropped                         22
├── format_set                      24
├── lbu_cb_ctx_0                    12
├── lbu_output_0                    16
├── log_backend_uart0               16
├── log_backend_uart_api            28
├── log_backend_uart_init           21
├── log_const_log_uart               8
├── panic                           29
└── process                         45
=======================================
log_backend_uart.c (RAM)            53
├── backend_cb_log_backend_uart0     8
├── lbu_buffer_0                     1
├── lbu_data_0                      32
└── lbu_output_0_control_block      12
main PR
RAM 122912 122912
ROM 56856 56864 +8
char_out 49 51 +2
dropped 16 22 +6
format_set 15 24 +9
log_backend_uart 16 16
log_backend_uart_api 28 28
log_backend_uart_init 1 21 +20
log_const_log_uart 8 8
log_output_uart 16 16
panic 23 29 +6
process 36 45 +9

This PR, using zephyr,log-uart, 2 instances

Path                              Size
=======================================
log_backend_uart.c                 332
├── backend_cb_log_backend_uart0     8
├── backend_cb_log_backend_uart1     8
├── char_out                        51
├── dropped                         22
├── format_set                      24
├── lbu_cb_ctx_0                    12
├── lbu_cb_ctx_1                    12
├── lbu_output_0                    16
├── lbu_output_1                    16
├── log_backend_uart0               16
├── log_backend_uart1               16
├── log_backend_uart_api            28
├── log_backend_uart_init           21
├── log_const_log_uart               8
├── panic                           29
└── process                         45
=======================================
log_backend_uart.c (RAM)           106
├── backend_cb_log_backend_uart0     8
├── backend_cb_log_backend_uart1     8
├── lbu_buffer_0                     1
├── lbu_buffer_1                     1
├── lbu_data_0                      32
├── lbu_data_1                      32
├── lbu_output_0_control_block      12
└── lbu_output_1_control_block      12

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

subsys/logging/backends/log_backend_uart.c Outdated Show resolved Hide resolved
subsys/logging/backends/log_backend_uart.c Outdated Show resolved Hide resolved
subsys/logging/backends/log_backend_uart.c Outdated Show resolved Hide resolved
samples/hello_world/prj.conf Outdated Show resolved Hide resolved
samples/hello_world/src/main.c Outdated Show resolved Hide resolved
@cfriedt cfriedt changed the title logging: uart: support multiple backends logging: uart: support multiple instances Nov 7, 2023
@ycsin ycsin force-pushed the pr/lbu_multi branch 8 times, most recently from 790999f to 2beb974 Compare November 8, 2023 10:43
Set the backend's context to its control block's context
instead of NULL when doing `log_backend_enable`, as the log
backend UART uses that later.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin ycsin force-pushed the pr/lbu_multi branch 3 times, most recently from 6886265 to 11748e7 Compare November 9, 2023 03:30
Comment on lines 256 to 262
/*
* #if DT_NODE_HAS_COMPAT(DT_CHOSEN(zephyr_log_uart), zephyr_log_uart)
* DT_FOREACH_PROP_ELEM_SEP(DT_CHOSEN(zephyr_log_uart), uarts, LBU_PHA_FN, ());
* #else
* LBU_DEFINE(DT_CHOSEN(zephyr_log_uart), 0);
* #endif
*/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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


/ {
chosen {
zephyr,log-uart = &log_uarts;
Copy link
Member

Choose a reason for hiding this comment

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

I think my preference would be either to use phandles property in the chosen node, or to simply use a child DT node under each UART selected to be a log backend.

However, the phandles approach does not seem to work for in the chosen node.

Personally, I think the chosen + node + phandles is kind of overkill.

Copy link
Member

@cfriedt cfriedt Nov 10, 2023

Choose a reason for hiding this comment

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

Alternatively, if it were possible to support a phandle array in chosen, that would be kind of ideal, but it might require some additional devicetree hacks.

@ycsin ycsin marked this pull request as ready for review November 10, 2023 06:32
@zephyrbot zephyrbot added the area: Devicetree Binding PR modifies or adds a Device Tree binding label Nov 10, 2023
cfriedt and others added 2 commits November 10, 2023 17:01
Extends the log_backend_uart to support logging to multiple
UART instances.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
Add testcase for the multi-instance log backend uart using
emulated uart.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Can you do a build without these changes and with these changes (ARM cortex ideally) with 1 device and note the flash/RAM usage on both?

@nordic-krch
Copy link
Contributor

+1 to @nordicjm suggestion. I guess there will be increase so it would be good to wrap instance getting in some macro so that there is no code addition when 1 uart is used (99.99% of cases).

@ycsin
Copy link
Collaborator Author

ycsin commented Nov 11, 2023

@nordic-krch @nordicjm

Building samples/hello_world with qemu_x86 using additional Kconfigs:

CONFIG_LOG=y
CONFIG_LOG_BACKEND_UART=y

ROM reports:

main branch

Path                              Size
=======================================
log_backend_uart.c (ROM)           208
├── char_out                        49
├── dropped                         16
├── format_set                      15
├── log_backend_uart                16
├── log_backend_uart_api            28
├── log_backend_uart_init            1
├── log_const_log_uart               8
├── log_output_uart                 16
├── panic                           23
└── process                         36
=======================================
log_backend_uart.c (RAM)            26
├── backend_cb_log_backend_uart      8
├── in_panic                         1
├── log_format_current               4
├── log_output_uart_control_block   12
└── uart_output_buf                  1

This PR, using zephyr,console

Path                              Size
=======================================
log_backend_uart.c (ROM)           280
├── backend_cb_log_backend_uart0     8
├── char_out                        51
├── dropped                         22
├── format_set                      24
├── lbu_cb_ctx_0                    12
├── lbu_output_0                    16
├── log_backend_uart0               16
├── log_backend_uart_api            28
├── log_backend_uart_init           21
├── log_const_log_uart               8
├── panic                           29
└── process                         45
=======================================
log_backend_uart.c (RAM)            53
├── backend_cb_log_backend_uart0     8
├── lbu_buffer_0                     1
├── lbu_data_0                      32
└── lbu_output_0_control_block      12
main PR
RAM 122912 122912
ROM 56856 56864 +8
char_out 49 51 +2
dropped 16 22 +6
format_set 15 24 +9
log_backend_uart 16 16
log_backend_uart_api 28 28
log_backend_uart_init 1 21 +20
log_const_log_uart 8 8
log_output_uart 16 16
panic 23 29 +6
process 36 45 +9

This PR, using zephyr,log-uart, 2 instances

Path                              Size
=======================================
log_backend_uart.c                 332
├── backend_cb_log_backend_uart0     8
├── backend_cb_log_backend_uart1     8
├── char_out                        51
├── dropped                         22
├── format_set                      24
├── lbu_cb_ctx_0                    12
├── lbu_cb_ctx_1                    12
├── lbu_output_0                    16
├── lbu_output_1                    16
├── log_backend_uart0               16
├── log_backend_uart1               16
├── log_backend_uart_api            28
├── log_backend_uart_init           21
├── log_const_log_uart               8
├── panic                           29
└── process                         45
=======================================
log_backend_uart.c (RAM)           106
├── backend_cb_log_backend_uart0     8
├── backend_cb_log_backend_uart1     8
├── lbu_buffer_0                     1
├── lbu_buffer_1                     1
├── lbu_data_0                      32
├── lbu_data_1                      32
├── lbu_output_0_control_block      12
└── lbu_output_1_control_block      12

Copy link
Contributor

@nordic-krch nordic-krch left a comment

Choose a reason for hiding this comment

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

Checked that for cortex-m and riscv32 overhead is even smaller. Looks ok for me.

@carlescufi carlescufi merged commit 6d9fcd8 into zephyrproject-rtos:main Nov 13, 2023
20 checks passed
@ycsin ycsin deleted the pr/lbu_multi branch November 13, 2023 09:23
ycsin added a commit to ycsin/zephyr that referenced this pull request Nov 13, 2023
Following zephyrproject-rtos#64917, the name of the UART log backend(s) are now
defined to be `log_backend_uart##idx`, update the name used in
the test to look for `log_backend_uart0`, which is the name of
the UART logging backend when using the default
`zephyr,console` node.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
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.

None yet

6 participants