Skip to content
This repository was archived by the owner on Mar 7, 2026. It is now read-only.

Feature: Space-efficient 12-symbol serialno generation#1716

Merged
dragonmux merged 3 commits into
blackmagic-debug:mainfrom
ALTracer:feature/utoa_upper
Jan 7, 2024
Merged

Feature: Space-efficient 12-symbol serialno generation#1716
dragonmux merged 3 commits into
blackmagic-debug:mainfrom
ALTracer:feature/utoa_upper

Conversation

@ALTracer
Copy link
Copy Markdown
Contributor

@ALTracer ALTracer commented Jan 7, 2024

  • This can be considered a new feature (because I don't see how it was usable previously)
  • There is an existing problem of BMPBootloader with DFU_SERIAL_LENGTH=13 becoming larger than 16 KiB due to siprintf pulled from newlib (not nano).
  • The PR solves this problem by replacing the single expensive sprintf() call with a few calls of simpler functions and introducing a variant of utoa().

12-symbol serialno consists of three halfwords calculated by pulling 16-bit values from Device Electronic Signature memory. I don't believe hexify() can be useful here.

I would not like to choose to mitigate this by downgrading the F4 BMPBootloader to newlib-nano because it would provide a small and inefficient memcpy, which is important for flash writes/upgrade reprogramming speed. And it's not like a bootloader would use *printf anyway.

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) -- and is not applicable
  • It builds as BMDA (make PROBE_HOST=hosted) -- and is not applicable
  • 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

Copy link
Copy Markdown
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 like the idea of this PR, the execution needs some love though - please see our review notes below.

Comment thread src/platforms/common/stm32/serialno.c Outdated
Comment thread src/platforms/common/stm32/serialno.c Outdated
Comment thread src/platforms/common/stm32/serialno.c Outdated
Comment thread src/platforms/common/stm32/serialno.c Outdated
Comment thread src/platforms/common/stm32/serialno.c Outdated
Comment thread src/platforms/common/stm32/serialno.c Outdated
Comment thread src/platforms/common/stm32/serialno.c Outdated
@dragonmux dragonmux added this to the v2.0 release milestone Jan 7, 2024
@dragonmux dragonmux added Enhancement General project improvement BMP Firmware Black Magic Probe Firmware (not PC hosted software) Foreign Host Board Non Native hardware to runing Black Magic firmware on labels Jan 7, 2024
Comment thread src/platforms/common/stm32/serialno.c Outdated
@ALTracer ALTracer force-pushed the feature/utoa_upper branch from addabe3 to 5f63e54 Compare January 7, 2024 12:46
Comment thread src/platforms/common/stm32/serialno.c
Comment thread src/platforms/common/stm32/serialno.c Outdated
Copy link
Copy Markdown
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.

Most of the review items have been addressed and this is looking better for it - there are a couple of things we've spotted this run through and think once those are addressed this should be about ready for merge if you could squash commits appropriately.

Comment thread src/platforms/common/stm32/serialno.c Outdated
Comment thread src/platforms/common/stm32/serialno.c Outdated
@ALTracer ALTracer force-pushed the feature/utoa_upper branch from a0f92d8 to 0690b80 Compare January 7, 2024 17:56
dragonmux
dragonmux previously approved these changes Jan 7, 2024
Copy link
Copy Markdown
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 - we're happy to merge this once the copyright and authorship notice issue is fixed as noted on Discord.

ALTracer and others added 3 commits January 7, 2024 21:21
* Use programming practices applicable to bootloaders
* Keep a 32-byte static buffer in local scope

Co-Authored-By: dragonmux <git@dragonmux.network>
* STM32F4 case with 12-byte serial numbers simply relied on sprintf()
* Use three calls to `utoa_upper()` to build the `serial_no[]` instead
* This drastically reduces F4 usbdfu BMPBootloader binary size
@ALTracer
Copy link
Copy Markdown
Contributor Author

ALTracer commented Jan 7, 2024

Squashed intermediate progress into two distinct commits: one adding the function, one adjusting the callsite to use it (so no builds fail). Added a separate commit to refresh copyright info on the file.

Thank you for propelling this PR with your extensive review. We're one step closer to making BMPBootloader/F4 DFU nicer to use (and went through the practice of adding a helper function).

@dragonmux
Copy link
Copy Markdown
Member

Merging, thank you for the contribution!

@dragonmux dragonmux merged commit 6261eb5 into blackmagic-debug:main Jan 7, 2024
@ALTracer ALTracer deleted the feature/utoa_upper branch April 26, 2024 17:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

BMP Firmware Black Magic Probe Firmware (not PC hosted software) Enhancement General project improvement Foreign Host Board Non Native hardware to runing Black Magic firmware on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants