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
Closed

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

Show file tree
Hide file tree
Changes from 8 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
43 changes: 43 additions & 0 deletions boards/shields/cdc_acm/Kconfig.defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Copyright (c) 2021 Nordic Semiconductor ASA
# SPDX-License-Identifier: Apache-2.0

if SHIELD_CDC_ACM_CONSOLE || SHIELD_CDC_ACM_SHELL

# TEST_LOGGING_DEFAULTS option sets all log levels to INF,
# including the USB_CDC_ACM_LOG_LEVEL and that can not work.
config TEST_LOGGING_DEFAULTS
default n

config SERIAL
default y

config UART_LINE_CTRL
default y

config USB_DEVICE_STACK
default y

config USB_DEVICE_INITIALIZE_AT_BOOT
default y

endif #if SHIELD_CDC_ACM_CONSOLE || SHIELD_CDC_ACM_SHELL

if SHIELD_CDC_ACM_CONSOLE

config CONSOLE
default y

config UART_CONSOLE
default y

endif #if SHIELD_CDC_ACM_CONSOLE

if SHIELD_CDC_ACM_SHELL

config SHELL_BACKEND_SERIAL_CHECK_DTR
default y

config LOG_PRINTK
default y

endif #if SHIELD_CDC_ACM_SHELL
8 changes: 8 additions & 0 deletions boards/shields/cdc_acm/Kconfig.shield
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Copyright (c) 2021
# SPDX-License-Identifier: Apache-2.0

config SHIELD_CDC_ACM_CONSOLE
def_bool $(shields_list_contains,cdc_acm_console)

config SHIELD_CDC_ACM_SHELL
def_bool $(shields_list_contains,cdc_acm_shell)
6 changes: 3 additions & 3 deletions ...les/subsys/shell/shell_module/usb.overlay → ...s/shields/cdc_acm/cdc_acm_console.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@

/ {
chosen {
zephyr,shell-uart = &cdc_acm_uart0;
zephyr,console = &console_cdc_acm_uart0;
};
};

&zephyr_udc0 {
cdc_acm_uart0: cdc_acm_uart0 {
console_cdc_acm_uart0: console_cdc_acm_uart0 {
compatible = "zephyr,cdc-acm-uart";
label = "CDC_ACM_0";
label = "CONSOLE_CDC_ACM_0";
};
};
6 changes: 3 additions & 3 deletions ...s/drivers/uart/uart_basic_api/usb.overlay → boards/shields/cdc_acm/cdc_acm_shell.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@

/ {
chosen {
zephyr,console = &cdc_acm_uart0;
zephyr,shell-uart = &shell_cdc_acm_uart0;
};
};

&zephyr_udc0 {
cdc_acm_uart0: cdc_acm_uart0 {
shell_cdc_acm_uart0: shell_cdc_acm_uart0 {
compatible = "zephyr,cdc-acm-uart";
label = "CDC_ACM_0";
label = "SHELL_CDC_ACM_0";
};
};
64 changes: 64 additions & 0 deletions boards/shields/cdc_acm/doc/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
.. _cdc_acm_shield:

Generic shields for CDC ACM UART
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.

Shields today are a piece of hardware you may attach to another board.
file:///projects/github/ncs/zephyr/doc/_doc/html/boards/shields/index.html

This commit 4d53a79 uses the shield feature for something that is not a shield.

That is a direction I dislike.

It's another approach of #14740 and #33656, just through shield this time.

Using shield this way makes it unclear what a shield is, and it also suffers the same disadvantage / complexity as mentioned here: #14740 (comment)

If you would like to propose a solution like the shield then please make a dedicated PR with such proposal the can be discussed, also at dev-review. For example the METACFG name as mentioned here: #40220 (review)

Not being against such idea, my concern is still related to:

  • How does end-user know when this meta-config can be used.
  • One more place (in an already complex system) from where files are picked up
  • What would be the threshold when new meta-config should be introduced.

Copy link
Contributor Author

@jfischer-no jfischer-no Nov 16, 2021

Choose a reason for hiding this comment

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

I am convinced that shield approach is a better fit for CDC ACM UART, similar to how one would connect a real UART controller through I2C/SPI using a shield.

################################

Overview
********

This is a generic shield that provides devicetree and configuration overlays,
and configures USB device stack so that CDC ACM UART can be used as backend
for console, logging, and shell. It is mainly intended to be used with boards
that do not have a debug adapter or native UART, but do have a USB device
controller.
This approach allows us to avoid many identical overlays in samples and tests
directories (see :ref:`usb_device_cdc_acm` for more details).
It also simplifies the configuration of the above mentioned boards,
they can stay with the minimal configuration which minimizes the conflicts
especially with different in-tree samples.

These shields enable :kconfig:`CONFIG_USB_DEVICE_INITIALIZE_AT_BOOT` and
configure USB device stack so that it is automatically initialized.
This is important for the boards like :ref:`nrf52840dongle_nrf52840`,
otherwise in-tree samples, that do not enable USB device support, are
not usable. But it also means that in-tree samples, like :ref:`usb_cdc-acm`,
that initialize USB device support themselves cannot be used with these shields.
This is a good compromise which provides maximum coverage of usable samples for
these specific USB dongles.

Current supported chosen properties
===================================

+-----------------------+---------------------+
| Chosen property | Shield Designation |
| | |
+=======================+=====================+
| ``zephyr,console`` | ``cdc_acm_console`` |
+-----------------------+---------------------+
| ``zephyr,shell-uart`` | ``cdc_acm_shell`` |
+-----------------------+---------------------+

Requirements
************

This shield can only be used with a board which provides a USB device
controller.

Programming
***********

Set ``-DSHIELD=cdc_acm_shell`` when you invoke ``west build``. For example:

.. zephyr-app-commands::
:zephyr-app: samples/subsys/shell/shell_module
:board: nrf52840dongle_nrf52840
:shield: cdc_acm_shell
:goals: build

Or ``-DSHIELD=cdc_acm_console``, for example:

.. zephyr-app-commands::
:zephyr-app: samples/basic/threads
:board: nrf52840dongle_nrf52840
:shield: cdc_acm_console
:goals: build
8 changes: 6 additions & 2 deletions doc/reference/usb/uds_cdc_acm.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ With the CDC ACM UART node from above and ``zephyr,console`` property of the
chosen node, we can describe that CDC ACM UART is to be used with the console.
A similar overlay file is used by :ref:`cdc-acm-console`.
If USB device support is enabled in the application, as in the console sample,
:kconfig:`CONFIG_USB_UART_CONSOLE` must be enabled,
which does nothing but change the initialization time of the console driver.
:kconfig:`CONFIG_USB_UART_CONSOLE` can be enabled, which is only used
to enable relevant options in backend configuration (if any).

.. code-block:: devicetree

Expand Down Expand Up @@ -106,3 +106,7 @@ CDC ACM UART as backend for a subsystem or application:
* ``zephyr,shell-uart`` used by shell for serial backend,
for example see :zephyr_file:`samples/subsys/shell/shell_module`
* ``zephyr,uart-mcumgr`` used by :ref:`smp_svr_sample`

In-tree samples that do not require any USB device classes other than
CDC ACM UART for console, logging, or shell should be built with
:ref:`cdc_acm_shield`.
11 changes: 1 addition & 10 deletions 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 Expand Up @@ -87,15 +87,6 @@ config UART_CONSOLE_INPUT_EXPIRED_TIMEOUT
Fixed amount of time which unit is milliseconds to keep the UART
console in use flag true.

config USB_UART_CONSOLE
bool "Use USB port for console outputs"
select UART_CONSOLE
select USB_CDC_ACM
help
Enable this option to use the USB CDC ACM class for console.
As for the console driver, this option only changes the initialization
level.

config RAM_CONSOLE
bool "Use RAM console"
select CONSOLE_HAS_DRIVER
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
5 changes: 0 additions & 5 deletions samples/subsys/shell/shell_module/overlay-usb.conf
View file
Open in desktop

This file was deleted.

3 changes: 1 addition & 2 deletions samples/subsys/shell/shell_module/sample.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ tests:
tags: shell usb
harness: keyboard
min_ram: 40
extra_args: OVERLAY_CONFIG="overlay-usb.conf"
DTC_OVERLAY_FILE="usb.overlay"
extra_args: SHIELD="cdc_acm_shell"
sample.shell.shell_module.minimal:
filter: CONFIG_SERIAL and dt_chosen_enabled("zephyr,shell-uart")
tags: shell
Expand Down
15 changes: 0 additions & 15 deletions 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 @@ -344,19 +344,4 @@ void main(void)
if (IS_ENABLED(CONFIG_SHELL_START_OBSCURED)) {
login_init();
}

#if defined(CONFIG_USB_UART_CONSOLE)
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
}
6 changes: 6 additions & 0 deletions subsys/usb/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,12 @@ config USB_WORKQUEUE_PRIORITY
priority. This means that any work handler, once started, won't
be preempted by any other thread until finished.

config USB_DEVICE_INITIALIZE_AT_BOOT
bool "Initialize USB device support at boot"
depends on USB_CDC_ACM
help
Use CDC ACM UART as backend for console, shell, or logging.

source "subsys/usb/class/Kconfig"

endif # USB_DEVICE_STACK
7 changes: 7 additions & 0 deletions subsys/usb/class/Kconfig.cdc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ config CDC_ACM_DTE_RATE_CALLBACK_SUPPORT
by Arduino style programmers to reset the device into the
bootloader.

config USB_UART_CONSOLE
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.

Could we get rid of this at all? Maybe something with dt_compat_enabled could be used instead, i.e. here

zephyr/subsys/shell/backends/Kconfig.backends

Line 76 in 23ab949

default y if USB_UART_CONSOLE

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.

No, it is there for reasons.

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.

deprecated now

bool "Use CDC ACM UART as backend for console (DEPRECATED)"
help
This option is deprecated.
This option is only used to enable relevant options in backend
configuration (if any), and for backward compatibility.
Comment on lines +60 to +61
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.

This sound like a setting we don't really assume is in use, but we don't know.

I think it should be investigated and marked deprecated so that it can be completely removed in two releases from now.

Else I fear this setting will just stay and never get cleaned up.

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.

No, it is in use and is investigated, see commit message:
"Kconfig option USB_UART_CONSOLE does not have any dependencies
on console drivers, but is used to select backend specific
Kconfig option in subsys/shell/backends/Kconfig.backends.
Keep the name of the option USB_UART_CONSOLE but move it
to CDC ACM configuration and update description."

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.

then I suggest to rework the comment, cause that is not clear to me.

And as this setting is deprecated, are there any plans / tasks to follow up on removing the dependency in subsys/shell/backends/Kconfig.backends ?

Copy link
Contributor Author

@jfischer-no jfischer-no Nov 16, 2021

Choose a reason for hiding this comment

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

Commit was reworked. I will remove Kconfig option USB_UART_CONSOLE and dependency in subsys/shell/backends/Kconfig.backends in subsequent PR because MCUboot needs to be adapted as well.


module = USB_CDC_ACM
module-str = usb cdc acm
source "subsys/logging/Kconfig.template.log_config"
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);
15 changes: 12 additions & 3 deletions subsys/usb/usb_device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1551,7 +1551,8 @@ int usb_enable(usb_dc_status_callback status_cb)
k_mutex_lock(&usb_enable_lock, K_FOREVER);

if (usb_dev.enabled == true) {
ret = 0;
LOG_WRN("USB device support already enabled");
ret = -EALREADY;
goto out;
}

Expand Down Expand Up @@ -1636,9 +1637,10 @@ int usb_enable(usb_dc_status_callback status_cb)
static int usb_device_init(const struct device *dev)
{
uint8_t *device_descriptor;
int ret;

if (usb_dev.enabled == true) {
return 0;
return -EALREADY;
}

/* register device descriptor */
Expand All @@ -1650,7 +1652,14 @@ static int usb_device_init(const struct device *dev)

usb_set_config(device_descriptor);

if (IS_ENABLED(CONFIG_USB_DEVICE_INITIALIZE_AT_BOOT)) {
ret = usb_enable(NULL);
if (ret) {
return ret;
}
}

return 0;
}

SYS_INIT(usb_device_init, POST_KERNEL, CONFIG_KERNEL_INIT_PRIORITY_DEVICE);
SYS_INIT(usb_device_init, POST_KERNEL, CONFIG_APPLICATION_INIT_PRIORITY);
6 changes: 0 additions & 6 deletions tests/bluetooth/shell/cdc_acm.conf
View file
Open in desktop

This file was deleted.

6 changes: 1 addition & 5 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,11 +123,6 @@ static void hrs_notify(void)

void main(void)
{
if (IS_ENABLED(CONFIG_USB_UART_CONSOLE)) {
usb_enable(NULL);
k_sleep(K_SECONDS(2));
}

printk("Type \"help\" for supported commands.");
printk("Before any Bluetooth commands you must `bt init` to initialize"
" the stack.\n");
Expand Down
3 changes: 1 addition & 2 deletions tests/bluetooth/shell/testcase.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ tests:
harness: keyboard
min_flash: 145
bluetooth.shell.cdc_acm:
extra_args: OVERLAY_CONFIG=cdc_acm.conf
DTC_OVERLAY_FILE="usb.overlay"
extra_args: SHIELD="cdc_acm_shell"
depends_on: usb_device
platform_allow: native_posix native_posix_64 nrf52840dk_nrf52840
platform_exclude: nrf52dk_nrf52810
Expand Down
18 changes: 0 additions & 18 deletions tests/bluetooth/shell/usb.overlay
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions tests/drivers/uart/uart_basic_api/overlay-usb.conf
View file
Open in desktop

This file was deleted.

Loading