Rust UI: improve messages returned to python #2138

mmilata · 2022-02-19T13:45:44Z

Built on top of #2092. Pyright fails since we no longer generate the .pyi files, can we do better than just ignoring these issues? Some other questions inline.

Fixes #2118.

core/embed/rust/src/ui/layout/result.rs

mmilata · 2022-02-19T13:49:57Z

core/embed/rust/src/ui/model_t1/layout.rs

- args: *const Obj,
- kwargs: *const Map,
-) -> Obj {
+extern "C" fn new_confirm_text(n_args: usize, args: *const Obj, kwargs: *mut Map) -> Obj {


Note that I changed *const Map to *mut Map to pass type check.

I can't check it now, but it should match the type the C-side expects.

mmilata · 2022-02-19T13:51:41Z

core/embed/rust/src/ui/model_tt/component/keyboard/pin.rs

@@ -18,7 +18,7 @@ use crate::{
 };

 pub enum PinKeyboardMsg {
- Confirmed,
+ Confirmed(Vec<u8, MAX_LENGTH>),


Since the pin() method exists I guess it was supposed to be used to obtain the pin, but haven't figured out how to get a reference to the layout object at the time the result is constructed.

This is exactly the usecase the ObjComponentMsg trait (hope I remember the name right) is supposed to address.

If you omit the TryInto impl for the msg type, you should be able to impl the above trait and get a ref to the component for the conversion. Then you can pull the PIN out of it and convert into Obj.

When I implement the trait like

trezor-firmware/core/embed/rust/src/ui/model_tt/layout.rs

Lines 57 to 65 in 31fc2a4

impl ComponentMsgObj for PinKeyboard {

fn msg_try_into_obj(&self, msg: Self::Msg) -> Result<Obj, Error> {

let result: (Obj, Obj) = match msg {

PinKeyboardMsg::Cancelled => (CANCELLED.as_obj(), Obj::const_none()),

PinKeyboardMsg::Confirmed => (CONFIRMED.as_obj(), self.pin().try_into()?),

};

result.try_into()

}

}

then I get error regarding conflicting implementations:

error[E0119]: conflicting implementations of trait `ui::layout::obj::ComponentMsgObj` for type `ui::model_tt::component::keyboard::pin::PinKeyboard` --> src/ui/model_tt/layout.rs:57:1 | 57 | impl ComponentMsgObj for PinKeyboard | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `ui::model_tt::component::keyboard::pin::PinKeyboard` | ::: src/ui/layout/obj.rs:37:1 | 37 | / impl<T> ComponentMsgObj for T 38 | | where 39 | | T: Component, 40 | | T::Msg: TryInto<Obj, Error = Error>, ... | 44 | | } 45 | | } | |_- first implementation here

What am I missing?

The TryInto implementation is not there anymore, perhaps the compiler is not smart enough to figure out that T::Msg: TryInto<Obj, Error = Error> is false?

As discussed, we may be hitting rust-lang/rust#20400. I rewrote all the TryInto conversions to explicit ComponentMsgObj impls, it's a bit ugly but uses less magic.

mmilata · 2022-02-19T13:52:25Z

core/embed/rust/src/ui/model_tt/component/keyboard/pin.rs

@@ -145,7 +145,7 @@ impl Component for PinKeyboard {

 fn event(&mut self, ctx: &mut EventCtx, event: Event) -> Option<Self::Msg> {
 if let Some(Clicked) = self.confirm_btn.event(ctx, event) {
- return Some(PinKeyboardMsg::Confirmed);
+ return Some(PinKeyboardMsg::Confirmed(self.digits.clone()));


If we keep this, is there a way to zero out the pin being cloned in a way that doesn't get optimized out?

See above, we can get rid of PIN being stored in the message altogether. Re. reliable data zeroing, there are some crates, let's investigate how they work in a nostd env.

https://docs.rs/zeroize/latest/zeroize/ is such a crate.

Not storing the pin in the message anymore so zeroing isn't needed (yet). But good to know such crate exists.

maybe you should zero out the source buffer for the PIN though?

Good point, though that would require to modify the type of ComponentMsgObj::msg_try_into_obj to fn msg_try_into_obj(&mut self, msg: Self::Msg) -> Result<Obj, Error>. I propose to solve this in another PR, wanted to add this as a TODO item to #1922 but github is probably broken and won't let me edit:/

👍
added the TODO

honestly, couldn't we just consume the layout object instead? self instead of &mut self
(probably not because something something reference in the Python-held object, but still something that seems semantically more correct)

core/embed/rust/src/micropython/macros.rs

matejcik · 2022-02-21T12:37:38Z

we can extend tools/build_mocks relatively easily to also walk Rust files (maybe only obj.rs ?) and generate mocks in the same format as the modtrezor*.h files.

mmilata · 2022-03-10T14:21:53Z

I've addressed the comments and extended build_mocks to also scan .rs file for definitions, please take a look. I think this shouldn't conflict with #2155 much so marking as ready for review.

mmilata · 2022-03-10T14:22:23Z

Also rebased to master.
cc @grdddj

core/embed/rust/src/micropython/macros.rs

matejcik

LGTM structure wise, leaving the details to @jpochyla

big 👍 on build_mocks handling

core/SConscript.unix

core/embed/rust/src/micropython/macros.rs

core/embed/rust/src/micropython/obj.rs

mmilata · 2022-03-11T17:04:15Z

Noticed that rustc doesn't like /// for the mocks:

warning: unused doc comment
   --> src/ui/model_tt/layout.rs:153:5
    |
153 |     /// CONFIRMED: object
    |     ^^^^^^^^^^^^^^^^^^^^^
154 |     Qstr::MP_QSTR_CONFIRMED => CONFIRMED.as_obj(),
    |     ----------------------- rustdoc does not generate documentation for expressions
    |
    = note: `#[warn(unused_doc_comments)]` on by default
    = help: use `//` for a plain comment

I've added #[allow(unused_doc_comments)] but let me know if using different prefix is preferable.

matejcik

small nitpicks, otherwise very nice

core/embed/rust/src/ui/geometry.rs

core/embed/rust/src/ui/model_tt/layout.rs

[no changelog] Co-authored-by: Martin Milata <martin@martinmilata.cz>

[no changelog]

mmilata commented Feb 19, 2022

View reviewed changes

core/embed/rust/src/ui/layout/result.rs Show resolved Hide resolved

mmilata commented Feb 19, 2022

View reviewed changes

core/embed/rust/src/micropython/macros.rs Outdated Show resolved Hide resolved

mmilata mentioned this pull request Mar 4, 2022

Rust UI: keyboards tweaks and fixes #2154

Merged

mmilata force-pushed the mmilata/rs2py-messaging branch from 6c51190 to 834e584 Compare March 10, 2022 14:11

mmilata marked this pull request as ready for review March 10, 2022 14:21

mmilata requested a review from prusnak as a code owner March 10, 2022 14:21

mmilata commented Mar 10, 2022

View reviewed changes

core/embed/rust/src/micropython/macros.rs Show resolved Hide resolved

mmilata requested review from jpochyla and matejcik and removed request for prusnak March 10, 2022 14:26

matejcik approved these changes Mar 10, 2022

View reviewed changes

jpochyla reviewed Mar 10, 2022

View reviewed changes

core/SConscript.unix Show resolved Hide resolved

core/embed/rust/src/micropython/macros.rs Show resolved Hide resolved

core/embed/rust/src/micropython/obj.rs Outdated Show resolved Hide resolved

mmilata mentioned this pull request Mar 14, 2022

Rust: Add obj_module! macro #2092

Closed

mmilata force-pushed the mmilata/rs2py-messaging branch 4 times, most recently from 2c3647d to df9cdc3 Compare March 23, 2022 23:17

matejcik reviewed Apr 1, 2022

View reviewed changes

core/embed/rust/src/ui/geometry.rs Show resolved Hide resolved

core/embed/rust/src/ui/model_tt/layout.rs Show resolved Hide resolved

matejcik approved these changes Apr 2, 2022

View reviewed changes

jpochyla and others added 2 commits April 4, 2022 10:48

feat(core/rust): Add obj_module! macro

d9436d1

[no changelog] Co-authored-by: Martin Milata <martin@martinmilata.cz>

build(core): fix TREZOR_EMULATOR_DEBUGGABLE

9b7e8bf

[no changelog]

feat(core/rust/ui): return Layout results as singleton objects

03866bd

[no changelog]

mmilata force-pushed the mmilata/rs2py-messaging branch from e75c848 to 03866bd Compare April 4, 2022 08:48

mmilata merged commit 38f4ab0 into master Apr 4, 2022

mmilata deleted the mmilata/rs2py-messaging branch April 4, 2022 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust UI: improve messages returned to python #2138

Rust UI: improve messages returned to python #2138

mmilata commented Feb 19, 2022 •

edited

Loading

mmilata Feb 19, 2022

jpochyla Feb 20, 2022

mmilata Feb 19, 2022

jpochyla Feb 20, 2022

jpochyla Feb 20, 2022

jpochyla Feb 20, 2022

mmilata Feb 20, 2022 •

edited

Loading

mmilata Feb 20, 2022

mmilata Mar 10, 2022

mmilata Feb 19, 2022

jpochyla Feb 20, 2022

jpochyla Feb 22, 2022

mmilata Mar 10, 2022

matejcik Mar 10, 2022

mmilata Mar 28, 2022

matejcik Mar 28, 2022

mmilata Oct 11, 2022

matejcik commented Feb 21, 2022

mmilata commented Mar 10, 2022

mmilata commented Mar 10, 2022

matejcik left a comment

mmilata commented Mar 11, 2022

matejcik left a comment

	impl ComponentMsgObj for PinKeyboard {
	fn msg_try_into_obj(&self, msg: Self::Msg) -> Result<Obj, Error> {
	let result: (Obj, Obj) = match msg {
	PinKeyboardMsg::Cancelled => (CANCELLED.as_obj(), Obj::const_none()),
	PinKeyboardMsg::Confirmed => (CONFIRMED.as_obj(), self.pin().try_into()?),
	};
	result.try_into()
	}
	}

Rust UI: improve messages returned to python #2138

Rust UI: improve messages returned to python #2138

Conversation

mmilata commented Feb 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilata Feb 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik commented Feb 21, 2022

mmilata commented Mar 10, 2022

mmilata commented Mar 10, 2022

matejcik left a comment

Choose a reason for hiding this comment

mmilata commented Mar 11, 2022

matejcik left a comment

Choose a reason for hiding this comment

mmilata commented Feb 19, 2022 •

edited

Loading

mmilata Feb 20, 2022 •

edited

Loading