Next-Flip / Momentum-Firmware

🐬 Feature-rich, stable and customizable Flipper Firmware
https://momentum-fw.dev
GNU General Public License v3.0
4.29k stars 167 forks source link

Fix MNTM-DEV git hash formatting #104

Closed zacharyweiss closed 5 months ago

zacharyweiss commented 5 months ago

What's new

For a (currently unknown) reason, the built version.inc.h #define GIT_COMMIT on my dev branch commit cacdc68 was populated with the first 8 characters of the commit hash when built, rather than the standard first 7 characters. As a result, when displayed in momentum_app_scene_start the hash header spills over the edge of the screen.

While I don't diagnose what causes the oddity in # chars of the built GIT_COMMIT, this PR adds a simple max strlen of 7 for the displayed hash. strlen still checked, in the event there are fewer than 7 chars.


For the reviewer

Willy-JL commented 5 months ago

Good catch. Still I'll try to find why it's 8 instead of 7. Honestly it might just be that we are starting to get ambiguous commit hashes? Sounds far fetched but maybe

zacharyweiss commented 5 months ago

Definitely an odd bug; just popped this superficial fix on as it was bugging me this morning. Do you know where in the build tools / code those version defines get made? Haven't spent any time digging through fbt / the toolchain yet, so wasn't sure where to start looking for the root cause.

zacharyweiss commented 5 months ago

Honestly it might just be that we are starting to get ambiguous commit hashes?

Gotta start running chosen prefix attacks to test how the system handles ambiguous truncated hashes lmao

zacharyweiss commented 5 months ago

Found it. The script had 8 hardcoded as the length; must've just gotten lucky with the character widths up till now for the 8th character to be fully hidden offscreen (is the font here not fixed-width?).

Willy-JL commented 5 months ago

the font is not fixed width unfortunately, its the primary font used for titles and such.

considering length of 8 hardcoded in fbt, there could be an expectation that this be respected across different firmwares, so im not sure if changing it here without discussing with OFW would be a good idea, especially considering flipperdevices/flipperzero-firmware#2497, ill ask aku or zlo if they know anything

zacharyweiss commented 5 months ago

Good thinking. At worst can leave as just the fix at the display step

Willy-JL commented 5 months ago

for some context, heard back from aku that it always was 8, and at some point it became 7 on some systems so they fixed inconsistencies like that. ill double check the devbuild system and artifacts and decide what to do for those, but for the actual githash in firmware, that should stay 8 charatcers

Willy-JL commented 5 months ago

indeed, the CI build used 8 characters like that, while the local build used 7 since i made that and never noticed. atleast the devbuilds part doesnt need changes