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 all 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
10 changes: 0 additions & 10 deletions boards/arm/arduino_nano_33_ble/Kconfig.board
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,6 @@ config BOARD_ARDUINO_NANO_33_BLE
bool "Arduino Nano 33 BLE board"
depends on SOC_NRF52840_QIAA

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

so all boards cleaned up in this commit 9c03497 must now use -DSHIELD=cdc_acm_console

That sounds like a step backward in user-friendliness if those boards are expected to always use the usb as console.

For example, this board bl654_usb doesn't seem to have any other option than using USB for console:
https://docs.zephyrproject.org/latest/boards/arm/bl654_usb/doc/bl654_usb.html

@kieran-mackey FYI.

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.

No, it is a step forward, only with these patches it becomes possible to use CDC ACM UART (not usb) as backend for console. The configuration of this board in the tree is not usable.

Copy link
Contributor

@tejlmand tejlmand Nov 17, 2021

Choose a reason for hiding this comment

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

The configuration of this board in the tree is not usable.

Which board or all boards ?

Read my comment:

so all boards cleaned up in this commit 9c03497 must now use -DSHIELD=cdc_acm_console

i'm refering to the entire commit 9c03497, not a single board, but github requires me to put the comment on a single line.
That is, my comment covers all those boards you are modifying in that commit:

  • arduino_nano_33_ble
  • bl654_usb
  • degu_evk
  • nrf52840dongle_nrf52840

are you saying that the current configuration in Zephyr of those boards are not working for any of them ?

Copy link
Contributor

@tejlmand tejlmand Nov 17, 2021

Choose a reason for hiding this comment

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

as far as I'm aware, the general problem started with this cleanup:
d12331e
which again was fixing:
#38403
caused by this:
#37512

So maybe we should go back to the origin of the problem, instead of completely changing how users must use the boards.

bool "Sends the console output over the USB port"
depends on BOARD_ARDUINO_NANO_33_BLE
select SERIAL
select USB_DEVICE_STACK
select CONSOLE
select PRINTK
select UART_INTERRUPT_DRIVEN
select USB_UART_CONSOLE

config BOARD_ARDUINO_NANO_33_BLE_INIT_SENSORS
bool "Initializes the internal I2C sensors on the board"
depends on BOARD_ARDUINO_NANO_33_BLE
10 changes: 0 additions & 10 deletions boards/arm/bl654_usb/Kconfig.defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,6 @@ config FLASH_LOAD_OFFSET
default 0x1000
depends on !USE_DT_CODE_PARTITION

if USB_DEVICE_STACK

config USB_UART_CONSOLE
default y

config UART_LINE_CTRL
default y

endif # USB_DEVICE_STACK

if IEEE802154

config IEEE802154_NRF5
Expand Down
8 changes: 0 additions & 8 deletions boards/arm/bl654_usb/bl654_usb.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@
chosen {
zephyr,sram = &sram0;
zephyr,flash = &flash0;
zephyr,console = &bl654_cdc_acm_uart;
zephyr,shell-uart = &bl654_cdc_acm_uart;
zephyr,bt-c2h-uart = &bl654_cdc_acm_uart;
zephyr,code-partition = &slot0_partition;
};

Expand Down Expand Up @@ -104,9 +101,4 @@
zephyr_udc0: &usbd {
compatible = "nordic,nrf-usbd";
status = "okay";

bl654_cdc_acm_uart: bl654_cdc_acm_uart {
compatible = "zephyr,cdc-acm-uart";
label = "CDC_ACM_0";
};
};
3 changes: 0 additions & 3 deletions boards/arm/bl654_usb/bl654_usb_defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ CONFIG_CONSOLE=y
# Enable GPIO
CONFIG_GPIO=y

# Enable USB
CONFIG_USB_DEVICE_STACK=y

# Additional board options
CONFIG_GPIO_AS_PINRESET=y

Expand Down
16 changes: 0 additions & 16 deletions boards/arm/degu_evk/Kconfig.defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,6 @@ if BOARD_DEGU_EVK
config BOARD
default "degu_evk"

if USB_DEVICE_STACK

config USB_DEVICE_PRODUCT
default "Degu Evaluation Kit"

config USB_UART_CONSOLE
default y

config UART_INTERRUPT_DRIVEN
default y

config UART_LINE_CTRL
default y

endif # USB_DEVICE_STACK

if DISK_DRIVER_FLASH

config DISK_FLASH_DEV_NAME
Expand Down
9 changes: 2 additions & 7 deletions boards/arm/degu_evk/degu_evk.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
chosen {
zephyr,sram = &sram0;
zephyr,flash = &flash0;
zephyr,console = &degu_cdc_acm_uart;
zephyr,shell-uart = &degu_cdc_acm_uart;
zephyr,console = &uart0;
zephyr,shell-uart = &uart0;
zephyr,code-partition = &slot0_partition;
};

Expand Down Expand Up @@ -143,9 +143,4 @@
zephyr_udc0: &usbd {
compatible = "nordic,nrf-usbd";
status = "okay";

degu_cdc_acm_uart: degu_cdc_acm_uart {
compatible = "zephyr,cdc-acm-uart";
label = "CDC_ACM_0";
};
};
3 changes: 0 additions & 3 deletions boards/arm/degu_evk/degu_evk_defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ CONFIG_SERIAL=y
CONFIG_CONSOLE=y
CONFIG_UART_CONSOLE=y

# enable USB
CONFIG_USB_DEVICE_STACK=y

# additional board options
CONFIG_GPIO=y
CONFIG_GPIO_AS_PINRESET=y
Expand Down
8 changes: 0 additions & 8 deletions boards/arm/nrf52840dongle_nrf52840/Kconfig.defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,6 @@ config FLASH_LOAD_OFFSET
default 0x1000
depends on BOARD_HAS_NRF5_BOOTLOADER && !USE_DT_CODE_PARTITION

if USB_DEVICE_STACK

# Enable UART driver, needed for CDC ACM
config SERIAL
default y

endif # USB_DEVICE_STACK

config BT_CTLR
default BT

Expand Down
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 || SHIELD_CDC_ACM_BT_C2H

# 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
11 changes: 11 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,11 @@
# 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)

config SHIELD_CDC_ACM_BT_C2H
def_bool $(shields_list_contains,cdc_acm_bt_c2h)
6 changes: 3 additions & 3 deletions tests/bluetooth/shell/usb.overlay → ...ds/shields/cdc_acm/cdc_acm_bt_c2h.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,bt-c2h-uart = &bt_c2h_cdc_acm_uart0;
};
};

&zephyr_udc0 {
cdc_acm_uart0: cdc_acm_uart0 {
bt_c2h_cdc_acm_uart0: bt_c2h_cdc_acm_uart0 {
compatible = "zephyr,cdc-acm-uart";
label = "CDC_ACM_0";
label = "BT_C2H_CDC_ACM_0";
};
};
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 samples/boards/sensortile_box/app.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";
};
};
75 changes: 75 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,75 @@
.. _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`` |
+------------------------+---------------------+
| ``zephyr,bt-c2h-uart`` | ``cdc_acm_bt_c2h`` |
+------------------------+---------------------+

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

With ``-DSHIELD=cdc_acm_bt_c2h``, :ref:`bluetooth-hci-uart-sample` can use
CDC ACM UART as interface to the host:

.. zephyr-app-commands::
:zephyr-app: samples/bluetooth/hci_uart
:board: nrf52840dongle_nrf52840
:shield: cdc_acm_bt_c2h
:goals: build
7 changes: 4 additions & 3 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 @@ -51,9 +51,6 @@ Console over CDC ACM UART
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.

.. code-block:: devicetree

Expand Down Expand Up @@ -106,3 +103,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
1 change: 1 addition & 0 deletions samples/boards/sensortile_box/README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ Build and flash the sample in the following way:
.. zephyr-app-commands::
:zephyr-app: samples/boards/sensortile_box
:board: sensortile_box
:shield: cdc_acm_console
:goals: build flash

Please note that flashing the board requires a few preliminary steps described
Expand Down
6 changes: 1 addition & 5 deletions samples/boards/sensortile_box/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,9 @@ CONFIG_IIS3DHHC_TRIGGER_OWN_THREAD=y
CONFIG_LIS2MDL=y
CONFIG_LIS2MDL_TRIGGER_NONE=y

# config USB and USB console
# config USB device support
CONFIG_USB_DEVICE_VID=0x0483
CONFIG_USB_DEVICE_PID=0x1234
CONFIG_USB_DEVICE_STACK=y
CONFIG_USB_DEVICE_PRODUCT="Zephyr CDC SensorTile.box"

CONFIG_USB_UART_CONSOLE=y
CONFIG_UART_INTERRUPT_DRIVEN=y
CONFIG_UART_LINE_CTRL=y
CONFIG_CBPRINTF_FP_SUPPORT=y
1 change: 1 addition & 0 deletions samples/boards/sensortile_box/sample.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ tests:
platform_allow: sensortile_box
tags: sensors
depends_on: i2c spi gpio
extra_args: SHIELD="cdc_acm_console"
Loading