-
-
Notifications
You must be signed in to change notification settings - Fork 671
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
base: main
Are you sure you want to change the base?
fix fading issues #4492
Conversation
|
a0e930b
to
500ecf5
Compare
500ecf5
to
525a891
Compare
core/src/apps/base.py
Outdated
@@ -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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 🤷♀️
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see below
There was a problem hiding this 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 _))); |
There was a problem hiding this comment.
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));
}
There was a problem hiding this comment.
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 Duration
s 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!
There was a problem hiding this comment.
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?)
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.