Skip to content

usb: patches to resolve issues around Kconfig option USB_UART_CONSOLE #40220

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.

Sign up for GitHub

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

Closed
wants to merge 13 commits into from
+184 −168
Closed

usb: patches to resolve issues around Kconfig option USB_UART_CONSOLE #40220

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ee570c8
usb: cdc_acm: use same init level and priority as serial drivers
jfischer-no Nov 9, 2021
b0dc396
usb: limit scope of CONFIG_USB_UART_CONSOLE
jfischer-no Nov 9, 2021
623ace0
usb: deprecate Kconfig option USB_UART_CONSOLE
jfischer-no Nov 9, 2021
9b88cba
usb: add option to initialize USB device support at boot
jfischer-no Nov 12, 2021
4d53a79
shields: add generic shield to use CDC ACM UART as backend
jfischer-no Nov 12, 2021
1d52620
samples: shell_module: use generic CDC ACM shield
jfischer-no Nov 12, 2021
a92896b
tests: bluetooth: shell: use generic CDC ACM shield
jfischer-no Nov 12, 2021
561c894
tests: uart_basic_api: use generic CDC ACM shield
jfischer-no Nov 12, 2021
461b174
samples: sensortile_box: use generic CDC ACM shield
jfischer-no Nov 12, 2021
74b92d7
samples: usb: console: fix configuration
jfischer-no Nov 12, 2021
9c03497
boards: clean up CDC ACM configuration for nRF52840 based boards
jfischer-no Nov 12, 2021
105e3da
usb: remove the remains of USB_UART_CONSOLE
jfischer-no Nov 12, 2021
6c32698
shields: cdc_acm: add support for zephyr,bt-c2h-uart property
jfischer-no Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion drivers/console/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ config CONSOLE_HANDLER

config CONSOLE_INIT_PRIORITY
int "Console init priority"
default 95 if USB_UART_CONSOLE || UART_MUX
default 95 if UART_MUX
Copy link
Contributor

@tejlmand tejlmand Nov 11, 2021

Choose a reason for hiding this comment

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

First observation, the default priority of 95 was chosen because of the USB stack initialization, see:
df2bb46

usb: console: Initialize USB console after USB Device stack

It does make sense to initialize USB console after USB Device stack.
Note that the value is selected only if we specify USB_UART_CONSOLE
in prj.conf, not in menuconfig afterwards.

Fixes #16518

So what has changed, since console can now be initialized at 60 instead ?
This commit only changed the symbol used: d8bc37b
but the default value is still 50
SERIAL_INIT_PRIORITY defaults to KERNEL_INIT_PRIORITY_DEVICE default == 50

Second observation:
The CONSOLE_INIT_PRIORITY defaults to KERNEL_INIT_PRIORITY_DEFAULT == 40 when UART_CONSOLE is not enabled, and the setting is having a prompt.
This means this setting suffers from the stuck symbols syndrome:
https://docs.zephyrproject.org/latest/guides/build/kconfig/tips.html#stuck-symbols-in-menuconfig-and-guiconfig

meaning that if anyone later enables the USB_UART_CONSOLE or the UART_CONSOLE, then the priority will not change, in which case the 40 value would risk still being active and not the preferred default of 60.

I believe majority of boards are having CONFIG_UART_CONSOLE=y in their _defconfig so it might not be a real problem, but it still looks as we might have a risk of strangely behaving system.

And it sounds to me as there is also some upper bound dependency since 95 is no longer a good default.
So should this priority in reality be a range ?
Where the upper and lower bounds might be defined by other settings.

Or should the setting be promptless in certain cases ?
Note, a promptless setting can still be controlled through a Kconfig file, for example Kconfig.board

Copy link
Contributor Author

@jfischer-no jfischer-no Nov 11, 2021

Choose a reason for hiding this comment

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

So what has changed, since console can now be initialized at 60 instead ?
This commit only changed the symbol used: d8bc37b
but the default value is still 50
SERIAL_INIT_PRIORITY defaults to KERNEL_INIT_PRIORITY_DEVICE default == 50

CDC ACM UART driver has been reworked a bit and does not block when USB device support is not initialized, (behaves like regular UART).

Or should the setting be promptless in certain cases ?

I am in favor of this option.

