boards: add common configuration for CDC ACM UART #81308

jfischer-no · 2024-11-12T23:13:51Z

Many boards have similar code to configure the USB and CDC ACM UART that
they want to use as a logging or shell backend. Some of them have an
incorrect or incomplete configuration.

These boards do not have a built-in debug adapter, but a SoC with a USB
device controller and a bootloader with USB device support. Introduce
common CDC ACM UART configuration that these boards should use for
logging or shell backend to avoid duplicate or incorrect configuration.

Derived from #81220 and #81228, as first step (or alternative).

rerickson1

Ezurio boards look ok

nordicjm · 2024-11-13T07:46:07Z

boards/common/cdc_acm_serial.dts

+};
+
+zephyr_udc0: &usbd {
+	cdc_acm_uart: cdc_acm_uart {


should this not be cdc_acm_uart0 if it supports multiple instances?

cdc_acm_uart0 is likely to conflict with samples or user applications, we should rather name it board_cdc_acm_uart, like we prefix snippets and shields.

board_* prefix sounds good

I also moved them to boards/common/usb/

erwango · 2024-11-13T08:19:30Z

boards/arduino/opta/Kconfig.defconfig

-endif # LOG
-
-endif # USB_DEVICE_STACK
+source "${ZEPHYR_BASE}/boards/common/Kconfig.cdc_acm_serial"


Nitpick:
Harmonize path between Kconfig and dts:
source "../boards/common/Kconfig.cdc_acm_serial"
#include <../boards/common/cdc_acm_serial.dts>
or
source "${ZEPHYR_BASE}/boards/common/Kconfig.cdc_acm_serial"
#include <${ZEPHYR_BASE}/boards/common/cdc_acm_serial.dts>

${ZEPHYR_BASE} cannot be resolved in dt.

Can we then use ../boards/common for Kconfig ?

boards/st/sensortile_box_pro/Kconfig.defconfig

carlescufi

+1 for the approach

bryceschober · 2024-12-04T00:50:55Z

boards/st/steval_stwinbx1/doc/index.rst

-                compatible = "zephyr,cdc-acm-uart";
-        };
-     };
+- through common CDC ACM UART backend configuraiton for all boards


This typo caught my eye, and has at least one other instance in this PR.

Suggested change

- through common CDC ACM UART backend configuraiton for all boards

- through common CDC ACM UART backend configuration for all boards

tejlmand

lgtm.

Still some minor details, but those are very much depending on the perspective from where the solution is seen, so not something which should block this PR.

Many boards have similar code to configure the USB and CDC ACM UART that they want to use as a logging or shell backend. Some of them have an incorrect or incomplete configuration. These boards do not have a built-in debug adapter, but a SoC with a USB device controller and a bootloader with USB device support. Introduce common CDC ACM UART configuration that these boards should use for logging or shell backend to avoid duplicate or incorrect configuration. Signed-off-by: Johann Fischer <[email protected]>

carlescufi · 2024-12-16T12:22:38Z

@rerickson1 @nordicjm @tejlmand @erwango could you take another look?

boards/arduino/opta/arduino_opta_stm32h747xx_m7_defconfig

boards/makerdiary/nrf52840_mdk_usb_dongle/Kconfig

boards/nordic/thingy53/thingy53_nrf5340_cpuapp_ns.dts

Remove all USB and CDC ACM configuration in favor of common configuraiton. Do not adapt board-specific configurations such as unknown PID/VID or string descriptors. There is no justification for using them on specific boards, and we do not have formal approval to use them in the project tree. Also, we need more uniform configuration, since the main reason for enabling CDC ACM here is to allow users to run examples like hello_world right out of the box. Of course, anyone is free to customize these settings in their fork or downstream project. Signed-off-by: Johann Fischer <[email protected]>

The board hardware has no network interfaces, although the ECM/EEM class implementation can provide Ethernet-like functionality and export it to the host, this is no reason to default to a specific USB class implementation.We do not make this kind of configuration for other boards that have a USB device controller. Also, it may not be what a user expects when using these boards with networking and a USB stack. Signed-off-by: Johann Fischer <[email protected]>

jfischer-no added this to the v4.1.0 milestone Nov 12, 2024

jfischer-no requested review from henrikbrixandersen, tejlmand, carlescufi and nordicjm November 12, 2024 23:13

zephyrbot added platform: nRF Nordic nRFx platform: Ezurio platform: STM32 ST Micro STM32 labels Nov 12, 2024

zephyrbot requested review from anangl, erwango, FRASTM, gautierg-st, GeorgeCGV, jaz1-nordic, kl-cruz, magp-nordic, marwaiehm-st, masz-nordic and mathieuchopstm November 12, 2024 23:14

