-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add error applet support + error applet panic hook #162
Conversation
df88c27
to
dc29593
Compare
this is super cool |
e305fa6
to
a75ac1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
fa96126
to
a9ded19
Compare
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());
}
}))
} |
409f6fe
to
c6d5cdc
Compare
c72559b
to
d32cd15
Compare
@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 |
The way I see it, providing this hook is an overall win because:
|
I pushed a change that makes I'm also not sure if this method of checking for |
94c5691
to
a67c8c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ctru-rs/src/applets/error.rs
Outdated
popup.set_text(&payload); | ||
|
||
unsafe { | ||
popup.launch_unchecked(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check my understanding, this will block the app until the popup is closed, and then the app will quit (since there's no loop
or anything)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you've got that right. Though the app can technically continue if the panic is stopped by a thread boundary or catch_unwind
or whatever.
// Libctru's `errorConf` struct contains two enums that depend on this narrowing property for size and alignment purposes, | ||
// and since `bindgen` generates all enums with `c_int` sizing, we have to blocklist those types and manually define them | ||
// here with the proper size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really interesting! Similar to what I mentioned in #166 , maybe we can just force these constants to generate using errorReturnCode
as the type instead of c_int
?
To catch the struct size/alignment issues, I wonder if we could have caught that using bindgen's layout tests. I think they were omitted before because we didn't have any good way to run them, but with test-runner
now we might be able to actually do it in CI. I'll file an issue (edit: #167)
Edit: bindgen
also has a .fit_macro_constants(true)
we could try to use for this purpose, but I'm not sure if it would help with the actual struct size.
8c7d406
to
7823ed5
Compare
7823ed5
to
ffbe660
Compare
|
||
/// Configuration struct to set up the Error applet. | ||
#[doc(alias = "errorConf")] | ||
pub struct PopUp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I kind of liked the ErrorApplet name from before, but this works I guess. If not the original name, maybe it could be clearer that it's a pop up message. PopUpMessage
?
Thanks for adding this! |
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.