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

Add a warning about duplicate USB pullup for STM32F4 boards #26

Merged

Conversation

matthijskooijman
Copy link
Contributor

This is suboptimal hardware design, which might need some special care
on the software side. This noticed in the Arduino_Core_STM32, see
stm32duino/Arduino_Core_STM32#1029

For these boards, a schematic is available to confirm the resistor:

  • STM32F407VET6-STM32-F4VE-V2.0
  • STM32F407ZGT6-VCC-GND-Large
  • STM32F407VET6-VCC-GND-Small

These boards have not schematic, but there is a 1.5kΩ resistor clearly
visible in the USB data path that is almost certainly a pullup:

  • STM32F401RCT6-STM32F-Core-Board
  • STM32F407VET6-Euse-M4-DEMO-Medium
  • STM32F407ZGT6-STM32F-Core-Board
  • STM32F407VGT6-SR-Board

All other F4 boards have no such resistor in the schematic or visible in
the images. Other series might also have this problem, but were not
checked.

@matthijskooijman
Copy link
Contributor Author

It might be nice to also specify whether external pullups are present on the USB pins (either fixed, as in these boards, but also switchable pullups controllable through an I/O pin such as on the Maple Mini), but I'm not sure how to specify this (and I won't be able to review all boards to check for pullups...).

@matthijskooijman
Copy link
Contributor Author

Hm, found out that STM32F401RCT6-STM32F-Core-Board actually has a transistor-switched pullup (controlled by PD2), based on https://github.com/stm32duino/Arduino_Core_STM32/blob/1eac7c56ab65bd73b79d4237c00121a767cbcb24/variants/Generic_F401Rx/variant.h#L128-L132 (and on second inspection, I can indeed see the transistor that I thought was a regulator). The same holds for the STM32F407ZGT6-STM32F-Core-Board, though I can't tell what pin controls the transistor.

I also doublechecked the others, but STM32F407VET6-Euse-M4-DEMO-Medium has the pullup clearly connected to a regulator and I can't find any transistor at all on the STM32F407VGT6-SR-Board so I'm fairly confident those do have a fixed pullup. I'll push an update removing the warning for the CoreBoards tomorrow.

@ThomasGravekamp
Copy link
Contributor

Thanks for creating this pull request.

...but I'm not sure how to specify this...

Yeah, I'm still working on getting more information in those pin definitions. Right now there's only one text field (to) that needs to be expanded into an object and can then contain more information like target (PA0, GND, VBAT, ...), via (a list of components), and pull (up/down with information on the resistor used). Another thing that I will have to work out is how it will be rendered.

Hm, found out that STM32F401RCT6-STM32F-Core-Board actually has a transistor-switched pullup (controlled by PD2)

Yup, I encountered it while looking into the jumpers on that board. I can take a look at the STM32F407ZGT6-STM32F-Core-Board to see which pin controls the transistor.

@matthijskooijman
Copy link
Contributor Author

Ok, did a force push with an updated. I also found a schematic for the Core Board at https://github.com/stm32duino/Arduino_Core_STM32/pull/604/files so I also added that.

Note that I haven't built the documentation to preview it, I couldn't quickly figure out how to do that.

Yeah, I'm still working on getting more information in those pin definitions. Right now there's only one text field (to) that needs to be expanded into an object and can then contain more information like target (PA0, GND, VBAT, ...), via (a list of components), and pull (up/down with information on the resistor used). Another thing that I will have to work out is how it will be rendered.

I guess that pullups and via info would indeed be nice in a separate field.

For controllable pullups, you would probably need to additionally record what pin controls it. Then it might make sense to (also) list the pullup as a device or output, rather than just as part of the USB connector.

Yup, I encountered it while looking into the jumpers on that board. I can take a look at the STM32F407ZGT6-STM32F-Core-Board to see which pin controls the transistor.

I'm not particularly interested in that info (I've already drifted a few steps away from my original work), but if you figure out how to store it in the boards list, it would be nice to add this pin number as well.

matthijskooijman added a commit to matthijskooijman/Arduino_Core_STM32 that referenced this pull request Apr 28, 2020
These boards were broken in commit e1d409f Refactor USB pullup
handling. Before that commit, all boards without external controllable
pullups were assumed to have fixed external pullups and use the DP write
trick. Since that commit, boards that have internal pullups are assumed
to *not* have any external pullups and the internal pullups are
automatically used.

It turns out there exist some boards that have internal pullups in the
chips, but also have an external fixed pullup. This can probably be
considered a hardware bug, but since the boards exist, we should support
them.

This commit allows variants to define USBD_FIXED_PULLUP to explicitly
state 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 be
defined on boards with a fixed external pullup without internal pullups.

This problem was prompted by the "Black F407VE" board, which has the
problematic pullup. All other F4 boards were checked and one other was
found to have the pullup, all others seem ok. None of the other series
have been checked, so there might still be board broken.

See also STM32-base/STM32-base.github.io#26 for
some additional inventarisation of this problem.

This fixes stm32duino#1029.
matthijskooijman added a commit to matthijskooijman/Arduino_Core_STM32 that referenced this pull request Apr 28, 2020
These boards were broken in commit e1d409f Refactor USB pullup
handling. Before that commit, all boards without external controllable
pullups were assumed to have fixed external pullups and use the DP write
trick. Since that commit, boards that have internal pullups are assumed
to *not* have any external pullups and the internal pullups are
automatically used.

It turns out there exist some boards that have internal pullups in the
chips, but also have an external fixed pullup. This can probably be
considered a hardware bug, but since the boards exist, we should support
them.

This commit allows variants to define USBD_FIXED_PULLUP to explicitly
state 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 be
defined on boards with a fixed external pullup without internal pullups.

This problem was prompted by the "Black F407VE" board, which has the
problematic pullup. All other F4 boards were checked and one other was
found to have the pullup, all others seem ok. None of the other series
have been checked, so there might still be board broken.

See also STM32-base/STM32-base.github.io#26 for
some additional inventarisation of this problem.

This fixes stm32duino#1029.
@@ -38,7 +38,8 @@
"mounting": "None"
},
"remarks": [
{ "type": "warning", "content": "The +5V pins on this board are directly connected to the +5V pin of the USB connector. There is no protection in place. Do not power this board through USB and an external power supply at the same time." }
{ "type": "warning", "content": "The +5V pins on this board are directly connected to the +5V pin of the USB connector. There is no protection in place. Do not power this board through USB and an external power supply at the same time." },
{ "type": "warning", "content": "This board has an external switchable (disabled by default) USB pullup (R11) which is not actually needed (since automatic internal pullups are present) and might actually be harmful (when both pullups are enabled, the USB specification is violated)." }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicking, but a little bit clearer warning with shorter sentences and without many of the parentheses:

"The microcontroller on this board features internal pull-up resistors for the USB data lines. However, this board has an additional switchable pull-up resistor (R11), which is disabled by default. This resistor is not needed and violates the USB specification when used with the internal pull-up resistors. This may cause errors while using USB on this board."

And for the non-switchable variant:

"The microcontroller on this board features internal pull-up resistors for the USB data lines. However, this board has an additional pull-up resistor (R...). This resistors is not needed and violates the USB specification when used with the internal pull-up resistors. This may cause errors while using USB on this board."

@ThomasGravekamp
Copy link
Contributor

Ok, did a force push with an updated. I also found a schematic for the Core Board at https://github.com/stm32duino/Arduino_Core_STM32/pull/604/files so I also added that.

Thanks, I wasn't able to find one with a quick search.

Note that I haven't built the documentation to preview it, I couldn't quickly figure out how to do that.

Looks like you've produced valid JSON. Should be fine.

For controllable pullups, you would probably need to additionally record what pin controls it. Then it might make sense to (also) list the pullup as a device or output, rather than just as part of the USB connector.

I like this idea, I'll look into this somewhere in the future.

I'm not particularly interested in that info (I've already drifted a few steps away from my original work), but if you figure out how to store it in the boards list, it would be nice to add this pin number as well.

I am, as in, if I can make the information more complete, I will. It is connected to pin PA8. For now the notes in the remarks section will suffice. I can add these notes later.

I've left one comment on the message itself, let me know what you think of it.

This is suboptimal hardware design, which might need some special care
on the software side. This noticed in the Arduino_Core_STM32, see
stm32duino/Arduino_Core_STM32#1029

For these boards, a schematic is available to confirm the resistor:
 - STM32F407VET6-STM32-F4VE-V2.0
 - STM32F407ZGT6-VCC-GND-Large
 - STM32F407VET6-VCC-GND-Small

These boards have not schematic, but there is a 1.5kΩ resistor clearly
visible in the USB data path that is almost certainly a fixed pullup:
 - STM32F407VET6-Euse-M4-DEMO-Medium
 - STM32F407VGT6-SR-Board

These boards have a pullup, but it is switched by a transistor. Since
there are also internal pullups, a warning is still in order, but using
slightly different wording:
 - STM32F401RCT6-STM32F-Core-Board
 - STM32F407ZGT6-STM32F-Core-Board

All other F4 boards have no such resistor in the schematic or visible in
the images. Other series might also have this problem, but were not
checked.
These were taken from
stm32duino/Arduino_Core_STM32#604. It is not
entirely clear where they originally came from (there is an aliexpress
link, which has an image with the dimensions but no PDF and no
schematics).
@matthijskooijman
Copy link
Contributor Author

I've left one comment on the message itself, let me know what you think of it.

Better wording indeed. I've copied it and made a few more changes:

  • Fixed a typo (resistors -> resistor)
  • Added "on D+" to make it even more explicit where the pullup is located.
  • Added "controlled by Pxx" for the switchable pullups, to at least document this pin somewhere for now.
  • Improved the wording related to "used with the internal pull-up resistors" to be more explicit.

I've force pushed the new version.

@ThomasGravekamp
Copy link
Contributor

Alright, looks good. I will merge these changes. Thanks for your contribution!

@ThomasGravekamp ThomasGravekamp merged commit 108bdb9 into STM32-base:master Apr 29, 2020
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.

None yet

2 participants