refactor(core): simplify fonts handling #4194

obrusvit · 2024-09-18T22:19:19Z

This PR aims to further simplify the font handling in core.

The main objective was to get rid of the numerous macros, which are quite cumbersome to work with. This is achieved by introducing struct font_info_t which is filled with needed data for each font during font generation by gen_font.py. Then there is a function get_font_info defined In the core/embed/gfx/fonts/fonts.c which is used to access font data.

TODO:

_UPPER fonts seems not effective
problems with translated strings

github-actions · 2024-09-18T22:37:50Z

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)

TychoVrahe · 2024-09-19T07:12:42Z

i think we will drop entire buffers.c/h when we get rid of old rendering, which will hopefully be soon enough.

obrusvit · 2024-09-19T11:32:37Z

All UI-diffs on TS3 seems to be caused by now usage of BOLD font in Size: x bytes which is actually correct.

obrusvit · 2024-09-19T11:36:03Z

Can the change of order in translation jsons cause some problems moving forward @matejcik ? If so, I can revert it and just define the enum font_id_t starting from 1 and not change the order. I simply though that the new ordering is a little bit more logical, e.g. having NORMAL and NORMAL_UPPER in succession.

The reason to start from 0 is to use the last enum variant FONTS_COUNT.

matejcik · 2024-09-19T11:39:36Z

yeah the ids are baked into the translation blobs, this would really mess up the upgrade/downgrade scenario. please revert.

aside, I believe that there was a reason for skipping 0 for font index, but perhaps it doesn't apply anymore with this refactor?

obrusvit · 2024-09-19T13:20:56Z

I reverted the re-assignment of font IDs. The PR is ready for review.

For future reference: after we drop buffers.h/.c when getting rid of "old rendering", then we can completely remove these definitions for all fonts (also remove this from gen_font.py and also remove the FONT_MAX_HEIGHT calculation in fonts.h
#define Font_PixelOperatorMono_Regular_8_MAX_HEIGHT 8
EDIT: the comment above applied on rebase

matejcik

overall comment: why not get rid of font_id_t and instead use const struct font_info_t* pervasively? then it's the responsibility of the (first) caller to pass in a valid font_info_t, and everyone else can look at the attributes directly.

then instead of FONT_NORMAL being an enum value, you would use &FONT_NORMAL as a pointer to a const struct font_info_t in all places

in Rust you would change pub enum Font to

pub type FontInfo = ffi::font_info_t;
pub type Font = &'static FontInfo;

impl font to impl fontinfo, modify the self owning arguments to references because FontInfo is no longer Copy/Clone, adjust the rest...

one thing we'd need to do is keep the mapping to translation blob font ids somewhere. presumably only in Rust? because it's only Rust that interfaces with translation blobs

core/embed/lib/fonts/fonts_types.h

core/embed/lib/fonts/fonts.c

Hannsek · 2024-10-10T06:50:52Z

How are we with this one? @obrusvit

obrusvit · 2024-12-02T22:49:15Z

I rebased the branch on top of main.

core/embed/rust/src/trezorhal/display.rs

core/embed/rust/src/ui/display/font.rs

matejcik · 2024-12-09T09:28:41Z

core/embed/rust/src/ui/display/font.rs

    }

    pub fn text_height(self) -> i16 {
-        display::text_height(self.into())
+        display::get_font_info(self.into()).map_or(i16::MAX, |font| font.height.try_into().unwrap())


i'm thinking, just unwrap!() the whole thing, asking for a text_height on an invalid font id should be an unrecoverable error, no? (dtto below)

I kept it the same as it is in other equivalent functions in the file. Do you insist on unwrap!() instead?

yeah, I meant in all of them, not in text_height specifically.
i think unwrap! is better here, it seems pointless to spend code on returning value for a nonexistent font?

ah but there's still an unwrap() in there. we don't want to call fn unwrap basically ever, always use unwrap!

this should be unwrap!(display::get_font_info(self.into())).height as i16 (honestly wouldn't worry about doing a try_into ... speaking of, why is it an int in the struct and not uint16_t ?)

I see. unwrap() removed.

I kept int as it was before. If you don't mind, as changing it to uint16_t is a slightly larger change (fonts generator, generated fonts, function headers and callers), I'd postpone it to another PR not to prolong this one.

not stopping the PR on it anymore but you could have avoided the map() if you just unwrap!() the result of get-font-info, which would have given you an infallible non-option struct with the height attribute

yes, removed the map on rebase

core/embed/rust/src/ui/display/font.rs

cepetr

It looks good to me. I've just commented on a few minor issues.

core/embed/gfx/fonts/fonts.h

core/embed/rust/src/ui/display/font.rs

core/embed/gfx/fonts/fonts.c

core/embed/rust/src/translations/blob.rs

core/embed/rust/src/ui/display/font.rs

core/embed/rust/src/translations/blob.rs

core/embed/rust/src/ui/display/font.rs

matejcik · 2024-12-12T12:37:06Z

core/embed/rust/src/ui/display/font.rs

@@ -85,7 +82,7 @@ impl Glyph {
        c_data >> 4
    }

-    pub fn bitmap(&self) -> Bitmap<'static> {
+    pub fn bitmap(&self) -> Bitmap<'a> {


speaking of: it looks like Bitmap::new does not check that there's enough data provided here? @cepetr can you comment?

core/embed/rust/src/ui/display/font.rs

matejcik · 2024-12-12T12:59:07Z

core/embed/rust/src/ui/display/font.rs

    }

    pub fn text_height(self) -> i16 {
-        display::text_height(self.into())
+        display::get_font_info(self.into()).map_or(i16::MAX, |font| font.height.try_into().unwrap())


yeah, I meant in all of them, not in text_height specifically.
i think unwrap! is better here, it seems pointless to spend code on returning value for a nonexistent font?

Introduce a new flag `_NAME` for each font and reduce the usage of `_ENABLE` flag to purely compilation guard. [no changelog]

This commit removes the usage of macros for font data definitions. Instead, it includes data as const structs of newly introduced font_info_t type. [no changelog] refactor(core): use font_id_t instead of plain int [no changelog]

Change to the new structures and preserve manual changes. This commit also removes duplicated definition of nonprintable glyph for _UPPER fonts. [no changelog]

obrusvit · 2024-12-14T22:58:16Z

@matejcik this check at Glyph::load discovered an error in the gen_font.py.

ensure!(data.len() == size, "Invalid glyph data size");

The problem was that padding to full bytes was applied also if the glyph was already aligned. Then the size of the data read from translation blob and the expected size calculated from width and height didn't match.
For example, the czech glyph ý was wrongly aligned causing a crash in some cs tests:

"ý": "060807000718ccf3cdf0de00",  // wrong
"ý": "060807000718ccf3cdf0de",    // fixed

Note that the problem was invisible for regular ASCII glyphs (in C arrays) because the last zero byte was simply ignored. However, I fixed it also for these cases.

Please, see c09fd18

matejcik · 2024-12-16T09:22:59Z

good fix but this also means that this will cause a nasty crash at text render time if you run with an outdated translation blob
please change the check to data.len() >= size. we don't really care about trailing garbage, the font data is high-trust.

- re-implement some fonts handling functions in Rust and delete them from C - C code only needs to handle ASCII characters [no changelog]

[no changelog]

obrusvit added core Trezor Core firmware. Runs on Trezor Model T and T2B1. code Code improvements labels Sep 18, 2024

obrusvit self-assigned this Sep 18, 2024

obrusvit force-pushed the obrusvit/simplify-fonts-handling branch from 00e4847 to 1fce051 Compare September 19, 2024 11:01

obrusvit marked this pull request as ready for review September 19, 2024 11:38

obrusvit requested review from matejcik and prusnak as code owners September 19, 2024 11:38

obrusvit force-pushed the obrusvit/simplify-fonts-handling branch from 1fce051 to 94db1ee Compare September 19, 2024 13:15

matejcik reviewed Sep 30, 2024

View reviewed changes

core/embed/lib/fonts/fonts_types.h Outdated Show resolved Hide resolved

core/embed/lib/fonts/fonts.c Outdated Show resolved Hide resolved

obrusvit force-pushed the obrusvit/simplify-fonts-handling branch from 94db1ee to 27a9b80 Compare December 2, 2024 22:46

obrusvit requested a review from cepetr December 2, 2024 22:51

obrusvit force-pushed the obrusvit/simplify-fonts-handling branch from 27a9b80 to e62b11b Compare December 2, 2024 23:23

matejcik reviewed Dec 9, 2024

View reviewed changes

core/embed/rust/src/trezorhal/display.rs Show resolved Hide resolved

matejcik reviewed Dec 9, 2024

View reviewed changes

cepetr requested changes Dec 9, 2024

View reviewed changes

core/embed/gfx/fonts/fonts.h Outdated Show resolved Hide resolved

core/embed/rust/src/ui/display/font.rs Outdated Show resolved Hide resolved

core/embed/gfx/fonts/fonts.c Outdated Show resolved Hide resolved

matejcik requested changes Dec 10, 2024

View reviewed changes

matejcik requested changes Dec 12, 2024

View reviewed changes

matejcik approved these changes Dec 13, 2024

View reviewed changes

obrusvit added 3 commits December 14, 2024 23:39

refactor(core): separate concerns of FONT macros

fd42120

Introduce a new flag `_NAME` for each font and reduce the usage of `_ENABLE` flag to purely compilation guard. [no changelog]

refactor(core): simplify font handling

d00f746

This commit removes the usage of macros for font data definitions. Instead, it includes data as const structs of newly introduced font_info_t type. [no changelog] refactor(core): use font_id_t instead of plain int [no changelog]

refactor(core): change gen_font.py and regen fonts

79ecde9

Change to the new structures and preserve manual changes. This commit also removes duplicated definition of nonprintable glyph for _UPPER fonts. [no changelog]

obrusvit force-pushed the obrusvit/simplify-fonts-handling branch from c1f3d3e to c09fd18 Compare December 14, 2024 22:43

obrusvit added the translations Put this label on a PR to run tests in all languages label Dec 14, 2024

cepetr approved these changes Dec 16, 2024

View reviewed changes

obrusvit added 3 commits December 16, 2024 12:24

refactor(core): remove UTF-8 support from C

b62cf08

- re-implement some fonts handling functions in Rust and delete them from C - C code only needs to handle ASCII characters [no changelog]

refactor(core): safe iface for get_glyph_data

487c0f6

[no changelog]

fix(core): font generator padding

a3680e6

[no changelog]

obrusvit force-pushed the obrusvit/simplify-fonts-handling branch from c09fd18 to a3680e6 Compare December 16, 2024 12:01

obrusvit merged commit 78cce0b into main Dec 16, 2024
138 of 139 checks passed

obrusvit deleted the obrusvit/simplify-fonts-handling branch December 16, 2024 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): simplify fonts handling #4194

refactor(core): simplify fonts handling #4194

obrusvit commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

TychoVrahe commented Sep 19, 2024

obrusvit commented Sep 19, 2024

obrusvit commented Sep 19, 2024 •

edited

Loading

matejcik commented Sep 19, 2024

obrusvit commented Sep 19, 2024 •

edited

Loading

matejcik left a comment

Hannsek commented Oct 10, 2024

obrusvit commented Dec 2, 2024

matejcik Dec 9, 2024

obrusvit Dec 10, 2024

matejcik Dec 12, 2024

obrusvit Dec 13, 2024

matejcik Dec 13, 2024

obrusvit Dec 13, 2024

matejcik Dec 13, 2024

obrusvit Dec 14, 2024

cepetr left a comment

matejcik Dec 12, 2024

matejcik Dec 12, 2024

obrusvit commented Dec 14, 2024

matejcik commented Dec 16, 2024

refactor(core): simplify fonts handling #4194

refactor(core): simplify fonts handling #4194

Conversation

obrusvit commented Sep 18, 2024 • edited Loading

github-actions bot commented Sep 18, 2024 • edited Loading

TychoVrahe commented Sep 19, 2024

obrusvit commented Sep 19, 2024

obrusvit commented Sep 19, 2024 • edited Loading

matejcik commented Sep 19, 2024

obrusvit commented Sep 19, 2024 • edited Loading

matejcik left a comment

Choose a reason for hiding this comment

Hannsek commented Oct 10, 2024

obrusvit commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cepetr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obrusvit commented Dec 14, 2024

matejcik commented Dec 16, 2024

obrusvit commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

obrusvit commented Sep 19, 2024 •

edited

Loading

obrusvit commented Sep 19, 2024 •

edited

Loading