- Notifications
You must be signed in to change notification settings - Fork1k
Fix external and internal usb pullup#1048
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix external and internal usb pullup#1048
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Works for me |
Uh oh!
There was an error while loading.Please reload this page.
These boards were broken in commite1d409f Refactor USB pulluphandling. Before that commit, all boards without external controllablepullups were assumed to have fixed external pullups and use the DP writetrick. Since that commit, boards that have internal pullups are assumedto *not* have any external pullups and the internal pullups areautomatically used.It turns out there exist some boards that have internal pullups in thechips, but also have an external fixed pullup. This can probably beconsidered a hardware bug, but since the boards exist, we should supportthem.This commit allows variants to define USBD_FIXED_PULLUP to explicitlystate they have a fixed pullup on the D+ line. This will cause the D+output trick to be applied even when internal pullups are present,fixing these boards.This define is only needed on these specific boards, but it can also bedefined on boards with a fixed external pullup without internal pullups.This problem was prompted by the "Black F407VE" board, which has theproblematic pullup. All other F4 boards were checked and one other wasfound to have the pullup, all others seem ok. None of the other serieshave been checked, so there might still be board broken.See alsoSTM32-base/STM32-base.github.io#26 forsome additional inventarisation of this problem.Thisfixesstm32duino#1029.
While reviewing some of these boards, it was not immediately clear to mewhat boards they referred to (especially with the relatively unbrandedboards from aliexpress). Just in case this helps anyone else, this addssome urls with more info on those boards I found from the git historyand github issues.
This board also has internal pullups, so the external one is notactually needed (and will even violate the USB spec when both are usedtogether). This commit disabled the external pullups, but leaves thedefines in comments, as future documentation.See alsostm32duino#1029.
91a2a02
tod3fdc1f
CompareHeh, I had actually thought Ifixed the indentation of that comment, didn't realize astyle had another opinion about it :-p Anyway, thanks for the fix, I've squashed it into my original commit. @stas2z Thanks for testing! |
Uh oh!
There was an error while loading.Please reload this page.
Tested on:
|
Thanks for the extensive testing! |
Welcome. |
@matthijskooijman on the CoreBoard F401RC the pullup is not permanent but driven by PD2 |
Yeah, that's what I already said (or at least tried to say :-P). Regardless, that pullup is still problematic when used together with the internal pullup, so this PR changes the code to not use the external pullup anymore. If you could confirm that USB still works on this board, that would be awesome! |
@matthijskooijman I don't want to impact my install too much. Is changing these files enough to test? |
Yeah, I think it should be. |
mrguen commentedApr 30, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@matthijskooijman I tried it succesfully. I hope the test is significant. Here's what I did:
|
Awesome, thanks! |
This fixes boards that have external and internal USB pullups, which were broken by#997, as reported in#1029.
This needs a new define in the variant for these broken boards. I added this for two boards for which I could confirm that they had an external pullup.@stas2z, you have a black-based board, could you test this PR on your board?
I found that the CoreBoard F401RC had a switchable external pullup, but also internal pullups, so I changed that one to no longer use the external pullup and just the internal pullup.@mrguen, I think you have one of these boards, could you maybe test this PR on that board?
I tested this on a custom STM32F401 board, where I added an external pullup for testing. I also tested on an unmodified Blue pill, which still works as expected.