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.

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

Fix MNTM-DEV git hash formatting #104

Merged
merged 4 commits into from
Apr 22, 2024
Merged

Conversation

zacharyweiss
Copy link
Contributor

@zacharyweiss zacharyweiss commented Apr 21, 2024

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

  • I've uploaded the firmware with this patch to a device and verified its functionality
  • I've confirmed the bug to be fixed / feature to be stable

@Willy-JL
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

This reverts commit c7cb790.
Copy link
Member

@Willy-JL Willy-JL left a comment

Choose a reason for hiding this comment

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

LGTM! thanks

@Willy-JL Willy-JL merged commit 9d4a6e6 into Next-Flip:dev Apr 22, 2024
2 checks passed
@Willy-JL Willy-JL added the bugfix Something isn't working label Apr 22, 2024
@Willy-JL
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants