nukeykt/nuked-opll

Cycle accurate Yamaha YM2413 and VRC7 emulator

Hi there 👋 I'm nukeykt

• 🔭 I’m currently working on: SC-55 emulation
• 👀 I'm interested in: reverse engineering old games and integrated circuits
• 🌱 I’m currently learning: reverse engineering circuits and verilog
• 📫 How to reach me: nukeykt on discord, @nukeykt on twitter, mail: alexeytf2 at gmail.com

nuked-opll's Issues

Question on formula in application of block in the phase increment calculation

This is rather a question than issue.

How did you come to the following code:

freq = chip->c_fnum << 1;
...
freq += freq >> 7;
....
/* Apply block */
freq = (freq << block) >> 1;
chip->pg_inc = (freq * pg_multi[chip->c_multi]) >> 1;

this code will take one 17-bit mux for barrel shifter and at least 15x5 multiplier, while the following code

chip->pg_inc = (((chip->c_fnum << 1) + vibrato) * pg_multi[chip->c_multi]) << block) >> 2;

will require 11x5 multiplier and 19-bit-wide mux. 4 extra bits for multiplication must have more cost in silicon than a mux (unless multiplication is pipelined, and I doubt that given the timing constraints).

The footprint would not be a problem, if your code would not lose two LSbs when block is 0.
And again, this would not be a problem if freq is always even, but you add vibrato after shifting the freq left one bit, and the resulting value may become odd.

Example:

• F-number of 'b1_1100_0000;

• multi is 'b1100

• vibrato enabled.

  freq = chip->c_fnum << 1;

leads to freq='b11_1000_0000

case 2:
    freq += freq >> 7;

leads to freq='b11_1000_0111

freq = (freq << block) >> 1;

leads to freq='b1_1100_0011

chip->pg_inc = (freq * pg_multi[chip->c_multi]) >> 1;

leads to 'ha92 ('b1010_1001_0010) with lowest bits having their weight lost (with another code at the beginning of the post returning 'ha95).

Cycle count decoders

I see many references to the cycle count state in the code. In C code it makes sense to carry the cycle count around as a decimal number. I wonder if it was done like that in the chip. Was there some sort of hot-one encoding used? Was the cycle count encoded in a 5-bit number or were decoded signals sent to relevant blocks?

For instance, in the LFO, it looks like it is only important to know if you are below 9, at zero or at the last one. It would be enough to get 3 signals from a central cycle counter instead of the 5-bit number and then decode it. Do you remember what the original approach was?

Nuked-OPLL/opll.c

Line 1037 in 3f25893

if ((chip->lfo_am_step || (chip->testmode & 0x08)) && chip->cycles < 9) {

Potential ch8 and ch9 edge case bug

It's either: actual YM2413 bug, Furnace bug or Nuked-OPLL bug.
OPLL.zip - contains .fur file and Furance .wav output using Nuked-OPLL core. There is a slight whine after note off on OPLL channels 8 and 9. This is probably caused by setting egt on op1 causing it to sustain in ch 8 and 9 only. Problem is, is this intentional, as register view is apparently fine....

Actual hw recording would be needed, but I dont have access to one... It doesnt occur on MAME and emu2413 cores.

Compilation error with MSYS2 mingw64 gcc v. 10.3.0

After this commit 1bc16d6 I've this error:

/msys64/home/FHorse/src/puNES/src/core/mappers/opll.c: In function 'OPLL_PhaseGenerate':
/msys64/home/FHorse/src/puNES/src/core/mappers/opll.c:490:13: error: a label can only be part of a statement and a declaration is not a statement
  490 |             uint8_t rm_bit = (chip->rm_hh_bit2 ^ chip->rm_hh_bit7)
      |             ^~~~~~~

to compile it correctly it is necessary revert part of the previous commit:

diff --git a/src/core/mappers/opll.c b/src/core/mappers/opll.c
index ffe202d..7d8a5e6 100644
--- a/src/core/mappers/opll.c
+++ b/src/core/mappers/opll.c
@@ -456,6 +456,7 @@
 static void OPLL_PhaseGenerate(opll_t *chip) {
     uint32_t ismod;
     uint32_t phase;
+    uint8_t rm_bit;
     uint16_t pg_out;
 
     chip->pg_phase[(chip->cycles + 17) % 18] = chip->pg_phase_next + chip->pg_inc;
@@ -487,7 +488,7 @@
         switch (chip->cycles) {
         case 13:
             /* HH */
-            uint8_t rm_bit = (chip->rm_hh_bit2 ^ chip->rm_hh_bit7)
+            rm_bit = (chip->rm_hh_bit2 ^ chip->rm_hh_bit7)
                    | (chip->rm_hh_bit3 ^ chip->rm_tc_bit5)
                    | (chip->rm_tc_bit3 ^ chip->rm_tc_bit5);
             pg_out = rm_bit << 9;

Some multiplier values seem missing

In your multiplier line I see some values missing, like 22, 26 and 28. Is that a typo? Are you possitive that the chip was missing those values because I have not seen any reference in Yamaha documentation stating that. I see the same for OPL3 in your other file.

If that is correct, is it because Yamaha only used a simplified multiplier instead of a full one?

Thank you for your attention

Nuked-OPLL/opll.c

Line 219 in 3f25893

1, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 20, 24, 24, 30, 30

Possible error in EG attack phase implementation ( incorrect step values for 11.1 to 11.3 attack rates)

I recently improved MAME YM2413 core EG implementation for attack phase, based on reverse engineering notes from andete (https://www.smspower.org/Development/YM2413ReverseEngineeringNotes2017-01-26) and when I double-checked my implementation against yours, it appeared that the steps patterns matched for all rates except attack rates 11.1, 11.2, 11.3.

Looking at the results given by your implementation, I believe there is a small error on your side as 11.2 and 11.3 resulting patterns are both identical and identical to 12.0 pattern, i.e resulting in not(env)/16 "decrement" on each sample, which seems incorrect.

Looking at your code (OPLL_EnvelopeGenerate function), I believe there is an error with the following code

            int32_t shift = chip->eg_rate_hi - 11 + chip->eg_inc_hi;
            if (chip->eg_inc_lo) {
                shift = 1;
            }

Indeed, for rates 11.x, this means the shift value depends on both chip->eg_inc_hi and chip->eg_inc_low value, which results in shift value being always set for rates 11.2 and 11.3 (as either chip->eg_inc_hi or chip->eg_inc_low are set on any given sample for these two ones).

I believe a more correct implementation would be
int32_t shift = (chip->eg_rate_hi > 11) ? (chip->eg_rate_hi - 11 + chip->eg_inc_hi) : chip->eg_inc_lo;
so that chip->eg_inc_hi is only taken in account for rates greater or equal to 12.0 (and chip->eg_inc_lo only for rates lower than 12.0), like it is the case for decay phases.

I quickly tested this implementation and, as expected, it results in exact same patterns as previous implementation except for rates 11.1 to 11.3 where it now results in expected patterns.

See the attached file for the sourcecode of the test program I used to test your implementation, which is basically your OPLL_EnvelopeGenerate function (with fixed algorithm as described above) called for 18 x N cycles, with chip->eg_rate fixed to tested rate and chip->eg_state / chip->eg_level forced to attack state and max attenuation on each cycle to check for the whole increment/decrement pattern. I also included the original results for 11.x and 12.0 rates, as well as the fixed 11.1 to 11.3 patterns.

opll_env.zip

Also note that you can remove the check for condition shift > 4 as this condition never occurs with your implementation, maximal value of chip->eg_rate_hi being 14 for this calculation to occur (when chip->eg_rate_hi = 15, chip->eg_maxrate is also set so there is no envelope update) so maximal value of shift is 4 anyways.