probably USBD_reenumerate have issue after update

[stas2z]

after updating local repo to the latest changes found my f405ve custom board (no special detaching/attaching pins, generic usb schematic like black f407) USB CDC stopped working properly

reverting e1d409f fixes it

i had no time to investigate it, but seems like reenumerate is not working properly after NVIC_SystemReset but i faced it a few times after detaching/attaching usb cable also
ive only tried to make CDC_deInit/CDC_Init to check if it come back to life, but it doesn't

@matthijskooijman
can you check it?

if it will be ok from your side, i will try to find a time to sit with debugger to investigate

[fpistm]

Hi @stas2z
I will test on F407.
I guess you have this message during build?

Arduino_Core_STM32/cores/arduino/stm32/usb/usbd_if.c

Line 147 in 088081d

#warning "No USB attach/detach method, USB might not be reliable through system resets"

[stas2z]

Hi @stas2z
I will test on F407.
I guess you have this message during build?

Arduino_Core_STM32/cores/arduino/stm32/usb/usbd_if.c

Line 147 in 088081d

#warning "No USB attach/detach method, USB might not be reliable through system resets"

oh, haven't seen it, project im working on atm have alot of warnings, cuz i always compile with -Wall)) so it's possible i missed it)
will check it a bit later

[fpistm]

No worries 😉
Probably in that case we should do also a toggling like before.

[matthijskooijman]

The F407 has built-in USB pullups and an SDIS bit, so the commit will have changed behaviour to no longer apply the force-DP-low-trick, since that should not be needed anymore.

However, it seems the Black F407 actually has an external fixed pullup:

@stas2z, can you confirm that you have the same on your board?

It's a bit weird to see a board like this, which actually reduces flexibility by fixing the pullup externally (and might actually also violate the USB spec since the two pullups combine to an effective 750Ω rather than the specified 1.5k). @stas2z if possible, you might want to consider removing the external pullup.

So I guess for boards like this, we need to apply the force low trick even when they have internal pullups. This needs some kind of define in the variant. We might also need to actually disable the internal pullup entirely (by keeping SDIS set, I think) to keep the pullup value proper, but I'm not sure how reliable that will be.

[stas2z]

yes, my board and black f407 have this pullup

[fpistm]

I've made some experiment with my Black F407.
By default, the COM port is not recognized but if we plug/unplug the USB cable it works.

Adding this in variant.h allows to get it working without plug/unplug. Anyway as @matthijskooijman mentioned this is not reliable, the main issue is that the hardware is not correct.
Note, that I did not handle SDIS part.

I will try with R21 removed.

[matthijskooijman]

Adding this in variant.h allows to get it working without plug/unplug. Anyway as @matthijskooijman mentioned this is not reliable, the main issue is that the hardware is not correct.

Do note that this unreliability was already present in the original code before my refactor. So we're not losing much by reintroducing this. We could add a big warning to the define in the variant file to encourage people to fix their hardware rather than enable the define, but I guess that's not always feasible.

[stas2z]

@matthijskooijman
don't add warnings, most ppl got finished boards and not be able to fix their hardware, it will be an annoying and usless stuff only

[matthijskooijman]

don't add warnings, most ppl got finished boards and not be able to fix their hardware, it will be an annoying and usless stuff only

Sorry, I meant a warning in a comment in the template variant.h, to be looked at by anyone adding a variant. Indeed, compile-time warnings that are unfixable are only noise :-)

[fpistm]

I guess the best would be to add needed definition to all impacted variants (for now Black F407, don't know it there are other impacted).
Anyway, as I said I've only added :

#define USBD_ATTACH_PIN         PA12 //USB_OTG_FS_DP
#define USBD_ATTACH_LEVEL       HIGH

which leads to do this:

  /* Detach */
  pin_function(USBD_PULLUP_CONTROL_PINNAME, STM_PIN_DATA(STM_MODE_OUTPUT_PP, GPIO_NOPULL, 0));
  digitalWriteFast(USBD_PULLUP_CONTROL_PINNAME, USBD_DETACH_LEVEL);

  /* Wait */
  delay(USBD_ENUM_DELAY);

  /* Attach */
  digitalWriteFast(USBD_PULLUP_CONTROL_PINNAME, USBD_ATTACH_LEVEL);

Before we made this:

pin_function(USB_OTG_FS_DP, STM_PIN_DATA(STM_MODE_OUTPUT_PP, GPIO_NOPULL, 0));
digitalWriteFast(USB_OTG_FS_DP, LOW);
delay(10);
pin_function(USB_OTG_FS_DP, STM_PIN_DATA(STM_MODE_INPUT, GPIO_NOPULL, 0));

which is not exactly the same.
To have exactly the same way, the D+trick should be defined also:
#define USBD_DP_TRICK

@matthijskooijman what would you suggest ?

[lyusupov]

AN2606 does not prohibit from soldering a 1.5KOhm pull-up onto D+.
It just specifies for some MCU models that external pull-up is required.
Or, otherwise, it specifies "not required" when an MCU has built-in D+ pull-up contol feature.

This is example of AN2606 section for L07/8:

Thus, it is not a violation of any rule for a board designer to solder 1.5K pull-up on D+
It could be excessive , but definitely not a bug.

[matthijskooijman]

Thus, it is not a violation of any rule for a board designer to solder 1.5K pull-up on D+

I think it violates the USB specification, which requires having a 1.5k +/- 5% pullup on the DP/DM line. The MCU probably does not care, and in practice I suspect that most USB hosts won't care either (they would have to source twice as much current, but that is probably well within their margin), so it probably just works, but it does violate the spec.

@matthijskooijman what would you suggest ?

I would think maybe adding something like this in the variant.h template:

// This indicates that there is an external and fixed 1.5k pullup
// on the D+ line. This define is not normally needed, since a
// fixed pullup is assumed by default. It is only required when
// the USB peripheral has an internal pullup *and* an external
// fixed pullup is present (which is not recommended, since just
// the internal pullup is sufficient and having two pullups
// violates the USB specification).
// #define USBD_FIXED_PULLUP

And then replace

Arduino_Core_STM32/cores/arduino/stm32/usb/usbd_if.c

Line 100 in 088081d

#elif !defined(USBD_HAVE_INTERNAL_PULLUPS)

with

#elif !defined(USBD_HAVE_INTERNAL_PULLUPS) || defined(USBD_FIXED_PULLUP)

The code that figures out what the DP pin is for the OTG_FS and OTG_HS peripherals should also be restored (I removed that, because these always have internal pullups, but that is no longer enough).

I would prepare a PR for this, but I'm quite busy atm, so I'll do it quickly like this for now. I might be able to look more closely next week, if you have not already.

Writing this, I realize that all of the code we have now is geared towards USB full speed devices with a pullup on D+. I think low-speed could be supported if needed, but I guess that as long as there are no boards with fixed or controllable pullups on D-, there's not much point in adding more defines for that now, we can always add additional defines (and rename the existing ones) later.

[lyusupov]

Why would not read D+ pull-up status by before applying MCU's pull-up ?

pinMode (PA12, INPUT_PULLDOWN);
pu_state = digitalRead(PA12);
pinMode (PA12, INPUT);

then to act correspondingly...

Internal MCU GPIO pull-up and pull-down resistors are typically having much greater value that 1.5K, so this measurement should give pretty much realistic answer on question if an external PU resistor is soldered to D+ or not.

[matthijskooijman]

Why would not read D+ pull-up status by before applying MCU's pull-up ?

I would rather keep the startup as simple and fast as possible, especially since as part of #710 this code might run in really, really early startup.

Also, it is the job of the variant file to describe the hardware you're building for, so making this explicit there would IMHO be the better place for this.

[fpistm]

I would prepare a PR for this, but I'm quite busy atm, so I'll do it quickly like this for now. I might be able to look more closely next week, if you have not already.

I'm agree with your proposal. I'm currently working on validating all Cube update to release the 1.9.0. So If you are able to provide the PR that would be fine and really appreciated.
Thanks in advance.

[matthijskooijman]

I created a fix, see #1048. Testing welcome :-)