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

boards/*nrf52*: Enable tinyusb support for all nrf52-based boards #20774

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mguetschow
Copy link
Contributor

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 the usbus 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 the usb_board_reset feature.

Issues/PRs references

Hinted to by @dylad on Matrix

@mguetschow mguetschow requested a review from aabadie as a code owner July 4, 2024 08:45
@github-actions github-actions bot added Area: build system Area: Build system Area: boards Area: Board ports labels Jul 4, 2024
@mguetschow mguetschow changed the title Enable tinyusb support for all nrf52-based boards boards/*nrf52*: Enable tinyusb support for all nrf52-based boards Jul 4, 2024
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 4, 2024
@dylad
Copy link
Member

dylad commented Jul 4, 2024

Did you tried usb_board_reset alongside with stdio_tinyusb_cdc_acm ? Does it work well ?

@riot-ci
Copy link

riot-ci commented Jul 4, 2024

Murdock results

✔️ PASSED

a715fce Update cpu/nrf52/Makefile.features

Success Failures Total Runtime
10177 0 10178 12m:48s

Artifacts

Copy link
Member

@dylad dylad left a 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 :)

@mguetschow
Copy link
Contributor Author

FWIW, this seems to be a (small) subset of #18998

@maribu maribu added this pull request to the merge queue Jul 4, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 4, 2024
boards/*nrf52*: Enable tinyusb support for all nrf52-based boards
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 4, 2024
@mguetschow
Copy link
Contributor Author

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

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Jul 4, 2024
@mguetschow mguetschow requested review from dylad and maribu July 4, 2024 11:14
@mguetschow
Copy link
Contributor Author

With the last commit, I've moved FEATURES_PROVIDED += periph_usbdev and FEATURES_PROVIDED += tinyusb_dev to be CPU instead of board dependant. That should enable tinyusb for all nrf52-based boards that actually have the USBD peripheral.

Disclaimer: I'm not sure whether this is the right way to model this in RIOT, please have a closer look.

Comment on lines 15 to 20
# 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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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).

cpu/nrf52/Makefile.features Outdated Show resolved Hide resolved
Co-authored-by: Dylan Laduranty <dylan.laduranty@mesotic.com>
@mguetschow
Copy link
Contributor Author

Hum :/

Modules should not check the content of FEATURES_PROVIDED/REQUIRED/OPTIONAL:
	cpu/nrf52/Makefile.features:17:ifneq (,$(filter periph_usbdev,$(FEATURES_PROVIDED)))

@dylad
Copy link
Member

dylad commented Jul 4, 2024

Maybe you could tried to check periph_usbdev against USEMODULE or FEATURES_USED instead ?

@mguetschow
Copy link
Contributor Author

But that's not what I want: I want to express that tinyusb_device is supported iff periph_usbdev is on any nrf52 boards, not that the former is supported only if I actually want to use the latter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants