Refactor USB pullup handling

[matthijskooijman]

Previously, variants could define USB_DISC_PIN when they had an USB
attach pullup that was disabled by default and needed a pin to
be written LOW to enable it. Other hardware configurations could only
overwrite the USBD_reenumerate function, like the M200 board did.

This commit makes the pullup configuration more flexible. By defining
the appropriate macros, enabled-by-default and disabled-by-default
pullups are both supported. The output level of the pin can also be
configured. Finally, for disabled-by-default, the output mode for the
enabled state can also be configured (INPUT to leave the pin floating
and let any external transistor gate pullup enable the USB pullup, or
OUTPUT to keep the pin as OUTPUT an actively drive the transistor gate).

The new macros also take PinName constants rather than pin numbers. This
allows merging the code that controls an external pullup with the code
that writes directly to the USB DP pin to fake a disabled pullup (this
code only knows the pin name of the DP pin, not the number).

Finally, for CPUs that have internal USB pullup (as indicated by the
presence of a SDIS config bit), the write-to-DP-trick is now not
performed (since the pullup is automatically managed by the USB hardware
already).

This fixes #885, also see that issue for discussion leading up to this
change.

[matthijskooijman]

I've only done limited testing on this PR so far. I only have access to an F4 board with native USB, which has internal pullups and works well. I did do compile testing for a few other boards.

I've ordered a blue pill and black pill to test, but it would be good to also test on a maple mini board (since that has external pullup) and the MALYAN M200 V1 (since that used to have special pullup handling). @xC0000005 maybe you could do the latter?

(I also did a few force pushes to fix style errors, should be good now).

[xC0000005]

I'l test on V1 and V2 (pullup and no pullup) tonight.

…

On Thu, Mar 19, 2020 at 10:19 AM Matthijs Kooijman ***@***.***> wrote: I've only done limited testing on this PR so far. I only have access to an F4 board with native USB, which has internal pullups and works well. I've ordered a blue pill and black pill to test, but it would be good to also test on a maple mini board (since that has external pullup) and the MALYAN M200 V1 (since that used to have special pullup handling). @xC0000005 <https://github.com/xC0000005> maybe you could do the latter? (I also did a few force pushes to fix style errors, should be good now). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#997 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHVGS4LDLDPX5TNXILY6WO3RIJHZFANCNFSM4LPN23MQ> .

[fpistm]

And I will test on Maple. 😉

[matthijskooijman]

Oh, remove the _INPUT, that should come from here: https://github.com/stm32duino/Arduino_Core_STM32/pull/997/files#diff-d707080ac76ca2646a4a2457bcb86ad9R44

[xC0000005]

I'm not entirely sure how this works, but shouldn't one of these be low and the other high?

[matthijskooijman]

What do you mean? The PINNAME indicates the pin to control the pullup, and the LEVEL indicates the level it needs to have to ATTACH. What should be HIGH here? Also see the comments in variants/board_template/variant.h.

[matthijskooijman]

Sorry for the short comment earlier, I was just on my way out.

Turns out with my earlier compile-testing, I had forgotten to enable USB (for our custom board we have it enabled by default, didn't realize that the default boards did not). I did that now and found a dozen other preprocessor typos, I messed up the _PIN to _PINNAME rename and some other stuff. I should really have tested this more thoroughly, sorry.

Anyway, I pushed a fixup commit that at least compiles properly for the bluepill, maple mini and M200, so that should make testing easier.

Thanks!

[xC0000005]

I pulled the most current code and USB is working for both M200V1 and V2 (103 and 070).

…

On Mar 19, 2020, at 3:00 PM, Matthijs Kooijman ***@***.***> wrote: Sorry for the short comment earlier, I was just on my way out. Turns out with my earlier compile-testing, I had forgotten to enable USB (for our custom board we have it enabled by default, didn't realize that the default boards did not). I did that now and found a dozen other preprocessor typos, I messed up the _PIN to _PINNAME rename and some other stuff. I should really have tested this more thoroughly, sorry. Anyway, I pushed a fixup commit that at least compiles properly for the bluepill, maple mini and M200, so that should make testing easier. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#997 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHVGS4KH5CD5GUFNBPNKU33RIKIW7ANCNFSM4LPN23MQ>.

[matthijskooijman]

Thanks for testing :-)

I've done another push, to squash the fixup commit I pushed yesterday (only changes history, not the actual code).

[ABOSTM]

Hi @matthijskooijman
Thanks for this PR.
There is a remaining definition of USB_DISC_PIN in variant: Generic_F401Rx
Even if you handled compatibility with this definition (That is smart) it is better to use new implementation.

[fpistm]

Tested with a Maple mini and works as expected.

[matthijskooijman]

Thanks for the testing and reviews. I've also tested on a bluepill, which works as expected too.

There is a remaining definition of USB_DISC_PIN in variant: Generic_F401Rx

Ah, I started working on top of an older branch where that variant did not exist yet. I'll convert it.

The new macros also take PinName constants rather than pin numbers. This
allows merging the code that controls an external pullup with the code
that writes directly to the USB DP pin to fake a disabled pullup (this
code only knows the pin name of the DP pin, not the number).

I thought a bit more on this, but I think it might be nicer to specify pin numbers rather than pin names in the variant, which is more consistent and thus also less likely to cause mistakes. In terms of code that just means the PIN constants must be redefined as PINNAME constants in usbd_if.c, which should just be a few more lines but nothing spectacular (and just one extra lookup at runtime, but that's ok).

[matthijskooijman]

Ok, I pretty much rewrote the entire thing. In a nutshelll:

• All three hardware configurations (builtin pullups, external fixed pullups and external controllable pullups) are now handled in the same way: Detach, wait, attach.
• A variant-specified control pin is now always push-pull, the "open-collector" treatment is applied only for the D+ output trick.
• Variants can now just configure pin numbers instead of pin names.

I tested this on an F3 with internal pullups and on a bluepill. Testing on the MALYAN printers and a maple mini again would be appreciated.

For some more details about what I did and tried, read on.

Nitpicking: those sanity check should be under switch : #ifndef USBD_REENUM_DISABLED

I now moved all of the code under this check, with just an empty USBD_reenumerate() function in the else.

Internal pullups

I found that the internal pullups can be controlled portably using the USB_DevConnect and USB_DevDisconnect LL HAL functions, which the code now does.

Looking through the HAL code I found that the SDIS bit is used only by "USB_OTG_FS" and "USB_OTG_HS" peripherals, apparently some "USB" peripherals have an internal pullup controlled by the "USB_BCDR_DPPU" bit, so I adapted the code to check for both. I used git grep -A 10 USB_DevConnect.*\)$ to check that no other bits are used for pullup control.

Even more, I found that all USB_OTG_HS and USB_OTG_FS peripherals have the SDIS bit, so there is really no need to have code to figure out the D+ trick for them (so I removed that code). I confirmed this observation using a check script I wrote today which compiles a short header file against all known STM32 cpus (see the script for info). I let the script compile this, which worked for all boards:

#include <stm32_def.h>

#if (defined(USB_OTG_FS) || defined(USB_OTG_HS)) && !defined(USB_OTG_DCTL_SDIS)
#error "Foo"
#endif

I have not been able to test the DPPU bit, in particular I'm not sure how it behaves on startup when the USB peripheral is not enabled yet (I think it will just be a no-op, but it might end up enabling the USB pullup earlier than normal, though that should not be a problem either. I see that the STM32F070x6/B has this bit, which includes the MALYAN M200 V2, so that provides a nice opportunity to test this (also, it explains why these V2 boards have no external pullup, since it is included in the CPU now).

I also considered the option of overriding the USB_DevConnect and USB_DevDisconnect functions with our custom code to control the external pullup (or apply the D+ trick), since they are always called from the PCD code (even on chips that do not have internal pullups). However, the dummy definitions of these functions on chips without internal pullups are not defined weak, so we cannot actually override them.

Re-enumerating while connected

The code as it is now, should be usable to re-enumerate while running (instead of just at reset) too. I don't think we should actively advertise this, but it is interesting to know if it would work.

This works well with the F301RE with SDIS support I have, so it seems the USB stack handles external re-attachment fine. There did seem to be a small issue, possibly with buffering while disconnected (I got some duplicate serial output when the port was reconnected), but I did not investigate this.

I also tried on a bluepill, which has the D+ output trick. Switching to OUTPUT and back to INPUT would of course not work, since then the USB peripheral would lose control of the pin. So I tried using the USB pinmap to switch the pin back to USB AF mode.

However, detaching by switching to OUTPUT did not even work on F103C8, since it seems that the USB peripheral (when enabled) forcibly claims the DP pin, regardless of the AF settings. Even when I switch it to output and leave it at that, USB serial keeps working normally. Even more, the pin map in the variant does not even bother to set the pins into AF mode and just leaves them in input (verified using debugger, so there is no bit of code elsewhere that does initialize to AF). I could not find any reference to this in the documentation, and just one mention of it online, but that thread quickly focuses on getting things to work on hardware that does not seem to misbehave in this way. Since I could not test this properly, and the code was a bit fragile anyway (it would put pins in USB mode during startup, way before the USB peripheral was initialized), I reverted this, the pin mode is now reverted back to INPUT and intended to work at reset time only.

I have no board with DPPU support to test with, though. In case anyone wants to test, here is the sketch I used:

void setup() {
  // put your setup code here, to run once:
  Serial.begin(115200);
  while (!Serial);
}

unsigned i = 0;

extern "C" void USBD_reenumerate(void);

void loop() {
  // put your main code here, to run repeatedly:
  Serial.println(i++);
  delay(1000);
  if (i % 10 == 0) {
      USBD_reenumerate();
  }
}

[ABOSTM]

LGTM great job !!
Minor typo in comment

[ABOSTM]

I tested your sketch with PNUCLEO_WB55RG which supports DPPU , it is working !!

[fpistm]

Testing on the MALYAN printers and a maple mini again would be appreciated.

@matthijskooijman I've tested the maple and it works as expected with your sketch.

@xC0000005 did you made the test on Mylyan ?

[xC0000005]

I’ve just tested M200 V1, V2, and M300. USB works on all of them with this version.

…

On Mar 25, 2020, at 3:42 AM, Frederic Pillon ***@***.***> wrote: Testing on the MALYAN printers and a maple mini again would be appreciated. @matthijskooijman <https://github.com/matthijskooijman> I've tested the maple and it works as expected with your sketch. @xC0000005 <https://github.com/xC0000005> did you made the test on Mylyan ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#997 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHVGS4KLLX6ASDC5TTJONZTRJHNXXANCNFSM4LPN23MQ>.

[fpistm]

LGTM.
Wait typo fix raised by @ABOSTM before merge.

[matthijskooijman]

W00ps, forgot about the typo. Force pushed a version just fixing the typo, should be good to merge now.

[ABOSTM]

Thanks @matthijskooijman
LGTM