Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature: Fancy DFU on blackpill-f4 #1717

Merged
merged 7 commits into from
Jan 14, 2024
Merged

Feature: Fancy DFU on blackpill-f4 #1717

merged 7 commits into from
Jan 14, 2024

Conversation

ALTracer
Copy link
Contributor

@ALTracer ALTracer commented Jan 7, 2024

Detailed description

  • This looks like a feature to end users but is just a collection of tweaks and ported code.
  • The existing problem is an inefficient DFU configuration for blackpill-f4 family of platforms (and probably other STM32F4 boards).
  • This PR solves the problem by tightening DFU F4 delays, introducing an LED show/blinky in BMPBootloader (usbdfu), and provisioning it all to fit under 16 KiB for optimal reachable performance.

The first couple commits belong to the serialno PR this PR is based on (or blocks on). I fail to see what mechanism checks that the usbdfu binary fits under the allocated space (8 1-KiB sectors for F1, first 16 KiB sector for F4, etc.), but I noticed that flipping the DFU_SERIAL_LENGTH from 9 to 13 pulled sprintf and bloated the dfu binary past first sector. See that PR for details.

The dfu_f4.c changes are required and beneficial for BMPBootloader to work how I tested it to, but can be split into its separate PR if needed. I could submit similar delay tunings for dfu_f1.c I am using on F103CB boards.

The last commit is a general platform cleanup, but it can be amended or rebased away depending on situation.

Measurements for reference:
ST MaskROM DFU ~ 7.75 KiB/s (fine for flashing BMPBootloader)
BMPBootloader on main ~12 KiB/s (excessive delays)
BMPBootloader on this PR branch ~ 43 KiB/s (up to 54 KiB/s if fitting under 0x08020000 and tuning delays further)
4x reduced upload time (down to ~3 seconds) is worth it for developers reflashing their boards often.

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do

Closing issues

Closes #1516, or so I suppose (after a possible removal of LED_BOOTLOADER for good)

@dragonmux dragonmux added this to the v2.0 release milestone Jan 7, 2024
@dragonmux dragonmux mentioned this pull request Jan 7, 2024
Merged
6 tasks
@dragonmux
Copy link
Member

If you could please rebase this PR now the other two precursors are merged, we'll get to try and review it and give it an honest look.

@dragonmux dragonmux added the Foreign Host Board Non Native hardware to runing Black Magic firmware on label Jan 7, 2024
@ALTracer ALTracer force-pushed the feature/blackpill-f4-dfu-fancy branch from 83bab96 to 094d436 Compare January 7, 2024 20:35
dragonmux
dragonmux requested changes Jan 7, 2024
View reviewed changes
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've got a couple of queries and some stylistic items to address, but we think this is looking pretty decent and is close to being merge-able.

src/platforms/common/blackpill-f4/blackpill-f4.c Show resolved Hide resolved
src/platforms/common/blackpill-f4/usbdfu.c Outdated Show resolved Hide resolved
src/platforms/common/blackpill-f4/usbdfu.c Show resolved Hide resolved
src/platforms/common/stm32/dfu_f4.c Show resolved Hide resolved
src/platforms/common/stm32/dfu_f4.c Outdated Show resolved Hide resolved
src/platforms/common/blackpill-f4/usbdfu.c Outdated Show resolved Hide resolved
src/platforms/common/blackpill-f4/usbdfu.c Outdated Show resolved Hide resolved
dragonmux
dragonmux reviewed Jan 8, 2024
View reviewed changes
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to us now - if you can do final pass with squashing the fixup commits, we'll get this merged.

@ALTracer ALTracer force-pushed the feature/blackpill-f4-dfu-fancy branch from 619f745 to 2f57c58 Compare January 8, 2024 14:53
@ALTracer
Copy link
Contributor Author

ALTracer commented Jan 8, 2024

Propagated the "unsigned type" and commentaries to relevant commits, it wasn't as simple as a fixup autosquash.
Please state what commit prefix should be used for files src/common/stm32/dfu_f4.c, src/common/blackpill-f4/{blackpill-f4.c,usbdfu.c}. I could make them consistent. Otherwise I think this is up-to date against main. Please check with clang-tidy or whatnot, I don't have that set up (which would have prevented me adding pre-standards code with lints etc.)

@dragonmux
Copy link
Member

For dfu_f4.c, it'd be common/stm32/dfu_f4: and for the Black Pill bits, common/blackpill-f4: or with the file name minus .c also included before the : depending on combined-or-separate commits for each file.

@ALTracer
common/blackpill-f4/usbdfu: Enable heartbeat on PC13 in BMP DFU
3693196 
* Set up SysTick interrupt to blink both LED_IDLE_RUN and LED_BOOTLOADER
@ALTracer
common/blackpill-f4/usbdfu: Run a slow LED show in BMP DFU
a814abd 
* Drop the toggling of LED_BOOTLOADER in favor of SET_IDLE_STATE()
* Tone down the blinking from 10 Hz, 50% duty cycle to 1 Hz, 10% duty cycle
@ALTracer
common/stm32/dfu_f4: Use dfu_event() for feature parity with dfu_f1
64d308b
@ALTracer
common/blackpill-f4/usbdfu: Enable dynamic blinking in write phase of…
330d819 
… BMP DFU

* Implement the dfu_event callback and make it inhibit the show for 1 second
* Toggle the platform Idle LED on every callback hit (usually 1 KiB)
@ALTracer
common/stm32/dfu_f4: Extract sector erase time constants into an enum
25e277a
@ALTracer
common/stm32/dfu_f4: Reduce erase and write delays advertised by BMP DFU
7a3a04b 
* Previous timings corresponded to max delays per datasheets "guaranteed by characterization results"
* It is safe to back off the host for less time and then deal with polling
* Use typical times (which happen to be twice as short for erase and 25x for write) from same datasheets
@ALTracer
common/blackpill-f4: Refresh voodoo
aec136d 
* Drop RCC_OTGFS enabling because it happens in stm32f107_usbd_init, and move RCC_CRC up to GPIOs
* Drop GPIO_OSPEEDR setting of PA1/13/14 because that mapping is obsolete
* Bump OSPEEDR setting from 2MHz to 25MHz for GPIO TAP pins
* Reduce default "max" frequency for robust UX
@ALTracer ALTracer force-pushed the feature/blackpill-f4-dfu-fancy branch from 2f57c58 to aec136d Compare January 9, 2024 21:11
dragonmux
dragonmux approved these changes Jan 14, 2024
View reviewed changes
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all LGTM now, so merging. Thank you for the contribution!

@dragonmux dragonmux merged commit b9969de into blackmagic-debug:main Jan 14, 2024
6 checks passed
@ALTracer ALTracer deleted the feature/blackpill-f4-dfu-fancy branch April 26, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dragonmux dragonmux dragonmux approved these changes

Assignees
No one assigned
Labels
Foreign Host Board Non Native hardware to runing Black Magic firmware on
Projects
None yet
Milestone
v2.0 release
Development

Successfully merging this pull request may close these issues.

blackpill-f4: bootloader LED issues
2 participants
@ALTracer @dragonmux