boards: add common configuration for CDC ACM UART #81308
Conversation
jfischer-no
requested review from
henrikbrixandersen,
tejlmand,
carlescufi and
nordicjm
zephyrbot
added
platform: nRF
zephyrbot
requested review from
anangl,
erwango,
FRASTM,
gautierg-st,
GeorgeCGV,
jaz1-nordic,
kl-cruz,
magp-nordic,
marwaiehm-st,
masz-nordic and
mathieuchopstm
boards/common/cdc_acm_serial.dts
Outdated
| }; | ||
|
|
||
| zephyr_udc0: &usbd { | ||
| cdc_acm_uart: cdc_acm_uart { |
There was a problem hiding this comment.
should this not be cdc_acm_uart0 if it supports multiple instances?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
board_* prefix sounds good
There was a problem hiding this comment.
I also moved them to boards/common/usb/
| endif # LOG | ||
|
|
||
| endif # USB_DEVICE_STACK | ||
| source "${ZEPHYR_BASE}/boards/common/Kconfig.cdc_acm_serial" |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
${ZEPHYR_BASE} cannot be resolved in dt.
There was a problem hiding this comment.
Can we then use ../boards/common for Kconfig ?
jfischer-no
force-pushed
the
pr-boards-common-cdc_acm_serial-only
branch
2 times, most recently
from
70aa27a
to
4414ed1
Compare
| @@ -9,4 +9,6 @@ if BOARD_ADAFRUIT_FEATHER_NRF52840 | |||
| config BT_CTLR | |||
| default BT | |||
|
|
|||
| source "${ZEPHYR_BASE}/boards/common/usb/Kconfig.cdc_acm_serial" | |||
There was a problem hiding this comment.
This would enable CDC ACM on the 2 non UF2 variants of this board which currently don't have it enabled. Is that the intended outcome?
I struggled to deal with this when creating these variants. I've since realised perhaps using if BOARD_TARGET = would have been a better way of dealing with some of the variations. Perhaps that would work here. It would be great to be using common definitions.
There was a problem hiding this comment.
Since you have only selected this for the specific boards in Kconfig you don’t need the if here do you?
There was a problem hiding this comment.
Correct, it can now be removed. Thanks.
jfischer-no
force-pushed
the
pr-boards-common-cdc_acm_serial-only
branch
from
4414ed1
to
8e2ebe9
Compare
jfischer-no
dismissed stale reviews from rerickson1, marwaiehm-st, and erwango
via
3c7c271
jfischer-no
force-pushed
the
pr-boards-common-cdc_acm_serial-only
branch
2 times, most recently
from
3c7c271
to
1f36b38
Compare
| # Logger cannot use itself to log | ||
| CONFIG_USB_CDC_ACM_LOG_LEVEL_OFF=y |
There was a problem hiding this comment.
Thanks, removed.
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]>
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
force-pushed
the
pr-boards-common-cdc_acm_serial-only
branch
from
1f36b38
to
b9a3072
Compare
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).