zephyrbot assigned rerickson1 Nov 12, 2024

rerickson1 previously approved these changes Nov 12, 2024

View reviewed changes

nordicjm reviewed Nov 13, 2024

View reviewed changes

erwango reviewed Nov 13, 2024

View reviewed changes

boards/st/sensortile_box_pro/Kconfig.defconfig Outdated Show resolved Hide resolved

erwango reviewed Nov 13, 2024

View reviewed changes

boards/st/sensortile_box_pro/Kconfig.defconfig Show resolved Hide resolved

jfischer-no dismissed rerickson1’s stale review via 70aa27a November 13, 2024 10:27

jfischer-no force-pushed the pr-boards-common-cdc_acm_serial-only branch 2 times, most recently from 70aa27a to 4414ed1 Compare November 13, 2024 10:34

jfischer-no mentioned this pull request Nov 13, 2024

boards: adafruit feather nrf52840 uf2 initalize USB at boot #80038

Open

nordicjm previously approved these changes Nov 13, 2024

View reviewed changes

carlescufi reviewed Nov 20, 2024

View reviewed changes

jfischer-no force-pushed the pr-boards-common-cdc_acm_serial-only branch from ab20804 to 78d913e Compare November 22, 2024 15:02

This was referenced Nov 22, 2024

boards: add common configuration for CDC ACM UART #81228

Closed

samples: usb: move legacy USB code to legacy directory and cleanup samples #80985

Draft

bryceschober reviewed Dec 4, 2024

View reviewed changes

jfischer-no force-pushed the pr-boards-common-cdc_acm_serial-only branch from 78d913e to 21bf925 Compare December 10, 2024 12:17

erwango mentioned this pull request Dec 12, 2024

drivers: usb: udc_stm32: Handle HAL callbacks on separate thread #80820

Open

tejlmand previously approved these changes Dec 12, 2024

View reviewed changes

jfischer-no force-pushed the pr-boards-common-cdc_acm_serial-only branch from 21bf925 to ae8123a Compare December 12, 2024 14:57

jfischer-no dismissed tejlmand’s stale review via 4c74867 December 12, 2024 22:34

jfischer-no force-pushed the pr-boards-common-cdc_acm_serial-only branch from ae8123a to 4c74867 Compare December 12, 2024 22:34

jfischer-no requested a review from tejlmand December 12, 2024 22:34

carlescufi requested review from bryceschober, jacobw and marwaiehm-st December 16, 2024 12:20

tejlmand previously approved these changes Dec 16, 2024

View reviewed changes

nordicjm reviewed Dec 16, 2024

View reviewed changes

boards/arduino/opta/arduino_opta_stm32h747xx_m7_defconfig Outdated Show resolved Hide resolved

boards/makerdiary/nrf52840_mdk_usb_dongle/Kconfig Outdated Show resolved Hide resolved

boards/nordic/thingy53/thingy53_nrf5340_cpuapp_ns.dts Show resolved Hide resolved

jfischer-no added 2 commits December 16, 2024 15:05

jfischer-no dismissed tejlmand’s stale review via b9b0789 December 16, 2024 14:06

jfischer-no force-pushed the pr-boards-common-cdc_acm_serial-only branch from 4c74867 to b9b0789 Compare December 16, 2024 14:06

jfischer-no requested a review from nordicjm December 16, 2024 14:10

nordicjm approved these changes Dec 16, 2024

View reviewed changes

nordicjm requested a review from tejlmand December 16, 2024 14:12

henrikbrixandersen approved these changes Dec 16, 2024

View reviewed changes

rerickson1 approved these changes Dec 16, 2024

View reviewed changes

kartben merged commit 2ddb576 into zephyrproject-rtos:main Dec 16, 2024
26 checks passed

jfischer-no deleted the pr-boards-common-cdc_acm_serial-only branch December 17, 2024 07:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boards: add common configuration for CDC ACM UART #81308

boards: add common configuration for CDC ACM UART #81308

jfischer-no commented Nov 12, 2024

rerickson1 left a comment

nordicjm Nov 13, 2024

jfischer-no Nov 13, 2024

nordicjm Nov 13, 2024

jfischer-no Nov 13, 2024

erwango Nov 13, 2024

jfischer-no Nov 13, 2024

erwango Nov 13, 2024

carlescufi left a comment

bryceschober Dec 4, 2024

tejlmand left a comment

carlescufi commented Dec 16, 2024

	- through common CDC ACM UART backend configuraiton for all boards
	- through common CDC ACM UART backend configuration for all boards

boards: add common configuration for CDC ACM UART #81308

boards: add common configuration for CDC ACM UART #81308

Conversation

jfischer-no commented Nov 12, 2024

rerickson1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

carlescufi commented Dec 16, 2024