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

Refactor fonts handling to rust #4536

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

obrusvit
Copy link
Contributor

@obrusvit obrusvit commented Jan 24, 2025

This PR changes the font handling from enum Font to direct usage of struct FontInfo. While doing so, it became apparent that generating rust files instead of C is a better way go. The main problem was that const C variables are bound to static variables in Rust (and static variables cannot be used in const context). Each font is supplied in the following struct:

pub struct FontInfo {
    pub height: i32,
    pub max_height: i16,
    pub baseline: i16,
    pub glyph_data: &'static [&'static [u8]],
    pub glyph_nonprintable: &'static [u8],
}

// with a convenient typedef to be passed around similarly to the previous `enum Font`:
pub type Font = &'static FontInfo;

The benefits are:

  • getting rid of enum Font allows more flexible fonts handling - each layout can now specify the fonts and their operational names on their own
    • for the ease of refactoring and review, the old names were preserved
    • this will mainly benefit Eckhart, which will have more fonts not easily mapped to the enum Font
    • to fully use this benefit we need to get rid of using Font in
      • FormattedText where it should be replaced by TextStyle
      • Paragraphs - usage inChecklist, which should be separated by layouts or implemented through FormattedText
  • usage is safer because getting font data is conditioned by using actual variable (we got rid of mapping of enum value to some data which might be null), this results in removing some unsafe blocks and unwraps
  • the Rust code is hopefully easier to understand because we got rid of bindings to C
  • the font data is now organized based on layouts

The drawbacks are:

  • we need to keep the old font handling for now for prodtest and bootloader_ci. This will be a private impl.
  • we need to preserve mapping to translation blob font IDs - defined in core/embed/rust/src/ui/display/fonts/mod.rs

Measurement of FLASH size on T3T1 showed negligible increase by 1 block only (512 B).

TODO:

  • the prodtest and bootloader_ci build does not work yet - trying to have one or two const font_info_t*
  • figure out having "conflicting" names across layouts for clippy compiling with all features
  • previously SCons incorporated only some source files based on stage (either "bootloader" or "firmware"). I'm not sure that the current approach gets this right. Only some fonts are used from the bootloader so I pressume that the compiler does not include the others for bootloader binary.
  • ideally get rid of the C fonts completely
  • should review the use of USE_RGB_COLOURS

- application layer should not deal with fonts at all
- distinction between MONO and others is preserved by bool argument in
`should_show_more` interpreted as `is_data`

[no changelog]
- font usage changed from enum Font to pointers to FontInfo structs
- components from all layouts and also common components changed

[no changelog]
@obrusvit obrusvit added core Trezor Core firmware. Runs on Trezor Model T and T2B1. code Code improvements rust Pull requests that update Rust code labels Jan 24, 2025
@obrusvit obrusvit self-assigned this Jan 24, 2025
@obrusvit obrusvit requested review from TychoVrahe and removed request for prusnak January 24, 2025 22:25
Copy link

github-actions bot commented Jan 24, 2025

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@obrusvit
Copy link
Contributor Author

Unfortunately, designated initializer is not possible if FONT_NORMAL is of type const font_info_t* because initializer element is not a compile-time constant.

gfx_text_attr_t normal_text_attr = {
    .font = FONT_NORMAL,
    .fg_color = COLOR_BL_FG,
    .bg_color = COLOR_BL_BG,
};

Alternative for bootloader_ci and prodtest could be either:

  • initializing them in main()
  • reverting to enum font_id_t with just NORMAL and BOLD
    WDYT @TychoVrahe

@TychoVrahe
Copy link
Contributor

I think we can revert to font_id_t in C code. (and aim for moving the rest of the UI to rust sooon).

While we are structuring the fonts by layouts, maybe we can move the to the existing layout folders, i.e.:

/ui/layout_bolt/fonts/...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements core Trezor Core firmware. Runs on Trezor Model T and T2B1. rust Pull requests that update Rust code
Projects
Status: 🔎 Needs review
Development

Successfully merging this pull request may close these issues.

2 participants