Copy link
Contributor

@tejlmand tejlmand Nov 16, 2021

Choose a reason for hiding this comment

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

CDC ACM UART driver has been reworked a bit and does not block when USB device support is not initialized, (behaves like regular UART).

I would like to see this sentence in the commit message, as that is an important piece of information in order to understand why changing the default from 95 to 60 is safe.

default 60 if UART_CONSOLE || XTENSA_SIM_CONSOLE
default KERNEL_INIT_PRIORITY_DEFAULT
help
Expand Down
4 changes: 1 addition & 3 deletions drivers/console/uart_console.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,9 +606,7 @@ static int uart_console_init(const struct device *arg)

/* UART console initializes after the UART device itself */
SYS_INIT(uart_console_init,
#if defined(CONFIG_USB_UART_CONSOLE)
APPLICATION,
#elif defined(CONFIG_EARLY_CONSOLE)
#if defined(CONFIG_EARLY_CONSOLE)
PRE_KERNEL_1,
#else
POST_KERNEL,
Expand Down
2 changes: 1 addition & 1 deletion samples/subsys/shell/shell_module/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ void main(void)
login_init();
}

#if defined(CONFIG_USB_UART_CONSOLE)
#if DT_NODE_HAS_COMPAT(DT_CHOSEN(zephyr_shell_uart), zephyr_cdc_acm_uart)
const struct device *dev;
uint32_t dtr = 0;

Expand Down
2 changes: 1 addition & 1 deletion subsys/usb/class/cdc_acm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,7 @@ static const struct uart_driver_api cdc_acm_driver_api = {
\
DEVICE_DT_INST_DEFINE(idx, cdc_acm_init, NULL, \
&cdc_acm_dev_data_##idx, &cdc_acm_config_##idx, \
POST_KERNEL, CONFIG_KERNEL_INIT_PRIORITY_DEVICE, \
PRE_KERNEL_1, CONFIG_SERIAL_INIT_PRIORITY, \
&cdc_acm_driver_api);

DT_INST_FOREACH_STATUS_OKAY(CDC_ACM_DT_DEVICE_DEFINE);
17 changes: 14 additions & 3 deletions tests/bluetooth/shell/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <sys/byteorder.h>
#include <zephyr.h>
#include <usb/usb_device.h>
#include <drivers/uart.h>

#include <shell/shell.h>

Expand Down Expand Up @@ -122,10 +123,20 @@ static void hrs_notify(void)

void main(void)
{
if (IS_ENABLED(CONFIG_USB_UART_CONSOLE)) {
usb_enable(NULL);
k_sleep(K_SECONDS(2));
#if DT_NODE_HAS_COMPAT(DT_CHOSEN(zephyr_shell_uart), zephyr_cdc_acm_uart)
Copy link
Contributor

@nordic-krch nordic-krch Nov 10, 2021

Choose a reason for hiding this comment

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

maybe you could extract it into a function, something like:

if (DT_NODE_HAS_COMPAT(DT_CHOSEN(zephyr_shell_uart), zephyr_cdc_acm_uart)) {  
  wait_usb_ready();
}

That would make it obvious why it is here.

Copy link
Contributor Author

@jfischer-no jfischer-no Nov 12, 2021

Choose a reason for hiding this comment

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

This part is removed now.

const struct device *dev;
uint32_t dtr = 0;

dev = DEVICE_DT_GET(DT_CHOSEN(zephyr_shell_uart));
if (!device_is_ready(dev) || usb_enable(NULL)) {
return;
}

while (!dtr) {
uart_line_ctrl_get(dev, UART_LINE_CTRL_DTR, &dtr);
k_sleep(K_MSEC(100));
}
#endif

printk("Type \"help\" for supported commands.");
printk("Before any Bluetooth commands you must `bt init` to initialize"
Expand Down
2 changes: 1 addition & 1 deletion tests/drivers/uart/uart_basic_api/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ void test_uart_pending(void)

void test_main(void)
{
#if defined(CONFIG_USB_UART_CONSOLE)
#if DT_NODE_HAS_COMPAT(DT_CHOSEN(zephyr_console), zephyr_cdc_acm_uart)
const struct device *dev;
uint32_t dtr = 0;

Expand Down