feat: implement wrappers for GUI APIs

[JarvisCraft]

Description

This is a WIP PR to provide generic safe bindings for Flipper's GUI APIs.

Current status:

ViewPort

• [x] Features:
  • [x] safe enums;
  • [x] creation and removal;
  • [x] conversions between raw and wrapper representations;
  • [x] getters and setters;
  • [x] callback management.

Canvas

• [ ] Features:

Gui

• [ ] Features:
  • [x] creation and removal;
  • [x] conversions between raw and wrapper representations;
  • [x] getters;
  • [x] lockdown;
  • [x] view port addition;
  • [x] canvas access;
  • [ ] callbacks.

Icon

• [x] Features:
  • [x] creation and removal;
  • [x] conversions between raw and wrapper representations;
  • [x] getters;
  • [x] XBM API.

IconAnimation

• [x] Features:
  • [x] creation and removal;
  • [x] conversions between raw and wrapper representations;
  • [x] getters;
  • [x] start and stop methods;
  • [x] instance creation;
  • [x] callback management.
• [ ] Unresolved questions:
  • [ ] should start and stop be available via view (i.e. is it sound to call them from callbacks IconAnimationCallback).

View

• [ ] Features:

SceneManager

• [ ] Features:

ViewDispatcher

• [ ] Features:

ViewStack

• [ ] Features:

Unresolved questions

• [ ] Currently, there are no guards to ensure that API functions are called from valid context but it may be important to prohibit their usage from IRQ.

Safety notes

I am trying my best to make the API as safe as possible but the chances are high that the initial implementation will contain unsound bugs. Thus intensive review will be much appreciated.

[JarvisCraft]

By the way, while the API is in its early stage, I would like to hear general thoughts from you if I am working in the right direction.

[dcoles]

So far this is looking great. I think everyone will appreciate not having to use sys to put something on the screen.

One issue I did run into in the past was how to implement GUI callbacks. It would be really nice to use Rust closures, but closures require a "fat pointer" which is tricky to use with the standard void* context API (Perhaps we say closures require alloc::Box and leave it at that).

I am trying my best to make the API as safe as possible but the chances are high that the initial implementation will contain unsound bugs. Thus intensive review will be much appreciated.

So far everything looks good. I think even things like as_ptr are safe (so long as you never dereference the pointer).

The one I'm not 100% sure about is NonNull, because it implies the type is covariant. I think it's OK (&'static ViewPort would be a subtype of &'a ViewPort), but I don't understand variance all that well.

[JarvisCraft]

One issue I did run into in the past was how to implement GUI callbacks.

As for now, I think about one way to implement this: theoretically, we could provide dispatcher-functions matching the C-signature and pass the actual closures via context. The dispatcher-function would simply deref context calling its actual function.

It still is just a concept with which I am going to experiment (at least, I expect some Pin trickery).

I think even things like as_ptr are safe (so long as you never dereference the pointer).

Seems true, as for now I marked them as unsafe (partially, to make myself think twice before using them), but it seems that they really aren't unsafe and ATM the safety contract I write for as_ref() matches that of the following usage of the pointer, so, most probably, I will safen them.

But I don't understand variance all that well.

Same problem :D I will have to re-read documentation about it once the API is closer to its final form.

---

Thanks for your review and the comments! And have a happy new year by the way :rocket:

[dcoles]

As for now, I think about one way to implement this: theoretically, we could provide dispatcher-functions matching the C-signature and pass the actual closures via context. The dispatcher-function would simply deref context calling its actual function.

That's where I ran into an issue:

pub fn set_draw_callback(&self, callback: &dyn Fn(&Canvas)) {
    // this won't compile because callback is a fat pointer (i.e. `sizeof(&dyn Fn()) > sizeof(*void)`)
    let callback = callback as *mut c_void;

    unsafe {
        sys::view_port::draw_callback_set(self.inner, Self::draw_callback, callback);
    }
}

Without a fat pointer, Rust wouldn't know the correct vtable for the concrete trait implementation.

[JarvisCraft]

&dyn Fn(&Canvas) may be stored in some wrapper struct so that we would have to only have the word-sized pointer to the structure. Something similar to:

struct CallbackWrapper<'a>(&'a dyn Fn(&Canvas));

impl<'a> CallbackWrapper<'a> {
    fn call(&self, canvas: &Canvas) {
        self.0(canvas) // vcall ocurrs here
    } 
}

[JarvisCraft]

Here are some good news: examples/gui is now #![forbid(unsafe_code)] and also has additional functionality:

• it now uses queue to exit on BACK key;
• it now has a timer controlled by buttons.

Demo

[JarvisCraft]

Another update here with enhanced XBM support:

[JarvisCraft]

Hi, @dcoles! Since you've added gui as an explicit module ni your recent commits, should I migrate current code to it?

UPD: just moved gui to flipperzero crate

[dcoles]

Hi, @dcoles! Since you've added gui as an explicit module ni your recent commits, should I migrate current code to it?

Yes please. I'm trying to roughly maintain the same structure as in the Flipper Zero SDK.

This would be particularly useful if we want to put various modules under a feature flag.

[JarvisCraft]

As a small status update on this PR: I'm out for my holidays thus I'm taking some rest from development (also, I don't have my flipper with me for testing my changes), but I will get back to work as soon as I come back :)

[dcoles]

As a small status update on this PR: I'm out for my holidays thus I'm taking some rest from development (also, I don't have my flipper with me for testing my changes), but I will get back to work as soon as I come back :)

Awesome! Have a great break! 🏝️

[tocklime]

One issue I did run into in the past was how to implement GUI callbacks. It would be really nice to use Rust closures, but closures require a "fat pointer" which is tricky to use with the standard void* context API (Perhaps we say closures require alloc::Box and leave it at that).

I think this is doable using the trick detailed here ('improving ergonomics' section) - I haven't tested this much, and I'm sure it's not sound, but something like this seems to be working, at least on a trivial example:

    unsafe extern "C" fn wrapped_callback<Env>(
        canvas: *mut sys::Canvas,
        context: *mut core::ffi::c_void,
    ) where
        Env: Sized + FnMut(&Canvas),
    {
        unsafe {
            let f_raw_ptr: *mut Env = context.cast();
            let f = f_raw_ptr.as_mut().expect("Null fptr");
            let canvas = Canvas { inner: canvas };
            f(&canvas);
        }
    }

    pub fn draw_callback_set<F>(&self, callback: &F)
    where
        F: FnMut(&Canvas),
    {
        unsafe {
            sys::view_port_draw_callback_set(
                self.inner,
                Some(Self::wrapped_callback::<F>),
                callback as *const F as *mut core::ffi::c_void,
            );
        }
    }

which is used something like u

    let hi = core::ffi::CStr::from_bytes_with_nul(b"Hi There!\0").unwrap();
    let cb = |c: &Canvas| {
        c.draw_str(39, 31, hi);
    };
    view_port.draw_callback_set(&cb);

[JarvisCraft]

Actually, callbacks are already implemented for some components here. For example ViewPort uses the solution based on dispatch-function.

[tocklime]

You're right, they do, of course. I was experimenting with closure-based callbacks, ideally so the calling code can be something like view_port.on_draw(|canvas| { /*...do things with canvas... */ };, rather than using ViewPortCallbacks.

[JarvisCraft]

It can be replaced with singular callbacks (in fact, current Callbacks structure is not that different). Moreover, we can add a struct implementing ViewPortCallbacks which would allow specifying callbacks via its fields, a la

struct MutViewPortCallbacks {
    draw_callback: Box<dyn Fn(&mut CanvasView)>,
}

impl ViewPortCallbacks for MutViewPortCallbacks {
    fn on_draw(&mut self, canvas: &mut CanvasView) {
        self.draw_callback(canvas);
    } 
}

impl MutViewPortCallbacks {
    fn set_on_draw(&mut self, callback: impl Fn(&mut CanvasView)) {
        self.draw_callback = Box::new(callback);
    }
}

It would only require adding a method to ViewPort for reading its ViewPortCallback so that its closures can be hot-swapped.

[dcoles]

Hi @tocklime,

I think this is doable using the trick detailed here ('improving ergonomics' section) - I haven't tested this much, and I'm sure it's not sound, but something like this seems to be working, at least on a trivial example:

That's pretty clever. I never thought of using generics for a extern "C" function, but it's not really much different than if you had written it yourself.

At a quick glance, it seems to be sound (I wrote a cut-down example), but it does still need somewhere to store the actual callback (the set_c_callback only stores a pointer to some data, not the actual data itself).

So you'd still need to store the callback environment in either main or using on the heap using Box. But it still potentially saves you one level of Box-ing (i.e. no need for a Box<dyn Fn(&Canvas)>).

[JarvisCraft]

So you'd still need to store the callback environment in either main or using on the heap using Box.

Using stack location is problematic since we would need a way to enforce the immovability of the pointer (pinning of it).

As can be seen here, generic extern-C function is already used for this purpose. Specific callback instance is stored in a Box, there are no vcalls involved and the only level of indirection is the need for dispatch function which seems to be obligatory.

[str4d]

There are other places we also need callback functionality (e.g. my WIP thread wrapping PR, where I box twice to get a thin pointer), so we should probably standardise on an approach and then add some helper methods to flipperzero-sys that we can reuse.

[JarvisCraft]

@str4d, apologies for the late reply.

The approach I currently use is the following:

• a single trait represents all callbacks of the domain (such as ViewVallbacks trait representing the callbacks of View domain);
  • the trait has no-op default implementations for all methods;
  • the trait is implemented for ();
• instance of callbacks-object is passed to the costructor function and boxed for sake of fixing its memory address;
• memory address of the callbacks-object is stored as the callbacks' context;
• all callbacks are registered via dispatch-functions which internally delegate to the callbacks methods by dereferencing the passed context (being an address);
  • a micro-optimization is made in order to not register separate callbacks whose implementation seems to be default (i.e. whose memory addresses are the same as these of <() as FooCallbacks>);
• functionality to dynamically override callbacks may be implemented by adding a specialized implementation of callbacks and producing &mut-access to it from the domain object (see https://github.com/flipperzero-rs/flipperzero/pull/29#issuecomment-1460277552 for a bit more details).

This approach may have its downsides and I am a bit unsure what to do with the callbacks which don't have callback context, but this is probable solvable (in the end, we may request the required functionality in flipperzero-firware).

[JarvisCraft]

@dcoles, @str4d, hi there! Since most of the other PRs involving other APIs have been merged successfully, I guess it is the moment we get back to the discussion of the best API for calbacks in GUIs (see the prvious message).

[dcoles]

Hey @JarvisCraft. Sorry for the delay in responding. I'm traveling at the moment, so I've been limited on time in front of a computer.

I think this is a good solution that balances ergonomics with efficiency. I'm happy if you want to move forward with this approach.

[str4d]

My without-review inclination is to merge this PR as-is and make further improvements in subsequent PRs. I'm going to attempt to review the PR sometime in the next week or two, and see if that changes my opinion.

@JarvisCraft there are merge conflicts that need resolving before the PR can move forward.

[str4d]

Re: the callback approach, I think we should work with what is implemented now, and then a subsequent PR could completely change how the callbacks are implemented. We don't need to consider the current approach final, and in some respects it would be better to get some experience with using it, to figure out how many downsides we run into in practice.

[JarvisCraft]

I think this is a good solution that balances ergonomics with efficiency. I'm happy if you want to move forward with this approach.

@dcoles, Thanks for your review! in this case, I will stick to this approach.

My without-review inclination is to merge this PR as-is and make further improvements in subsequent PRs. I'm going to attempt to review the PR sometime in the next week or two, and see if that changes my opinion.

@str4d, good point. I like the idea of merging this PR as-is (ensuring compilabilty and test&example-stability, of cource) in order to provide "beta" version of this wrappers (hidden behind a feature flag such as unstable-service-gui) and then iteratively upgrade it to finally fully and soundly cover the APIs.

@JarvisCraft there are merge conflicts that need resolving before the PR can move forward.

Yup, I will try to resolve them throught this week.

[JarvisCraft]

@dcoles, @str4d, I guess it's time you start reviewing this monstrocity of a PR

Highlights

• A log of gui service APIs are implemented although many more are to come in the future PRs.

• Callback approach has been unchanged since the previous highlights.

  • Mutable callbacks have not yet been implemented since we will have to double-check their soundness and they don't seem to be a feature with which we should rush.

• All APIs for services have been hidden under the respective feature flags which has been suggested here by @dcoles.

  • This has broken the tests machinery since we now need to enable --all-features for it. I am not sure what would be the right way to do this (maybe just hide tests in tests/) this @str4d's suggestions would great.

• Some of the existing examples have been rewritten using the new APIs making messagess API almost 100%-safe (with the only exception being immage creation for which we may implement a proc-macro in the future).

• An API has been added for XBM images including basic image manipulation and an ability to safely compile-time embed them.

  • There is also a decl-macro xbm! allowing embedding of XBM-formatted images into a macro (usage can be seen in the gui example). It was more of a proof-of-concept (especially a trick to validate names in a decl-macro) and will probably be replaced with a proc-macro in the future (also allowing loading from external files such as by include_str!).

• I've enhanced documentation:

  • Enabled automatic generation of feature information:
  • Enabled generation of feature-gate info on items:
  • Fixed a few broken docs (specifically, invalid links) in other items which were reported by cargo doc (it's worth mentioning that references to crypto-APIs are still broken as their dependencies are dev-only; also doxygen creates a ton of broken links):

• A few small APIs have been added via kernel.rs which may come in handy in the future.

• Some existing APIs were minorly changed to fit better (e.g. UnsafeRecord::as_ptr has been renamed to UnsafeRecord#as_raw so that the name i smore universal).

• A few unstable features have been added, namely:

  • unstable_intrinsics enabling the use of not-yet-stable Rust features instead of manually implemented ad-hoc (the only exammple now being flipperzero::internals::ops providing div_ceil_u8).
  • unstable_lints enabling unstable rustc lints i.e. must_not_suspend on a LockGuard.

• I've made flipperzero_alloc re-export of items from alloc so that users can only extern the former without a need to extern the latter (this has been reflected in the examples involving alloc feature).

• Clippy has been invoked in some places (especially, storage.rs).

  • I would suggest enabling automation for it in the CI.

Soundness notes

As mentioned before, I do have some concerns about soundness. Especially, I stil am not that sure if current callbacks correclty provide &mut-access. I guess that it it fone to go with the current approach as a starting point and explore this aspect in the future Issues/PRs.

[JarvisCraft]

By the way, I suggest that we use root crate (the on with workspaces) to host flipperzero crate since this would simplify project structure a bit also allowing us to configure [package.metadata.docs.rs] which for some reason does not work with workspaces when specified at crate-level.

[str4d]

By the way, I suggest that we use root crate (the on with workspaces) to host flipperzero crate since this would simplify project structure a bit also allowing us to configure [package.metadata.docs.rs] which for some reason does not work with workspaces when specified at crate-level.

I disagree with this suggestion. It complicates the project structure (no longer simply flat) and can cause issues with cargo not knowing what you want run (IIRC meaning you basically have to pass the --workspace flag to every cargo invocation).

[package.metadata.docs.rs] doesn't work in a virtual root, but it should work in each sub-crate fine (I've done this in other projects). cargo also supports the workspace.metadata key (https://github.com/rust-lang/cargo/issues/8309), so if docs.rs doesn't listen to that for common configs across the workspace, we should open an issue upstream.

[str4d]

Re: this PR, the summary indicates that there are a lot of changes that are maybe used by the GUI changes, but are themselves unrelated to the GUI. To keep the complexity manageable, I would prefer that we review those in separate smaller PRs, and keep this PR for the (still substantial) GUI changes (rebasing as necessary when the smaller PRs are merged).

[JarvisCraft]

[package.metadata.docs.rs] doesn't work in a virtual root, but it should work in each sub-crate fine (I've done this in other projects). cargo also supports the workspace.metadata key (https://github.com/rust-lang/cargo/issues/8309), so if docs.rs doesn't listen to that for common configs across the workspace, we should open an issue upstream.

Per-crate approach failed to work, at least for things like rustdoc-args and cargo-args but I will try workspace.metadata approach.

I would prefer that we review those in separate smaller PRs, and keep this PR for the (still substantial) GUI changes (rebasing as necessary when the smaller PRs are merged).

Fair point, I will try to extract the parts unrelated to GUI APIs (documentation fixes and enhancments, feature-flags, minor refactoring) to separate PRs. As for now the essential part of this PR may be reviewed at flipperzero/src/gui.

[JarvisCraft]

First separated PR is ready: #81