Fix save states

[jdgleaver]

This was a fun bug...

At present, the core randomly hard locks the system when loading save states on the 3DS (issue #103). This also happens on the Vita (issue #97).

After a great deal of debugging misery (working with the 3DS is a nightmare), it turns out that this problem actually affects all platforms. It seems that when the core was ported to RetroArch, two boolean state variables were omitted from the 'StateSaver' code: spu.ch1.duty.high and spu.ch2.duty.high. Since these values are never getting set when loading a state, we're left with uninitialised automatic variables - i.e. complete garbage.

It just so happens that most of the time, on most platforms, we get lucky and they end up as 0 - but not always, and not on the 3DS. Quite apart from the fact that this means save states have never worked 'properly' (the spu isn't restored correctly), these booleans are used to generate some important counter values - if they don't resolve as either 0 or 1, then these counters break and the core gets stuck in infinite loops. Hence the system lock ups on the 3DS. I was also able to reproduce this under Linux (eventually...).

This pull request fixes the issue! While I was editing the StateSaver, I also added two other missing network-related state variables - every parameter is now accounted for.

This should close issues #103 and #97 (I don't own a Vita and so cannot test the latter, but the bug and the fix were clear and obvious in the end).

Note that this will invalidate any existing save states made with the core, but since they were incomplete anyway I don't think this is much of a loss.

[hizzlekizzle]

whoa, nice sleuthing! Is this something we should submit upstream, as well? (I haven't checked to see where upstream is these days, as far as progress; they may have corrected this one themselves long ago)

[jdgleaver]

Okay, I just checked - I think upstream is https://github.com/sinamas/gambatte, and it looks like this was fixed in commit https://github.com/sinamas/gambatte/commit/a9039eb8d2bd661f0f63b15a43a0803124006c08 (ffs... why didn't I see this before I started debugging...?!). So it was probably just a copy/paste error of some kind in the libretro core (the StateSaver code was overhauled quite heavily during the porting process, so I could see how it might happen).

I guess someone ought to check upstream for any other changes we might want... There's also https://github.com/Dabomstew/gambatte-speedrun which has some very nice additions... I guess I may try backporting some features from this 'speedrun' fork at some point - if only there were more hours in the day...

EDIT: After checking properly, I can see that this bug never affected upstream: the duty.high state variables were added in commit https://github.com/sinamas/gambatte/commit/a9039eb8d2bd661f0f63b15a43a0803124006c08, and the StateSaver code was updated correctly at the same time. So this was definitely a copy/paste oversight at our end!

[hizzlekizzle]

AFAIK, gambatte was one of the first cores ported and hasn't really been touched since, so it's likely many years out of date. It'd be good if pretty much anything can be backported/rebased.

[jdgleaver]

From a quick scan of the commit history it looks like we're actually in good shape with respect to the 'official' upstream repo - I can't see any significant differences (other than some irrelevant - for us - frontend stuff, and the removal of booleans from the savestate struct - not really sure why they did that...).

The https://github.com/Dabomstew/gambatte-speedrun fork, on the other hand, is under active development, and has: accuracy improvements, a cycle based RTC, huc3 support, SGB support, timing fixes, etc...

Getting these updates into the libretro core looks like a fairly substantial task, but hey, I'll take a stab at it as time allows. I've got a couple of other things I want to work on first, though (wish I could play with this stuff full-time!).

[bluesuitriot]

@jdgleaver Thank you so much for addressing this issue!