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

Fix to not incorrectly include TINYUSB on nrf52832 targets #7

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

Conversation

geeksville
Copy link

Rather than checking the name of the board check the name of the chip
which allows this code to work properly on any nrf52832. This processor
does not include a USB controller. TinyUSB can't build properly on this arch.

Rather than checking the name of the board check the name of the chip
which allows this code to work properly on any nrf52832.  This processor
does not include a USB controller.
@geeksville geeksville marked this pull request as draft July 10, 2020 18:21
@geeksville
Copy link
Author

Seems correct on my system, but I'll wait till I'm done with bringup to change this PR into not a draft.

@ivankravets ivankravets requested a review from valeros July 10, 2020 18:22
@valeros
Copy link
Member

valeros commented Jul 13, 2020

It might be a good idea to add TinyUSB only when mcu is one of nRF52833, nRF52840.

@geeksville
Copy link
Author

@valeros Instead of this change which is just doing the opposite - not adding it on a nrf52832? I'd bet the odds of USB being included on future chips is higher now that they have the IP.

@geeksville geeksville marked this pull request as ready for review July 13, 2020 14:16
@valeros
Copy link
Member

valeros commented Jul 13, 2020

Besides future chips, there are a few old ones like nRF52805, nRF52810, nRF52811, nRF52820 which don't have the USB controller but might be added to the framework. I'm simply suggesting to include TinyUSB only on the officially supported devices specified in https://github.com/hathach/tinyusb#supported-mcus

@geeksville
Copy link
Author

geeksville commented Jul 13, 2020 via email

@geeksville
Copy link
Author

btw - I updated my branch. Do you want this PR?

@geeksville
Copy link
Author

I'm not sure if this project is still being maintained, but in case it is: I just updated my PR with the changes needed to correctly build the latest arduinonrf52 master on nrf52840 cpus (there is a new crypto lib dependency). I'm not sure if my fix is really the ideal fix, but I'm adding it in case it is useful for others.

@sachaw
Copy link

sachaw commented Mar 11, 2022

@valeros Could we please get this merged, we're trying to minimize the amount of out-of-sync forks we have.

I believe all the requested fixes have been performed.

Thanks.

@cujomalainey
Copy link

Just hit this as well, would appreciate a merge

@maxxafari
Copy link

Is this issue solved elsewhere?

@x3e
Copy link

x3e commented Jan 22, 2024

Seems like this is still needed. A merge would be nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants