search

MiSTer-devel / ao486_MiSTer

ao486 port for MiSTer
Other
252 stars 69 forks source link

Replace OPL3 with opl3_fpga #139

Closed gtaylormb closed 2 months ago

gtaylormb commented 2 months ago

I've recently cut the area of https://github.com/gtaylormb/opl3_fpga in half, so now it's only using 4% more ALMs than the existing NextZ80CPU implementation. I have some ideas to cut this down further. But it currently fits, builds, and meets timing.

I just ordered my DE10-Nano board, so once I get that I can verify/debug this.

I'm leaving this PR in draft form so anyone can take a look at what I have so far and maybe try it out.

sorgelig commented 2 months ago

That's good news. If i remember correctly, the main issue was some bugs in implementation making some incorrect playbacks. I remember Cyb (dos game) had weird sounds in some places.

gtaylormb commented 2 months ago

Current status: seems to be working in every game I've tried. ALM count just under the current core on my last build. Some things I want to tidy up.

I want to do some more verification which really just involves listening at this point. I have a RetroWave OPL3 board on the way so I'll have a real chip to compare to; might take awhile for that to arrive though. For now I can just compare against my Zybo version (which in theory should be identical) and DosBox/ScummVM implementations of OPL3.

I only just got my DE-10 Nano so I haven't done any comparisons against the current core. Please let me know what you think thus far and any issues you see (or hear). @sorgelig in particular if you have some spare cycles. If this is already a big improvement vs the current core I'm fine with merging now and polishing up later, or if you want to wait I'm fine with that too.

jsmolina commented 2 months ago

My ears feel it better, at least in the games I tried. Duke nukem 2 now sounds more 'punchy', more like my OPL3LPT.

I will try some more maybe this night.

OPL3LPT https://youtu.be/3dash3NlI7o?si=gMfgh1MB7TUqcAeZ&t=272

Your core https://www.youtube.com/watch?v=cLBNbIpcfMs (I know, I should try to do same condition and well recorded)

sorgelig commented 2 months ago

It's not an urgent task as current implementation is more or less fine. I will try your version in upcoming days. Specifically i'm curious if that weird bug in Cyb game disappeared. Do you have that DOS game? In settings you can switch melodies and some of them have "baaam" loud weird sound. I don't care about that game actually, just about the bug itself.

jsmolina commented 2 months ago

I've recorded from DN2, and it sounds better image sound.zip

duke2.mp3 is the new core duke2_old.mp3 is the old (current release) of the core.

New one (from this PR) sounds cleaner and more 'punchy' on bass, also for me is more pleasant.

Someone said that some notes are missing on new one (0:58), but I don't hear it, and it might happen on a real opl.

gtaylormb commented 2 months ago

It's not an urgent task as current implementation is more or less fine. I will try your version in upcoming days. Specifically i'm curious if that weird bug in Cyb game disappeared. Do you have that DOS game? In settings you can switch melodies and some of them have "baaam" loud weird sound. I don't care about that game actually, just about the bug itself.

I'll try out this game and let you know. Currently getting the clock speed to be an exact multiple of the OPL3 sample rate using a PLL, and going through the operator connections. Kinda crazy but even slight changes I can hear the pitch change, or at least I think I can (I need to record).

It's too early to say but there's potentially something wrong with 4-channel operators, just based on some inspection I'm doing. The channel connections are freaking complicated with all the different modes and the datasheet is not the best. Still waiting on that real RetroWave OPL3 chip to compare.

gtaylormb commented 2 months ago

The weird instrument sounds I was thought I hearing in Doom2 was due to differences in the FastDoom port vs the original. I confirmed in DosBox. I got a hold of the original version and everything sounds as it should again. Wasted a good few days trying to figure out why it sounded all janky--close but not the same. Pretty annoying for debugging stuff like this when your source is bad. The original also produces a bit louder OPL3 output which is great.

gtaylormb commented 2 months ago

It's not an urgent task as current implementation is more or less fine. I will try your version in upcoming days. Specifically i'm curious if that weird bug in Cyb game disappeared. Do you have that DOS game? In settings you can switch melodies and some of them have "baaam" loud weird sound. I don't care about that game actually, just about the bug itself.

@sorgelig I tried out what I think is Cyb which I obtained from here: https://www.dosgamesarchive.com/file/cybersphere/cybfull

It sounds great and is using stereo. I tried all the melodies in settings and didn't hear any loud "baam" sounds. Good music in this game.

I think this PR is pretty ready to go. I fixed the stuff I wanted: 4-operator connections, the core is running at the original OPL3 clock frequency of 14.31818MHz and the full 16-bit precision is going to the DAC. ALMs jumped up a tad to a few hundred more than the current core, probably mostly due to the full 16-bit precision and clock domain crossing logic.

One outstanding issue that I want to fix eventually is -opl3 -phase is not getting picked up in Doom giving you stereo sound, which is annoying because this was one of my favorite games. The current core doesn't pick it up either. I suspect there's some additional detection logic it's using that we need to fake out, but that might take a while to figure out. If you have any ideas, that would be great. The FastDoom port variants use stereo but they sound pretty bad otherwise (see above comment).

Moving from Draft to Ready for Review.

sorgelig commented 2 months ago

i think new version of write enable:

wire opl_we = (address[2:1] == 0) //220-221,228-229
           || (address[3:1] == 1) //222-223
           || (address[1] == 0)   //388-389
           || (address[1] == 1);  //38A-38B

wire opl_wr = write && !cms_wr && opl_we;
wire opl_rd = read && (address == 8);
wire opl_cs = sb_cs || fm_cs;

is incorrect. sb_cs should match with address of sb, and fm_cs should match address of fm.

this part is always true:

           || (address[1] == 0)   //388-389
           || (address[1] == 1);  //38A-38B
sorgelig commented 2 months ago

hmm.. it seems the whole write/fm_mode logic is broken. Not sure how it even works now.

sorgelig commented 2 months ago

should be like this:

wire opl_cs = (           address[2:1] == 0 && sb_cs)  //220-221,228-229
           || (fm_mode && address[3:1] == 1 && sb_cs)  //222-223
           || (             address[1] == 0 && fm_cs)  //388-389
           || (fm_mode &&   address[1] == 1 && fm_cs); //38A-38B

wire opl_wr = write && !cms_wr;
wire opl_rd = read && (address == 8);
gtaylormb commented 2 months ago

should be like this:

wire opl_cs = (           address[2:1] == 0 && sb_cs)  //220-221,228-229
           || (fm_mode && address[3:1] == 1 && sb_cs)  //222-223
           || (             address[1] == 0 && fm_cs)  //388-389
           || (fm_mode &&   address[1] == 1 && fm_cs); //38A-38B

wire opl_wr = write && !cms_wr;
wire opl_rd = read && (address == 8);

Was messing around with this when I originally integrated during debugging and never revisited. Will PR your suggested code.

sorgelig commented 2 months ago

also it would be good if you would fix all warning like: truncated value with size 32 to match size of target (26) it's easy to miss some real error/mistype if there are tons of such blue meaningless warnings.

jsmolina commented 2 months ago

duke2_new_samples.zip new samples with some note skipping

gtaylormb commented 2 months ago

I'm away this weekend but try to get to this early next week.

@jsmolina thanks for the samples. Anything you can do to describe how to exactly recreate the issue will help with debugging. Are the note drops consistent?

I have an idea that I think might be it. I don't think we're missing key-on events but not everything is listening to them, specifically 2 of the counters in the envelope generator I believe should be reset. My hypothesis is we're going into the attack phase but using stale counter values.

jsmolina commented 2 months ago

I'm away this weekend but try to get to this early next week.

@jsmolina thanks for the samples. Anything you can do to describe how to exactly recreate the issue will help with debugging. Are the note drops consistent?

I have an idea that I think might be it. I don't think we're missing key-on events but not everything is listening to them, specifically 2 of the counters in the envelope generator I believe should be reset. My hypothesis is we're going into the attack phase but using stale counter values.

I feel it has something to do with the drums, specially with kick drum? It happens on duke nukem2 menu, 'theme for duke nukem 2', you can download the VGM and play it from a VGM player https://vgmrips.net/packs/pack/duke-nukem-ii-ibm-pc-at#03-theme-to-duke-nukem-ii

gtaylormb commented 2 months ago

@jsmolina can you try this new build? https://github.com/MiSTer-devel/ao486_MiSTer/blob/20ca47c65d3b65a6020ab1e8891276a0a0f12f0c/releases/ao486_20240422.rbf

There was a potential overflow in envelope on attack. That would result in the volume dropping way down on a new key-on event, which could potentially sound like a missed kick drum. I don't hear any missed notes but you may want to double check.