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

Improve error handling / RSOD screen #3938

Merged
merged 5 commits into from
Jun 17, 2024
Merged

Improve error handling / RSOD screen #3938

merged 5 commits into from
Jun 17, 2024

Conversation

cepetr
Copy link
Contributor

@cepetr cepetr commented Jun 11, 2024

This PR improves error handling in both C and Rust code:

and finally saves about 12KB of FLASH (without affecting the functionality)

  1. Multiple implementations of error handling code were removed from common.c/common.h and moved to the platform-independent lib/error_handling.h/.c.
  2. Duplicated code, especially in the simulator, was removed and replaced by calls to error_shutdown().
  3. Fixed invalid footer text in show_pin_too_many_screen() and show_wipe_code_screen().
  4. Simplified the interface of error_shutdown() and introduced error_shutdown_ex() for more complex cases.
  5. Replaced calls to __fatal_error() with error_shutdown() as the former is low-level and private to error_handling.c.
  6. Moved show_install_restricted_screen() to a more appropriate location (from secret.c to error_handling.c).
  7. Final error handling for Bootloader/Firmware is now done in Rust in the error_shutdown() function, eliminating dual implementations in Rust and C.
  8. Fixed the problem with nested calls to render_on_display when an error occurred during drawing on the T3T1 model.
  9. Introduced the dbg_trace! macro, a print!-like macro for printing to the debug console.
  10. Added missing error locations in console output when raising ensure!, unwrap!, and fatal_error!.
  11. Replaced calls to panic! by calls to fatal_error!. fatal_error! has now just single parameter - message
  12. Redefined assert() and ensure() macro to not print evaluated expression and filename with full path.
  13. Also finally fixed Cleanup error_shutdown vs ensure(secfalse,msg) #744

IMPORTANT: Unfortunately, (12) triggered a wave of changes in storage and legacy code, just because of the shared prototype of __fatal_error(). All affected code was changed appropriately, and as a result, the T1 model no longer reports the expression and function name on the fatal error screen (there's just the file and line - which is still more than on other models).

@cepetr cepetr added T2B1 Trezor Safe 3 (F4) T2T1 Trezor Model T T3T1 Trezor Safe 5 labels Jun 11, 2024
@cepetr cepetr requested review from matejcik and TychoVrahe June 11, 2024 12:15
@cepetr cepetr self-assigned this Jun 11, 2024
@cepetr cepetr force-pushed the cepetr/rsod-fix branch from 08befee to b2b31b1 Compare June 11, 2024 13:15
Copy link

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

@cepetr cepetr force-pushed the cepetr/rsod-fix branch 2 times, most recently from dd3e4c2 to ae7618d Compare June 12, 2024 09:51
@cepetr cepetr marked this pull request as ready for review June 12, 2024 10:06
@cepetr cepetr added the T1B1 legacy Trezor One label Jun 12, 2024
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

very nice. OK from me, someone else should also look at it from embedded POV.

while you're at this, maybe you would like to look at #744 ? (not required for this PR but it might be an opportunity to fold it in)

core/embed/rust/src/lib.rs Show resolved Hide resolved
core/embed/rust/src/trezorhal/fatal_error.rs Show resolved Hide resolved
core/embed/rust/src/ui/api/common_c.rs Show resolved Hide resolved
core/embed/rust/src/ui/shape/display/bumps.rs Outdated Show resolved Hide resolved
@cepetr
Copy link
Contributor Author

cepetr commented Jun 13, 2024

while you're at this, maybe you would like to look at #744 ? (not required for this PR but it might be an opportunity to fold it in)

It looks like error_shutdown(..) can now perfectly replace the ensure(secfalse, ...) macro. The thing to consider is that we will lose file and line information in debug builds. If this is not a problem (and I think it isn't since all these ensure(secfalse instances contain quite unique messages), I can make this change within this PR. Should I? @matejcik

@matejcik
Copy link
Contributor

f this is not a problem (and I think it isn't since all these ensure(secfalse instances contain quite unique messages)

yeah, exactly.
please go ahead and do it

@cepetr
Copy link
Contributor Author

cepetr commented Jun 13, 2024

f this is not a problem (and I think it isn't since all these ensure(secfalse instances contain quite unique messages)

yeah, exactly. please go ahead and do it

I made the changes in commit af45de2:

  1. In the storage, I've replaced ensure(secfalse with handle_fault.
  2. In the storage tests, I've left it unchanged.
  3. I've also change it Ii the display/i2c drivers, but we should not call it at all and return with an error instead. However, this is not related to this PR.

@matejcik

@matejcik
Copy link
Contributor

matejcik commented Jun 13, 2024

i don't think that is correct, as handle_fault will additionally bump the fault counter, which makes no sense in that location (we wiped the storage right before)

Aaaa, I didn't notice that. So I reverted this change here. In this file, it seems better to be consistent and not introduce a call to error_shutdown. See 8b23004

@matejcik
Copy link
Contributor

Aaaa, I didn't notice that. So I reverted this change here. In this file, it seems better to be consistent and not introduce a call to error_shutdown. See 8b23004

it's helpful if you post in your own comment, instead of editing mine :)

otherwise, all good from my side

core/embed/lib/error_handling.c Outdated Show resolved Hide resolved
core/embed/lib/error_handling.c Outdated Show resolved Hide resolved
core/embed/rust/src/trezorhal/fatal_error.rs Show resolved Hide resolved
core/embed/rust/src/trezorhal/fatal_error.rs Outdated Show resolved Hide resolved
@TychoVrahe TychoVrahe linked an issue Jun 17, 2024 that may be closed by this pull request
@cepetr cepetr force-pushed the cepetr/rsod-fix branch from 640dee6 to cf2d942 Compare June 17, 2024 14:31
@cepetr cepetr merged commit ba56ff8 into main Jun 17, 2024
81 of 83 checks passed
@cepetr cepetr deleted the cepetr/rsod-fix branch June 17, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1B1 legacy Trezor One T2B1 Trezor Safe 3 (F4) T2T1 Trezor Model T T3T1 Trezor Safe 5
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

fatal error during rendering causes nested run_with_bumps Cleanup error_shutdown vs ensure(secfalse,msg)
3 participants