From e004385613fc3c70efde842f0729eda471b2cdf8 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Mon, 27 Apr 2020 16:18:26 +0200 Subject: [PATCH 1/3] Support boards with internal *and* external pullups again These boards were broken in commit e1d409f1 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 https://github.com/STM32-base/STM32-base.github.io/pull/26 for some additional inventarisation of this problem. This fixes #1029. --- cores/arduino/stm32/usb/usbd_if.c | 18 +++++++++++------- variants/BLACK_F407XX/variant.h | 7 +++++++ variants/BLUE_F407VE_Mini/variant.h | 7 +++++++ variants/board_template/variant.h | 12 +++++++++++- 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/cores/arduino/stm32/usb/usbd_if.c b/cores/arduino/stm32/usb/usbd_if.c index 3116faca28..e579140f72 100644 --- a/cores/arduino/stm32/usb/usbd_if.c +++ b/cores/arduino/stm32/usb/usbd_if.c @@ -68,6 +68,9 @@ #if defined(USBD_DETACH_PIN) && !defined(USBD_DETACH_LEVEL) #error "USBD_DETACH_PIN also needs USBD_DETACH_LEVEL defined" #endif /* defined(USBD_DETACH_PIN) && !defined(USBD_DETACH_LEVEL) */ +#if (defined(USBD_DETACH_PIN) || defined(USBD_ATTACH_PIN)) && defined(USBD_FIXED_PULLUP) + #error "Cannot define both USBD_FIXED_PULLUP and USBD_ATTACH_PIN or USBD_DETACH_PIN" +#endif /* (defined(USBD_DETACH_PIN) || defined(USBD_ATTACH_PIN)) && defined(USBD_FIXED_PULLUP) */ /* Either of these bits indicate that there are internal pullups */ #if defined(USB_BCDR_DPPU) || defined(USB_OTG_DCTL_SDIS) @@ -78,10 +81,13 @@ * in USBD_LL_Init in usbd_conf.c. */ #if defined(USE_USB_HS) #define USBD_USB_INSTANCE USB_OTG_HS + #define USBD_DP_PINNAME USB_OTG_HS_DP #elif defined(USB_OTG_FS) #define USBD_USB_INSTANCE USB_OTG_FS + #define USBD_DP_PINNAME USB_OTG_FS_DP #elif defined(USB) #define USBD_USB_INSTANCE USB + #define USBD_DP_PINNAME USB_DP #endif /* @@ -97,15 +103,13 @@ #elif defined(USBD_DETACH_PIN) #define USBD_PULLUP_CONTROL_PINNAME digitalPinToPinName(USBD_DETACH_PIN) #define USBD_ATTACH_LEVEL !(USBD_DETACH_LEVEL) -#elif !defined(USBD_HAVE_INTERNAL_PULLUPS) +#elif !defined(USBD_HAVE_INTERNAL_PULLUPS) || defined(USBD_FIXED_PULLUP) /* When no USB attach and detach pins were defined, and there are also * no internal pullups, assume there is a fixed external pullup and apply - * the D+ trick. This should happen only for the USB peripheral, since - * USB_OTG_HS and USB_OTG_FS always have internal pullups. */ - #if !defined(USB) - #error "Unexpected USB configuration" - #endif - #define USBD_PULLUP_CONTROL_PINNAME USB_DP + * the D+ trick. Also do this when there are internal *and* external + * pulups (which is a hardware bug, but there are boards out there with + * this). */ + #define USBD_PULLUP_CONTROL_PINNAME USBD_DP_PINNAME #define USBD_DETACH_LEVEL LOW // USBD_ATTACH_LEVEL not needed. #define USBD_DP_TRICK diff --git a/variants/BLACK_F407XX/variant.h b/variants/BLACK_F407XX/variant.h index 341421c87b..14154e7926 100644 --- a/variants/BLACK_F407XX/variant.h +++ b/variants/BLACK_F407XX/variant.h @@ -301,6 +301,13 @@ extern "C" { #define HAL_DAC_MODULE_ENABLED #define HAL_SD_MODULE_ENABLED +// This indicates that there is an external and fixed 1.5k pullup +// on the D+ line. This define is only needed on boards that have +// internal pullups *and* an external pullup. Note that it would have +// been better to omit the pullup and exclusively use the internal +// pullups instead. +#define USBD_FIXED_PULLUP + #ifdef __cplusplus } // extern "C" #endif diff --git a/variants/BLUE_F407VE_Mini/variant.h b/variants/BLUE_F407VE_Mini/variant.h index 36843ac0e4..dcd3106026 100644 --- a/variants/BLUE_F407VE_Mini/variant.h +++ b/variants/BLUE_F407VE_Mini/variant.h @@ -184,6 +184,13 @@ extern "C" { #define HAL_DAC_MODULE_ENABLED #define HAL_SD_MODULE_ENABLED +// This indicates that there is an external and fixed 1.5k pullup +// on the D+ line. This define is only needed on boards that have +// internal pullups *and* an external pullup. Note that it would have +// been better to omit the pullup and exclusively use the internal +// pullups instead. +#define USBD_FIXED_PULLUP + #ifdef __cplusplus } // extern "C" #endif diff --git a/variants/board_template/variant.h b/variants/board_template/variant.h index d43a4d5cec..2be918617a 100644 --- a/variants/board_template/variant.h +++ b/variants/board_template/variant.h @@ -174,7 +174,17 @@ extern "C" { //#define USBD_ATTACH_LEVEL LOW //#define USBD_DETACH_PIN x //#define USBD_DETACH_LEVEL LOW - +// +// 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 actually a hardware bug, since just +// the internal pullup is sufficient and having two pullups violates the +// USB specification). In this case, defining this forces +// the "write D+ LOW"-trick to be used. In the future, it might also +// disable the internal pullups, but this is not currently implemented. +// #define USBD_FIXED_PULLUP #ifdef __cplusplus } // extern "C" #endif From 40af51871c88e75f053b285d50b1ebd8c11239af Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Mon, 27 Apr 2020 21:58:51 +0200 Subject: [PATCH 2/3] Add some urls to boards.txt While reviewing some of these boards, it was not immediately clear to me what boards they referred to (especially with the relatively unbranded boards from aliexpress). Just in case this helps anyone else, this adds some urls with more info on those boards I found from the git history and github issues. --- boards.txt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/boards.txt b/boards.txt index cb44e8c65b..e763020f39 100644 --- a/boards.txt +++ b/boards.txt @@ -1210,6 +1210,7 @@ GenF4.build.series=STM32F4xx GenF4.build.cmsis_lib_gcc=arm_cortexM4lf_math # Black F407VE +# https://github.com/mcauser/BLACK_F407VEZ GenF4.menu.pnum.BLACK_F407VE=Black F407VE GenF4.menu.pnum.BLACK_F407VE.upload.maximum_size=524288 GenF4.menu.pnum.BLACK_F407VE.upload.maximum_data_size=131072 @@ -1218,6 +1219,7 @@ GenF4.menu.pnum.BLACK_F407VE.build.product_line=STM32F407xx GenF4.menu.pnum.BLACK_F407VE.build.variant=BLACK_F407XX # Black F407VG +# https://github.com/mcauser/BLACK_F407VEZ with bigger chip GenF4.menu.pnum.BLACK_F407VG=Black F407VG GenF4.menu.pnum.BLACK_F407VG.upload.maximum_size=1048576 GenF4.menu.pnum.BLACK_F407VG.upload.maximum_data_size=131072 @@ -1226,6 +1228,7 @@ GenF4.menu.pnum.BLACK_F407VG.build.product_line=STM32F407xx GenF4.menu.pnum.BLACK_F407VG.build.variant=BLACK_F407XX # Black F407ZE +# https://github.com/mcauser/BLACK_F407ZE GenF4.menu.pnum.BLACK_F407ZE=Black F407ZE GenF4.menu.pnum.BLACK_F407ZE.upload.maximum_size=524288 GenF4.menu.pnum.BLACK_F407ZE.upload.maximum_data_size=131072 @@ -1234,6 +1237,7 @@ GenF4.menu.pnum.BLACK_F407ZE.build.product_line=STM32F407xx GenF4.menu.pnum.BLACK_F407ZE.build.variant=BLACK_F407XX # Black F407ZG +# https://github.com/mcauser/BLACK_F407ZG GenF4.menu.pnum.BLACK_F407ZG=Black F407ZG GenF4.menu.pnum.BLACK_F407ZG.upload.maximum_size=1048576 GenF4.menu.pnum.BLACK_F407ZG.upload.maximum_data_size=131072 @@ -1274,6 +1278,7 @@ GenF4.menu.pnum.BLACKPILL_F401CC.build.product_line=STM32F401xC GenF4.menu.pnum.BLACKPILL_F401CC.build.variant=Generic_F401Cx # BlackPill F411CE +# https://github.com/mcauser/WEACT_F411CEU6 GenF4.menu.pnum.BLACKPILL_F411CE=BlackPill F411CE GenF4.menu.pnum.BLACKPILL_F411CE.upload.maximum_size=524288 GenF4.menu.pnum.BLACKPILL_F411CE.upload.maximum_data_size=131072 @@ -1282,6 +1287,7 @@ GenF4.menu.pnum.BLACKPILL_F411CE.build.product_line=STM32F411xE GenF4.menu.pnum.BLACKPILL_F411CE.build.variant=Generic_F411Cx # Core board F401RCT6 +# https://stm32-base.org/boards/STM32F401RCT6-STM32F-Core-Board GenF4.menu.pnum.CoreBoard_F401RC=Core board F401RCT6 GenF4.menu.pnum.CoreBoard_F401RC.upload.maximum_size=262144 GenF4.menu.pnum.CoreBoard_F401RC.upload.maximum_data_size=65536 @@ -1298,6 +1304,7 @@ GenF4.menu.pnum.FEATHER_F405.build.product_line=STM32F405xx GenF4.menu.pnum.FEATHER_F405.build.variant=FEATHER_F405 # ThunderPack F411xxE +# https://github.com/jgillick/ThunderPack/tree/STM32F4 GenF4.menu.pnum.THUNDERPACK_F411=ThunderPack v1.1+ GenF4.menu.pnum.THUNDERPACK_F411.upload.maximum_size=524288 GenF4.menu.pnum.THUNDERPACK_F411.upload.maximum_data_size=131072 From d3fdc1f021e7abd8122b11fcdd1989ea56804cdf Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Tue, 28 Apr 2020 12:24:02 +0200 Subject: [PATCH 3/3] Do not use external pullups on CoreBoard F401RC This board also has internal pullups, so the external one is not actually needed (and will even violate the USB spec when both are used together). This commit disabled the external pullups, but leaves the defines in comments, as future documentation. See also #1029. --- variants/Generic_F401Rx/variant.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/variants/Generic_F401Rx/variant.h b/variants/Generic_F401Rx/variant.h index db3d19394f..2c6567ab2a 100644 --- a/variants/Generic_F401Rx/variant.h +++ b/variants/Generic_F401Rx/variant.h @@ -127,8 +127,11 @@ extern "C" { #ifdef ARDUINO_CoreBoard_F401RC // USB, pull this pin low to enable the USB attach pullup -#define USBD_ATTACH_PIN PD2 -#define USBD_ATTACH_LEVEL LOW +// It is documented here, but not actually used, since there are also +// internal pullups which are automatically used and using both would +// violate the USB specification for pullup strength. +//#define USBD_ATTACH_PIN PD2 +//#define USBD_ATTACH_LEVEL LOW #endif #ifdef __cplusplus