Generalize USB pullup attach/detach support

[matthijskooijman]

I previously commented about this in the jump-to-bootloader, but I think this is something that warrants a bit more discussion, and can be solved separately as well. See #710 (comment) This is getting a bit of a long post, but I'm also trying to capture my thoughts and, while writing this, moving towards a proposal as well.

USB needs a pullup on the D+ line (or D- for low speed), which indicates the "attachment" status (i.e. pullup present signals the USB host that the device is present, while lack of a pullup detaches from the bus) and allows discriminating between low speed and fullspeed (or higher).

There are various ways this pullup can be implemented in the circuit. In particular, I've seen:

• A fixed pullup
• A pullup that can be switched on or off using a transistor, which can be default on or off.
• A pullup builtin to some MCUs.

Control of this pullup happens in the USBD_reenumerate() function, which currently handles two cases:

• When USB_DISC_PIN is defined by the variant (e.g. on the maple mini), the pullup is assumed to be disabled by default, and can be enabled by making the defined pin OUTPUT LOW.
• When USB_DISC_PIN is not defined, it is assumed that there is an external fixed pullup resistor. To force re-enumeration, the DP pin itself is briefly put into output low mode, which overrides the pullup and looks like an USB detach to the host (though it is a bit of a hack and probably not entirely standards-compliant).

These two cases seem to be somehwat mismatched: When USB_DISC_PIN is defined, the function actually only attaches to the USB bus (which causes enumeration to happen), while otherwise the function actually detaches and reattaches (which causes reenumeration to happen). So maybe for the USB_DISC_PIN case, it should be driven output high (or input/floating) for a short while before switching to low, so it actually causes reenumeration if already enumerated (e.g. when called later after startup).

Note that the internal pullup and default-on pullup configurations are not currently handled.

More generally, you might want to have an API that actually just supports "Attach" and "Detach" operations, so any caller can decide what they need. However, this is problematic because:

• In the fixed pullup configuration, detaching is really a hack that should probably not last for more than a few ms.
• In the internal pullup configuration, the USB core automatically controls the pullup state (in particular, on the F401 we're using, the pullup is enabled automatically when USB device mode is enabled and VBUS is detected). It can be disabled using the SDIS (soft disconnect) bit, so some control is still available.
• In the different configurations, the default / startup state is different, so different sequences of attach/detach might be required.

So, it is probably better to not support separate attach/detach, but instead have USB_reenumerate() (possibly under a different name) just handle all usecases that need to be handled. Looking at those, I can see:

• Startup, which needs to trigger USB attachment. At first glance, it seems this only needs work for the external switched pullup case (just enable the pullup), but for the fixed pullup and default on configurations, the pullup stays on through a reset, so this actually needs to disable the pullup before enabling it. For the internal configuration, nothing is needed (the pullup will be enabled as part of USB initialization). Note that this is the only usecase present in the current code (i.e. the only call to USBD_reenumerate() is at startup).
• Jumping to the bootloader, which needs to detach from USB and then reattach to allow the bootloader to provide its own descriptors. This case is added by [Beta] Jump to system memory boot from user application #710. In the current implementation, the bootloader happens after a reset, which makes this case essentially identical to the normal startup case.
• Re-enumerating during runtime. I can imagine one wants to, as part of their sketch, force re-enumeration (e.g. to change device type or configuration) without resetting the MCU. This is pretty similar to the startup case, except that it might need to explicitly disable an external pullup, since no MCU reset has done so.
• Detaching from the USB bus for a longer period of time. One might want to disable USB connectivity based on some trigger, user interaction, etc. This is something that is not feasible with fixed pullups, but is possible in the other configurations.

Looking at the above, it seems that the first three cases are pretty identical (the third one needs some extra work in case no reset happened beforehand, but that extra work should not hurt if a reset happened, so maybe best to just always do that). The fourth usecase is different, but also not currently supported. Hence, USBD_reenumerate() could be made to support the first three cases, and then we could (if we need it later) add USBD_detach() or something to support the fourth case.

This means that the contract of USBD_reenumerate() can be:

• When internal pullups are used, do nothing
• Otherwise, when pullups are enabled, disable them for a few ms
• Then (re-)enable pullups
  This function should then be called from the startup/USB code, it is not intended to be called from user code directly.

This supports the first three usecases (though it might need some extra work with internal pullups for the third usecase, reenumerating during runtime, when that is implemented).

Specifying the configuration

Currently, only two configurations are handled, based on the USB_DISC_PIN macro: External fixed pullup, and external switched pullup that is detached by default and attaches on OUTPUT LOW. I do not really like the USB_DISC_PIN macro name here (it suggests "disconnect pin", while the pin really connects rather than disconnects). Also, it supports only one specific external pullup configuration. I would propose generalizing this by using a few macros. In particular, here's what I would add to the variant.h template:

// If the board has external USB pullup (on DP/DP depending on speed)
// that can be controlled using a GPIO pin, define these.
//  - If the the pullup is disabled (detached) by default, define
//    USB_ATTACH_PIN to the pin that, when written to USB_ATTACH_LEVEL,
//    attaches the pullup.
//  - If the the pullup is enabled (attached) by default, define
//    USB_DETACH_PIN to the pin that, when written to USB_DETACH_LEVEL,
//    detaches the pullup.
//#define USB_ATTACH_PIN x
//#define USB_ATTACH_LEVEL LOW
//#define USB_DETACH_PIN x
//#define USB_DETACH_LEVEL LOW

I think this could also be used to simplify the MALYANM200 variant, which currently has a customized USBD_reenumerate() function because, I think, it has an active-high attach pin (Maybe @xC0000005 can confirm that the pullup is indeed disabled by default and enabled by making PB9 high?).

Then, we also need to handle the internal pullup case. Ideally, we would autodetect that internal pullups are supported. We could check for the USB_OTG_DCTL_SDIS macro, which is the Soft Disconnect bit - if you can soft disconnect, that probably means that pullups are builtin. Note sure if this catches all of them, though.

If there are no internal pullups and USB_ATTACH_PIN/USB_DETACH_PIN are not defined, we can fall back to the DP-pin-low hack. Alternatively, we could say that such a hack should not be a default and thus must be explicitly enabled for a board, but I guess that does not add so much (we can always add a macro to disable it later when needed).

Summarizing

It's become a bit of a wall of text, but unless I missed something above, I think this should be something we can implement now. In particular, this needs:

• Changes to the template variant.h to document the new macros
• Changes to USBD_reenumerate() to:
  • Support different external switchable pullup configurations
  • Disable the external switchable pullup and wait a bit, just like with the fixed pullup hack.
  • Detect internal pullups and do nothing.

[xC0000005]

On Malyan M200 variants which have the pullup (almost all boards have the pad, not all have the transistor), the pin is active high. Simply setting it high did not work to get reenumeration to work, I had to do the toggle (I tried enough variations of this to know).

…

On Jan 23, 2020, at 9:45 AM, Matthijs Kooijman ***@***.***> wrote: I previously commented about this in the jump-to-bootloader, but I think this is something that warrants a bit more discussion, and can be solved separately as well. See #710 (comment) <#710 (comment)> This is getting a bit of a long post, but I'm also trying to capture my thoughts and, while writing this, moving towards a proposal as well. USB needs a pullup on the D+ line (or D- for low speed), which indicates the "attachment" status (i.e. pullup present signals the USB host that the device is present, while lack of a pullup detaches from the bus) and allows discriminating between low speed and fullspeed (or higher). There are various ways this pullup can be implemented in the circuit. In particular, I've seen: A fixed pullup A pullup that can be switched on or off using a transistor, which can be default on or off. A pullup builtin to some MCUs. Control of this pullup happens in the USBD_reenumerate() function, which currently handles two cases: When USB_DISC_PIN is defined by the variant (e.g. on the maple mini), the pullup is assumed to be disabled by default, and can be enabled by making the defined pin OUTPUT LOW. When USB_DISC_PIN is not defined, it is assumed that there is an external fixed pullup resistor. To force re-enumeration, the DP pin itself is briefly put into output low mode, which overrides the pullup and looks like an USB detach to the host (though it is a bit of a hack and probably not entirely standards-compliant). These two cases seem to be somehwat mismatched: When USB_DISC_PIN is defined, the function actually only attaches to the USB bus (which causes enumeration to happen), while otherwise the function actually detaches and reattaches (which causes reenumeration to happen). So maybe for the USB_DISC_PIN case, it should be driven output high (or input/floating) for a short while before switching to low, so it actually causes reenumeration if already enumerated (e.g. when called later after startup). Note that the internal pullup and default-on pullup configurations are not currently handled. More generally, you might want to have an API that actually just supports "Attach" and "Detach" operations, so any caller can decide what they need. However, this is problematic because: In the fixed pullup configuration, detaching is really a hack that should probably not last for more than a few ms. In the internal pullup configuration, the USB core automatically controls the pullup state (in particular, on the F401 we're using, the pullup is enabled automatically when USB device mode is enabled and VBUS is detected). It can be disabled using the SDIS (soft disconnect) bit, so some control is still available. In the different configurations, the default / startup state is different, so different sequences of attach/detach might be required. So, it is probably better to not support separate attach/detach, but instead have USB_reenumerate() (possibly under a different name) just handle all usecases that need to be handled. Looking at those, I can see: Startup, which needs to trigger USB attachment. At first glance, it seems this only needs work for the external switched pullup case (just enable the pullup), but for the fixed pullup and default on configurations, the pullup stays on through a reset, so this actually needs to disable the pullup before enabling it. For the internal configuration, nothing is needed (the pullup will be enabled as part of USB initialization). Note that this is the only usecase present in the current code (i.e. the only call to USBD_reenumerate() is at startup). Jumping to the bootloader, which needs to detach from USB and then reattach to allow the bootloader to provide its own descriptors. This case is added by #710 <#710>. In the current implementation, the bootloader happens after a reset, which makes this case essentially identical to the normal startup case. Re-enumerating during runtime. I can imagine one wants to, as part of their sketch, force re-enumeration (e.g. to change device type or configuration) without resetting the MCU. This is pretty similar to the startup case, except that it might need to explicitly disable an external pullup, since no MCU reset has done so. Detaching from the USB bus for a longer period of time. One might want to disable USB connectivity based on some trigger, user interaction, etc. This is something that is not feasible with fixed pullups, but is possible in the other configurations. Looking at the above, it seems that the first three cases are pretty identical (the third one needs some extra work in case no reset happened beforehand, but that extra work should not hurt if a reset happened, so maybe best to just always do that). The fourth usecase is different, but also not currently supported. Hence, USBD_reenumerate() could be made to support the first three cases, and then we could (if we need it later) add USBD_detach() or something to support the fourth case. This means that the contract of USBD_reenumerate() can be: When internal pullups are used, do nothing Otherwise, when pullups are enabled, disable them for a few ms Then (re-)enable pullups This function should then be called from the startup/USB code, it is not intended to be called from user code directly. This supports the first three usecases (though it might need some extra work with internal pullups for the third usecase, reenumerating during runtime, when that is implemented). Specifying the configuration Currently, only two configurations are handled, based on the USB_DISC_PIN macro: External fixed pullup, and external switched pullup that is detached by default and attaches on OUTPUT LOW. I do not really like the USB_DISC_PIN macro name here (it suggests "disconnect pin", while the pin really connects rather than disconnects). Also, it supports only one specific external pullup configuration. I would propose generalizing this by using a few macros. In particular, here's what I would add to the variant.h template: // If the board has external USB pullup (on DP/DP depending on speed) // that can be controlled using a GPIO pin, define these. // - If the the pullup is disabled (detached) by default, define // USB_ATTACH_PIN to the pin that, when written to USB_ATTACH_LEVEL, // attaches the pullup. // - If the the pullup is enabled (attached) by default, define // USB_DETACH_PIN to the pin that, when written to USB_DETACH_LEVEL, // detaches the pullup. //#define USB_ATTACH_PIN x //#define USB_ATTACH_LEVEL LOW //#define USB_DETACH_PIN x //#define USB_ATTACH_LEVEL LOW I think this could also be used to simplify the MALYANM200 variant, which currently has a customized USBD_reenumerate() function because, I think, it has an active-high attach pin (Maybe @xC0000005 <https://github.com/xC0000005> can confirm that the pullup is indeed disabled by default and enabled by making PB9 high?). Then, we also need to handle the internal pullup case. Ideally, we would autodetect that internal pullups are supported. We could check for the USB_OTG_DCTL_SDIS macro, which is the Soft Disconnect bit - if you can soft disconnect, that probably means that pullups are builtin. Note sure if this catches all of them, though. If there are no internal pullups and USB_ATTACH_PIN/USB_DETACH_PIN are not defined, we can fall back to the DP-pin-low hack. Alternatively, we could say that such a hack should not be a default and thus must be explicitly enabled for a board, but I guess that does not add so much (we can always add a macro to disable it later when needed). Summarizing It's become a bit of a wall of text, but unless I missed something above, I think this should be something we can implement now. In particular, this needs: Changes to the template variant.h to document the new macros Changes to USBD_reenumerate() to: Support different external switchable pullup configurations Disable the external switchable pullup and wait a bit, just like with the fixed pullup hack. Detect internal pullups and do nothing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#885?email_source=notifications&email_token=AHVGS4NOGAXE25VLOYXVHWDQ7HJTDA5CNFSM4KK2MLL2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IIKCGJQ>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHVGS4NUJHH2F6V3YNO234LQ7HJTDANCNFSM4KK2MLLQ>.

[fpistm]

@matthijskooijman
Right the support is basic based on community experience.
USB_DISC_PIN is a legacy method and simply ported to this core.
That's fine to enhance it.
I'm currently very busy on several other task and could not check all USB related contents soon.
Any help and feedback on those topics are welcome (tests, PR review, ...)

[matthijskooijman]

On Malyan M200 variants which have the pullup (almost all boards have the pad, not all have the transistor), the pin is active high.

@xC0000005, Do you know in more detail how this is wired? I guess there is a transistor to switch the USB pullup on and off? Does the transistor gate have a pulldown/pullup too (i.e. what is its default state)? And a high signal enables the pullup, low disables it, right?

Simply setting it high did not work to get reenumeration to work, I had to do the toggle (I tried enough variations of this to know).

Ok, so that suggests to me that the pullup is on by default and re-enumeration can be done by making the pin low for a while? That would mean that going from INPUT to LOW back to INPUT would make this work (not necessarily HIGH -> LOW -> HIGH)?

[xC0000005]

Yes, the code in the M200 variant does this right - I switch it high (just to be sure), pull low (to signal) and push it high, just as you suggested. V2 of the printer doesn’t include the pullup (and has some reported issues with enumeration).

…

On Mar 18, 2020, at 11:47 AM, Matthijs Kooijman ***@***.***> wrote: On Malyan M200 variants which have the pullup (almost all boards have the pad, not all have the transistor), the pin is active high. @xC0000005 <https://github.com/xC0000005>, Do you know in more detail how this is wired? I guess there is a transistor to switch the USB pullup on and off? Does the transistor gate have a pulldown/pullup too (i.e. what is its default state)? And a high signal enables the pullup, low disables it, right? Simply setting it high did not work to get reenumeration to work, I had to do the toggle (I tried enough variations of this to know). Ok, so that suggests to me that the pullup is on by default and re-enumeration can be done by making the pin low for a while? That would mean that going from INPUT to LOW back to INPUT would make this work (not necessarily HIGH -> LOW -> HIGH)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#885 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHVGS4JU7Z3KBYH4453VIWLRIEJLZANCNFSM4KK2MLLQ>.

[matthijskooijman]

Yes, the code in the M200 variant does this right - I switch it high (just to be sure), pull low (to signal) and push it high, just as you suggested. V2 of the printer doesn’t include the pullup (and has some reported issues with enumeration).

I'm trying to figure out some more details, since I'm hoping to remove the custom code in the M200 variant and just have some generic code. Easiest would be INPUT -> LOW -> INPUT (Since then the same code can be used for the DP-LOW-hack as well), so I'm trying to figure out what options there are.

Good point about the V2 board, I forgot to ask that in my previous comment. You say it does not include the pullup. I presume it does have a D+ pullup, but you probably mean that they do not have a pullup on the transistor gate? Or is there no transistor at all?

[matthijskooijman]

I implemented this, see #997. Compared to my plan above, I:

• Changed the macro names to e.g. USBD_* instead of USB_* for consistency with the since added USBD_VBUS_DETECTION_ENABLE macro.
• Changed the macros to accept PinName constants rather than pin numbers, to slightly simplify the code (the same code can now handle toggling an external pulup and the DP-output-low-trick). If we don't like this, some extra macro code could be added to automatically translate pin numbers to pin names as well, of course.