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 fading issues #4492

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

Conversation

TychoVrahe
Copy link
Contributor

@TychoVrahe TychoVrahe commented Jan 12, 2025

fixes #4392 fixes #4465

The slow fade in bootloader was due to function raising backlight was taking too long, waiting for display TE signal each time. Waiting for TE is removed as its no longer needed due to different mechanism using pending update - so now we will normally wait only once, after the display content is changed. In addition, the backlight fading function is rewritten to ensure constant fading time, at the possible expense of smoothness.

To mitigate the residual image issue, additional wait cycle is added when raising backlight.

As part of this fix i did i minor refactor, fading from python is now done by calling rust functions. The behavior of the fade is unified, so fading from python will behave a bit differently

Additional problem with fading is done before display refresh in progress layouts is fixed.

@TychoVrahe TychoVrahe self-assigned this Jan 12, 2025
Copy link

github-actions bot commented Jan 12, 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)

@TychoVrahe TychoVrahe force-pushed the tychovrahe/bootloader/slow_fade_fix branch 2 times, most recently from a0e930b to 500ecf5 Compare January 12, 2025 16:35
@TychoVrahe TychoVrahe force-pushed the tychovrahe/bootloader/slow_fade_fix branch from 500ecf5 to 525a891 Compare January 13, 2025 08:32
@TychoVrahe TychoVrahe marked this pull request as ready for review January 13, 2025 08:48
@TychoVrahe TychoVrahe requested review from cepetr and removed request for prusnak January 13, 2025 08:48
@@ -8,6 +8,7 @@
from trezor.ui.layouts import confirm_action
from trezor.wire import context
from trezor.wire.message_handler import filters, remove_filter
from trezorui_api import backlight_fade
Copy link
Contributor

Choose a reason for hiding this comment

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

trezor.ui imports backlight_fade symbol so changes to this file are not needed

(also we generally don't import trezorfoo modules directly in app code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 1c7bde1

core/src/boot.py Outdated
@@ -17,6 +17,7 @@
show_pin_timeout,
)
from trezor.ui.layouts.homescreen import Lockscreen
from trezorui_api import backlight_fade
Copy link
Contributor

Choose a reason for hiding this comment

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

trezor.ui imports backlight_fade symbol so changes to this file are not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 1c7bde1

@@ -17,7 +18,7 @@ def test_orientation(self):

def test_backlight(self):
for b in range(256):
display.backlight(b)
backlight_set(b)
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if there's any point in keeping this testcase around? it's just a smoke test for existence of the function, we're not verifying any result here 🤷‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, we should either make something useful of it or remove it. I'd say its out of scope here though

while Instant::now() < end {
let i = Instant::now()
.checked_duration_since(start)
.unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

unwrap! please

here's a slightly tidier version of this function though:

#[cfg(feature = "backlight")]
pub fn fade_backlight_duration(target: u8, duration_ms: u32) {
    let target = target as i32;
    let current = backlight() as i32;
    let duration = Duration::from_millis(duration_ms);

    if animation_disabled() {
        set_backlight(target as u8);
        return;
    }

    let mut now = Instant::now();
    let start = now;
    let end = unwrap!(start.checked_add(duration));

    while now < end {
        let elapsed = unwrap!(now.checked_duration_since(start));
        let val = i32::lerp(current, target, elapsed / duration);
        set_backlight(val as u8);
        time::sleep(Duration::from_millis(1));
        now = Instant::now();
    }
    //account for imprecise rounding
    set_backlight(target as u8);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see below

Copy link
Contributor

@cepetr cepetr left a comment

Choose a reason for hiding this comment

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

It looks like the problems have been solved. I just added a comment suggesting a minor improvement.

}

let start = Instant::now();
let end = unwrap!(start.checked_add(Duration::from_millis(duration_ms as _)));
Copy link
Contributor

Choose a reason for hiding this comment

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

There’s a small chance that the app might crash when the timer wraps from its maximum value to 0. How about using StopWatch instead of the timer here? It could slightly simplify the code and resolve this minor issue:

    let timer = Stopwatch::new_started();

    while timer.elapsed() < Duration::from_millis(duration_ms as _) {
        let elapsed = timer.elapsed().to_millis();
        let val = i32::lerp(current, target, elapsed as f32 / duration_ms as f32);
        set_backlight(val as u8);
        time::sleep(Duration::from_millis(1));
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I suspected there's an existing code that could help here.

(also note that Durations support division and the result is a f32, see my suggested version of this function)

while you're touching this, please also convert this unwrap() to unwrap!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to the combo of your suggestions: 1c7bde1

i replaced that unwrap(), but note that there are unwrap()s in bunch of other places in our rust codebase, so probably some larger cleanup should take place. (would there be a way to automatically check for this?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 Needs review
Development

Successfully merging this pull request may close these issues.

Display flashes old content before refreshing T3T1 - slow screen fading in bootloader
3 participants