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

Tokyo Disneyland - Fantasy Tour hanging/crash #2482

Closed
medigi opened this issue Nov 17, 2020 · 17 comments · Fixed by #2934
Closed

Tokyo Disneyland - Fantasy Tour hanging/crash #2482

medigi opened this issue Nov 17, 2020 · 17 comments · Fixed by #2934
Labels
Core: SameBoy (2.6.3 and older) (Deprecated) Super Game Boy (SGB) core Repro: Affects 2.5.2 Working as intended (wontfix) Intended behaviour perceived as a bug, or an unreasonable feature request

Comments

@medigi
Copy link

medigi commented Nov 17, 2020

Tokyo Disneyland - Fantasy Tour (Japan)
SHA1:E413B0AB506CB12B68439CCC0A72E4664C112378
MD5:6DB7AA2399E9B6723E41E47C73379216

Crashing in Minnie's mini-game "Minnie's House" for unknown for me reason. Tested in cores: Gambatte, SameBoy, GBHawk (Bizhawk 2.5.2)
Tokyo Disneyland - Fantasy Tour states.zip

For reproduce, load savestate in any of these cores and press "A" button.

@nattthebear
Copy link
Contributor

Crashes in three completely unrelated cores? Hmm

@nattthebear nattthebear added Core: Gambatte (Alt.) Game Boy / Color (GB/GBC) core Core: GBHawk Game Boy / Color (GB/GBC) core Core: SameBoy (2.6.3 and older) (Deprecated) Super Game Boy (SGB) core labels Nov 17, 2020
@medigi
Copy link
Author

medigi commented Nov 17, 2020

Yes, except BSNES core

@nitro2k01
Copy link

nitro2k01 commented Nov 21, 2020

This seems to happen because unmapped SRAM reads $FF which is generally correct because there are pull-up resistors, but on hardware it's a bit more complicated than that. Because of bus capacitance, the last value that existed on the bus (in this case the opcode for the read) remains for a short period when reading from non-existent or disabled SRAM. Note that this is an analog effect and the exact behavior may vary depending on the model and revision of Gameboy as well as the cartridge being used. It's also not always deterministic.

Observations from my tests on two Gameboys so far.

  • On a DMG, it consistently reads back the value of the read opcode. (In my test this was ld A,[DE] and read back $1A)
  • On a GBA SP, the value reads back using the same code varied and could be any one of $1A,$5A,$DA,$FA. This indicates that the voltage level on the bus is right at the logic level threshold and crosses it on some cycles but not on other cycles for the various data lines.

As for the bug in the game in question, the gory details are as follows: The game is using a table to look up the VRAM locations for the items in the game. Unfortunately, the code at 08:4BAB has a bug and iterates 17 times instead of 16. (The number of items in the minigame is 16.)

At 08:4B79 this leads to an out of bounds read from the table starting at $4FFB. The out of bound read reads a pointer from $501B which is the value $A698, which points to SRAM. This is the first value of the next table and is actually supposed to represent $98A6 in the opposite endianess, which is a target address in VRAM.

The effect is that $A698 is read and treated as a target pointer for a write later down the line. It's read with the instruction ld A,[BC] which has opcode $0A.

  • On hardware, this typically means a value like $0A0A which is a harmless write to the MBC.
  • On most emulators, this produces a write to $FFFF in the end, which happens to be the interrupt enable register. Because of the data written, this may disable the VBlank interrupt and make the game freeze.

You can prove that this really is what is causing the freeze using BGB. Set an access breakpoint to reads from SRAM. (A000-BFFF) Trigger the bug in the minigame. When it halts, step over the read, edit the A register so it is 0A. Do this again for the second read. Observe that the game does not crash.

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented Nov 21, 2020

To elaborate further on this "non-deterministic" behavior, the results can be affected by temperature, dirt on the pins, etc. And you can see with the GBA SP you can just already see non-deterministic results (minor note, the GB Player has results like the DMG nitro tested) And then you can consider that for some systems (like cgb and cgb-b), FF read could be correct and you might actually find for some systems the game would actually crash like emulators do.

Also for why BSNES doesn't crash, this code is likely to blame:

uint8 Cartridge::ram_read(unsigned 0addr) {
if(ramsize == 0) return 0x00;

0x00 reads are just impossible (0x00 is nop, and pullups only turn 0s into 1s, not vice versa). So it doesn't crash on BSNES due to an inaccuracy, although other emus it crashes due to them being closer to being truly accurate, but still (kinda) not truly accurate.

@alyosha-tas
Copy link
Contributor

Neat, thanks for the breakdown. Is this a general thing or only MBC1?

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented Nov 22, 2020

General thing, lack of SRAM/locked SRAM have fun:tm: open bus results, and MBC shouldn't affect it.

EDIT: Well, the MBC2 is a potential odd case, since the upper 4 bits of RAM are open bus, but the lower 4 bits have an actual value, not sure what the upper 4 bits should have (probably following current ff approach just make it OR 0xF0? Don't know how the last opcode thing would work out?)

@alyosha-tas
Copy link
Contributor

alyosha-tas commented Nov 23, 2020

I implemented basic open bus behaviour for bad SRAM accesses for mbc1 in GBHawk. I'm not going to spend time on analog effects here.

EDIT: looks like the dev build is broken though @YoshiRulz

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented Nov 23, 2020

The behaviour definitely affects other MBCs, my tests with the GB Player have been with an MBC3 flashcart, and previous tests (from like 2 years ago lol) have been done using ACE/ARE on Pokemon Gold, which uses the MBC3. Also definitely doesn't need an MBC, no SRAM is essentially the same as locked SRAM for these purposes (and I guess to this point, TDFT has no SRAM anyways lol).

The fix is also just inaccurate, and goes under the assumption everything uses the same memory bus, when in reality they don't. For the GB Player at least, ROM and SRAM share their bus, while VRAM/WRAM/OAM/HRAM do not share that bus. This matters since the read does not necessarily have to be executed in ROM, it could be executed from the 4 memory chunks I laid out that do not share their bus with SRAM. If the read is executed from one of those 4 locations, nothing is going to be pushed into the ROM/SRAM bus at all (the opcode value will go into a different bus), so you'll end up reading 0xFF. This is only coming from my tests on the GB Player, bus arrangements can vary based on revision (but I'm pretty sure in no model everything would share the same bus).

This also assumes every opcode will produce this behavior for all reads, which is just incorrect (and generally needs much more testing). For every test to my knowledge, only opcodes without operands have been used. For an opcode with an operand, let's take ld a,[xxyy]. Would it return FA? Or would it return A0-BF? The CB prefix also could apply with this (although only for the MBC2, with the swap [hl] opcode edit: I am wrong, other prefix opcodes could be affected and wouldn't be exclusive to MBC2). And there is the case with pop/ret. pop/ret do 2 separate 8-bit reads. From my testing on the GB Player, the first read returns the last opcode executed, while the second read will just return FF.

There is also a fast elephant in the room, being double speed mode. While this is more for the GBC/GBA, it hasn't been tested as far as I know, although it likely would have an effect on this behavior. edit: double speed mode has no effect for most cases. however, it does change the edge case of pops/rets, making the "second read will return FF" not true and instead just having the last opcode executed. double speed mode also allows jumping from cart bus location into sram without instantly rst 38'ing, and executing the last opcode executed again. this also leads to an extreme edge case if this open bus value executed reads from open bus too (only examples being or/xor/and/cp (hl), which return $FE | L, yes the L register matters for some reason lol)

Even ignoring the freezer and sticking to the practically deterministic results, there's still a lot that needs testing regarding this behavior.

@alyosha-tas
Copy link
Contributor

Ok I updated the emulation of this to only effect the cart busses. It still won't do the 2 consecutive reads thing correctly, but it's good enough for now.

It's pretty expandable and doesn't really slow things down so it can be improved later if necessary. But I'll probably only do so if some specific game or TAS is actually broken.

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented Dec 10, 2020

Seems like my suspicions were right about operands, ld a,[xxyy] does indeed take in the last operand (ie, for reading to $A000, $A0 would be returned). Goes to show that knowledge is incomplete with this lol

EDIT: Noticing that this is actually implemented in GBHawk, nice. Also seems like open bus is only returning FF for no MBC? Again, no MBC is actually needed for this behavior, so seems wrong.

@alyosha-tas
Copy link
Contributor

default mapper (no MBC) does return open bus if no SRAM is present. Open bus behaviour is emulated on no MBC, MBC1 and MBC3 currently.

@nitro2k01
Copy link

EDIT: Well, the MBC2 is a potential odd case, since the upper 4 bits of RAM are open bus, but the lower 4 bits have an actual value, not sure what the upper 4 bits should have (probably following current ff approach just make it OR 0xF0? Don't know how the last opcode thing would work out?)

When SRAM is disabled it should nominally work the same as anything else because the MBC is not driving the data bus. In practice, there may some small timing difference between the upper and lower nibble because the high impedance MBC connection may still make the data bus settle to FF slightly faster or slower.

Nintendo could however have decided to implement the SRAM weirdly due to the integration of SRAM into the MBC, including:

  • Reads actually still work as normal and only writes are disabled.
  • The lower nibble is driven, but to 0 or F and not the value stored in RAM.

When SRAM is enabled, the upper nibble should exhibit open bus, and show the last value that was on the bus, for the upper nibble only. However, I would expect the impact of getting this wrong is low in terms of games malfunctioning, since it was well documented by Nintendo that the upper nibble should be discarded, and most games probably did do so in practice. Because of the small size of the SRAM, it's also likely that most games only access it one place to copy it in or out of SRAM, making bugs that rely on the upper nibble less likely.

Over all, more research needed!

@medigi
Copy link
Author

medigi commented May 17, 2021

Just tested this game in the new implented bsnes version 115 (as a new core, bizhawk dev build) = doesn't crashing. However, if possible, update SameBoy core as soon as you can. So i can test the full entire game, and if there will be no crash - issue can be closed as fixed.

Crashing in next cores:

  • Gambatte
  • SameBoy

Doesn't crashing in:

  • GBHawk
  • SubGBHawk
  • BSNES
  • BSNESv115+ (as a new core in dev build)

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented May 17, 2021

Upstream Gambatte-Speedrun has a draft PR that can fix the crash in its current state (stalled until I get the time/motivation to do more hw tests). SameBoy is another nightmare as that thing is a super old version and upstream still crashes afaik anyways. And considering upstream BSNES now just uses SameBoy for its GB core, I'm going to take a guess this new BSNES core did not bother updating the GB core (so SGB emulation is still trash lol), which as noted earlier is a bug in itself as it is always returning 0 for open bus values, which is absolutely wrong in nearly every context (GB pulls up, not down)

EDIT: Oh I also noted for GBHawk in another unrelated issue that its open bus code seems to be a bit off (writes do not affect the bus from what I've tested, and GBHawk seems to be treating writes as such). Edge cases still need to be tested, and GBHawk probably gets those wrong too.

@alyosha-tas
Copy link
Contributor

I just assumed writes would effect the bus as well. Once an actual test is out there verifying otherwise, I can easily fix it.

@medigi
Copy link
Author

medigi commented May 18, 2021

Tested this game on SameBoy 0.14.3 and confirming a crash on that mini-game. This is probably one of problematic gameboy games to emulate

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented May 18, 2021

Tested this game on SameBoy 0.14.3 and confirming a crash on that mini-game. This is probably one of problematic gameboy games to emulate

No, the entire problem is disabled SRAM read behavior. It's rather just due to a bug in this game, and the crashing behavior seen on emulators could very well happen on certain consoles too, as disabled SRAM reads are (more technically) non-deterministic (more practically, they are deterministic with certain consoles).

@alyosha-tas alyosha-tas removed the Core: Gambatte (Alt.) Game Boy / Color (GB/GBC) core label Jun 14, 2021
@YoshiRulz YoshiRulz added Working as intended (wontfix) Intended behaviour perceived as a bug, or an unreasonable feature request and removed Core: GBHawk Game Boy / Color (GB/GBC) core labels Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core: SameBoy (2.6.3 and older) (Deprecated) Super Game Boy (SGB) core Repro: Affects 2.5.2 Working as intended (wontfix) Intended behaviour perceived as a bug, or an unreasonable feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants