agb icon indicating copy to clipboard operation
agb copied to clipboard

Implement GBA-specific synchronization primitives and save data support

Open Lymia opened this issue 3 years ago • 3 comments

This is a port of code I wrote for the mostly abandoned gba crate. Licensing shouldn't be an issue as the gba crate was under the same tri-license as the agb crate, and furthermore, I was the primary author of all the code included here. (Original PRs: https://github.com/rust-console/gba/pull/107 https://github.com/rust-console/gba/pull/109) (gba crate relicensing: https://github.com/rust-console/gba/pull/116)

Changes made:

  • Added an agb::sync module that implements some GBA-specific primitives that take advantage of the fact that the only real "multi-threading" on the GBA is interrupts and otherwise, there is just the main thread.
  • Changed a lot of the code in agb that relied on bare_metal::Mutex to instead use GBA-specific mutexes. This does make interrupts more restricted in what they can do, but reduces the amount of code in the crate that disables interrupts - allowing effects like wave.rs (which have a very simple interrupt handler, but will result in bugs if missed) to work a lot more reliably.
  • Added a new agb::save API that supports all the save media types found in official Nintendo cartridges.
  • Added tests for all of the above.

This is a pretty invasive change, so, if you'd like this to be broken into multiple PRs, or have specific parts removed, that won't be a problem.

Lymia avatar Aug 17 '22 13:08 Lymia

Thanks for this! I don't have time at the moment to look through in detail, but I'll do so when I can, which should be fairly soon.

One quick thing is this

Licensing shouldn't be an issue as the gba crate was under the same tri-license as the agb crate

We don't have a tri-license, the code in agb is licensed under MPL-2.0.

corwinkuiper avatar Aug 18 '22 00:08 corwinkuiper

Thanks for this! I don't have time at the moment to look through in detail, but I'll do so when I can, which should be fairly soon.

One quick thing is this

Licensing shouldn't be an issue as the gba crate was under the same tri-license as the agb crate

We don't have a tri-license, the code in agb is licensed under MPL-2.0.

Thanks for the clarification. For clarity, then, I'm willing to relicense any of the code here I can to full MPL-2.0. From the git logs, nobody else added anything more than small updates - of which are no longer relevant due to the porting.

Lymia avatar Aug 18 '22 01:08 Lymia

Any updates on this?

Lymia avatar Sep 01 '22 19:09 Lymia

Hiya. @corwinkuiper and I have been super busy lately (they're getting a new job and I am currently moving house). We haven't had the time to spend on agb that we would've liked, and likely won't be able to spend much time on it until early November TBH.

This feature is awesome and was something that was on my todo list so really appreciate you spending the time to get this up and running. Am looking forward to trying this out on our bootleg cartridges once the house move is complete and I have access to my stuff again :P.

We'd appreciate if you would split this PR into a few smaller ones which we can handle more easily with the little time we currently have :).

Some general comments:

  • I really like the sync module, the 'atomics' it gives us are cool and super useful - happy to PR this on its own?
  • we're not a fan of the mutex implementation which panics if locked. Also, the mixer demo runs noticeably slower with this (you can test that yourself by doing just run-example-release stereo_sound and checking the number of cycles it displays there). We go from 6971 cycles per frame in current master to 7540 cycles per frame in this PR
  • We're probably happy to remove the agb testing feature (or at least make it default) which avoids the need for the agb-test top level folder as it can be integrated directly into agb - we'll do this today to make this easy :)

Again, we apologise for the radio silence, but this has been a really stressful time for both of us for different reasons.

gwilymk avatar Sep 15 '22 21:09 gwilymk

I really like the sync module, the 'atomics' it gives us are cool and super useful - happy to PR this on its own?

Yeah, I can. I'll get to that sometime when I have time.

we're not a fan of the mutex implementation which panics if locked. Also, the mixer demo runs noticeably slower with this (you can test that yourself by doing just run-example-release stereo_sound and checking the number of cycles it displays there). We go from 6971 cycles per frame in current master to 7540 cycles per frame in this PR

I'll have to take a look at that properly. There'll always be a tradeoff between the potential for IRQs to get skipped (when disabling IRQs) and performance, but back when I wrote this for the gba crate there was no real point of comparison wrt a safe approach to disabling IRQs ala the existing bare_metal::Mutex.

Lymia avatar Sep 16 '22 04:09 Lymia

Updated this PR according to the recommendations. Waiting on the other PR to figure out what to do with Mutex.

Lymia avatar Sep 16 '22 06:09 Lymia

Thanks so much for this! :D

Once I've finished moving house I'll have access to all my GBA stuff again and I look forward to being able to make a proper copy of hyperspace roll on one of my bootleg cartridges :D.

gwilymk avatar Oct 01 '22 18:10 gwilymk