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

shields: add generic shield to use CDC ACM UART as backend #40645

Closed

Conversation

jfischer-no
Copy link
Collaborator

@jfischer-no jfischer-no commented Nov 24, 2021

Add generic shields that provide devicetree and configuration
overlays, and configure USB device stack so that CDC ACM UART
can be used as backend for console, logging, or shell.
This shield is not for a real hardware device but for an emulated UART controller,
analogous to how we have emulated boards.
This approach allows us to avoid many identical overlays in
samples and tests directories, and cumbersome board configurations.

These shields use Kconfig option CONFIG_USB_DEVICE_INITIALIZE_AT_BOOT
and is mainly intended to be used with boards like
nrf52840dongle_nrf52840 or bl654_usb.

Draft for discussion (derived from #40220).

@nordicjm
Copy link
Collaborator

Hi @jfischer-no I'm thankful for this work but I have a big problem with this being "manually opt in by specifying a shield" as per

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

This is just a non-starter for our customers, they need an easy way of getting zephyr and being able to test things as they do with any other board, not have to jump through hoops trying to get the most basic sample applications working. For our BL654 USB dongle it's basically a given that almost everyone will be using the CDC port either for Bluetooth or as a general UART and this should cater for those majority of users.

@jfischer-no
Copy link
Collaborator Author

This is just a non-starter for our customers, they need an easy way of getting zephyr and being able to test things as they do with any other board, not have to jump through hoops trying to get the most basic sample applications working. For our BL654 USB dongle it's basically a given that almost everyone will be using the CDC port either for Bluetooth or as a general UART and this should cater for those majority of users.

I cannot take this seriously.

  1. BL654_USB, https://docs.zephyrproject.org/latest/boards/arm/bl654_usb/doc/bl654_usb.html, looks like an end product, it is not a development board, even if it would (like nrf52840dongle) it is still very limited in terms of capabilities.
  2. Zephyr's main tree is not a BSP for BL654_USB, you have to compromise and understand that not everything here is about your board.
  3. It is obvious that we cannot enable USB device support by default at boot time. And there will always be a conflict between configuration at board level and configuration in the samples. So we need additional option for the boards like BL645_USB if it should have best possible coverage of the samples. There is no way around something like: west build -b nrf52840dongle_nrf52840 samplel -- -DADDITIONAL_OPTION=y, or a board variant like bl654_usb_usb_on, which is not better.

Taking (1) and (2) into account, what are most basic applications for BL654_USB dongle? Probably none. But useful applications, such as hci_usb or ot-coprocessor, work fine even with minimal board configuration, https://docs.zephyrproject.org/latest/guides/porting/board_porting.html#general-recommendations.
Finally, considering the steps required to get the firmware onto the board, is the additional option (3), like -DSHIELD=cdc_acm_shell, to build the firmware is the least evil of all.

@Willmish
Copy link

This PR addresses a problem which one way or the other has to be solved, and this seems like a rather elegant solution, getting rid of the need to create overlays for each new program/tailored configs for each board enabling usb at boot.

I myself am working on a port of Wio Terminal board, which doesn't have a native UART, so needs to use CDC ACM to open a console accessible over USB.

@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Dec 3, 2021

This shield is not for a real hardware device but for an emulated UART controller,
analogous to how we have emulated boards.

I remain opposed to the idea of using SHIELD for this.

@jfischer-no jfischer-no added the dev-review To be discussed in dev-review meeting label Dec 9, 2021
@galak
Copy link
Collaborator

galak commented Dec 16, 2021

Dev-review (Dec 16, 2021):

@tejlmand
Copy link
Collaborator

Dev-review (Dec 16, 2021):

@galak if I recall correctly, it was also desired at the dev-review that this commit 9294f23 was taken into a dedicated PR, cause the ability to initialize USB device at boot time is a useful feature regardless of the opposition to use shield mechanism for cdc acm uart.

@tejlmand
Copy link
Collaborator

see also this comment: #40669 (comment)

believe the variant approach that has been discussed time back on slack should be continued.

@MaureenHelm MaureenHelm removed the dev-review To be discussed in dev-review meeting label Mar 3, 2022
@github-actions github-actions bot added the Stale label May 3, 2022
@jfischer-no jfischer-no removed the Stale label May 3, 2022
@github-actions github-actions bot added the Stale label Jul 3, 2022
Add  generic shields that provide devicetree and configuration
overlays, and configure USB device stack so that CDC ACM UART
can be used as backend for console, logging, or shell.
This approach allows us to avoid many identical overlays in
samples and tests directories, and cumbersome board configurations.

These shields use Kconfig option CONFIG_USB_DEVICE_INITIALIZE_AT_BOOT
and is mainly intended to be used with boards like
nrf52840dongle_nrf52840 or bl654_usb.

Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Remove CDC ACM UART devicetree and config overlay, and explicit
USB support initialization in favor of generic CDC ACM shield.
For shell support via USB CDC ACM, this example is to be built
as follows:

west build -b nrf52840dongle_nrf52840
  samples/subsys/shell/shell_module -- -DSHIELD=cdc_acm_shell

Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Remove CDC ACM UART devicetree and config overlay, and explicit
USB support initialization in favor of generic CDC ACM shield.
For shell support via USB CDC ACM, this example is to be built
as follows:

west build -b nrf52840dongle_nrf52840
  tests/bluetooth/shell -- -DSHIELD=cdc_acm_shell

Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Remove CDC ACM UART devicetree and config overlay, and explicit
USB support initialization in favor of generic CDC ACM shield.
For console support via USB CDC ACM, this example is to be built
as follows:

west build -b reel_board
  tests/drivers/uart/uart_basic_api -- -DSHIELD=cdc_acm_console

Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Remove CDC ACM UART devicetree overlay, and explicit
USB support initialization in favor of generic CDC ACM shield.
For console support via USB CDC ACM, this example is to be built
as follows:

west build -b sensortile_box
  samples/boards/sensortile_box -- -DSHIELD=cdc_acm_console

Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Remove CDC ACM UART devicetree nodes, chosen properties and explicit
USB support configuration in favor of generic CDC ACM shield.
For console support via USB CDC ACM, in-tree samples for this boards
is to be built as follows:

west build -b nrf52840dongle_nrf52840
  samples/basic/threads -- -DSHIELD=cdc_acm_console

For shell support via USB CDC ACM, in-tree samples for this boards
is to be built as follows:

west build -b nrf52840dongle_nrf52840
  samples/subsys/shell/shell_module -- -DSHIELD=cdc_acm_shell

Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jun 18, 2023
@github-actions github-actions bot closed this Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Boards area: Devicetree area: Documentation area: Samples Samples area: Shields Shields (add-on boards) area: Tests Issues related to a particular existing or missing test area: USB Universal Serial Bus Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants