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

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

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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
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)
18 changes: 18 additions & 0 deletions boards/shields/cdc_acm/cdc_acm_console.overlay
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright (c) 2021 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

/ {
chosen {
zephyr,console = &console_cdc_acm_uart0;
};
};

&zephyr_udc0 {
console_cdc_acm_uart0: console_cdc_acm_uart0 {
compatible = "zephyr,cdc-acm-uart";
label = "CONSOLE_CDC_ACM_0";
};
};
18 changes: 18 additions & 0 deletions boards/shields/cdc_acm/cdc_acm_shell.overlay
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright (c) 2021 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

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

&zephyr_udc0 {
shell_cdc_acm_uart0: shell_cdc_acm_uart0 {
compatible = "zephyr,cdc-acm-uart";
label = "SHELL_CDC_ACM_0";
};
};
64 changes: 64 additions & 0 deletions boards/shields/cdc_acm/doc/index.rst
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
Collaborator

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
Collaborator Author

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
4 changes: 4 additions & 0 deletions doc/reference/usb/uds_cdc_acm.rst
Original file line number Diff line number Diff line change
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`.