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

refactor(core/mercury): separate upy args parsing #4182

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

Conversation

obrusvit
Copy link
Contributor

@obrusvit obrusvit commented Sep 15, 2024

This PR separates the Rust flows from micropython layer.

The idea is to do the uPy argument parsing in ui/model_mercury/layout.rs and then pass them as regular arguments to code in ui/model_mercury/flow/**.rs, i.e. they won't be passed as args: &[Obj], kwargs: &Map. The goal is to keep the rust flows independent from micropython sub-crate. This separation is done for two reasons:

  • it simplifies the code in for flows creation which
  • it paves the way for apps done completely in Rust

This PR has no impact on UI or features.

TODO:

  • some lifetime issues to FIX
  • there is a few places with the usage of Obj and IterBuf
    • last place is get_address.rs, postponing this for now

@obrusvit obrusvit added code Code improvements T3T1 labels Sep 15, 2024
@obrusvit obrusvit self-assigned this Sep 15, 2024
@obrusvit
Copy link
Contributor Author

Similar thing was done here for bootloader and "common". Need to take a look how to take the same/similar approach for firmware. #3658

@obrusvit obrusvit force-pushed the obrusvit/mercury/refactor-rust-layout-upy-parsing branch 4 times, most recently from b616b27 to 58347bc Compare September 30, 2024 10:28
@obrusvit obrusvit marked this pull request as ready for review September 30, 2024 10:30
@obrusvit obrusvit requested review from mmilata and removed request for prusnak September 30, 2024 10:30
@obrusvit
Copy link
Contributor Author

obrusvit commented Sep 30, 2024

The only conflict with #3686 should be: Layout::new moved to layout.rs and renamed to Layout::new_root

Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Wrote some thoughts inline but overall this should be a harmless refactor that can be merged when CI passes.

case_sensitive: bool,
account: Option<TString<'static>>,
path: Option<TString<'static>>,
xpubs: Obj, // TODO: get rid of Obj
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this can use some kind of closure?

@@ -1540,16 +1866,16 @@ pub static mp_module_trezorui2: Module = obj_module! {
/// br_code: ButtonRequestType,
/// br_name: str,
/// ) -> LayoutObj[tuple[UiResult, int]]:
/// """Numer input with + and - buttons, description, and context menu with cancel and
/// """Numebr input with + and - buttons, description, and context menu with cancel and
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// """Numebr input with + and - buttons, description, and context menu with cancel and
/// """Number input with + and - buttons, description, and context menu with cancel and

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making a typo while fixing a typo.
Fixed on rebase.

matejcik and others added 16 commits October 24, 2024 14:32
It is being moved into a custom handler for the debug app.
otherwise if _handle_single_message raises an exception (which is
fortunately not something that should generally happen), the
finally-block would fail on "referenced before assignment"
* RequestPaint message is sent at construction time to force calculation
  of number of pages
* given that Attach corresponds to "start the layout" message, Child now
  responds to Attach the same way it responds to RequestPaint, by
  force-repainting everything.
This doesn't actually advance the progress while device erase is ongoing, but at least the user sees a loader.
@obrusvit obrusvit force-pushed the obrusvit/mercury/refactor-rust-layout-upy-parsing branch from 58347bc to 57d2b15 Compare October 26, 2024 12:23
Copy link

github-actions bot commented Oct 26, 2024

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)

Copy link

github-actions bot commented Oct 26, 2024

legacy UI changes device test(screens) main(screens)

@obrusvit obrusvit changed the base branch from main to matejcik/global-layout-only3 October 26, 2024 12:24
@obrusvit
Copy link
Contributor Author

Re-targeting to GFL branch #3686 and it can go right after.

@obrusvit obrusvit removed the request for review from andrewkozlik October 26, 2024 12:25
Supply the rust layout with dedicated paremeter type instead of plain
micropython::Obj. The types used are ConfirmBlobParams and
ShowInfoParams.

[no changelog]
@obrusvit obrusvit force-pushed the obrusvit/mercury/refactor-rust-layout-upy-parsing branch from 57d2b15 to 5086bd3 Compare October 27, 2024 11:55
@obrusvit obrusvit added the blocked Blocked by external force. Third party inputs required. label Nov 1, 2024
@matejcik matejcik force-pushed the matejcik/global-layout-only3 branch 4 times, most recently from 9297722 to 6b8585b Compare November 12, 2024 15:21
Base automatically changed from matejcik/global-layout-only3 to main November 12, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked by external force. Third party inputs required. code Code improvements T3T1
Projects
Status: 🏃‍♀️ In progress
Development

Successfully merging this pull request may close these issues.

4 participants