rust3ds / ctru-rs

Rust wrapper for libctru
https://rust3ds.github.io/ctru-rs/
Other
122 stars 18 forks source link

Add error applet support + error applet panic hook #162

Closed FenrirWolf closed 8 months ago

FenrirWolf commented 8 months ago

Adds support for the error applet, which is a simple applet that lets you display error text in a pop-up window on the bottom screen.

The applet is also capable of launching the EULA registration dialogue, as well as "infrastructure communications-related error messages" via error codes of a completely unspecified nature, but I've opted not to support either of these functions. The applet also supports changing the language of the "An error has occurred" header text at the top of the applet, but I don't think it's worth including that functionality either since it defaults to your system's preferred language anyway.

The real fun part about this PR was discovering that Bindgen was generating the errorConf struct with the wrong size and field offsets because the struct contains enum values that were themselves generated with the wrong sizes. Hopefully that's a problem we won't run into very often again, but it's one that's worth watching out for in the future.

adryzz commented 8 months ago

this is super cool

adryzz commented 8 months ago

Great, simple and powerful. As with all applets, it depends on Apt and Gfx, but I wonder how cool a panic hook using this would be!

tried it a few days ago, it's super super cool, but yeah, without memory crimes you can't easily do such a thing unfortunately.

FenrirWolf commented 8 months ago

tried it a few days ago, it's super super cool, but yeah, without memory crimes you can't easily do such a thing unfortunately.

Setting up a custom panic hook isn't too bad actually. For example, I could easily add a method like this to the PR if we want:

/// Sets a custom panic hook that uses the error applet to display panic messages.
///
/// SAFETY: The error applet requires that both the [`Apt`] and [`Gfx`] services are active when it launches.
/// By calling this function, you promise that you will keep those services alive until either the program ends
/// or you manually unregister this hook with [`std::panic::take_hook`].
pub unsafe fn set_panic_hook() {
    std::panic::set_hook(Box::new(|panic_info| {
        let mut popup = PopUp::new(Kind::Top);

        let thread = std::thread::current();

        let name = thread.name().unwrap_or("<unnamed>");

        let payload = format!("thread '{name}' {panic_info}");

        popup.set_text(&payload);

        unsafe {
            ctru_sys::errorDisp(popup.state.as_mut());
        }
    }))
}
FenrirWolf commented 8 months ago

Okay, we've got ourselves a panic hook now. It works pretty well!

2024-02-23_17-35-39 701_to 1

Meziu commented 8 months ago

@AzureMarker I’d like to hear your opinion on this addition since it directly continues this discussion.

With the use of the error applet, we can cut down the necessity of the Console functionality and just stand on Gfx and Apt. Still, the only way I can see this working is as an unsafe function, but it might still be a worthy alternative to stderr redirection with 3dslink.

FenrirWolf commented 8 months ago

The way I see it, providing this hook is an overall win because:

1) since launch_unchecked is private, the user cannot write their own equivalent to this code at all and they would have to start from scratch with raw ctru-sys calls. The error applet isn't very complicated to call, but it would still be a bit annoying for the user to create and audit multiple unsafe blocks to accomplish what we can provide here with just one, I guess we could instead expose launch_unchecked, but the only practical use for that fn being public would be to implement a hook like this one anyway.

2) the preconditions for set_panic_hook are pretty "easy" for most users to fulfill: just init Apt and Gfx at the top of your program and everything will Just Work. And coincidentally enough, that's what the vast majority of programs using ctru-rs are already doing anyway.

3) There might be room to further improve the safety situation. For example, the majority of libctru services are atomically reference-counted on each init and exit, meaning it should be safe to create and distribute multiple handles to those services, and some services like Apt are pre-initialized in every libctru program even if the user never does it anyway. So technically it should be perfectly fine for us to just Apt::new right inside the panic hook to guarantee access to an Apt handle. I believe that Gfx is one of the few handles that doesn't internally reference count in this way, but I did notice that we have a static GFX_ACTIVE value that the panic hook could check against before doing its thing.

FenrirWolf commented 8 months ago

I pushed a change that makes set_panic_hook a safe function. Are there any other sources of unsafety when calling the applet though, or is this change enough to cover all our bases?

I'm also not sure if this method of checking for Gfx access is good enough or if it's technically vulnerable to time-of-check-to-time-of-use problems. I might have to see if there's some way to smuggle a proper Gfx handle into the closure instead.

AzureMarker commented 8 months ago

Thanks for adding this!

FenrirWolf commented 8 months ago

By the way, the error applet actually does do scrolling for long panic messages. I'm just a dummy and had tested things wrong earlier:

2024-02-27_00-13-16 346_to 1

So let's get this thing merged.