-
Notifications
You must be signed in to change notification settings - Fork 2k
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
boards/*nrf52*: Enable tinyusb support for all nrf52-based boards #20774
base: master
Are you sure you want to change the base?
Conversation
Did you tried |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works, we're all good :)
FWIW, this seems to be a (small) subset of #18998 |
boards/*nrf52*: Enable tinyusb support for all nrf52-based boards
Thanks CI :) good catch: apparently not all nRF52 boards have the necessary USBD peripheral, see https://docs.nordicsemi.com/bundle/struct_nrf52/page/struct/nrf52.html |
With the last commit, I've moved Disclaimer: I'm not sure whether this is the right way to model this in RIOT, please have a closer look. |
cpu/nrf52/Makefile.features
Outdated
# The USBD peripheral is not available on all SoCs | ||
ifneq (,$(filter nrf52820xxaa nrf52833xxaa nrf52840xxaa,$(CPU_MODEL))) | ||
FEATURES_PROVIDED += periph_usbdev | ||
# the nrf usb stack is supported by tinyUSB | ||
FEATURES_PROVIDED += tinyusb_device | ||
endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have an issue here.
periph_usbdev
is designed to be used by USBUS
only.
So you must not provide this feature alongside with tinyusb_device
.
tinyUSB
provides its own low level peripheral driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we can try to summon @gschorcht for its guidance here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah really, I understood it as "the CPU has a usb peripheral", which is also what is documented here: https://doc.riot-os.org/md_doc_2doxygen_2src_2feature__list.html#autotoc_md2318
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway the boards before also exposed periph_usbdev
and using tinyusb was perfectly possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, what @gschorcht wanted to do was:
use USBUS by default for highlevel_stdio
but, if for whatever reason, tinyUSB
is used by user switch the USB stack to tinyUSB and thus use stdio_tinyusb_cdc_acm
.
In the end, the board can provide periph_usbdev
but if tinyUSB
is used, USBUS
must be discarded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, is it really a good idea to define the USB at CPU level ?
I think it's done at board level currently because a board doesn't always expose USB pins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can confirm this with make BOARD=feather-nrf52840-sense -C examples/hello-world info-modules
which still includes usbus
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, is it really a good idea to define the USB at CPU level ? I think it's done at board level currently because a board doesn't always expose USB pins.
That is indeed a good point.
So maybe we rather want the following: each board defines periph_usb
if present, and the cpu can check for periph_usbdev
and then additionally set tinyusb_device
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So maybe we rather want the following: each board defines periph_usb if present, and the cpu can check for periph_usbdev and then additionally set tinyusb_device?
Let's try this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the new commit (sorry for the force-push).
Co-authored-by: Dylan Laduranty <[email protected]>
Hum :/
|
Maybe you could tried to check |
But that's not what I want: I want to express that |
Contribution description
TinyUSB support is given for all nrf52-based boards, but only explicitly enabled for some selected ones. This PR enables support for all such boards.
I also noticed that
usb_board_reset
would currently only be enabled with theusbus
stack, fixed that as well.Testing procedure
make BOARD=feather-nrf52840-sense -C tests/pkg/tinyusb_cdc_acm_stdio flash term
or any other nrf52-based board. Without this PR, only works for selected boards (e.g.,nrf52840dk
). Flash again to test theusb_board_reset
feature.Issues/PRs references
Hinted to by @dylad on Matrix