Skip to content
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

Implement low hanging apt sleep/homemenu functions and chainloader #161

Merged
merged 9 commits into from
Feb 26, 2024

Conversation

adryzz
Copy link
Contributor

@adryzz adryzz commented Feb 17, 2024

Saw that these weren't added yet, although they're very simple.

TLDR, Implemented

  • aptSetSleepAllowed
  • aptIsSleepAllowed
  • aptSetHomeAllowed
  • aptIsHomeAllowed
  • aptJumpToHomeMenu
  • chainloader stuff
  • applist stuff

@adryzz
Copy link
Contributor Author

adryzz commented Feb 17, 2024

oh, looks like i made my PR on the wrong branch

edit: fixed

@FenrirWolf FenrirWolf self-requested a review February 17, 2024 03:01
@Meziu
Copy link
Member

Meziu commented Feb 17, 2024

Saw that these weren't added yet, although they're very simple.

It’s a pretty straightforward addition. Just one question, what does “sleep allowed” actually do? Can the app stop the system to sleep even if the shell is closed?

All the chainloader stuff is still not implemented, as that probably requires a big abstraction layer to list apps and other things.

Chainloader and applist stuff could be implemented with the already existing AM API (perhaps?).

@adryzz
Copy link
Contributor Author

adryzz commented Feb 17, 2024

It’s a pretty straightforward addition. Just one question, what does “sleep allowed” actually do? Can the app stop the system to sleep even if the shell is closed?

It's used in online multiplayer games and stuff like that. i've seen the no-sleep/no-home combo for example in Steel Diver: Sub Wars when playing online.

essentially, if you close the shell (or use the toggle switch on the O2DS), the displays turn off, but the console does not go to sleep. you can see this because the blue LED does not oscillate in brightness when it's not sleeping.

I should probably write better documentation, since it wasn't clear at first glance what it did.

Chainloader and applist stuff could be implemented with the already existing AM API (perhaps?).

edit: turns out i didn't know the existing AM API provided that, i have made a chainloader prototype, although i don't really like it as-is

@adryzz
Copy link
Contributor Author

adryzz commented Feb 17, 2024

Alright, implemented a chainloader API, the problem is that the bindings are missing a function.

https://github.com/devkitPro/libctru/blob/d0936b879bd2088cae61b69707fe70cb66ede651/libctru/source/services/apt.c#L111

@adryzz adryzz changed the title Implement low hanging apt sleep/homemenu functions Implement low hanging apt sleep/homemenu functions and chainloader Feb 17, 2024
@FenrirWolf
Copy link
Member

static functions like that one aren't exported outside of their compilation unit. In other words, they're private to libctru and you have to reimplement them if you want to use them. Fortunately the impl is pretty simple for that function and the other ones like it, so hopefully that won't be too much of a problem.

Copy link
Member

@ian-h-chamberlain ian-h-chamberlain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just a couple small tweaks I'd recommend

ctru-rs/src/services/apt.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/apt.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/apt.rs Outdated Show resolved Hide resolved
Copy link
Member

@Meziu Meziu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are small and contained. LGTM

@@ -90,3 +132,48 @@ impl Drop for Apt {
unsafe { ctru_sys::aptExit() };
}
}

/// Can launch other applications when the current one exits.
pub struct Chainloader<'a> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chainloader should clear on Drop, so that leftovers from previous usages won't influence in un-mutable environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did think about this, but then forgot to propose it.

the problem is that the destructor will run when you exit the app, as the chainloader goes out of scope.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, didn’t think about it.

Wait, does this mean the setup only works because there is a reference to Apt that doesn’t get uninitialised? (The one made in libctru, i mean). If this were to not work without that (because the apt service were to be unused) then we must find a way around this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at a first look it definitely looks that way.


impl<'a> Chainloader<'a> {
/// Gets a handle to the chainloader
pub fn new(apt: &'a Apt) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Chainloader should require a mutable reference to Apt, but that may not be necessary if no other aspect of Apt reads or uses data handled by Chainloader.

Tell me what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tried this, but then using it inside the main apt loop is a mess because it takes an immutable reference to loop

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I know, that’s exactly what I was thinking would happen. It’s not an issue as long as all of the chainloader code is only accessible from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are the only chainloader functions, so i think it's fine to have it be this way

Co-authored-by: Meziu <55318903+Meziu@users.noreply.github.com>
@ian-h-chamberlain ian-h-chamberlain merged commit eed5fc9 into rust3ds:master Feb 26, 2024
4 checks passed
@adryzz adryzz deleted the apt-new-stuff branch February 26, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants