MiSTer-devel / Gameboy_MiSTer

Gameboy for MiSTer
99 stars 46 forks source link

Fix APU DAC handling #217

Closed maij closed 1 year ago

maij commented 1 year ago

This follows from #216, which was merged then rolled back because the last commit was incorrect. I have remedied that here.

birdybro commented 1 year ago

This PR has also apparently introduced bugs. One user in discord mentioned that multiple games are not working. I can't get any games to load with this PR. The bios won't even load. More testing should be done with a wider number of users before a release.

Games I can't load:

Super Mario Land Super Mario Land 2 Link's Awakening Castlevania Castlevania II

Games where it looks like it loads but doesn't work:

Animaniacs (won't get past the Konami logo, just repeats back to the copyright text after pressing start)

maij commented 1 year ago

Strange, I see the issue with the test build as well. These issues did not occur with the image I generated. Would it be worth trying another test build? I used all the default files, didn't modify the project when I made my test images.

birdybro commented 1 year ago

I'm compiling one myself and will retest.

birdybro commented 1 year ago

The one I compiled myself works.

I am using Quartus 17.0.2 Standard on this PC, what version of Quartus did you use to compile it?

image

This image shows the one I just compiled, then the one from the PR 216 you attached, and then one from the unstable builds here --> https://github.com/MiSTer-unstable-nightlies/Gameboy_MiSTer/commit/eaac36b7ebbff35c35eb6a9ba94a1a233470e7e6

I believe the unstable build might be incorrectly compiled for some reason.

The one I just compiled --> Gameboy.zip

maij commented 1 year ago

I'm using Prime 17.0.0 image

birdybro commented 1 year ago

The difference between that and 17.0.2 is super minor so that wouldn't result in this I don't think. I'm pretty sure the unstable nightlies builds use linux's 17.0.2 build 602.

Kitrinx commented 1 year ago

This PR in the latest test build is causing large audio amplitude spikes that creates cutout and other problems on modern audio hardware, making the core unusable. Here is a demonstration from Final Fantasy Legend II intro:

Screenshot 2022-11-07 115326
maij commented 1 year ago

This PR in the latest test build is causing large audio amplitude spikes that creates cutout and other problems on modern audio hardware, making the core unusable. Here is a demonstration from Final Fantasy Legend II intro:

Screenshot 2022-11-07 115326

Thanks for testing the core out. Could you provide more detail to what is going wrong? What hardware does this present a problem in? Direct audio out/HDMI out, both?

I don't think reversing these changes is the best solution because it is a more accurate implementation of the gameboy (audio pops and all), but happy to look at a workaround. We could maybe have a menu option for a version that minimises these audio spikes.

I don't think 'unusable' is correct -- it works just fine with HDMI out to a typical TV/monitor set up.

birdybro commented 1 year ago

If the audio pops are too severe and too quick some AV devices can react to them as problematic voltage spikes and it can trigger cutouts due to safety in the design. I didn't hear a problem like that, but I didn't test FF Legend II the other day, maybe the pops in that game are more significant. If this change does make it so it's cutting audio out over HDMI on modern hardware, that would be worth fixing.

maij commented 1 year ago

Thanks birdy, that's clear! If that's the case then I would be happy to look into an audio device friendly solution.

@Kitrinx, @birdybro: do you think a low-pass filter (maybe using Sorg's IIR filter) would be a viable solution? It's hard to tell the exact timescale from the previous plots but I could try different LPF cut-off frequencies/orders. It looks like there's a way to strobe in the filter coefficients on the fly via the HPS. Do you know how this works?

Kitrinx commented 1 year ago

A low pass and high pass filter are already applied by the framework (you can adjust them as you like) but it would not be true to the original systems sound to apply a more aggressive one. Eyeballing it, I suspect the issue is that the high pass filter is responding to very sudden dropoffs as you're going from "some value" to "0" (or max). I think the characteristics of how it generates audio just have to be fixed.

maij commented 1 year ago

Fair enough, I think I know the problem in the core itself. I think it is to do with the volume amplifier at the output, it should be pushing +ve values more positive and negative values more negative, but for an unsigned implementation it just pushes everything to more positive values. I will report back after I attempt a proper fix, I think the easiest way is to do at least the final step using signed multiplication. This could mean the PR216 was actually more accurate... I was hoping it wouldn't be merged before more conclusive testing. Ah, well...