search

jotego / jt12

FM sound source written in Verilog, fully compatible with YM2612, YM3438 (JT12), YM2203 (JT03) and YM2610 (JT10)
GNU General Public License v3.0
120 stars 22 forks source link

Current/Enable Channel Timing change needed for MiSTer #53

Closed greyrogue closed 3 years ago

greyrogue commented 4 years ago

This change may not actually be a change to match hardware behavior, but is needed to work with MiSTer. If cur_ch and en_ch are shifted the same direction {(0,0), (1,1), (2,2), (3,3), (4,4), (5,5)}, then the time between 0,0 and 1,1 is 7 cycles, and the time between (5,5) and (0,0) is 1 cycle (7 5 + 1 1 = 36 total cycles). MiSTer does not like the 1 cycle difference when reading the sample ROMs. This change inverts the en_ch variable so the timing is instead (5 5 + 11 1 = 36 total cycles). This gives a minimum time between samples of 5 cycles. I tried just moving the en_ch instead of inverting the shift order, and that just moved which channel had the 1 cycle minimum (and which channel malfunctioned because of bad sample ROM reading). If this doesn't match real hardware, maybe a parameter (accurate vs slow timing requirement) could be added for implementations like MiSTer that require larger minimum read spacing?

jotego commented 4 years ago

Sorry @greyrogue I had lost track of this PR. Is this still needed? I actually merged in the ADPCM files from the NeoGeo core yesterday. I only reversed some edits that had been done which were only cosmetic/style. I kept the functionality changes. So I think this PR may not be needed now. Sorry again for having missed it at the time.

greyrogue commented 4 years ago

I don't see this change in your code. (see the changes tab, they are only a few lines total). Unfortunately, I've forgotten which game(s) this affected, but I seem to remember it being samples played on channel 3 or 4 (4 I think), being missed. I originally tried moving the enable/current minimum and that just moved which channel failed to play. This change causes the enable/current channel to shift in opposite directions, which means the minimum spacing between channels is 5 cycles, instead of 1. The spacing is easier to see in signal tap. If both shift in the same direction, then they line up every 7 cycles, until it repeats, with only 1 cycle between.

greyrogue commented 4 years ago

Ok. I dug through my old notes/conversations and found this:

I think there might be an issue with DDR access speed with the current version. For the ADPCMA channels, it has 5 gaps of 7 and one gap of 1 (between channel 6 and 1; actually, that was one of my previous changes; I think the current one has the gap of 1 between channel 3 and 4 accidentally; already fixed that), which is where the distortion on Twinkle Star Sprites was coming from. I switched this to 5 gaps of 5 and one gap of 11. Seems better now.

I just tried reverting this change in latest MiSTer code. If I do so, popping/distortion shows up on channel 3 of ADPCMA (very noticeable - if you listen to the attract mode in Twinkle Star Sprites you will hear it). This is easy to verify on MiSTer. If you press (escape) and (enter) on a keyboard at the same time with the OSD closed, when you open the OSD next, there will be options I added there to enable/disable for each channel/audio type (mutes when summing everything at the end). It's still possible I misdiagnosed this. Maybe it's not DDR access being too slow. In any case, Twinkle Star Sprites attract mode definitely shows the issue. And if you move where the gap of 1 current channel cycle is at, the channel with the issue moves with it, probably related to where this gap is (ch[2] == AMPCMA channel 3). // Two cycles early: wire active5 = { cur_ch[1:0], cur_ch[5:2] } == en_ch;

jotego commented 3 years ago

I have copied the hdl/adpcm folder from the MiSTer fork to the main repository.