MiSTer-devel / ao486_MiSTer

ao486 port for MiSTer
Other
252 stars 69 forks source link

New release build #148

Closed gtaylormb closed 1 month ago

gtaylormb commented 1 month ago

Just wondering, what's the release process for new binaries? I deleted all the release binaries from my last PRs but it doesn't look like a new one has been added in the last few weeks, so the latest is missing the new OPL3 core. I can build and submit as a PR, if that makes things easy.

sorgelig commented 1 month ago

Release isn't happen after each change. There are unstable builds automatically made daily in separate repository for anyone wanting to try current status.

gtaylormb commented 1 month ago

Is there a formal process for new stable releases?

sorgelig commented 1 month ago

i will make new release asap

sorgelig commented 1 month ago

I've built it and... old bug of opl3_fpga is there. So it's not for release yet.. You can hear the bug here: https://youtu.be/4gPerLaIwjw It seems vibrato effect is buggy, so it produces "boom" sound instead. Besides that, reset signal doesn't reset the OPL3 and it still produces the last sound. This must be fixed too.

gtaylormb commented 1 month ago

Oh yeah I'm able to replicate it. To me it sounds like the same instrument as the real OPL3 but the volume is way too high compared to the others. I swear I tried this game a few weeks ago and it was fine, so it may be a regression. I need to dig into it.

Reset should be fairly easy to fix. All the state now is in memories which don't get reset but I'll figure out a way around that.

gtaylormb commented 1 month ago

Completed in https://github.com/MiSTer-devel/ao486_MiSTer/pull/150. I guess when I checked this game a few weeks ago I didn't let the first song play out and instead went into trying the other songs. Regardless, it was a pretty serious bug that's been there for 10 years and it's now fixed, so thanks for pointing that out.

sorgelig commented 1 month ago

Btw, didn't you check this implementation? https://github.com/jotego/jtopl It doesn't support OPL3 but shouldn't be much work to add it. Usually @jotego follows original HW and implementation consumes less resources.

gtaylormb commented 1 month ago

I don't think this was around when I started opl3_fpga. I'll check it out. I'll certainly be impressed if it consumes less resources once all the OPL3 stuff is added. Opl3_fpga consumes about 1000 LUTs in a Xilinx 7-series which is quite low, but there's always room for improvement. The 7-series has superior memory implementation in that it has 0-cycle read distributed/LUT RAM, but I've tried to optimize the design for the Cyclone-V by registering most of the larger RAM outputs for BRAM implementation. Took me a while but I finally realized the design varies by hundreds of ALMs depending on the build seed; I really wasted a lot of time sometimes thinking I'd reduced or inflated the design.

BTW, I think my "more robust" trick sw detection algo actually broke detection for some games (notably SCUMM-based games), hold off on adding a build for now. Might have another PR incoming.

gtaylormb commented 1 month ago

Detection fixed in https://github.com/MiSTer-devel/ao486_MiSTer/pull/151. Passes everything I've tested thus far. If you find any more issues let me know, otherwise we should be good to go for a new release binary.

sorgelig commented 1 month ago

I don't think this was around when I started opl3_fpga.

it's about a year or so.

0-cycle read distributed/LUT RAM in cyclone-v it's called MLAB. So it's available too.

sorgelig commented 1 month ago

BTW, I think my "more robust" trick sw detection algo actually broke detection for some games (notably SCUMM-based games), hold off on adding a build for now. Might have another PR incoming.

I don't know why you are so hurry to make a release. Take it easy. Unstable builds are available for testing for those who want.

gtaylormb commented 1 month ago

0-cycle read distributed/LUT RAM in cyclone-v it's called MLAB. So it's available too.

Hmm doesn't seem to auto infer anyway. I get messages that memories are not placed in RAMs. I'll look into it though; sometimes these memories need a specific coding style to auto infer. If you have any docs or examples I'll check em out.

sorgelig commented 1 month ago

you can use Quartus IP generator where you can set "MLAB" type. or you can make module like this: https://github.com/MiSTer-devel/Saturn_MiSTer/blob/main/rtl/mlab.vhd

gtaylormb commented 1 month ago

I don't know why you are so hurry to make a release. Take it easy. Unstable builds are available for testing for those who want.

I wanted to give you a heads up that I had another PR incoming with a fix, in case you were in the process of a build. And then signing off that it's ready to go from my end. Just trying to be a good communicator... I personally find it helpful.

gtaylormb commented 1 month ago

you can use Quartus IP generator where you can set "MLAB" type. or you can make module like this: https://github.com/MiSTer-devel/Saturn_MiSTer/blob/main/rtl/mlab.vhd

Awesome, I'll play around with it, thanks!

gtaylormb commented 1 month ago

BTW, this is the message I get from the synthesizer: Info (276007): RAM logic "emu:emu|system:system|sound:sound|opl3:opl|channels:channels|control_operators:control_operators|mem_multi_bank:ar_dr_mem|mem_simple_dual_port:bankgen[1].mem_bank|ram" is uninferred due to asynchronous read logic File: /build/rtl/soc/sound/opl3/mem_simple_dual_port.sv Line: 61

Really makes it sound like this isn't supported. I'm also seeing this: https://www.intel.com/content/www/us/en/docs/programmable/683323/18-1/use-synchronous-memory-blocks.html

Says asynchronous memory isn't supported.

sorgelig commented 1 month ago

Sometimes it has hard time to infer the memory. Usually i use explicit memory IP to make sure it works as i plan. Some time ago i've made configurable module with different BRAM versions and use in many cores. https://github.com/MiSTer-devel/SNES_MiSTer/blob/master/rtl/bram.vhd

module is written in VHDL on purpose because only VHDL supports default non-zero parameters and port values.

gtaylormb commented 1 month ago

I just simulated using https://github.com/MiSTer-devel/Saturn_MiSTer/blob/main/rtl/mlab.vhd and sure enough, it appears it does 0-cycle asynchronous reads. Very weird that Altera is indicating it cannot infer this however, and even their documentation says it won't work. I prefer inference when possible for cross-platform support (e.g. I wrote opl3_fpga for Xilinx but the same exact code works on Altera).

Such is life I guess with FPGA toolchains!

image

gtaylormb commented 1 month ago

module is written in VHDL on purpose because only VHDL supports default non-zero parameters and port values.

FYI Verilog supports default non-zero parameters but not port values. If a port isn't connected most tools will throw a lint warning/error. Questa errors out during elaboration, Vivado gives a warning and continues haha. I've seen lots of production code where people leave off port connections. Kinda scary. Eh thinking about it now perhaps that was only output ports, which would not big that big of a deal... Input ports may fail, not sure, too lazy to check.

sorgelig commented 1 month ago

Yes, i mean default port values aren't supported. SystemVerilog is supposed to support default port values but it's not implemented in Quartus. Quartus compiles even without warnings if some ports aren't used. Probably you have some specific quartus settings not allowing you to compile.

gtaylormb commented 1 month ago

Good news, I found a synthesis directive that allows these RAMs to be inferred in MLABs. https://github.com/MiSTer-devel/ao486_MiSTer/pull/152

Glad we had this discussion! Substantial reduction in area.

sorgelig commented 1 month ago

Do you plan more fixes/tweaks?

gtaylormb commented 1 month ago

Nope, I think we're good.

sorgelig commented 1 month ago

Are you absolutely sure Cybersphere options music should sound like that? Now it doesn't make loud boom sound but it still sounds odd. If you will play this song with last release you will notice quite difference and that way sounds more harmonical than in fpga_opl3.

gtaylormb commented 1 month ago

I will try to fit in more testing tonight. I have a RetroWave OPL3 Express with real OPL3 on it playing through DosBox-X so I'm using that as my gold standard. On first glance the main thing is it's playing much slower through DosBox-X, but that's not really a function of the opl3.

I'll go through the other waveforms and make sure they're correct (if waveform 6 was wrong perhaps one or more of the other 3 opl3 ones are as well, waveforms 4,5,7). I implemented the waveforms before I implemented the envelope, and most games don't use these upper waveforms, only the lower opl2 ones, so I can see how that squarewave bug got through.

sorgelig commented 1 month ago

Can you record that music from real OPL3?

gtaylormb commented 1 month ago

I need a cable which I ordered but won't get here until tomorrow. Hopefully tomorrow night I'll have time to record, if not it will be next week sometime as I'm going out of town.

To me opl3_fpga and the opl3 sound very close. The old ao486 opl3 build sounds way different from both, this is true.

I've checked the rest of the waveforms and they all look perfect.

gtaylormb commented 1 month ago

I found an issue in 4-operator mode, PR here: https://github.com/MiSTer-devel/ao486_MiSTer/pull/153

opl3_fpga had a slightly over-modulated, dirtier sound compared to the real opl3 and I narrowed it down to this. The bass drum in particular was messy. They sound identical to me now.

Again, I'll try to get you the recordings tomorrow otherwise next week.

sorgelig commented 1 month ago

I don't need high quality audio recording.

btw, reset it seems not resetting everything immediately, and last note may play with volume fading out. What if note won't have fading effect? it may play indefinite time after reset probably.

gtaylormb commented 1 month ago

btw, reset it seems not resetting everything immediately, and last note may play with volume fading out. What if note won't have fading effect? it may play indefinite time after reset probably.

As of https://github.com/MiSTer-devel/ao486_MiSTer/pull/150 the reset forces all the operator envelopes into the RELEASE state, the equivalent of releasing the key on the keyboard (before they could stay in the SUSTAIN state forever as no more key-off events occur), that's why you hear the notes with the volume fading out. I chose to not reset anything else because it requires additional logic for a state machine to reset all memory locations. If you'd prefer a hard cut off I can add this to the envelope values themselves as well.

I don't need high quality audio recording.

No worries, this was kinda the only way for me to record since I just have headphones at my current place. Attached: opl3_recordings.zip

Note Zybo_opl3_fpga and MiSTer_opl3_fpga are the same code, but there is an audible difference as the Zybo version is connected directly to the DAC + analog differences outside the FPGA. Zybo has a very high quality DAC on it and no digital low-pass filter is applied; I have the 20KHz 3rd CH 1db filter applied on the MiSTer--I could possibly mess around with that but anyway, you get the idea. There's also a slight blip at the beginning of the Zybo version as I recorded just coming into the options menu.

sorgelig commented 1 month ago

As of #150 the reset forces all the operator envelopes into the RELEASE state, the equivalent of releasing the key on the keyboard (before they could stay in the SUSTAIN state forever as no more key-off events occur), that's why you hear the notes with the volume fading out. I chose to not reset anything else because it requires additional logic for a state machine to reset all memory locations. If you'd prefer a hard cut off I can add this to the envelope values themselves as well.

Force release if fine.

this was kinda the only way for me to record since I just have headphones at my current place. Attached

Sounds very similar with real OPL3. Probably i've get used to Z80 version voices balance. So, it seems good to release.

gtaylormb commented 1 month ago

Good to go on my end as well.