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

SI/DeviceGBA: Fix SI timings to actually closely match hardware #9552

Merged
merged 1 commit into from Apr 27, 2021

Conversation

endrift
Copy link
Contributor

@endrift endrift commented Mar 3, 2021

The 115200 baudrate that was previously in this connection was guessed, seemingly at random, and does not match hardware at all. It takes ~5 µs per bit when the GC sends and ~4 µs per bit when the GBA replies, confirmed by actual captures of logic analyzer data I did a few years back. The stop bit timing was also not present, which I have added somewhat rough approximations of. I can attach said data as proof if desired.

Marked as WIP because this will probably break VBA-M, and is mostly here for informational purposes for the time being.

@JMC47
Copy link
Contributor

JMC47 commented Mar 5, 2021

I was asked to test this with VBA-M connectivity to ensure things weren't broken. The good news is that it seems to be the opposite! Things actually work better overall.

The Legend of Zelda: Four Swords Adventures - Unchanged
Final Fantasy: Crystal Chronicles - Connects faster, does not hang nearly as much when transitioning between modes. Still drops inputs.
Mega Man X: Command Mission - Unchanged
Nintendo Puzzle Collection - Loads much faster.
Crash Bandicoot: Wrath of Cortex - Unchanged
Fire Emblem: Path of Radiance - Works, untested on master.
Pac-Man VS - Works
The Legend of Zelda: The Wind Waker - Has difficulty/desync issues on master in strenuous areas. Actually performs better in this PR. Was not able to get it to disconnect, where as I could in master.

@endrift endrift marked this pull request as ready for review March 5, 2021 22:58
@endrift
Copy link
Contributor Author

endrift commented Mar 5, 2021

Does Dolphin merge patches against latest master when building, or is it just whatever the state of the commit at the time was?

@endrift
Copy link
Contributor Author

endrift commented Mar 6, 2021

Rebased on top of master since merging the previous commit. Can you please retest the ones that had changed behavior in the previous testing?

@JMC47
Copy link
Contributor

JMC47 commented Mar 6, 2021

Everything seems unchanged after doing more extensive testing. I thought there were regressions, but it turned out that I was rather lucky last night. I'll update a few things.

1: Final Fantasy Crystal Chronicles can connect on this build but it still likes to reset after connecting for some reason. Sometimes it does, sometimes it doesn't.
2: While I didn't get Wind Waker to disconnect, VBA-M did deadlock once with the tingle tuner. I have no reason to believe this is related to the pull request though. The deadlocks occur with master as well.
3: I actually got Navi Trackers to boot this time around in Four Swords+. However, the audio is still broken on VBA-M. Not related to this PR.

Compared to master after more thorough testing, things are actually almost the same. Final Fantasy Crystal Chronicles connecting every time last night was a fluke, but it does connect a good bit more than on master.

Things seem no more broken than before.

@endrift
Copy link
Contributor Author

endrift commented Mar 7, 2021

So of these which works best do you think?

If the answer isn't the last one things might get hairy, but if the last one is on par with anything else this should be merged after review.

@JMC47
Copy link
Contributor

JMC47 commented Mar 8, 2021

I made all of the builds you requested and tested them back to back to back to back and swapped between them trying to find major differences. I really don't see much. No games appear to be fully fixed or broken. The timings do shift a bit depending on what build I use, but without testing every game on console I can't determine what is accurate.

@endrift
Copy link
Contributor Author

endrift commented Mar 9, 2021

That's not exactly what I asked for...

I was trying to gauge if either of the two PRs broke anything, either in isolation or together. If the answer to that is "no", then we should consider if this fixes anything, and if it does is the same set fixed with or without that other PR.

What I got instead was a bunch of "well it depends", which isn't useful for telling if this PR is worth merging. It's not actionable information.

@JMC47
Copy link
Contributor

JMC47 commented Mar 9, 2021

It's really hard to be certain because connecting to VBA-M is so inconsistent. Sometimes it just won't work and I don't know why. There's literally no feedback as to what happened.

That's why I tried to generalize behaviors across a lot of runs. In a 1:1 scenario, this is the only data I can measure reliably and it's connection/loading times. The other PR has no measurable effect outside of these random crashes which may be dumb luck or may be slightly better. However, this one has a noticeable difference.

Final Fantasy Crystal Chronicles doesn't connect... normally. However by resetting the game I can get it to consistently connect. On Master, it takes roughly 16 Nintendo flashes to connect. On this Pull Request it takes on average 28 Nintendo Flashes. This also means that the game disconnects for a long time between levels as it takes longer to reconnect if it manages to reconnect at all. Whether it reconnects or not seems to happen at the same interval as Master.

I can measure this on console tomorrow once I get my GBA connectors back.

@JMC47
Copy link
Contributor

JMC47 commented Mar 19, 2021

While I couldn't find my GBA connectors, @AdmiralCurtiss tested this for me. He says that FF:CC takes roughly 17 pulses to connect on console, which means that this pull request results in connections taking far too long for some reason. Further investigation as to why is necessary.

Edit; There is more to this than I originally anticipated, and the above notes may be incorrect depending on factors that I'm not entirely sure on yet. So, now it purely becomes a "I'm not entirely sure" situation.

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Mar 19, 2021

I'm guessing it's related to what the GC CPU is doing at the time. From what I played around with

  • Connecting while idling on the save file select screen or title screen takes ~17-18 flashes.
  • Connecting while the intro FMV plays takes ~19-20 flashes.
  • Connecting during game boot (as in, leave GBA in BIOS and connected, then boot the game) takes ~21 flashes.

So yeah, fun.

e: For reference, that's the US version of the game running off a legit disc on an EU Wii console, game booted with GeckoOS and Force NTSC. I suspect this doesn't matter much but just to note.

@endrift
Copy link
Contributor Author

endrift commented Mar 20, 2021

I have the US game and a US GameCube I can test on but I suspect it doesn't matter much either. I'm curious to see what went wrong though, since it should be transferring data faster.

@endrift
Copy link
Contributor Author

endrift commented Mar 28, 2021

Rebased on latest master since #9582 got merged.

@JMC47
Copy link
Contributor

JMC47 commented Mar 31, 2021

Unknown to me when I first tested this, this is required to make connections between Sonic Adventure 2 and Sonic Advance Tiny Chao Garden.

Verified to work between mGBA 0.9 and this pull request. Does not work in Dolphin Master alone.

Copy link
Contributor

@JMC47 JMC47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been going through the rest of the library and I think that this change is correct with the recent fixes to mGBA's side after the initial version of this pull request. The fix to Sonic Adventure 2 is nice and other games aren't broken.

I'm not super familiar with the code here but I have worked here before to try to fix GBA games so I have a fairly decent idea of what it does. The code looks clean and doesn't touch a ton of stuff.

Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really too much going on here, and I guess its good to go if JMC found no issues during testing.

@JMC47
Copy link
Contributor

JMC47 commented Apr 24, 2021

Now that we have the main announcement out of the way regarding support, can this be finished so we can get Sonic Adventure 2/Sonic Advance working properly?

@endrift
Copy link
Contributor Author

endrift commented Apr 24, 2021

Is it not already finished, just pending merge?

E] I see I forgot to add the GBA stop bits back to the no-reply code path.

@JMC47
Copy link
Contributor

JMC47 commented Apr 27, 2021

Let's fix Sonic Adventure 2 and get the better timings in.

@JMC47 JMC47 merged commit 4d10023 into dolphin-emu:master Apr 27, 2021
10 checks passed
@endrift endrift deleted the gba-timing branch April 27, 2021 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants