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

Add Rust UI and layouts #1542

Merged
merged 3 commits into from Oct 7, 2021
Merged

Add Rust UI and layouts #1542

merged 3 commits into from Oct 7, 2021

Conversation

jpochyla
Copy link
Contributor

@jpochyla jpochyla commented Mar 23, 2021

PoC

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.

This is shaping up nicely!

I'm wondering how we can use different component (and layout) directories when compiling for T1 and for TT. Can we just wrap two versions of module in e.g. #[cfg(feature = "model_1")] or does this need to be handled by build.rs?

I'm a bit tempted to start implementing the T1 components since we don't have all of them in python yet. Then we could do it like this?

  • implement T1 components in rust, keep TT components in python, both layouts in python
  • swap TT components for rust versions
  • eventually rewrite layout functions to rust

We'll probably still need a python layer of layouts to handle ButtonRequests/other async stuff.

Is there anything missing that would be needed to implement e.g. confirm_action layout using the rust ui?

core/embed/rust/src/ui/component/button.rs Outdated Show resolved Hide resolved
core/embed/rust/src/ui/component/component.rs Outdated Show resolved Hide resolved
@jpochyla
Copy link
Contributor Author

This is shaping up nicely!

<3

I'm wondering how we can use different component (and layout) directories when compiling for T1 and for TT. Can we just wrap two versions of module in e.g. #[cfg(feature = "model_1")] or does this need to be handled by build.rs?

It doesn't need any changes in the build script. In case you want to export same (or very compatible) interface, but use different impl under each feature (or a platform), see here for an example, it's a rather common Rust pattern. If you just want to conditionally include a module, under it's usual path, #[cfg(feature = "model_1")] is enough.

I'm a bit tempted to start implementing the T1 components since we don't have all of them in python yet. Then we could do it like this?

* implement T1 components in rust, keep TT components in python, both layouts in python

* swap TT components for rust versions

* eventually rewrite layout functions to rust

We'll probably still need a python layer of layouts to handle ButtonRequests/other async stuff.

Is there anything missing that would be needed to implement e.g. confirm_action layout using the rust ui?

I don't think anything substantial is missing. Although I haven't done any work re. T1 components, they should be much simpler than the TT counterparts, so we can try.

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.

Looking good, I appreciate moving the components under model_tt, gonna make the T1 work easier. The diff is large but there are lots of formatting/import changes. I haven't done super in-depth review of the TT components since our customers currently won't see them, though the code generally looks good.

From my point of view two things are missing, which I can add if @jpochyla is working on something else:

  • additional feature flag (ui?) that would allow us not to build the ui module for production builds, but still allow us to use the code in CI, for T1, and for TT experiments,
  • adding support for LayoutObj to matejcik's memory analyzer so that layouts are not reported as unused memory.

Also not sure about the fuzz crate, if I understand correctly it's not used in normal builds so we can keep it in this PR even though it's not really related?

core/embed/rust/src/lib.rs Show resolved Hide resolved
core/embed/rust/src/ui/component/base.rs Show resolved Hide resolved
core/embed/rust/src/ui/layout/obj.rs Show resolved Hide resolved
core/embed/rust/src/ui/layout/obj.rs Outdated Show resolved Hide resolved
@mmilata
Copy link
Member

mmilata commented Oct 5, 2021

I've implemented the changes suggested above. They are probably not straightforward enough so I won't push them directly, feel free to cherry pick them if they make sense:

  • 062e921 adds ui feature flag that is only enabled on T1 or when NEW_UI=1 variable is set when invoking SCons
  • 65657b7 extends the memory analyzer to recognize rust layouts, this is very basic since it reports no children (e.g. pointers to strings held in embedded struct Buffers)

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.

looks mostly good.

we will probably want to squash all of this except maybe rustfmt/clippy changes, if it will be easy to separate them out.

core/src/trezor/ui/__init__.py Outdated Show resolved Hide resolved
core/embed/extmod/modtrezorui/modtrezorui.c Outdated Show resolved Hide resolved
core/embed/rust/src/micropython/obj.rs Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/obj.rs Outdated Show resolved Hide resolved
First, we change the Protobuf definition includes to use an exact path relative to our crate's directory, instead of the OUT_DIR. This fixes build when a combination of stable and nightly toolchains is used (nightly is needed for the fuzzing targets).

Another change is a slight fix in the panic handler conditional compilation. Fuzzing is using the crate with `features = ["test"]`, but doesn't turn on the `test` cfg.

[no changelog]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants