Various bugs in CHIP-8 implementation

[gulrak]

I found this gem while looking for something completely different on cosmacelf.com. It is a nice project, and a very different take on the topic, so first of all welcome to the CHIP-8 world!

I had a quick look over the CHIP-8 interpreter and found various issues. A bunch of them might be connected to the use of Cowgod's technical reference and I made an errata for that: https://github.com/gulrak/cadmium/wiki/CTR-Errata

The main issue of the 8xyn opcodes is, that VF is set before Vx, but x could be 15, so the assignments could be in conflict. For that case the result of the opcodes must be the VF result (so 0 or 1) not the math result from Vx.

The flags errors can also be found with the 4-flags.ch8 test program from the very good CHIP-8 test suite by Timendus (https://github.com/Timendus/chip8-test-suite) that comes with an excellent readme.

Besides that, the key opcodes Ex9E/ExA1 do something like this:

    x = (instruction&0x0F00)>>8;
    if (keydown[x]!=0) {

This seems to use x as the key number, but the opcodes are expected to use the lower 4bit of Vx, so more like:

    x = (instruction&0x0F00)>>8;
    if (keydown[V[x] & 0xF]!=0) {

The masking is not only protecting from array-out-of-bounds access in case Vx is greater than 15, but is actual the expected behavior, and that is valid for original CHIP-8 as well as it's 90s revivals on the HP-48 (like SuperCHIP).

Btw, e.g. the integrated Invaders (by David Winter), apart from fixing the key opcodes, expects the SuperCHIP shift quirk. Quirks are little differences (mainly because of misunderstandings and sub-par documentation) in various CHIP-8 variants and the shift quirk is about 8xy6/8xyE just shifting Vx instead of using Vy. The y parameter is simply ignored.

Also, the Dxyn, the sprite drawing, is implemented with what is called the wrapping quirk, that is, it wraps all pixels that would fall outside the left/lower border. That is not how original CHIP-8 works, instead it only wraps the initial position, so the drawing starts at xpos = V[x] % num_cols and ypos = V[y] % num_rows and any subsequent pixels that would be outside the right or lower border are simply not drawn. This makes it necessary to use temps for vx and vy, as the register themselves should not be changed. Wrapping came only later with variants like XO-CHIP (in Octo, another Javascript IDE for CHIP-8 at https://johnearnest.github.io/Octo).

I See Fx0A is not implemented yet. Be aware, that contrary to some documentations it should be waiting for a key to be released, not pressed, and there are games/programs using Fx0A back to back with more key opcodes that will fail if the key returned by Fx0A is still pressed. Often Fx0A is implemented by decrementing the PC (or in your case not incrementing) if the conditions are not met and thus letting the program re-execute the opcode until that happened.

I can't say if that is all, and Javascript is not my strongest language, but fixing these should enhance the compatibility of the implementation.

As another resource for the opcode differences of a bunch of CHIP-8 variants I made this little reference opcode table that might come in handy: https://chip8.gulrak.net - Opcodes makes with a yellowish background are influenced by quirks or show different behavior on different variants. It's not a perfect replacement of opcode description as I kept the descriptions brief, but might still help.

[gulrak]

As an additional resource I want to add https://github.com/chip-8/chip-8-database - a project that collects metadata of existing programs (identified by the SHA1 checksum of their binary file), so that one can find out if a program should work with "original CHIP-8" or needs a different variant or specific quirks.

[gulrak]

You left out the masking of the value in Vx to the lower for bit, as I'n my suggested fix. That masking is not only preventing a value >15 from trying to access a non-existing key index, but it also is the documented behavior in the original COSMAC VIP manual describing Ex9E, ExA1 and Fx29 (there it is called LSD, which is meant as "Least Significant Digit" but meaning hex digits), and there are games that have upper bits set and do not work without that masking.

[BobKuczewski]

Fixed. Thanks!!

[gulrak]

Looking at the screenshots you posted:

F3 (which is Fx33) of corax+

This is probably failing, because you increment the I register, and while that is correct for Fx55/Fx65, it should not happen at Fx33, so just do:

    mem[I+0] = x2;
    mem[I+1] = x1;
    mem[I+2] = x0;

8xyn in the flags tests

It all boils down to setting VF before Vx. In CHIP-8 the flag result of a math opcode has priority over the math result, whenever there is a conflict. As you already use local variables for results in some of them, the fix is there is straightforward, the shifts need a result too:

 } else if ( (instruction & 0xF00F) == 0x8004) { // ADD Vx,Vy
    // console.log ( "ADD Reg" );
    result = V[(instruction&0x0F00)>>8] + V[(instruction&0x00F0)>>4];
    V[(instruction&0x0F00)>>8] = result & 0xFF;
    if (result > 0xFF) {
      V[0xF] = 1;
    } else {
      V[0xF] = 0;
    }
    PC = PC + 2;
  } else if ( (instruction & 0xF00F) == 0x8005) { // SUB Vx,Vy: Vx = Vx - Vy
    // console.log ( "SUB Reg" );
    result = V[(instruction&0x0F00)>>8] - V[(instruction&0x00F0)>>4];
    V[(instruction&0x0F00)>>8] = result & 0xFF;
    if (result < 0) {
      V[0xF] = 0;
    } else {
      V[0xF] = 1;
    }
    PC = PC + 2;
  } else if ( (instruction & 0xF00F) == 0x8006) { // SHR Vx,Vy: Vx = Vy >> 1
    // console.log ( "SHR" );
    result = V[(instruction&0x00F0)>>4] & 0x01; // store flag result
    V[(instruction&0x0F00)>>8] = V[(instruction&0x00F0)>>4] >> 1; // calculate Vx
    V[0xF] = result;  // then set VF
    PC = PC + 2;
  } else if ( (instruction & 0xF00F) == 0x8007) { // SUBN Vx,Vy: Vx = Vy - Vx
    // console.log ( "SUBN" );
    result = V[(instruction&0x00F0)>>4] - V[(instruction&0x0F00)>>8];
    V[(instruction&0x0F00)>>8] = result & 0xFF;
    if (result < 0) {
      V[0xF] = 0;
    } else {
      V[0xF] = 1;
    }
    PC = PC + 2;
  } else if ( (instruction & 0xF00F) == 0x800E) { // SHL Vx,Vy
    // console.log ( "SHL" );
    result = (V[(instruction&0x00F0)>>4] >> 7) & 0x01; // same pattern as 8xy6
    V[(instruction&0x0F00)>>8] = (V[(instruction&0x00F0)>>4] << 1) & 0xff;
    V[0xF] = result;
    PC = PC + 2;
  }

Quirks test

These are kinda optional and depend on if you want to stick to classic CHIP-8 as on the COSMAC VIP or support more options. (Any of the quirks could be an option to allow more rooms to be run, but are also more confusing to the user, if he doesn't know what to set. Cadmium auto-detects about 600 programs on their SHA1 checksum and selects the right variant and quirks for the user, so it is less intrusive.)

For simplicity I assume classic CHIP-8, and then it boils down to:

• VF RESET - 8xy1/8xy2/8xy3 should set VF to 0 (after setting Vx to the result of the logical operation!)
• MEMORY - this is your VIP-correct increment of I on Fx55/Fx65
• DISPLAY WAIT - This is limiting the execution of Dxyn to once per frame. The code doesn't seem to do that, but frame display and timer are also separate from each other, as you seem to have the timer automatically decrement at 60Hz independent of the frame display, you also display a frame after each opcode, while CHIP-8 only has one display update per 16.667ms or at 60Hz. I'm not sure why it thinks display wait is on, have to dig deeper into the code.
• CLIPPING - This is shown off, as your code wraps around any pixels of a draw that land outside of the bounds by always using modulo. While this is a safe way to draw, most CHIP-8 variants expect to not draw those at all. But they expect that modulo to happen at the beginning, so for the initial position. That is why we recommend to use two extra vars, e.g. xpos and ypos and initialise them as:

    xpos = V[( instruction & 0x0F00 ) >> 8] % num_cols;
    ypos = V[( instruction & 0x00F0 ) >> 4] % num_rows;

And use them in the draw loop. Then the loops would become:

    for (pixrow=0; pixrow<n; pixrow++) {
      if (ypos + pixrow >= num_rows) break;
      for (c=0; c<8; c++) {
        if (xpos + c >= num_cols) break;
 ...

• SHIFTING - this is what you already do, using Vy as the operand to the shift in 8xy6/8xyE
• JUMPING - this is Bnnn, which is unimplemented in the code (a NOP) but should be PC = (instruction & 0xFFF) + V[0];

Key test Fx0A

CHIP-8 expects to wait for a pressed key to be released and set Vx to its number, typically done by not incrementing the PC until that happened, so one needs a way to detect a key release (no JS expert) and unless that happened, re-execute this instruction. Many emulators one finds out there wrongly wait for a key-press, and that works for a bunch of programs, but fails for some, as those assume that the key really is not pressed after that and following key tests will fail breaking the program logic.

The beep

Currently the emulator beeps when ST, the sound timer, is set, but actually it is expected to beep while ST is not 0 and ST is expected to be decremented at the same pace as the delay timer. The workaround will at least beep when a beep is expected, while not as long as it should. Implementing asynchronous sound in JS is beyond my experience, so I can't help much with that part, but the current behavior should not break any program and gives a signal. :-)

I think that is it for now. Again, thank you for working on it to fix it, as it is a fun solution.

[gulrak]

To restate my main recommendation from above, Cowgod's technical reference is problematic in that it describes a mixture of SUPER-CHIP 1.1 and CHIP-8 behavior, thus not correctly describing that Vy is the input of 8xy6/8xyE, or that Fx55 and Fx65 should increment I by x+1. That is why I linked to my errata for it, if one actually wants to make a classic CHIP-8. So if in doubt, it is probably wrong. Matt Mikolay's reference is a more reliable resource in that regard, or my opcode table, albeit its descriptions are a bit short.