Fix CS, vibrato, envelope, remove a bunch of warnings

[gtaylormb]

ALM count just under the previous core.

Vibrato was a bit broken; not super noticeable. Envelope had a potential overflow condition on attack.

[jsmolina]

lot of thanks! each pr improves it way more 😄

although, it's not fully fixed I think, did you tested vgmplayer with duke nukem 2 menu song? https://vgmrips.net/packs/pack/duke-nukem-ii-ibm-pc-at#03-theme-to-duke-nukem-ii with player https://www.vogons.org/viewtopic.php?t=47059

[sorgelig]

That's NOT the way to fix the warnings. It will hide potential bugs.

how i fix such warning:

original:

myreg <= myreg + 1;

fixed:

myreg <= myreg + 1'd1;

you don't need to set the size of constant the same as myreg, just the size enough to fit the constant you adding/subtracting.

[gtaylormb]

It's your project, but I respectfully disagree. It's a trade off worth making IMHO. Manually determining and specifying the bit width can be incredibly error prone, more brittle with parameter changes (less maintainable), verbose and distracting (less readable), not to mention tedious. My preference is to code using a style allowing the synthesizer to determine the bit width which does a better job than a human. The synthesizer produces warnings on truncation yes, but those can be checked and verified (early), whereas an error in manual bit width silently compiles but breaks at run time (late). The warning suppression comment only applies to the file it's in so it wouldn't affect the rest of the code base. I also prefer to have a 1-to-1 mirror of the code at https://github.com/gtaylormb/opl3_fpga if possible, though that's secondary.

I actually spent a couple hours manually specifying bit widths, getting the warnings to go away but completely breaking the design. There are many non-trivial places in the code where truncation is intentionally occurring besides just adding a constant (you can see my feeble attempt at https://github.com/MiSTer-devel/ao486_MiSTer/compare/master...gtaylormb:ao486_MiSTer:fix_opl3_cs). Then I thought, what am I doing, this is a bit crazy. With builds taking ~18mins on my machine it's an incredibly slow turn around.

But again, it's your project. I'll go along with your decision.

[gtaylormb]

@jsmolina I haven't had a chance to test vgmplayer, but I'm playing the music from the menu and I don't notice any missed beats. Then again, sometimes it's hard to know what you're missing.

Latest build from today (April 25th) has potential fixes. There was some funky math in envelope but I did a deep dive and believe I've sorted it out. I'll try to get around to vgmplayer. If there's still missed beats the only thing I can think of is we're missing key-on transitions, which could potentially happen if they occur faster than the sampling rate (a key-off and key-on between samples), but this seems unlikely to me.

[jsmolina]

@jsmolina I haven't had a chance to test vgmplayer, but I'm playing the music from the menu and I don't notice any missed beats. Then again, sometimes it's hard to know what you're missing.

Latest build from today (April 25th) has potential fixes. There was some funky math in envelope but I did a deep dive and believe I've sorted it out. I'll try to get around to vgmplayer. If there's still missed beats the only thing I can think of is we're missing key-on transitions, which could potentially happen if they occur faster than the sampling rate (a key-off and key-on between samples), but this seems unlikely to me.

Yes I feel some missed beats, specially on the start of the song. The web I provided is very accurate with the expected.

[sorgelig]

My preference is to code using a style allowing the synthesizer to determine the bit width which does a better job than a human.

In many years of FPGA coding i didn't have problem with width determination. Especially it's usually add/sub of numbers up to 10. With wrong width you will get blue warning. This is why it's important to minimize false blue warnings, so you will quickly catch the error. By suppressing the warning you may get truncation (due to mistype) without noticing and then will spend hours to find the source of problem. So using such suppressions is bad way of coding.

Ok. up to you. But please remove all releases from PR.

[gtaylormb]

I see what you're saying, but I'd counter that you can just as easily mistype the width, truncate and not get the warning, and spend hours that way as well, e.g.

fb_cnt0_channel_mem_rd_address = connection_sel[2] ? 2'd2 : 2'd5;

compiles happily by Quartus with no warning but is broken. Whereas the less verbose, easier to read, and correct:

fb_cnt0_channel_mem_rd_address = connection_sel[2] ? 2 : 5;

gives you a truncation warning. I think linters are great for what they are, but as with most things, it's a tradeoff. Xilinx and Questa don't throw this lint check, and I haven't coded that way in probably 15 years, so this core wasn't coded with this particular check in mind.

Anyway, we could probably debate the merits of various lint checks for days. I see your point, hopefully you see mine. It's been a real time suck trying to fix all these, particularly ones on shifts, and I feel like it's kind of a silly exercise. I'd rather focus on other stuff.

I've gone ahead and deleted the releases I added (including the ones from my previous PR; hopefully that's correct).

[gtaylormb]

@jsmolina thanks, I'll work on replicating the issue and hopefully fixing in a separate PR. If it's happening at the beginning of a song this is actually easier to debug.

Still also frustratingly cannot get Doom to recognize the OPL3 and do stereo, so that's on my list as well. I've dumped out the reads and writes during startup in DosBox-X, so I can see what's it's doing, but it looks like the fairly standard timer detection scheme (performed twice). It also writes to the LSI test register right before it sets the "new" bit in bank 2, which may be a red herring as I don't see what this would do (and doesn't seem to effect the read output). Really weird.

[sorgelig]

I see what you're saying, but I'd counter that you can just as easily mistype the width, truncate and not get the warning, and spend hours that way as well, e.g.

fb_cnt0_channel_mem_rd_address = connection_sel[2] ? 2'd2 : 2'd5;

compiles happily by Quartus with no warning but is broken. Whereas the less verbose, easier to read, and correct:

fb_cnt0_channel_mem_rd_address = connection_sel[2] ? 2 : 5;

it's because you play with warning suppressors. It never compiles without warnings with default settings. I had several such mistypes in conditional assignments and always was warned - saved a lot of my time. Check other your settings or suppression of other warning. There are several similar warnings with different numbers.

[gtaylormb]

it's because you play with warning suppressors. It never compiles without warnings with default settings

Ah yes, my dumb example does produce a warning, it's just in a different spot so I missed it. I stand corrected.

Anyway, thanks for the merge!