Gambatte emu.frameadvance() + "Equal Length Frames" causing callbacks to run during DMA

[zig-for]

Summary

When running with "Equal Length Frames", calling emu.frameadvance can lead to lua running during a DMA operation. This will lead to garbage WRAM reads (open bus?) when doing a general "System Bus" read. See https://gbdev.io/pandocs/OAM_DMA_Transfer.html for a vague idea of what's happening. This may be desired behavior for some applications, but is probably unexpected for most users/script authors. I specifically saw this behavior in Link's Awakening DX.

In my specific case this causes issues with doing networked reads over RAM. I'm aware there are several workarounds:

1. Don't use that setting (unfortunately many of the users of my script use it for better audio)
2. Register a memoryexecute handler on the vblank handler (what I've done)
3. Translate between RAM addresses and system domains (annoying because I'm told Sameboy and Gambatte have different domain names nobody wants to do math on memory addresses in lua)

This was repro'd specifically on 2.8, but I've had reports of 2.9 as well and can repro there if desired. This could either be regarded as a BizHawk bug or a Gambatte bug, I'm happy to report to Gambatte, if given a url. I also understand if this is closed as expected behavior - technically it is "correct" by some reading of Bizhawk's docs and of the Gambatte setting description. I'm not sure what the fix here would be. Possibilities:

1. Sound resampling for Gambatte so that the option isn't needed (unlikely)
2. Skip lua callbacks if a DMA is in progress (might not be desired)
3. Attempt to run forward a bit more if DMA is in progress (??)

Repro

0. Ensure "Equal Length Frames" is set in Gambatte
1. Load up Link's Awakening DX in Gambatte

2. Load up this script

   while true do
   local sb = memory.read_bytes_as_array(0xC0FB, 4, "System Bus")
   local wr = memory.read_bytes_as_array(0x0FB, 4, "WRAM")
   
   if sb[1] ~= wr[1] then
       print("Hit error:")
       print(sb)
       print(wr)
   
       local pc = emu.getregister('PC')
       print(string.format("Bad PC: %04X", pc))
   end
   
   emu.frameadvance()
   end

3. Exit Tarin's house, go walk around opening and closing the menu and doing screen transitions

Output

"1": "120"
"2": "120"
"3": "120"
"4": "120"

"1": "0"
"2": "0"
"3": "0"
"4": "0"

Bad PC: FFC6

Host env.

• BizHawk 2.8, Win64
• Reports on 2.9, haven't personally reproduced but can if needed

[YoshiRulz]

This is not an upstream bug. The memory domain names should have been corrected in an earlier release, please do check that and we'll fix them ASAP.

[zig-for]

2.8: Gambatte:

"0": "WRAM"
"1": "ROM"
"2": "VRAM"
"3": "OAM"
"4": "HRAM"
"5": "System Bus"
"6": "CartRAM"

Sameboy:

"0": "ROM"
"1": "WRAM"
"2": "CartRAM"
"3": "VRAM"
"4": "HRAM"
"5": "MMIO"
"6": "BIOS"
"7": "OAM"
"8": "BGP"
"9": "OBP"
"10": "IE"
"11": "System Bus"

You know what, these look the same (or close enough, I'm never gonna need to read from BIOS or OAM directly), I'm going to go back and ask what the issue was.

[zig-for]

Ah, the issue was something more subtle - I think it was was a desire not to do domain translation, and additionally mainmemory.read_bytes_as_array using different domains between the two cores. I'll correct the original report.

[CasualPokePlayer]

For the "better audio" part, that's not why the setting exists in the first place. It existed a long time ago, once as the only option, so "frames" could use the "correct" FPS for TASVideos purposes. That is no longer the case with cycle count being used for timing along with general issues with that option (mainly input related causing "fake" lag) and it is strictly a legacy option. If better audio is desired, you can change the throttle to Audio Throttle (Config -> Speed/Skip -> Audio Throttle).

Also fyi, the audio being bad is due to resampling itself: Clock Throttle tries to slightly resample audio in order to account for small differences in core audio output compared to host audio output. This breaks down with GB cores like Gambatte and SameBoy and such which have "frames" which very little audio is output, throwing the resampling off and causing pitch issues until the "resampler" is reset (i.e. pause/unpause the emulator).

Also, this is strictly not Gambatte's issue but rather BizHawk's, as "Equal Length Frames" is nothing inherit to Gambatte (as the core just has its frontend request to run x amount of cycles, prematurely stopping in case of vblank, "Equal Length Frames" just requests for the emulator run again after this premature stopping; the same thing could be implemented in SameBoy but since again "Equal Length Frames" is an old legacy option I didn't bother).

mainmemory being different would be due to the cores not setting an explicit main memory domain, and so just using the first one registered (i.e. WRAM for Gambatte, ROM for SameBoy). That probably should be fixed (fairly easy to do so), but as a script it's easy enough to just use memory.* versions of the function and explicitly specify the domain as a workaround.

[zig-for]

Can confirm, using "Audio Throttle" seems to fix the issue for me. Unfortunately I still have to handle the other case. :( I'll recommend doing so to users.

[zig-for]

I wasn't trying to call out Gambatte is being broken, more unsure where best to report the defect.

[CasualPokePlayer]

Is there anything else actionable besides the main memory domain issue?

[zig-for]

Options might be:

• Removal of the deprecated option
• Better documentation around audio settings + Gambatte
• Better documentation around the deprecated option and the impact to scripting
• Improvement of the deprecated option to not stop or run lua in the middle of DMA

Edit:

• Maybe better documentation giving a warning about reading from the system bus on some platforms/options giving unexpected results.

[nattthebear]

Removal of the deprecated option

I don't love the option, but I'd rather not remove it as it already exists now so we've made our bed there. It should not be the default.

[CasualPokePlayer]

It hasn't been the default since 2.3.

[CasualPokePlayer]

I do suppose actually Equal Length Frames is actually redundant with the User Defined Frames option, as User Defined Frames with the "neutral" value will result in the exact same behavior. We could just potentially remove it and only have a simple bool decide if it should go subframe (or maybe can make it into a proper separate core that wasn't done before because I was an idiot)

[YoshiRulz]

I don't understand what this issue is about, but I'm taking that fact as a sign it shouldn't be a blocker for release.