blackpill v2 DFU fixes

[bm16ton]

Hello, sum simple fixes for blackboard dfu, Fixed the button on A0 its actually active low but without an active pullup would sometimes work the otherway and bounce around. The blackboardv2 can now switch into dfu flash and come back all via dfu-util without any user intervention. Use dfu-util v0.11 and dfu-util -a 0 --dfuse-address 0x08000000:leave -R -D blackmagic.bin . The only thing that may not fly is the usb string for the interface which is needed, but diff depending on stm32 part number. Also i see no adc for target, is this intentional or just hasnt been gotten to yet?

[dragonmux]

Thank you for the contribution bm16ton!

Seeing as we don't have any blackpillv2 hardware here, we'll review the code changes and all that but we're going to ask a couple of people who do have hardware to test this on their hardware just to check it doesn't break something for them - it's possible that the behaviour needed is different for different "blackpills" (quotes because it's all clone hardware and we've seen instances where one cloner did something different than another).

[bm16ton]

Oh i understand, this would be my first pr on a major project as tiny as it is i dont care im still proud! Should the dfu string be moved to the platform.h files for each variation or an if statement? im willing to scour the internet and find as many of the strings as possible. Thank you again! oh and the adc woukd that be something that would possibly get accepted if enabled?

…

On Sun, 31 Jul 2022, 7:36 am Rachel Mant, ***@***.***> wrote: Thank you for the contribution bm16ton! Seeing as we don't have any blackpillv2 hardware here, we'll review the code changes and all that but we're going to ask a couple of people who do have hardware to test this on their hardware just to check it doesn't break something for them - it's possible that the behaviour needed is different for different "blackpills" (quotes because it's all clone hardware and we've seen instances where one cloner did something different than another). — Reply to this email directly, view it on GitHub <#1152 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADAWMPZGMBTBSQ7I4XDXZ53VWZQLTANCNFSM55ELNZ2Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[bm16ton]

Should I do a new pull request rebased with newest source for the new file layout? I also have a few questions. I have adc support I can add, in order to work with 5v I did the math expecting a divide by half voltage divider, is that ok? Also im new to all this but f411 datasheet says 4 usb endpoints, am I correct in thinking the 2 interrupt endpoints and 2 bulk for the 2 usbcdc fill this limitation? I thought i saw the endpoint for tracing enabled by default (going on my memory i def need to recheck) but just curious if it worked with 5 endpoints. I myself dropped both interupt endpoints and just use usbserial instead of cdc_acm removing the interrupt endpoint requirement. This obviously wouldn't work as default mainline for the increase in support would be to much, trying to explain how/why etc (tho adding your usb vendor/product to mainline kernels usbserial table would remove all of that and itd behave same as cdc_acm does now to end user. I have no doubt they would accept that patch if you ever wanted to go that way) But to keep people who want tracing not entirely in the dark, I could update blackpills README simply stating it could be done but give no directions and maybe a link to kernel docs usbserial?

[dragonmux]

hi bm16ton, please update this PR using rebase and a force push to your fork. If you would like to give the platform ADC support, please open a separate PR for that and we'll give it a look.

There are some helpful instructions in CONTRIBUTING.md to help with the process

[ALTracer]

@bm16ton
Per ADC platform support you're velcome to add Vtarget measurements channel ADC code and instructions on connecting input dividers, if you're still interested.

Per DFU I think it's working by now -- jumping into (and back out of) BMPBootloader, sector erase, reprogramming, even PA0. It only doesn't blink its LED in any fashion (yet). Not sure if users want ST MaskROM bootloader instead, you can't return from it back to app without flashing with :leave.

Also im new to all this but f411 datasheet says 4 usb endpoints, am I correct in thinking the 2 interrupt endpoints and 2 bulk for the 2 usbcdc fill this limitation? I thought i saw the endpoint for tracing enabled by default (going on my memory i def need to recheck) but just curious if it worked with 5 endpoints. I myself dropped both interupt endpoints and just use usbserial instead of cdc_acm removing the interrupt endpoint requirement.

You raise a fair point on DWC2 endpoints usage. On current main I see this in lsusb:

• GDB CDC-ACM takes up EP 1 OUT + EP 1 IN for Bulk, and EP 2 IN for Interrupt; it implements GDB RSP and BMP remote;
• Target CDC-ACM takes up EP 3 OUT + EP 3 IN for Bulk, and EP 4 IN for Interrupt; it's rather a "multipurpose" console.
• DFU stub seems to work over Control EP0 if I understand this correctly;
• Trace Capture takes up EP 5 IN which would be out of bounds for this DWC2. I didn't get to testing undecoded TraceSWO yet.

The datasheet says that F411's OTG_FS in peripheral mode has 1 bidirectional ep0, 3 IN EPs & 3 OUT EPs, for a total of 8.

[dragonmux]

Sorry this PR has taken so long to review properly. We had to improve our understanding of the code some of this touches, and then forgot about it.

[dragonmux]

Having further reviewed all this code, we can with confidence say that this patch is not required and that if anything was, it would go in src/platforms/common/stm32/dfucore.c rather than here as the strings apply to DFU mode not application mode and so apply to the bootloader and not the main firmware:

blackmagic/src/platforms/common/stm32/dfucore.c

Lines 36 to 45 in 8e661a6

	#elif defined(STM32F4) || defined(STM32F7)
	#define DFU_IFACE_PAGESIZE 128
	#if APP_START == 0x08020000
	#define DFU_IFACE_STRING_OFFSET 62
	#define DFU_IFACE_STRING "@Internal Flash /0x08000000/1*016Ka,3*016Ka,1*064Ka,1*128Kg,002*128Kg"
	#elif APP_START == 0x08004000
	#define DFU_IFACE_STRING_OFFSET 54
	#define DFU_IFACE_STRING "@Internal Flash /0x08000000/1*016Ka,3*016Kg,1*064Kg,000*128Kg"
	#endif
	#endif

These definitions do look like they need a review though so you would be most welcome to figure out what's going on there and suggest any required patches in this PR

[bm16ton]

ha my god my memory is bad it was so long ago. The idea which I probly never said so my bad, was to use the internal dfu bootloader on models that support it. Using the stm dfu this would be required for the switching back and forth to work correctly. The usb descriptors for dfu would need to be set this way in the main firmware so upon reset into dfu mode dfu-util would know it was same dfu device. My idea was the in house dfu firmware wasn't needed, and using the internal dfu would save space. But using the inhouse dfu became the decided direction (can't blame yeah, tho I am curious for the reason, just so I can use that knowledge to become a better programmer/contributer)

[dragonmux]

Spaces vs tabs - this needs its indentation converting

[dragonmux]

We would prefer the blank line to be above this comment, not below

[bm16ton]

Will do better next time! Everything has changed a lot since this pr was sent. Ill hafta check if adc support has been brought in for various f4 series etc and a few other areas I had added and if anything is missing that I have would a pr attempt for such things be welcome?

[dragonmux]

You would be welcome to update this PR in that case to reduce the work on both of us