5
\$\begingroup\$

I wrote this chip8 emulator as practice recently since my coursework is mostly console programs. I've tested it with roms off Zophar's Domain and it appears to work with my limited testing. I'm hoping for general advice / suggestions about the code structure and the way it's written.

main.cpp

I think there's too much stuff in my main function and I feel like this file isn't very readable in general. I also had some trouble getting some lines under 80 characters (the SDL ones).

#include <SDL2/SDL.h>
#include <SDL2/SDL_mixer.h>
#include <array>
#include <cstdint>
#include <iostream>
#include "chip8.h"

constexpr int WIDTH = 64;
constexpr int HEIGHT = 32;
constexpr int SCALE = 10;
constexpr int FPS = 60;
constexpr int TICKS_PER_FRAME = 1000 / FPS;
constexpr int INSTRUCTIONS_PER_STEP = 10;

constexpr std::array<SDL_Keycode, 16> keymap{
    SDLK_x, SDLK_1, SDLK_2, SDLK_3,   // 0 1 2 3
    SDLK_q, SDLK_w, SDLK_e, SDLK_a,   // 4 5 6 7
    SDLK_s, SDLK_d, SDLK_z, SDLK_c,   // 8 9 A B
    SDLK_4, SDLK_r, SDLK_f, SDLK_v};  // C D E F

void sdl_error() {
    std::cerr << "SDL has encountered an error: ";
    std::cerr << SDL_GetError() << "\n";
    SDL_Quit();
    exit(1);
}

void init_sdl(SDL_Window*& window, SDL_Texture*& texture, SDL_Renderer*& renderer) {
    if (SDL_Init(SDL_INIT_VIDEO) < 0) {
        sdl_error();
    }
    window = SDL_CreateWindow("chip8", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, WIDTH * SCALE, HEIGHT * SCALE, SDL_WINDOW_SHOWN);
    if (window == nullptr) {
        sdl_error();
    }

    renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED);
    if (renderer == nullptr) {
        sdl_error();
    }

    texture = SDL_CreateTexture(renderer, SDL_PIXELFORMAT_RGBA8888, SDL_TEXTUREACCESS_STREAMING, WIDTH, HEIGHT);
    if (texture == nullptr) {
        sdl_error();
    }
}

void init_audio(Mix_Chunk*& chunk) {
    if (Mix_OpenAudio(44100, MIX_DEFAULT_FORMAT, 2, 2048) < 0) {
        std::cerr << "SDL_mixer has encountered an error: ";
        std::cerr << Mix_GetError() << "\n";
        SDL_Quit();
        Mix_Quit();
        exit(1);
    }
    chunk = Mix_LoadWAV("../resources/beep.wav");
    if (chunk == nullptr) {
        std::cerr << "SDL_mixer has encountered an error: ";
        std::cerr << Mix_GetError() << "\n";
        SDL_Quit();
        Mix_Quit();
        exit(1);
    }
}

int main(int argc, char* argv[]) {
    SDL_Window* window = nullptr;
    SDL_Texture* texture = nullptr;
    SDL_Renderer* renderer = nullptr;
    Mix_Chunk* chunk = nullptr;
    SDL_Event event;

    init_sdl(window, texture, renderer);
    init_audio(chunk);

    Chip8 chip8;
    if (argc > 1) {
        chip8.load_rom(argv[1]);
    } else {
        chip8.load_rom("../roms/PONG2");
    }

    std::uint32_t start_time;
    std::uint32_t delta_time;
    bool quit = false;
    while (!quit) {
        start_time = SDL_GetTicks();
        for (int i = 0; i < INSTRUCTIONS_PER_STEP; i++) {
            chip8.emulate_cycle();
        }
        while (SDL_PollEvent(&event)) {
            switch (event.type) {
                case SDL_QUIT:
                    quit = true;
                    break;
                case SDL_KEYDOWN:
                    for (int i = 0; i < keymap.size(); i++) {
                        if (event.key.keysym.sym == keymap[i]) {
                            chip8.press_key(i);
                        }
                    }
                    break;
                case SDL_KEYUP:
                    for (int i = 0; i < keymap.size(); i++) {
                        if (event.key.keysym.sym == keymap[i]) {
                            chip8.release_key(i);
                        }
                    }
                    break;
            }
        }

        if (chip8.get_sound_timer() > 0) {
            Mix_PlayChannel(-1, chunk, 0);
        }

        if (chip8.get_draw_flag()) {
            chip8.reset_draw_flag();
            std::uint32_t* pixels = nullptr;
            int pitch;
            SDL_LockTexture(texture, nullptr, reinterpret_cast<void**>(&pixels), &pitch);
            for (int i = 0; i < WIDTH * HEIGHT; i++) {
                pixels[i] = (chip8.get_pixel_data(i) == 0) ? 0x000000FF : 0xFFFFFFFF;
            }
            SDL_UnlockTexture(texture);
            SDL_RenderClear(renderer);
            SDL_RenderCopy(renderer, texture, nullptr, nullptr);
            SDL_RenderPresent(renderer);
        }
        delta_time = SDL_GetTicks() - start_time;
        if (TICKS_PER_FRAME > delta_time) {
            chip8.step_timers();
            SDL_Delay(TICKS_PER_FRAME - delta_time);
        }
    }
    Mix_FreeChunk(chunk);
    SDL_DestroyTexture(texture);
    SDL_DestroyRenderer(renderer);
    SDL_DestroyWindow(window);
    Mix_Quit();
    SDL_Quit();
    return 0;
}

chip8.h

I was taught to use getters and setters, but I feel like making some of my variables (keys, graphics) public would've been easier.

#ifndef CHIP8
#define CHIP8

#include <array>
#include <cstdint>
#include <random>
#include <string>

class Chip8 {
   private:
    std::array<uint8_t, 4096> memory;       // ram, first 512 bytes reserved
    std::array<uint8_t, 16> V;              // general registers, VF = carry bit
    std::array<uint16_t, 16> stack;         // subroutine return addresses
    std::array<uint8_t, 16> keys;           // stores hexadecimal keypad
    std::array<uint8_t, 64 * 32> graphics;  // holds pixel data
    std::uint8_t delay_timer;               // decrements at 60Hz when nonzero
    std::uint8_t sound_timer;               // decrements at 60Hz when nonzero
    std::uint16_t I;                        // stores memory addresses
    std::uint16_t pc;                       // currently executing address
    std::uint16_t sp;                       // points to top of stack
    std::uint16_t opcode;                   // current instruction
    std::random_device rd;                  // used to obtain seed for generator
    std::mt19937 gen;                       // generates pseudo-random numbers
    bool draw_flag;                         // true when gfx needs to be updated

   public:
    Chip8();
    void load_rom(std::string path);
    void emulate_cycle();
    void press_key(int keycode);
    void release_key(int keycode);
    void step_timers();
    bool get_draw_flag();
    void reset_draw_flag();
    std::uint8_t get_pixel_data(int i);
    std::uint8_t get_sound_timer();
};
#endif

chip8.cpp

#include "chip8.h"
#include <cstdint>
#include <fstream>
#include <iostream>
#include <random>
#include <vector>

Chip8::Chip8() {
    // program counter must start at memory location 0x200
    pc = 0x200;

    // the stack, display, memory, and key arrays must be cleared
    memory.fill(0);
    V.fill(0);
    keys.fill(0);
    graphics.fill(0);
    stack.fill(0);

    // all registers other than the program counter must also be set to 0
    delay_timer = 0;
    sound_timer = 0;
    I = 0;
    sp = 0;

    // no initial instruction
    opcode = 0;

    // nothing to draw initially
    draw_flag = false;

    // initialize random number generator
    gen.seed(rd());

    // load the fontset into memory
    std::array<std::uint8_t, 80> fontset = {{
        0xF0, 0x90, 0x90, 0x90, 0xF0,  // 0
        0x20, 0x60, 0x20, 0x20, 0x70,  // 1
        0xF0, 0x10, 0xF0, 0x80, 0xF0,  // 2
        0xF0, 0x10, 0xF0, 0x10, 0xF0,  // 3
        0x90, 0x90, 0xF0, 0x10, 0x10,  // 4
        0xF0, 0x80, 0xF0, 0x10, 0xF0,  // 5
        0xF0, 0x80, 0xF0, 0x90, 0xF0,  // 6
        0xF0, 0x10, 0x20, 0x40, 0x40,  // 7
        0xF0, 0x90, 0xF0, 0x90, 0xF0,  // 8
        0xF0, 0x90, 0xF0, 0x10, 0xF0,  // 9
        0xF0, 0x90, 0xF0, 0x90, 0x90,  // A
        0xE0, 0x90, 0xE0, 0x90, 0xE0,  // B
        0xF0, 0x80, 0x80, 0x80, 0xF0,  // C
        0xE0, 0x90, 0x90, 0x90, 0xE0,  // D
        0xF0, 0x80, 0xF0, 0x80, 0xF0,  // E
        0xF0, 0x80, 0xF0, 0x80, 0x80   // F
    }};
    for (int i = 0; i < 80; ++i) {
        memory[i] = fontset[i];
    }
}

void Chip8::load_rom(std::string path) {
    std::ifstream file(path, std::ios::binary | std::ios::ate);
    std::ifstream::pos_type file_size = file.tellg();
    std::vector<std::uint8_t> buffer(file_size);
    file.seekg(0, std::ios::beg);
    file.read(reinterpret_cast<char*>(buffer.data()), file_size);
    for (int i = 0; i < file_size; i++) {
        memory[i + 512] = buffer[i];  // first 512 bytes are reserved
    }
}

void Chip8::emulate_cycle() {
    opcode = memory[pc] << 8 | memory[pc + 1];  // get instruction

    std::uint16_t x = (opcode & 0x0F00) >> 8;  // second 4 bits e.g. 0xA(B)CD
    std::uint16_t y = (opcode & 0x00F0) >> 4;  // third 4 bits e.g. 0xAB(C)D
    std::uint16_t kk = opcode & 0x00FF;        // lower byte e.g. 0xAB(CD)
    std::uint16_t n = opcode & 0x000F;         // last 4 bits e.g. 0xABC(D)

    switch (opcode & 0xF000) {  // first 4 bits decide the instruction
        case 0x0000:            // possible instructions are 0x00E0 or 0x00EE
            switch (opcode) {
                case 0x00E0:  // clear display
                    graphics.fill(0);
                    draw_flag = true;
                    pc += 2;
                    break;
                case 0x00EE:  // return from a subroutine
                    pc = stack[--sp];
                    pc += 2;
                    break;
                default:  // invalid opcode found
                    std::cerr << "Undefined 0x0000 opcode: " << opcode << "\n";
            }
            break;
        case 0x1000:  // 0x1nnn, jump to address nnn
            pc = opcode & 0x0FFF;
            break;
        case 0x2000:           // 0x2nnn, call address nnn
            stack[sp++] = pc;  // store current address on stack first
            pc = opcode & 0x0FFF;
            break;
        case 0x3000:  // 0x3xkk, skip next instruction if Vx = kk
            pc += 2;
            if (V[x] == kk) {
                pc += 2;
            }
            break;
        case 0x4000:  // 0x4xk, skip next instruction if Vx != kk
            pc += 2;
            if (V[x] != kk) {
                pc += 2;
            }
            break;
        case 0x5000:  // 0x5xy0, skip next instruction if Vx == Vy
            pc += 2;
            if (V[x] == V[y]) {
                pc += 2;
            }
            break;
        case 0x6000:  // 0x6xkk, puts value kk into Vx
            pc += 2;
            V[x] = kk;
            break;
        case 0x7000:  // 0x7xkk, set Vx = Vx + kk
            pc += 2;
            V[x] += kk;
            break;
        case 0x8000:          // possible instructions are 0x8xy(0-7, E)
            switch (n) {      // check last 4 bits, 0xABC(D)
                case 0x0000:  // 0x8xy0, set Vx = Vy
                    pc += 2;
                    V[x] = V[y];
                    break;
                case 0x0001:  // 0x8xy1, set Vx = Vx OR Vy
                    pc += 2;
                    V[x] |= V[y];
                    break;
                case 0x0002:  // 0x8xy2, set Vx = Vx AND Vy
                    pc += 2;
                    V[x] &= V[y];
                    break;
                case 0x0003:  // 0x8xy3, set Vx = Vx XOR Vy
                    pc += 2;
                    V[x] ^= V[y];
                    break;
                case 0x0004:  // 0x8xy4, set Vx = Vx + Vy, VF = carry
                    pc += 2;
                    V[0xF] = (V[x] + V[y]) > 0xFF;  // VF = 1 if carry occurs
                    V[x] += V[y];
                    break;
                case 0x0005:  // 0x8xy5, set Vx = Vx - Vy, VF = NOT borrow
                    pc += 2;
                    V[0xF] = V[x] > V[y];
                    V[x] -= V[y];
                    break;
                case 0x0006:  // 0x8xy6, Vx = Vx SHR 1, VF = LSB prior to shift
                    pc += 2;
                    V[0xF] = V[x] & 1;  // set as V[x]'s least significant bit
                    V[x] >>= 1;
                    // V[x] = V[y] >> 1;  // breaks Zophar ROMS
                    break;
                case 0x0007:  // 0x8xy7, set Vx = Vy - Vx, VF = NOT borrow
                    pc += 2;
                    V[0xF] = V[y] > V[x];
                    V[x] = V[y] - V[x];
                    break;
                case 0x000E:  // 0x8xyE, Vx = Vx SHL 1, VF = MSB prior to shift
                    pc += 2;
                    V[0xF] = V[x] >> 7;  // MSB = 8th bit since VF is an uint8_t
                    V[x] <<= 1;
                    // V[x] = V[y] << 1;  // breaks Zophar ROMS
                    break;
                default:  // invalid opcode found
                    std::cerr << "Undefined 0x8000 opcode: " << opcode << "\n";
            }
            break;
        case 0x9000:  // 0x9xy0, skip next instruction if Vx != Vy
            pc += 2;
            if (V[x] != V[y]) {
                pc += 2;
            }
            break;
        case 0xA000:  // 0xAnnn, set I = nnn
            pc += 2;
            I = opcode & 0xFFF;
            break;
        case 0xB000:  // 0xBnnn, jump to location nnn + V0
            pc = (opcode & 0xFFF) + V[0];
            break;
        case 0xC000:  // 0xCxkk, set Vx = random byte and kk
            pc += 2;
            {  // to prevent initialization error
                V[x] = std::uniform_int_distribution<>(0, 255)(gen) & kk;
            }
            break;
        case 0xD000:  // 0xDxyn, draws sprite
            // sprite is 8 x n pixels and located at (Vx, Vy)
            pc += 2;
            draw_flag = true;
            V[0xF] = 0;
            std::uint8_t pixel_row;  // each pixel in a row is 1 bit
            for (int y_line = 0; y_line < n; ++y_line) {
                pixel_row = memory[I + y_line];  // sprite starts at I
                for (int x_line = 0; x_line < 8; ++x_line) {
                    // go through the row 1 bit at a time
                    // true if pixel needs to be drawn
                    if (pixel_row & (0b10000000 >> x_line)) {
                        // the coordinate in row-major form
                        // must be modded with 2048 for proper wrapping
                        std::uint16_t coord = (V[x] + x_line + ((V[y] + y_line) * 64)) % 2048;
                        bool collision = (graphics[coord] == 1);
                        // OR with collision because VF is 1 when there is at
                        // least one collision
                        V[0xF] |= collision;
                        graphics[coord] ^= 1;
                    }
                }
            }
            break;
        case 0xE000:  // possible instructions are 0xEx9E, 0xExA1
            switch (kk) {
                case 0x009E:  // 0xEx9E, skip next instruction if keypress = Vx
                    pc += 2;
                    if (keys[V[x]]) {
                        pc += 2;
                    }
                    break;
                case 0x00A1:  // 0xExA1, skip next instruction if keypress != Vx
                    pc += 2;
                    if (!keys[V[x]]) {
                        pc += 2;
                    }
                    break;
                default:  // invalid opcode found
                    std::cerr << "Undefined 0xEx00 opcode: " << opcode << "\n";
            }
            break;
        case 0xF000:  // possible instructions: 0xFx(07,0A,15,18,1E,29,33,55,65)
            switch (kk) {
                case 0x0007:  // 0xFx07, set Vx = delay timer value
                    pc += 2;
                    V[x] = delay_timer;
                    break;
                case 0x000A:  // 0xFx0A, wait for keypress, store value in Vx
                {
                    bool waiting = true;
                    for (int i = 0; i < keys.size(); ++i) {
                        if (keys[i] != 0) {
                            V[x] = i;
                            waiting = false;
                            break;
                        }
                    }
                    if (waiting) {
                        return;
                    }
                    pc += 2;
                    break;
                }
                case 0x0015:  // 0xFx15, set delay timer = Vx
                    pc += 2;
                    delay_timer = V[x];
                    break;
                case 0x0018:  // 0xFx18, set sound timer = Vx
                    pc += 2;
                    sound_timer = V[x];
                    break;
                case 0x001E:  // 0xFx1E, set I = I + Vx
                    pc += 2;
                    V[0xF] = (I + V[x]) > 0xFFF;  // check for carry
                    I += V[x];
                    break;
                case 0x0029:  // 0xFx29, set I = location of sprite for digit Vx
                    pc += 2;
                    I = V[x] * 5;  // sprites are 4x5
                    break;
                case 0x0033:
                    // 0xFx33, store BCD representation of Vx at I, I+1, I+2
                    pc += 2;
                    memory[I] = V[x] / 100;
                    memory[I + 1] = (V[x] / 10) % 10;
                    memory[I + 2] = V[x] % 10;
                    break;
                case 0x0055:  // 0xFx55, stores V0 - Vx in memory starting at I
                    pc += 2;
                    for (int i = 0; i <= x; i++) {
                        memory[I + i] = V[i];
                    }
                    break;
                case 0x0065:  // 0xFx65, read V0 - Vx from memory starting at I
                    pc += 2;
                    for (int i = 0; i <= x; i++) {
                        V[i] = memory[I + i];
                    }
                    break;
                default:  // invalid opcode found
                    std::cerr << "Undefined 0xF000 opcode: " << opcode << "\n";
            }
            break;
        default:  // invalid opcode found
            std::cerr << "Undefined opcode: " << opcode << "\n";
    }
}

void Chip8::step_timers() {
    if (delay_timer > 0) {
        delay_timer--;
    }
    if (sound_timer > 0) {
        sound_timer--;
    }
}

void Chip8::press_key(int keycode) {
    keys[keycode] = 1;
}

void Chip8::release_key(int keycode) {
    keys[keycode] = 0;
}

void Chip8::reset_draw_flag() {
    draw_flag = false;
}

std::uint8_t Chip8::get_pixel_data(int i) {
    return graphics[i];
}

bool Chip8::get_draw_flag() {
    return draw_flag;
}

std::uint8_t Chip8::get_sound_timer() {
    return sound_timer;
}
\$\endgroup\$
2
\$\begingroup\$

This is some really nifty code. Good job. :)


Use constructor initializer lists where practical (and initialize variables in the order written in the class). We can request value-initialization of the arrays to fill them with zeros, rather than using fill. We can use std::copy to put the fontset in memory:

Chip8::Chip8():
    memory(),
    V(),
    stack(),
    keys(),
    graphics(),
    delay_timer(0),
    sound_timer(0),
    I(0),
    pc(0),
    sp(0),
    gen(std::random_device()()),
    draw_flag(false)
{
    std::array<std::uint8_t, 80> fontset = ...;
    std::copy(fontset.begin(), fontset.end(), memory.begin());
}

It looks like rd isn't used anywhere except the constructor, so it doesn't need to be a member variable. opcode isn't used outside of emulate_cycle(), so it can be a local variable.


In load_rom() we can pass the path argument by const& to avoid an unnecessary copy:

void Chip8::load_rom(std::string const& path) { ...

There's a lot that can go wrong in this function, and we need to handle those cases. We don't want to keep running with invalid data.

bool Chip8::load_rom(std::string const& path) {
    std::ifstream file(path, std::ios::binary | std::ios::ate);

    if (!file) // failed to open file!
        return false;

    std::ifstream::pos_type file_size = file.tellg();

    if (file_size == std::ifstream::pos_type(-1)) // tell failed!
        return false;

    file.seekg(0, std::ios::beg);

    if (!file) // seek failed!
        return false;

    if (file_size > memory.size() - 512) // won't fit in memory!
        return false;

    std::vector<std::uint8_t> buffer(file_size);
    file.read(reinterpret_cast<char*>(buffer.data()), file_size);

    if (!file) // read failed!
        return false;

    std::copy(buffer.begin(), buffer.end(), memory.begin() + 512);
}

It would probably be a good idea to make that 512 offset a named constant of std::size_t.


As previously mentioned, it looks like opcode can be a local variable:

void Chip8::emulate_cycle() {
    std::uint16_t opcode = memory[pc] << 8 | memory[pc + 1];  // get instruction

The emulate_cycle functionality could helpfully be split into separate functions. There's a lot of duplication in the code (e.g. incrementing the current address) that we can eliminate. The code is quite concise, so adding functions will make it longer overall, but it should also become more readable and need fewer comments.

Note that x, y, kk and n aren't needed for every operation, so we can wait until we've identified the actual operation we need before extracting them. e.g.

...
    case 0x1000: // 0x1nnn
        op_jump(opcode);
        break;
    case 0x2000: // 0x2nnn
        op_call(opcode);
        break;
    case 0x3000: // 0x3xnn
        op_skip_eq_x_nn(opcode);
        break;
    case 0x4000: // 0x4xnn
        op_skip_ne_x_nn(opcode);
        break;
...

The nested switch statements also create a lot of overhead (in terms of code) in selecting the operation we need to execute. Ideally we'd like to map directly from opcode to the relevant operation... something like this:

// in the constructor:
std::map<std::uint16_t, op_t> ops = {
    { 0x00E0, op_clear_display },
    { 0x00EE, op_return },
    ...
};

// in emulate_cycle():
std::uint16_t opcode = ...;
auto ops_entry = ops.find(opcode);

if (ops_entry == ops.end()) ...; //  handle error

op_entry.second(opcode); // execute the operation

Obviously this won't work since the opcode isn't an exact value. But we can work around that by changing the index type to store a mask and defining some custom comparison operators. e.g.:

#include <cstdint>
#include <functional>
#include <iostream>
#include <map>

using op_t = std::function<void(std::uint16_t)>;

struct Mask
{
    std::uint16_t value;
    std::uint16_t mask;
};

bool operator<(Mask const& a, Mask const& b) {
    return a.value < (a.mask & b.value);
}

bool operator<(Mask const& a, std::uint16_t b) {
    return a.value < (a.mask & b);
}

bool operator<(std::uint16_t a, Mask const& b) {
    return b.value < (b.mask & a);
}

int main()
{
    std::cout << std::hex;

    op_t op_clear_display = [] (std::uint16_t) {
        std::cout << "clear" << "\n";
    };

    op_t op_return = [] (std::uint16_t) {
        std::cout << "return" << "\n";
    };

    op_t op_jump_nnn = [] (std::uint16_t opcode) {
        std::uint16_t address = (opcode & 0x0FFF);
        std::cout << "jump to: " << address << "\n";
    };

    std::map<Mask, op_t, std::less<>> ops = {
        { { 0x00E0, 0xFFFF }, op_clear_display },
        { { 0x00EE, 0xFFFF }, op_return },
        { { 0x1000, 0xF000 }, op_jump_nnn },
        // ...
        // { { 0x3000, 0xF000 }, op_skip_eq_x_nn },
        // ...
        // { { 0x8000, 0xF00F }, op_add_x_y },
        // ...
        // { { 0xA000, 0xF000 }, op_set_i_nnn },
        // ...
        // { { 0xE09E, 0xF0FF }, op_skip_eq_k_x },
        // ... 
    };

    std::uint16_t opcode = 0x10E0;

    auto ops_entry = ops.find(opcode);

    if (ops_entry == ops.end())
    {
        std::cout << "op not found!" << "\n";
        return EXIT_SUCCESS;
    }

    ops_entry->second(opcode);

    return EXIT_SUCCESS;
}

Member functions that don't change any internal state should be declared const, e.g.:

bool get_draw_flag() const;
std::uint8_t get_pixel_data(int i) const;
std::uint8_t get_sound_timer() const;

(I'll try to come back to this later if I have some more time).

\$\endgroup\$
2
\$\begingroup\$
  • I strongly recommend to increment pc once, right after the instruction is loaded:

        opcode = memory[pc] << 8 | memory[pc + 1];  // get instruction
        pc += 2;
    

    First, that's how the do in real life, and second, doing that in every instruction bloats the code and is prone to errors.

    Just make sure to not increment pc in case 0x00EE.

  • The indirect store instructions (via I) may access illegal memory. I am not versed with chip8 ISA, and I don't know how it is supposed to behave in such situation. The emulation surely is in the position to catch illegal accesses; otherwise you are open to UB.

    It seems that some implementations set VF if I gets beyond 0xfff. Looks prudent (yet still vulnerable).

  • I definitely don't like a huge C-style case, encompassing the entire emulation. Consider an Instruction class, which knows how to execute itself, and make the instruction decoding into a factory.

  • Once this change is made, emulate_cycle may (and should) be split into the natural stages: fetch, decode, and execute (and possibly commit).

  • 0Fx0A does not look correct. According to the spec, an instruction is halted until the next key event. Your implementation doesn't halt, but busy loops. In any case, return is not warranted, and complicates the design.

  • main as a clock source looks dubious. Consider setting up an alarm timer.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.