rust3ds / ctru-rs

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

Add a basic AM service wrapper and an example that uses it #95

Closed Maccraft123 closed 1 year ago

Maccraft123 commented 1 year ago

This PR adds basic AM wrapper, for use by programs that read title data, and a basic example. I was unsure where to put FsMediaType enum, initially it was in am.rs, but in libctru it was defined in fs.h, so it went into fs.rs I'm also unsure about the prelude, whether Am struct and FsMediaType enum should be added to it. As it usually makes sense to get all titles installed, instead of random n of them, get_title_list returns a Vec of all title ids on a given FsMediaType. Should TitleId be a newtype or struct so that it could have some helper methods like TitleId::upper() etc?

This PR was tested with the 'title-info' example on a New 3DS with Luma3DS v11.0 and system firmware v11.16.0-49E

Maccraft123 commented 1 year ago

Seems like you understood our standards well just by looking at our code! It looks really nice, I just wish there was a little more functionality added. On the other hand, looking at libctru's documentation, I don't see easy/safe stuff to port, so it's not a big deal.

Thanks! I wanted to implement more functionality of am, however I don't really know how to use it beyond what I did and I don't want to push any code that I haven't tested myself.

Also, I noticed that getting lower and higher 32 bits of TitleIds is quite useful, and doing it manually every time or using a custom function is kinda ugly. What do you think about adding a TitleId tuple struct with a u64, that would also have other methods other than lower and higher, like title_category?

I also have PTMU wrapper in feature/ptmu branch, here that does pretty much everything that libctru does, however there are some minor changes that I think would make it easier to use, but that all should just go in another PR that i'll do after this one is merged

ian-h-chamberlain commented 1 year ago

Also, I noticed that getting lower and higher 32 bits of TitleIds is quite useful, and doing it manually every time or using a custom function is kinda ugly. What do you think about adding a TitleId tuple struct with a u64, that would also have other methods other than lower and higher, like title_category?

Haveing a TitleId wrapper (or maybe just Title? it could store the ID only and get other stuff on-demand) makes sense to me, then we could also have things like Title::product_code() that calls AM_GetTitleProductCode, as well as some wrapper functions around AM_TitleEntry. I don't think it needs to be this PR but you could add here if you wanted.

Maccraft123 commented 1 year ago

Also, I noticed that getting lower and higher 32 bits of TitleIds is quite useful, and doing it manually every time or using a custom function is kinda ugly. What do you think about adding a TitleId tuple struct with a u64, that would also have other methods other than lower and higher, like title_category?

Haveing a TitleId wrapper (or maybe just Title? it could store the ID only and get other stuff on-demand) makes sense to me, then we could also have things like Title::product_code() that calls AM_GetTitleProductCode, as well as some wrapper functions around AM_TitleEntry. I don't think it needs to be this PR but you could add here if you wanted.

So, TitleId struct, type alias... or what? I'm not sure how to handle AM service init and exit, should it be done by Title::product_code()? Or should it have a reference to Am passed to it via an argument? And where that TitleId/Title type would be defined in? ctru-rs/src/service/am.rs? ctru-rs/src/titleid.rs? Or where else?

In any case, I'll modify this PR so that the bare minimum of TitleId/Title struct is implemented to not have to make changes to do it later.

ian-h-chamberlain commented 1 year ago

So, TitleId struct, type alias... or what? I'm not sure how to handle AM service init and exit, should it be done by Title::product_code()? Or should it have a reference to Am passed to it via an argument? And where that TitleId/Title type would be defined in? ctru-rs/src/service/am.rs? ctru-rs/src/titleid.rs? Or where else?

Based on what we've done in other modules for similar functionality, this is what I'd propose:

Title::get_product_code etc would just call AM_whatever under the hood, but as I write this I'm realizing you could do something like this:

let am = Am::init().unwrap();
let title = am.get_title_list(FsMediaType::Sd).unwrap()[0];
drop(am);
let _ = title.get_product_code(); // what happens here???

So maybe instead we should functions like Am::get_product_code(&self, title: TitleId) or something like that. The other alternative would be having something like struct Title<'a>{id: u64, _am: PhantomData<&'a Am> but that might not work super well if you need to pass title IDs around all over the place.

For now, probably just having struct TitleId(u64) is easiest and then any functions that need to call ctru_sys::AM* functions should be defined on Am itself, and take in a TitleId if relevant.

Maccraft123 commented 1 year ago

Based on what we've done in other modules for similar functionality, this is what I'd propose:

* Create a `struct Title{id: u64}` or something like that. The only way to get it should be by calling `Am` methods which always ensures `Am` is initialized

* All this stuff can live in `am.rs` since it's so closely tied together

Title::get_product_code etc would just call AM_whatever under the hood, but as I write this I'm realizing you could do something like this:

let am = Am::init().unwrap();
let title = am.get_title_list(FsMediaType::Sd).unwrap()[0];
drop(am);
let _ = title.get_product_code(); // what happens here???

So maybe instead we should functions like Am::get_product_code(&self, title: TitleId) or something like that. The other alternative would be having something like struct Title<'a>{id: u64, _am: PhantomData<&'a Am> but that might not work super well if you need to pass title IDs around all over the place.

For now, probably just having struct TitleId(u64) is easiest and then any functions that need to call ctru_sys::AM* functions should be defined on Am itself, and take in a TitleId if relevant.

Why not have all methods of TitleId struct take a reference to Am struct and use that reference to indirectly call ctru_sys::AM*? Or maybe even storing a reference to Am in TitleId struct?

Meziu commented 1 year ago

Why not have all methods of TitleId struct take a reference to Am struct and use that reference to indirectly call ctru_sys::AM*? Or maybe even storing a reference to Am in TitleId struct?

The Am service will mutate the internal state only in functions related to title installation (which we currently don't plan on supporting). As such, I suggest saving a PhantomData<&Am> inside of Title for zero-sized lifetime checking. We wouldn't really need the reference regardless, correct?

Maccraft123 commented 1 year ago

Why not have all methods of TitleId struct take a reference to Am struct and use that reference to indirectly call ctru_sys::AM*? Or maybe even storing a reference to Am in TitleId struct?

The Am service will mutate the internal state only in functions related to title installation (which we currently don't plan on supporting). As such, I suggest saving a PhantomData<&Am> inside of Title for zero-sized lifetime checking. We wouldn't really need the reference regardless, correct?

I... think we won't need, as all title-related ctru_sys::AM_* functions would be used in `TitleId::*

I experimented with implementing Title struct in this branch of my fork, to not affect this PR