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

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

Canvas

• Features:

Gui

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

Icon

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

IconAnimation

• Features:
  • creation and removal;
  • conversions between raw and wrapper representations;
  • getters;
  • start and stop methods;
  • instance creation;
  • 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.

[dcoles]

I'm trying to decide whether we should put this in the main flipperzero crate or not.

There's some argument to be made that for putting each service under its own flipperzero-{service} crate, but I'm wondering if it is worth the hassle (especially if it's all part of one SDK). The windows crate takes the approach of putting each API under its own feature flag, so you don't have to build the entire set of bindings for every app.

[JarvisCraft]

Yup, at the moment I am not that sure about the placement thus I use gui as some form of incubator for the new APIs, but I already see this separation being a problem (as can be seen by a comment about Align type in this Cargo.toml). In the end I am planning to either move all this APIs to flipperzero (probably behind feature-flags) all provide more logical separation.

[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 🚀

[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.

Screencast.from.2023-01-03.05-52-32.webm

[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

[JarvisCraft]

First separated PR is ready: #81

[str4d]

Flushing an initial partial review of flipperzero::gui.

[str4d]

These new public APIs should all be documented. That would also help with review, knowing the intent behind each API (particularly the ones that aren't just a type-safe wrapper around a single SDK function).

[str4d]

This new struct should be mentioned in the changelog in the Added section. See prior changelog entries for the style.

[str4d]

These new modules should be mentioned in the changelog under the Added section.

[str4d]

It occurs to me that records are probably not supposed to be opened by multiple threads simultaneously. If that is the case, then separately from this PR, we might end up defining the SDK-provided records in some common location, where we can control access via a mutex. An alternative approach would be to create a harness for defining "record singletons", which would store them as heap statics (#64); then each service module would use it to define its record type.

The context for the above thought was "is it okay to call Gui::new twice?". Nothing in the current API will prevent it, but if the backing records are changed to be mutex-gated, then Gui::new would necessarily also be. And if we instead provide a record singleton harness, then Gui::new would become a private method.

[str4d]

I don't see any examples with multiple view ports. Is that a thing we should expect to eventually support?

• If so, then storing &self inside GuiViewPort currently prevents this (because then we can't re-take a mutable reference to self), so we might need an alternative API for adding multiple view ports at once (or an alternative way to reference the parent instead of &self, but that still binds to the lifetime of Self rather than the &mut self or &self).
• If not, then Gui::add_view_port feels misnamed, and it should instead be something like Gui::connect_view_port.

[str4d]

Has this been renamed?

Suggested change

	// TODO: separate `GuiCanvas` (locking the parent)
	// TODO: separate `ExclusiveCanvas` (locking the parent)

[JarvisCraft]

By the way, @str4d, is there any way we could add suport for running MIRI?

Asking this since the whole callback magic is quite tricky from aliasing standpoint. I still am unsure which invariants should be upheld for *mut Ts originating from Box<T> so it would be quite helpful.

I tried running it but the MIRId app just crashed.

[dcoles]

You weren't kidding about this being a big code review!

That said, it's shaping up really nicely. It's really impressive to see that it's now possible to write a full graphical application without the user needing to write any unsafe code. Looking forward to merging it in. 🙌

[dcoles]

I wonder if we should provide a cstr! macro for cases like this.

The only issue I see is that I believe that #![forbid(unsafe_code)] will also forbid macros using unsafe, even if the macro ensures safe use of an unsafe function (e.g. calling CStr::from_bytes_with_nul_unchecked with the guarantee of nul termination).

[dcoles]

I think you can use if cfg!/else here:

if cfg!(feature = "unstable_intrinsics") {
    divident.div_ceil(divisor)
} else {
    let quotient = divident / divisor;
    let remainder = divident % divisor;

    if remainder > 0 && divisor > 0 {
        quotient + 1
    } else {
        quotient
    }
}

[JarvisCraft]

I don't think thjat it wik work as expected since it will expand to a boolean which will mean that the top branch will stil have to be valid which is not the case for builds on stable (which is the whole purpose of hising this behind a feature flag).

We could try using cfg_if, but at the moment it looks like an overkill (especially, considering that it's body is not affected by linters).

[dcoles]

Does PartialEq/Eq make sense on this? Would you ever do a comparison?

[dcoles]

I wonder if we should replace these with static_assertions (not necessarily in this PR).

I think the asserts make sense, though I do worry about the runtime/code-size overhead that we might be taking on. Probably worth doing a side-by-side with/without those asserts and see how much additional code they introduce.

Unfortunately binary size has a very direct impact on how quickly Flipper applications start.

[JarvisCraft]

Generally, this would normally be replaced with a proc-macro in the future.

As for now, I think that it should normally be remomved by the compiler, considering that it is intended for use in consts where the expression has to be CT-evaluated.

[JarvisCraft]

Just found the explicit information about aliasing rules for Box which seem to be more restrictive as I previously thought: https://doc.rust-lang.org/std/boxed/index.html#considerations-for-unsafe-code

Another point on allowing MIRI runs on flipper.

cc @mogenson, this may be important for what you currently doing with ViewDispatcher, although it is worth to just wait for me to write the right context- implementation here

[mogenson]

Good change. A tick period of zero means wait forever, which is probably not what the user wants if they are trying to register an on_tick callback.

[mogenson]

true means event handled. Although these default callback implementations should never be registered, let's not have them claim to handle events.

[mogenson]

Looks like you're exposing some more customization to the user when creating this type. For configuration options like view dispatcher type, enabling queue, and tick period, what's the benefit of putting them as generic types. eg:

pub struct ViewDispatcher<C: ViewDispatcherCallbacks, const QUEUE: bool, const TYPE: ViewDispatcherType, const TICK_PERIOD: NonZeroU32>

vs new() method arguments:

pub fn new(callbacks: C, queue: bool, view_dispatcher_type: ViewDispatcherType, tick_period: NonZeroU32) -> Self

vs callback getters:

pub trait ViewDispatcherCallbacks {
    fn tick_period(&self) -> NonZeroU32
    fn enable_queue(&self) -> bool
    fn view_dispatcher_type(&self) -> ViewDispatcherType
}

[JarvisCraft]

As for now, it is mostly my experiments with what we can and what we can't express with it.

My point of having queue as a const generic is to prohibit calls to methods requiring it at the type-system level.

PS: to be fair, I've actually commmited my ViewDispatcheer experiments by accident, and then just went with experimenting on them

[mogenson]

I think we need to provide a way to access a &mut ViewDispatcher from within the ViewDispatcherCallbacks. This is because run() is a blocking call and the only way to exit an app is to call stop() from some callback / event handler.

I made an attempt at adding a &mut ViewDispatcher argument to each ViewDispatcherCallback method by storing an NonNull<SysViewDispatcher> and using it to construct a ViewDispatcher in the extern "C" dispatch callbacks. You can see that here: mogenson@65bf582)
It works with the modified view_dispatcher.rs example, but I'm aware that the lack of lifetime parameters makes it really fragile.

[mogenson]

If run() takes an &mut self and blocks, does this mean that we can't safely use the instance of ViewDispatcher created by new() anywhere after calling run()? Even in ViewDispatcherCallbacks?

In the modified view_dispatcher.rs example here I'm curious why I got not compiler complains when storing an &mut ViewDispatcher in a separate struct then calling run().

[mogenson]

TODO: can you add the remaining view dispatcher methods. eg:

/// Register a view
    pub fn add_view(&mut self, view_id: u32, view: *mut sys::View) {
        unsafe { sys::view_dispatcher_add_view(self.raw.as_ptr(), view_id, view) };
    }

    /// Remove a view
    pub fn remove_view(&mut self, view_id: u32) {
        unsafe { sys::view_dispatcher_remove_view(self.raw.as_ptr(), view_id) };
    }

    /// Switch to a view
    pub fn switch_to_view(&mut self, view_id: u32) {
        unsafe { sys::view_dispatcher_switch_to_view(self.raw.as_ptr(), view_id) };
    }

    /// Send view dispatcher's view port to front
    pub fn send_to_front(&mut self) {
        unsafe { sys::view_dispatcher_send_to_front(self.raw.as_ptr()) };
    }

    /// Send view dispatcher's view port to back
    pub fn send_to_back(&mut self) {
        unsafe { sys::view_dispatcher_send_to_back(self.raw.as_ptr()) };
    }
    
    /// Send a custom event to the the custom event callback handler
    pub fn send_custom_event(&mut self, event: u32) {
        unsafe { sys::view_dispatcher_send_custom_event(self.raw.as_ptr(), event) };
    }

[JarvisCraft]

@dcoles, could you please bump the API version? There is a number of changes in there (specifically, ViewHolder is now part of GUI services) and it is important for local testing

[JarvisCraft]

Status update

Currently trying to understand the right mental model of aliasing with respect to Flipper's GUI lifecycle.

Since there is high risk of violating Rust's aliasing guarantees, there may be a need to use UnsafeCell (or its derivative somewhere), although I still am not that certain about it.