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.

Sign up for GitHub

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

fix fading issues #4492

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
525a891
fix(core): fix fading issues
TychoVrahe Jan 12, 2025
1c7bde1
fixup! fix(core): fix fading issues
TychoVrahe Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/.changelog.d/4492.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[T3T1] Fixed flashing old content when fading
9 changes: 1 addition & 8 deletions core/embed/io/display/st-7789/display_fb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ void display_fb_clear(void) {
static void bg_copy_callback(void) {
display_driver_t *drv = &g_display_driver;

drv->update_pending = 1;
drv->update_pending = 2;

fb_queue_put(&drv->empty_frames, fb_queue_take(&drv->ready_frames));
}
Expand Down Expand Up @@ -267,13 +267,6 @@ void display_ensure_refreshed(void) {
irq_unlock(irq_key);
__WFI();
} while (copy_pending);

// Wait until the display is fully refreshed
// (TE signal is low when the display is updating)
while (GPIO_PIN_RESET ==
HAL_GPIO_ReadPin(DISPLAY_TE_PORT, DISPLAY_TE_PIN)) {
__WFI();
}
}
#endif
}
Expand Down
1 change: 1 addition & 0 deletions core/embed/projects/bootloader/.changelog.d/4492.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[T3T1] Fix slow fade in/non responsiveness in bootloader UI
2 changes: 2 additions & 0 deletions core/embed/rust/librust_qstr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ static void _librust_qstrs(void) {
MP_QSTR_auto_lock__change_template;
MP_QSTR_auto_lock__title;
MP_QSTR_auto_lock__turned_on;
MP_QSTR_backlight_fade;
MP_QSTR_backlight_set;
MP_QSTR_backup__can_back_up_anytime;
MP_QSTR_backup__create_backup_to_prevent_loss;
MP_QSTR_backup__info_multi_share_backup;
Expand Down
29 changes: 29 additions & 0 deletions core/embed/rust/src/ui/api/firmware_micropython.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ use crate::{
};
use heapless::Vec;

#[cfg(feature = "backlight")]
use crate::ui::display::{fade_backlight_duration, set_backlight};

/// Dummy implementation so that we can use `Empty` in a return type of
/// unimplemented trait function
impl ComponentMsgObj for Empty {
Expand Down Expand Up @@ -1035,6 +1038,24 @@ pub extern "C" fn upy_check_homescreen_format(data: Obj) -> Obj {
unsafe { util::try_or_raise(block) }
}

pub extern "C" fn upy_backlight_set(_level: Obj) -> Obj {
let block = || {
#[cfg(feature = "backlight")]
set_backlight(_level.try_into()?);
Ok(Obj::const_none())
};
unsafe { util::try_or_raise(block) }
}

pub extern "C" fn upy_backlight_fade(_level: Obj) -> Obj {
let block = || {
#[cfg(feature = "backlight")]
fade_backlight_duration(_level.try_into()?, 150);
Ok(Obj::const_none())
};
unsafe { util::try_or_raise(block) }
}

#[no_mangle]
pub static mp_module_trezorui_api: Module = obj_module! {
/// from trezor import utils
Expand Down Expand Up @@ -1141,6 +1162,14 @@ pub static mp_module_trezorui_api: Module = obj_module! {
/// """Disable animations, debug builds only."""
Qstr::MP_QSTR_disable_animation => obj_fn_1!(upy_disable_animation).as_obj(),

/// def backlight_set(level: int) -> None:
/// """Set backlight to desired level."""
Qstr::MP_QSTR_backlight_set => obj_fn_1!(upy_backlight_set).as_obj(),

/// def backlight_fade(level: int) -> None:
/// """Fade backlight to desired level."""
Qstr::MP_QSTR_backlight_fade => obj_fn_1!(upy_backlight_fade).as_obj(),

/// def confirm_action(
/// *,
/// title: str,
Expand Down
17 changes: 16 additions & 1 deletion core/embed/rust/src/ui/display/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ use crate::{strutil::TString, trezorhal::display};
#[cfg(feature = "backlight")]
use crate::ui::lerp::Lerp;

#[cfg(feature = "backlight")]
use crate::{time::Instant, ui::util::animation_disabled};

// Reexports
pub use crate::ui::display::toif::Icon;
pub use color::Color;
Expand Down Expand Up @@ -42,7 +45,19 @@ pub fn fade_backlight_duration(target: u8, duration_ms: u32) {
let duration_ms = duration_ms as i32;
let current = backlight() as i32;

for i in 0..duration_ms {
if animation_disabled() {
set_backlight(target as u8);
return;
}

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?)


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

.to_millis();
let val = i32::lerp(current, target, i as f32 / duration_ms as f32);
set_backlight(val as u8);
time::sleep(Duration::from_millis(1));
Expand Down
25 changes: 0 additions & 25 deletions core/embed/upymod/modtrezorui/modtrezorui-display.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,29 +103,6 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_trezorui_Display_orientation_obj,
1, 2,
mod_trezorui_Display_orientation);

/// def backlight(self, val: int | None = None) -> int:
/// """
/// Sets backlight intensity to the value specified in val.
/// Call without the val parameter to just perform the read of the value.
/// """
STATIC mp_obj_t mod_trezorui_Display_backlight(size_t n_args,
const mp_obj_t *args) {
mp_int_t val;
if (n_args > 1) {
val = mp_obj_get_int(args[1]);
if (val < 0 || val > 255) {
mp_raise_ValueError("Value must be between 0 and 255");
}
val = display_set_backlight(val);
} else {
val = display_get_backlight();
}
return MP_OBJ_NEW_SMALL_INT(val);
}
STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_trezorui_Display_backlight_obj,
1, 2,
mod_trezorui_Display_backlight);

/// def save(self, prefix: str) -> None:
/// """
/// Saves current display contents to PNG file with given prefix.
Expand Down Expand Up @@ -162,8 +139,6 @@ STATIC const mp_rom_map_elem_t mod_trezorui_Display_locals_dict_table[] = {
{MP_ROM_QSTR(MP_QSTR_bar), MP_ROM_PTR(&mod_trezorui_Display_bar_obj)},
{MP_ROM_QSTR(MP_QSTR_orientation),
MP_ROM_PTR(&mod_trezorui_Display_orientation_obj)},
{MP_ROM_QSTR(MP_QSTR_backlight),
MP_ROM_PTR(&mod_trezorui_Display_backlight_obj)},
{MP_ROM_QSTR(MP_QSTR_save), MP_ROM_PTR(&mod_trezorui_Display_save_obj)},
{MP_ROM_QSTR(MP_QSTR_clear_save),
MP_ROM_PTR(&mod_trezorui_Display_clear_save_obj)},
Expand Down
6 changes: 0 additions & 6 deletions core/mocks/generated/trezorui.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,6 @@ class Display:
value.
"""

def backlight(self, val: int | None = None) -> int:
"""
Sets backlight intensity to the value specified in val.
Call without the val parameter to just perform the read of the value.
"""

def save(self, prefix: str) -> None:
"""
Saves current display contents to PNG file with given prefix.
Expand Down
10 changes: 10 additions & 0 deletions core/mocks/generated/trezorui_api.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ def disable_animation(disable: bool) -> None:
"""Disable animations, debug builds only."""


# rust/src/ui/api/firmware_micropython.rs
def backlight_set(level: int) -> None:
"""Set backlight to desired level."""


# rust/src/ui/api/firmware_micropython.rs
def backlight_fade(level: int) -> None:
"""Fade backlight to desired level."""


# rust/src/ui/api/firmware_micropython.rs
def confirm_action(
*,
Expand Down
3 changes: 2 additions & 1 deletion core/src/apps/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


from . import workflow_handlers

Expand Down Expand Up @@ -454,7 +455,7 @@ def reload_settings_from_storage() -> None:
storage_device.get_experimental_features()
)
if ui.display.orientation() != storage_device.get_rotation():
ui.backlight_fade(ui.BacklightLevels.DIM)
backlight_fade(ui.BacklightLevels.DIM)
ui.display.orientation(storage_device.get_rotation())


Expand Down
5 changes: 3 additions & 2 deletions core/src/boot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


from apps.common.request_pin import can_lock_device, verify_user_pin

Expand Down Expand Up @@ -61,7 +62,7 @@ async def bootscreen() -> None:
if can_lock_device():
enforce_welcome_screen_duration()
if utils.INTERNAL_MODEL == "T2T1":
ui.backlight_fade(ui.BacklightLevels.NONE)
backlight_fade(ui.BacklightLevels.NONE)
ui.display.orientation(storage.device.get_rotation())
if utils.USE_HAPTIC:
io.haptic.haptic_set_enabled(storage.device.get_haptic_feedback())
Expand All @@ -88,7 +89,7 @@ async def bootscreen() -> None:
# there is a slight delay before next screen is shown,
# so we don't fade unless there is a change of orientation
if utils.INTERNAL_MODEL == "T2T1":
ui.backlight_fade(ui.BacklightLevels.NONE)
backlight_fade(ui.BacklightLevels.NONE)
ui.display.orientation(rotation)
allow_all_loader_messages()
return
Expand Down
34 changes: 11 additions & 23 deletions core/src/trezor/ui/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,13 @@
from trezor.messages import ButtonAck, ButtonRequest
from trezor.wire import context
from trezor.wire.protocol_common import Context
from trezorui_api import AttachType, BacklightLevels, LayoutState
from trezorui_api import (
AttachType,
BacklightLevels,
LayoutState,
backlight_fade,
backlight_set,
)

if TYPE_CHECKING:
from typing import Any, Callable, Generator, Generic, Iterator, TypeVar
Expand Down Expand Up @@ -67,12 +73,12 @@ async def _alert(count: int) -> None:
long_sleep = loop.sleep(80)
for i in range(count * 2):
if i % 2 == 0:
display.backlight(BacklightLevels.MAX)
backlight_set(BacklightLevels.MAX)
await short_sleep
else:
display.backlight(BacklightLevels.DIM)
backlight_set(BacklightLevels.DIM)
await long_sleep
display.backlight(BacklightLevels.NORMAL)
backlight_set(BacklightLevels.NORMAL)
global _alert_in_progress
_alert_in_progress = False

Expand All @@ -87,24 +93,6 @@ def alert(count: int = 3) -> None:
loop.schedule(_alert(count))


def backlight_fade(val: int, delay: int = 14000, step: int = 15) -> None:
if utils.USE_BACKLIGHT:
if __debug__:
if utils.DISABLE_ANIMATION:
display.backlight(val)
return
current = display.backlight()
if current < 0:
display.backlight(val)
return
elif current > val:
step = -step
for i in range(current, val, step):
display.backlight(i)
utime.sleep_us(delay)
display.backlight(val)


class Shutdown(Exception):
pass

Expand Down Expand Up @@ -526,9 +514,9 @@ def start(self) -> None:

self.layout.request_complete_repaint()
painted = self.layout.paint()
backlight_fade(BacklightLevels.NORMAL)
if painted:
refresh()
backlight_fade(BacklightLevels.NORMAL)

def stop(self) -> None:
global CURRENT_LAYOUT
Expand Down
3 changes: 2 additions & 1 deletion core/tests/test_trezor.ui.display.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from common import * # isort:skip

from trezor.ui import display
from trezorui_api import backlight_set


class TestDisplay(unittest.TestCase):
Expand All @@ -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


def test_raw(self):
pass
Expand Down
Loading