Add Rust FFI #1540
Add Rust FFI #1540
Conversation
mmilata
force-pushed
the
rust-micropython-ffi
branch
from
ca8a90a to
cb9d73a
Compare
There was a problem hiding this comment.
Exciting stuff! I'm probably not skilled enough in unsafe Rust & micropython internals to say whether it's safe to merge, would definitely like more people to go through the code, but I guess with enough testing we should be fine? Anyway I left some questions and nitpicks in couple of places.
Also, there's a couple of TODOs, can you please go through them to see if there are any that are critical to resolve before merging?
| "-DTREZOR_MODEL=T", | ||
| "-DSTM32F405xx", | ||
| "-DUSE_HAL_DRIVER", | ||
| "-DSTM32_HAL_H=<stm32f4xx.h>", |
There was a problem hiding this comment.
Note to self: add branch with correct defines for T1 (and includes for STM32F2). Test on hw.
There was a problem hiding this comment.
Yup, didn't think about T1 too much yet.
| use super::ffi; | ||
|
|
||
| pub struct IterBuf { | ||
| iter_buf: ffi::mp_obj_iter_buf_t, |
There was a problem hiding this comment.
If anyone like me has no idea how micropython iterators work, this is somewhat helpful: https://micropython-usermod.readthedocs.io/en/latest/usermods_11.html
|
|
||
| pub fn raise_value_error(msg: &'static CStr) -> ! { | ||
| unsafe { | ||
| ffi::mp_raise_ValueError(msg.as_ptr()); |
There was a problem hiding this comment.
As previously discussed, I'm wondering whether exceptions raised in python code called from rust cannot bite us in some way. IIUC this means we cannot rely on destructors (Drop::drop()) always being executed. It shouldn't lead to memory leaks since heap is managed by micropython GC, but we have to keep this in mind & review all dependencies whether they rely on Drop for correctness.
Is it feasible to install exception handler (nlr_push()) every time we call rust->python and either panic when caught (imho reasonable for allocation failures), or choose to propagate the exception using Results to the underlying caller?
There was a problem hiding this comment.
It should be feasible, yes. There is some performance hit, but if we're actually calling Python it won't be relatively big, and we could replace the cases of C code that only sometimes call Python or raise exceptions themselves, like most of the conversion methods, with our non-raising variants.
More about leaking here: https://doc.rust-lang.org/nomicon/leaking.html
|
I've added some commentary and marked the corresponding conversations as resolved. TODO comments are there, I went through them and think that these are all fine at this stage. |
matejcik
force-pushed
the
rust-micropython-ffi
branch
from
333809c to
b310f0c
Compare
core: Remove dangling module decls core: Use new Cargo feature resolver, use external MacOS debug info core: Rust docs improvements core: Upgrade bindgen core: Add test target to Rust ci: build rust sources build(core): .ARM.exidx.text.__aeabi_ui2f in t1 firmware size It's an unwind table for softfloat function inserted by rustc, probably can be removed to save 8 bytes: https://github.com/rust-embedded/cortex-m-rt/blob/599c58db70c5dd4eb1dfb92e1dad7c80ed848937/link.x.in#L175-L182 scons: Remove dead code core: Move Rust target to build/rust core: Replace extern with a FFI version core: Add some explanatory Rust comments core: Use correct path for the Rust lib core: Remove Buffer::as_mut() Mutable buffer access needs MP_BUFFER_WRITE flag. TBD in the Protobuf PR. core: Improve docs for micropython::Buffer core: Minor Rust docs changes core: Rewrite trezor_obj_get_ll_checked core: Fix incorrect doc comment core: Remove cc from deps fixup! core: Rewrite trezor_obj_get_ll_checked core: update safety comments
matejcik
force-pushed
the
rust-micropython-ffi
branch
from
b310f0c to
d8cb55f
Compare
Adds Rust code, together with bindings to MicroPython and some parts of trezorhal.
Needs nightly Rust for the following reasons:
#![feature(never_type)]- Used later in the UI layout PR. Possible to get rid of by replacing with an empty enum.#![feature(unsize)],#![feature(coerce_unsized)],#![feature(dispatch_from_dyn)]used in theGctype to allow boxing unsized values. Needs further investigation, but probably possible to get rid of if we use regularBoxwith a global allocator plugged into the MicroPython GC.Cargofeatures = ["host_dep"], solved with the new resolver in Rust 1.51, we can get rid of this easily.