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
116 stars 18 forks source link

About the ADPCM's active5 signal. #73

Open kunichiko opened 1 year ago

kunichiko commented 1 year ago

I have a question about the HDL code on this line. https://github.com/jotego/jt12/blob/42b19189044e079a9d93a9d728f67e64fbd0326f/hdl/adpcm/jt10_adpcm_cnt.v#L74

This combinational logic looks asymmetric. Because..

(en_ch[1] && cur_ch[4])   ← diff = 3
(en_ch[2] && cur_ch[5])  ← diff = 3
(en_ch[2] && cur_ch[0])  ← diff = 4
(en_ch[3] && cur_ch[1])  ← diff = 4
(en_ch[4] && cur_ch[2])  ← diff = 4
(en_ch[5] && cur_ch[3])  ← diff = 4

Of course, I haven't fully understood the pipeline logic, but I believe this logic is incorrect.

Could you please inform me if there are any reasons why it is asymmetric?

jotego commented 1 year ago

I didn't make that change. That line looks wrong indeed, it isn't using bit 0 in en_ch either...

After I released JT10, MiSTer developers tinkered with it independently and I lost track of some of the edits. This is marked in MiSTer's repo as an experimental change:

https://github.com/MiSTer-devel/NeoGeo_MiSTer/blame/master/rtl/jt12/hdl/adpcm/jt10_adpcm_cnt.v#L74

I have raised the question in MiSTer's repo, let's wait and see. I think en_ch[0] should be used instead of using twice en_ch[2]. I would try this instead:

 wire active5 =  |({ cur_ch[3:0], cur_ch[5:4] }&en_ch);