13
\$\begingroup\$

I want to get into emulator development, so I tried to write a CHIP-8 interpreter in C. I implemented it using this guide, and tested it using the test programs provided here (the test cases pass).

There are some quirks, so I decided to implement two presets: original, which has the same quirks as shown in the quirks test of the linked test suite; and modern which implements the recommended behaviour from the guide.

I would like some overall feedback on the code please. I have some critiques myself:

  • Use of many magic numbers: I could have defined constants e.g. CHIP8_STACK_SIZE, CHI8_MEM_START, etc. and this is what I initially did. However, as these kept getting in the way, and as the specification for the CHIP8 is fixed, I just hard coded many constants.
  • Poor naming: I have used names from the documentation, e.g. there are global variables I and V which make sense as register names, but probably not to others reading the code.
  • Use of many globals: I could have encapsulated the emulator state in a struct, for example.

but I would appreciate more feedback.

The program depends on raylib. If this is installed, the program can be built with the command c99 -lraylib chip8.c -o chip8 and run as ./chip8 [--modern | --original] <chip8 file>.

#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

#include "raylib.h"


#define SCALE 8
#define SCREEN_HEIGHT (32 * SCALE)
#define SCREEN_WIDTH (64 * SCALE)

#define OPT_VF_RESET     1  /* on:  the AND, OR, and XOR opcodes reset the flags
                                    register to zero
                               off: the flags register is not modified */
#define OPT_INCREMENT_I  2  /* on:  the save and load opcodes (Fx55 & Fx65)
                                    increment the index register
                               off: the index register is not incremented */
#define OPT_DISPLAY_WAIT 4  /* on:  only a single draw instruction is executed
                                    per frame
                               off: multiple draw instructions may occur in a
                                    single frame */
#define OPT_SHIFT        8  /* on:  the shift opcodes (8xy6 and 8xyE) only
                                    operate on the Vx regsiter
                               off: the shift opcodes operate on Vy, storing the
                                    result in Vx */
#define OPT_JUMP         16 /* on:  the Bnnn instruction uses Vx
                               off: the Bnn instruction uses V0 */
#define OPT_RELEASE      32 /* on:  in Fx0a, the key is processed upon release
                               off: the key is processed when it is pressed */

#define ORIGINAL ~(OPT_JUMP | OPT_JUMP) & \
        (OPT_VF_RESET | OPT_INCREMENT_I | OPT_DISPLAY_WAIT | OPT_RELEASE)
#define MODERN   ~(OPT_DISPLAY_WAIT | OPT_RELEASE | OPT_INCREMENT_I) & \
        (OPT_SHIFT | OPT_JUMP)

#define die(...) do{ fprintf(stderr, __VA_ARGS__); exit(1); } while( false )


uint8_t  *pc;
uint16_t  I;
uint16_t  stack[16];
uint8_t   stack_ptr;
uint8_t   V[16];
uint8_t   screen[32][64];
uint8_t   delay_timer;
uint8_t   sound_timer;


uint8_t mem[4096] = { /* 4x5 pixel fonts */
        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 */
};


int keymap[] = {                                   /*  Emulated -> Original */
        KEY_X    , KEY_ONE , KEY_TWO , KEY_THREE , /*  1 2 3 4     1 2 3 C  */
        KEY_Q    ,   KEY_W ,   KEY_E ,     KEY_A , /*  Q W E R     4 5 6 D  */
        KEY_S    ,   KEY_D ,   KEY_Z ,     KEY_C , /*  A S D F     7 8 9 E  */
        KEY_FOUR ,   KEY_R ,   KEY_F ,     KEY_V , /*  Z X C V     A 0 B F  */
};


int     last_keypress;
uint8_t last_keypress_reg;


void load_rom(char* filename)
{
        FILE *fp = fopen(filename, "rb");
        if (fp == NULL)
                die("error reading ROM file '%s'\n", filename);
        fread(mem + 0x200, 1, 4096 - 0x200, fp);
        int error = ferror(fp);
        fclose(fp);
        if(error)
                die("error loading ROM file\n");
}


void dxyn(uint8_t vx, uint8_t vy, uint8_t n);
void fx0a(uint8_t x, bool opt_release);
int search_keymap(int key);

/* process opcode, returning true if screen drawn to */
bool run(uint8_t opts)
{
        uint16_t instruction, nnn;
        uint8_t x, y, n, kk, tmp;

        instruction = (*pc << 8) | pc[1];
        pc += 2;

        x   = (instruction & 0x0F00) >> 8;
        y   = (instruction & 0x00F0) >> 4;
        n   = instruction & 0x000F;
        kk  = instruction & 0x00FF; /* byte */
        nnn = instruction & 0x0FFF; /* addr */

        switch (instruction >> 12) {
        case 0x0:
                switch (instruction & kk) {
                case 0xE0:
                        memset(screen, 0, sizeof(screen));
                        return true;
                        break;
                case 0xEE:
                        if (stack_ptr == 0)
                                die("pop from empty stack\n");
                        pc = mem + stack[--stack_ptr];
                        break;
                default:; /* SYS addr; ignored by modern interpreters */
                }
                break;
        case 0x1:
                pc = mem + nnn;
                break;
        case 0x2:
                if (stack_ptr == 16)
                        die("stack overflow\n");
                stack[stack_ptr++] = pc - mem;
                pc = mem + nnn;
                break;
        case 0x3:
                if (V[x] == kk)
                        pc += 2;
                break;
        case 0x4:
                if (V[x] != kk)
                        pc += 2;
                break;
        case 0x5:
                if (V[x] == V[y])
                        pc += 2;
                break;
        case 0x6:
                V[x] = kk;
                break;
        case 0x7:
                V[x] += kk;
                break;
        case 0x8:
                switch (n) {
                case 0x0:
                        V[x] = V[y];
                        break;
                case 0x1:
                        V[x] |= V[y];
                        if (opts & OPT_VF_RESET)
                                V[0xF] = 0;
                        break;
                case 0x2:
                        V[x] &= V[y];
                        if (opts & OPT_VF_RESET)
                                V[0xF] = 0;
                        break;
                case 0x3:
                        V[x] ^= V[y];
                        if (opts & OPT_VF_RESET)
                                V[0xF] = 0;
                        break;
                case 0x4:
                        tmp = (uint8_t)(V[x] + V[y]) < V[x];
                        V[x] += V[y];
                        V[0xF] = tmp;
                        break;
                case 0x5:
                        tmp = V[x] >= V[y];
                        V[x] -= V[y];
                        V[0xF] = tmp;
                        break;
                case 0x6:
                        if (!(opts & OPT_SHIFT))
                                V[x] = V[y];
                        tmp = V[x] & 1;
                        V[x] >>= 1;
                        V[0xF] = tmp;
                        break;
                case 0x7:
                        tmp = V[y] >= V[x];
                        V[x] = V[y] - V[x];
                        V[0xF] = tmp;
                        break;
                case 0xE:
                        if (!(opts & OPT_SHIFT))
                                V[x] = V[y];
                        tmp = (V[x] >> 7) & 1;
                        V[x] <<= 1;
                        V[0xF] = tmp;
                        break;
                }
                break;
        case 0x9:
                if (V[x] != V[y])
                        pc += 2;
                break;
        case 0xA:
                I = nnn;
                break;
        case 0xB:
                pc = mem + nnn + V[(opts & OPT_JUMP) ? x : 0x0];
                break;
        case 0xC:
                V[x] = rand() & kk;
                break;
        case 0xD:
                dxyn(V[x], V[y], n);
                return true;
                break;
        case 0xE:
                switch (kk) {
                case 0x9E:
                        if (IsKeyDown(keymap[V[x]]))
                                pc += 2;
                        break;
                case 0xA1:
                        if (!IsKeyDown(keymap[V[x]]))
                                pc += 2;
                        break;
                default:
                        goto fail;
                }
                break;
        case 0xF:
                switch (kk) {
                case 0x0A:
                        fx0a(x, opts & OPT_RELEASE);
                        break;
                case 0x07:
                        V[x] = delay_timer;
                        break;
                case 0x15:
                        delay_timer = V[x];
                        break;
                case 0x18:
                        sound_timer = V[x];
                        break;
                case 0x1E:
                        I += V[x];
                        break;
                case 0x29:
                        I = 5 * V[x];
                        break;
                case 0x33:
                        mem[I] = V[x] / 100;
                        mem[I + 1] = (V[x] % 100) / 10;
                        mem[I + 2] = (V[x] % 10);
                        break;
                case 0x55:
                        for (tmp = 0; tmp <= x; tmp++)
                                mem[I + tmp] = V[tmp];
                        if (opts & OPT_INCREMENT_I)
                                I += tmp;
                        break;
                case 0x65:
                        for (tmp = 0; tmp <= x; tmp++)
                                V[tmp] = mem[I + tmp];
                        if (opts & OPT_INCREMENT_I)
                                I += tmp;
                        break;
                default:
                        goto fail;
                }
                break;
        default:
                goto fail;
        }
        return false ;
 fail:
        die("unrecognized instruction 0x%x\n", instruction);
}


void dxyn(uint8_t vx, uint8_t vy, uint8_t n)
{
        uint8_t x, y, sprite;
        bool sprite_px_set;

        y = vy % 32;
        vx %= 64;
        V[0xF] = 0;

        for (int r = 0; r < n; r++) {
                sprite = mem[I + r];
                x = vx;
                for (int i = 0; i < 8; i++) {
                        sprite_px_set = (sprite >> (7 - i)) & 1;
                        if (sprite_px_set) {
                                if (screen[y][x])
                                        V[0xF] = 1;
                                screen[y][x] ^= sprite_px_set;
                        }
                        if (++x >= 64)
                                break;
                }
                if (++y >= 32)
                        break;
        }
}


void fx0a(uint8_t x, bool opt_release)
{
        int key, pos;

        key = GetKeyPressed();
        pos = search_keymap(key);
        if (pos != -1) {
                if (opt_release) {
                        last_keypress = key;
                        last_keypress_reg = x;
                } else {
                        V[x] = pos;
                        return;
                }
        }
        pc -= 2;
}


int search_keymap(int key)
{
        for (int i = 0; i < 16; i++)
                if (keymap[i] == key)
                        return i;
        return -1;
}


void draw_screen(void)
{
        for (int h = 0; h < 32; h++)
                for (int w = 0; w < 64; w++) {
                        if (screen[h][w])
                                DrawRectangle(SCALE * w, SCALE * h, SCALE, SCALE, BLACK);
                        else if (sound_timer > 0)
                                DrawRectangle(SCALE * w, SCALE * h, SCALE, SCALE, PINK);
                }
}


void usage(char* program_name)
{
        fprintf(stderr,
                "usage: %s [--modern | --original] <file>\n", program_name);
        exit(2);
}


uint8_t getconfig(int argc, char**argv, char **filename) {
        uint8_t opts = MODERN;
        char* program_name = *argv;
        if (argc == 1)
                usage(program_name);
        for (++argv; *argv != NULL; argv++) {
                if (!strcmp(*argv, "--modern")) {
                        opts = MODERN;
                } else if (!strcmp(*argv, "--original")) {
                        opts = ORIGINAL;
                } else
                        *filename = *argv;
        }
        if (*filename == NULL)
                usage(program_name);
        return opts;
}


int main(int argc, char **argv)
{
        char *filename = NULL;
        uint8_t opts = getconfig(argc, argv, &filename);

        srand(time(NULL));
        pc = mem + 0x200;
        load_rom(filename);

        SetConfigFlags(FLAG_VSYNC_HINT);
        SetTraceLogLevel(LOG_ERROR);
        SetTargetFPS(60);
        InitWindow(SCREEN_WIDTH, SCREEN_HEIGHT, "chip-8");

        float delay_timer_tick = 0;
        float sound_timer_tick = 0;
        float frame_time;

        while (!WindowShouldClose()) {
                frame_time = GetFrameTime();
                delay_timer_tick += frame_time;
                sound_timer_tick += frame_time;

                if (delay_timer_tick > 1/60.0) {
                        delay_timer_tick -= 1/60.0;
                        if (delay_timer > 0)
                                delay_timer--;
                }

                if (sound_timer_tick > 1/60.0) {
                        sound_timer_tick -= 1/60.0;
                        if (sound_timer > 0)
                                sound_timer--;
                }

                for (int i = 0; i < 12 /* 700 cycles per sec */ ; i++)
                        if (run(opts) && (opts & OPT_DISPLAY_WAIT))
                                break;

                BeginDrawing();
                ClearBackground(RAYWHITE);
                draw_screen();
                EndDrawing();

                if ((opts & OPT_RELEASE) && IsKeyReleased(last_keypress)) {
                        pc += 2;
                        V[last_keypress_reg] = search_keymap(last_keypress);
                }
        }
        CloseWindow();
        return EXIT_SUCCESS;
}
\$\endgroup\$
5
  • \$\begingroup\$ #define ORIGINAL ~(OPT_JUMP | OPT_JUMP) ... A constant OR'd with itself should be itself... Maybe I'm not understanding something, but this looks strange to me... \$\endgroup\$
    – Fe2O3
    Commented May 31 at 2:20
  • \$\begingroup\$ Yep you're right @Fe2O3, it was lazy copy paste. \$\endgroup\$
    – wed1may
    Commented May 31 at 21:33
  • \$\begingroup\$ Those 4 lines of the 2 #defines have shuffled their tokens. Recommend shorter names and maintaining order (sequence) to highlight the similarity/difference between the two "compound" bit patterns. Can you understand this suggestion, or should I post a very short answer to demonstrate this recommendation? Cheers... :-) \$\endgroup\$
    – Fe2O3
    Commented May 31 at 21:40
  • 1
    \$\begingroup\$ Hi @Fe2O3, if you wouldn't mind posting a quick answer that'd be great :) \$\endgroup\$
    – wed1may
    Commented May 31 at 22:19
  • 1
    \$\begingroup\$ for (tmp = 0; tmp <= x; tmp++) I do not pretend that I've traced through the code, or that I know things about what the code should be doing... Just a general "reader of C" comment that <= of an array index gives me the willies... This may be perfectly correct in this app. Consider adding comment // yes, really! to those lines so that casual readers like me don't stumble and express their disquiet/mistrust... Cheers! :-) \$\endgroup\$
    – Fe2O3
    Commented Jun 1 at 1:35

4 Answers 4

13
\$\begingroup\$

Unneeded promotion

float delay_timer_tick = 0;
...
if (delay_timer_tick > 1/60.0) {  // delay_timer_tick promoted to double

Avoid casual FP type mixing

float delay_timer_tick = 0;
...
if (delay_timer_tick > 1/60.0f) {

or

double delay_timer_tick = 0;
...
if (delay_timer_tick > 1/60.0) {

Consider using double unless there is a compelling reason for float or long double.

Physical units

Quantities like time deserve units.

// uint8_t   delay_timer;
uint8_t   delay_timer; // Seconds

Check I/O results

//fread(mem + 0x200, 1, 4096 - 0x200, fp);
  size_t nmenb =  4096 - 0x200;
  if (fread(mem + 0x200, 1, nmenb, fp) != nmenb) {
    die("error reading ROM file '%s'\n", filename);
  }

Pedantic: Avoid undefined behavior (UB)

With 16-bit int, unsigned, *pc << 8 can shift into the signed bit (UB).

  uint16_t instruction, nnn;
  // instruction = (*pc << 8) | pc[1];
  instruction = ((unsigned)*pc << 8) | pc[1];  // or the like

Good use of sentinels in error messages

//                          v  v
die("Error reading ROM file '%s'.\n", filename);

Use sentence case for error messages

//die("error reading ROM file '%s'\n", filename);
die("Error reading ROM file '%s'.\n", filename);

Be more precise

 if (fp == NULL)
   // die("error reading ROM file '%s'\n", filename);
   die("Error opening ROM file '%s'\n", filename);

Use () about compound macro equations

This prevents surprising evaluations like int x = !ORIGINAL.

// #define ORIGINAL ~(OPT_JUMP | OPT_JUMP) & \
//    (OPT_VF_RESET | OPT_INCREMENT_I | OPT_DISPLAY_WAIT | OPT_RELEASE)
#define ORIGINAL (~(OPT_JUMP | OPT_JUMP) & \
    (OPT_VF_RESET | OPT_INCREMENT_I | OPT_DISPLAY_WAIT | OPT_RELEASE))

Many naked magic number

Rather than 16, 32, 64, etc., use STACK_N, SCREEN_ROWS, SCREEN_COLS, etc. and many others.

What?: for (int i = 0; i < 12 /* 700 cycles per sec */ ; i++)

How about?

// Derive a rounded iteration count from frequency
#define CPS_TO_COUNT(f) ((12*700 + (f)/2)/ (f))

#define CPS 700

for (int i = 0; i < CPS_TO_COUNT(CPS); i++)
\$\endgroup\$
5
  • \$\begingroup\$ Thanks for the feedback, especially on the UB and the type mixing/promotion. Not sure how I forgot to parenthesise the other macros too, so thanks for spotting that. \$\endgroup\$
    – wed1may
    Commented May 30 at 7:26
  • \$\begingroup\$ Actually I don't quite understand why the cast is required in instruction = ((unsigned)*pc << 8) | pc[1];. Since *pc is uint8_t (unsigned), I though well defined to shift it and that would also give an unsigned value. \$\endgroup\$
    – wed1may
    Commented May 30 at 7:35
  • 5
    \$\begingroup\$ @wed1may: integer promotion rules are weird and not always wonderful, uint8_t will be promoted to int (not unsigned int) before arithmetic operations (stackoverflow.com/questions/46073295/…) \$\endgroup\$
    – Mat
    Commented May 30 at 9:01
  • 1
    \$\begingroup\$ @wed1may: When C89 was written, something like uint1=ushort1*ushort2; would be equivalent to uint1=(unsigned)ushort1*ushort2; except on some rare platforms where code that could handle all operand values might be slower than code that only ushort1 values below INT_MAX/ushort2. There was never any doubt about how implementations for commonplace platforms should behave, and thus no perceived need to worry about whether short unsigned values promoted to signed or unsigned values in contexts where, like the above, signed values would have never be required to behave differently... \$\endgroup\$
    – supercat
    Commented May 30 at 21:07
  • 1
    \$\begingroup\$ ...from unsigned. When using gcc, however, the Standard's failure to mandate that a compiler refrain from generating code that arbitrarily corrupts memory if ushort1 exceeds INT_MAX/ushort2 is considered reason enough to generate such code rather than behaving as described in the published Rationale for the C99 Standard (describing the reasons behind the C89 promotion rules). \$\endgroup\$
    – supercat
    Commented May 30 at 21:14
11
\$\begingroup\$

I could have defined constants e.g. CHIP8_STACK_SIZE, CHI8_MEM_START, etc. and this is what I initially did. However, as these kept getting in the way, and as the specification for the CHIP8 is fixed, I just hard coded many constants.

Now if only you had not, I would have understood what 32, 64, 4096, 12, 7, 2, et cetera resemble. You are incorrectly expecting readers to have the specification for the CHIP8 memorized.

I opened the link you added in the post and searched for "4096", which resulted in this:

The memory should be 4 kB (4 kilobytes, ie. 4096 bytes) large. CHIP-8’s index register and program counter can only address 12 bits (conveniently), which is 4096 addresses.

I am certain good symbolic names can be thought of for this. Or if not, at least a comment mentioning the above would be helpful to readers.

For the opcodes too, you could define an enum. In C23, you could give them a type like this:

enum : unsigned char {
        OPCODE_...,
        OPCODE_...,
        // Or, OP_...
};

One advantage of using enumeration constants over preprocessor macros is that the constant name is preserved in debuggers, where the macro is not. Along with that, it makes the intent self-documenting, and the code easier to read and understand for everyone.

Use of many globals: I could have encapsulated the emulator state in a struct, for example.

Do so. That'd be nicer.

Use const for data that should not be modified, especially function parameters:

usage() should take a const char * to signal to the caller that it does not modify the argument. Same goes for other functions.

Use restrict for pointers that should not alias:

As getconfig() takes two pointers of the same type, adding the restrict qualifier can allow the compiler to better optimize the code, because it can assume that the two pointers would not alias.

Same applies to functions that take a char * and a pointer of another type, because a char * is allowed to alias any other type. And functions that take a void * and a pointer of any other type.

Use static keyword for array arguments:

In C99 and above, you can do array[static 1] to specify that array should point to at least one item.

This has some benefits:

  • Compilers can detect that a nullptr was passed and can output some diagnostic information. GCC has the best diagnostic information, Clang and Intel's compiler, not so much. For example, Clang and ICX only issue a warning when NULL/nullptr is passed as an argument, but not when a variable which happens to be a null pointer is passed, unlike GCC.
  • It better documents the code, as having at least 1 element indicates that a nullptr is not a valid value for this argument. This way, a comment about the function invoking undefined behavior on being passed a nullptr is not necessary.
  • It can eliminate one or more branches (assuming the called function was checking the validity of the arguments).

After adding the static keyword to program_name, usage() looks like this:

void usage(char program_name[static 1])

Note that this notation can not be used for local variables, return values, or for functions like memcpy() because arrays of void are not valid. To specify that a function returns a value that is not a null pointer, use GCC's __attribute__((returns_nonnull)). It is also supported by Clang and Intel's compiler. For void *s (and others too), you may use GCC's __attribute__((nonnull)). For local variables, wait for the C committee to add an _Optional type, or for Clang, use _Nonnull. This extension is not supported by GCC.

Use EXIT_FAILURE to signal failure:

You have excellently used EXIT_SUCCESS in main(), but have used 1, 2, et cetera to indicate failure. Consider using EXIT_FAILURE instead.

Use descriptive function and variable names:

What exactly do dxyn and fx0a mean? There's no comment explaining them either. There are a lot of single-letter variable names in your code like r, x, y, vy, vx, n, et cetera. Those may be commonplace in Go code, but not so in C. Single-letter variable names are okay and expected when used as loop indices, but not in other places.

Make everything static:

Currently, all the file-scope variables and functions have external linkage. They should be made static. And the magic numbers should be replaced with descriptive symbolic names. Some comments would also be nice.

Pedantic: argv[0] can be a nullptr:

There's an assumption here that argv[0] is always pointing to the program name:

char* program_name = *argv;
...
for (++argv; *argv != NULL; argv++) {

That may not always be the case. One can easily pass a nullptr through some exec() syscall, and your code would then dereference a nullptr in usage(), and hence invoke undefined behavior.

All programs in the GNU coreutils package call set_program_name() to do some setup work. It checks whether argv[0] is nullptr and calls abort() when it is. See the source code yourself. See also: Runnable POSIX example of argv[0] == NULL.

I would also change die() to take the format string (die(fmt, ...) instead of die(...)) and then add "" to it for some rudimentary type-checking.

#define die(fmt, ...) do { fprintf(stderr, fmt "", __VA_ARGS__); exit(1); } while( false )

Consecutive string literals are concatenated in C, one can take advantage of that by concatenating the macro argument provided by the caller with the empty string literal. If the caller provides something other than a string literal the compiler will complain.

Though of course, one can still break the macro somehow. You could add extra expressions to it, but the added robustness may not be worth the added complexity.

\$\endgroup\$
4
  • \$\begingroup\$ Hi Harith, thanks for reviewing my code again. I agree with you on all points now; I should've at least given comments. dxyn and fx0a are opcodes, and those functions were helpers for them to avoid cluttering the switch. Especially about restrict and the non-null pointer trick, I didn't know those at all. I remember last time you mentioned memchr, but I forgot about it here again, oops. For the return 2, it was because I thought that it is UNIX tradition to return 2 for incorrect usage of CLI arguments, but maybe that is incorrect. \$\endgroup\$
    – wed1may
    Commented May 29 at 21:29
  • \$\begingroup\$ @wed1may See: manpages.ubuntu.com/manpages/lunar/man3/sysexits.h.3head.html Use EX_USAGE from <sysexits.h>. Naked 2 doesn't hold much meaning to most people. I believe it would be better if you un-accept this answer for the time being (say 3 days), you may get more, better and broader reviews that way. You can then accept whichever one you found most useful. \$\endgroup\$
    – Harith
    Commented May 29 at 21:37
  • \$\begingroup\$ I see, thank you. For future reference, would you suggest I use EX_USAGE, or prefer to just return EXIT_FAILURE? \$\endgroup\$
    – wed1may
    Commented May 29 at 21:40
  • 3
    \$\begingroup\$ <sysexits.h> is BSD specific. <stdlib.h> is ISO C. I would suggest the latter. \$\endgroup\$
    – Harith
    Commented May 29 at 21:42
7
\$\begingroup\$

General Observations

There are 2 other reviews that provide good information. I will mostly comment on what hasn't been covered.

The code compiles without warnings using -Wall -Wextra -pedantic -Werror, this is good.

Emulating the hardware does not necessarily mean that you have to use the actual word size of the processor in the emulation. If you used the word size of your computer the program would execute faster.

There are several possible maintenance issues that I see.

Avoid Global Variables

It is very difficult to read, write, debug and maintain programs that use global variables. Global variables can be modified by any function within the program and therefore require each function to be examined before making changes in the code. In C and C++ global variables impact the namespace and they can cause linking errors if they are defined in multiple files. The answers in this stackoverflow question provide a fuller explanation.

If you are going to use global variables, restrict them to the file they are declared in by using the static keyword.

static uint8_t  *pc;
static uint16_t  I;
static uint16_t  stack[16];
static uint8_t   stack_ptr;
static uint8_t   V[16];
static uint8_t   screen[32][64];
static uint8_t   delay_timer;
static uint8_t   sound_timer;

Make sure the global variable names are unique. Global variable names like I, V and pc can lead to problems. I assume pc means program counter, but that isn't clear. I have no idea what the variables I and V mean.

I spotted pc in a function where it was uninitialized which made me search for it.

Comments

The C programming language accepts both // and /* ... */ for comments. The /* ... */ is for multi-line block comments, the // is for one line or the end of the line comments.

Multi-line comments should be complete sentences so they are understood. The format should be:

/*
    Comment.
 */

By putting the opening and closing of the comment on their own lines it is much easier to find where they start and end.

/*
 * on:  the AND, OR, and XOR opcodes reset the flags register to zero
 * off: the flags register is not modified
 */
#define OPT_VF_RESET     1

/*
 * on:  the save and load opcodes (Fx55 & Fx65) increment the index register
 * off: the index register is not incremented
 */
#define OPT_INCREMENT_I  2

Macros

Generally the code follows the best practice that macros should be all capitals. The one exception is the die macro. I would suggest making this macro a function or capitalizing it.

Complexity

Two of the functions run() and main() are too complex (do too much). Both should be broken up into smaller functions. Generally functions should be limited in size to the number of lines that fit on a screen. Functions that are larger than a screen are generally hard to understand and hard to maintain. The run() function is more than 3 screens, at least on my laptop.

Prefer Braces { and } Around Single Statements in if or loops

Some programmers consider this a style issue, but it makes it much easier to read and maintain the code if each in an if, else or loop block is embedded within braces. Extending the functionality of these statements can be problematic when the braces are not used. For a more in depth discussion of this see the first 2 answers on this Stack Overflow question. As one of the answers points out this is true in all C like languages (C, C++, C#, JavaScript, Java, etc.). I have worked at multiple companies where this was required in the coding standard and flagged during code reviews.

\$\endgroup\$
11
  • \$\begingroup\$ Thanks for the feedback, good point about die and the globals; for my next project I will be sure to use globals much more sparingly. My editor automatically makes the indents for me, but I guess there is no harm in getting into the habit of preferring braces too. \$\endgroup\$
    – wed1may
    Commented May 30 at 13:48
  • 2
    \$\begingroup\$ @wed1may About the last point mentioned in this review, see: imperialviolet.org/2014/02/22/applebug.html \$\endgroup\$
    – Harith
    Commented May 30 at 13:49
  • 2
    \$\begingroup\$ @wed1may The apple bug link does point out the problem about lack of braces. While I understand why the Linux style guide makes that statement, I disagree, from a code maintainability stand point. \$\endgroup\$
    – pacmaninbw
    Commented May 30 at 15:08
  • 3
    \$\begingroup\$ @Jonbonson I'll assume you have never had to fix someone else's bug at midnight after working for 18 hours. A one line fix sometimes isn't a one line fix. \$\endgroup\$
    – pacmaninbw
    Commented May 30 at 19:11
  • 1
    \$\begingroup\$ Not that it is a bad thing... The style with each brace having a line to itself also means that logic may require twice a much vertical space. This leads to scrolling, or refactoring like a zealot to the point where the human mind's "stack" explodes, and tracing the flow has to start all over again... Code should be correct and clear. Coders should not "work with their elbows"... Religions aren't the solution... \$\endgroup\$
    – Fe2O3
    Commented May 31 at 2:56
3
\$\begingroup\$

Lots of good answers already. Here's a humble offering regarding the first thing the reader sees...

The #define block at the top of the code is fairly good, especially good for the comments that are there. Kudos!

Paraphrasing an elephant named Horton: "A token's a token, no matter how small."

Other Q&A have pointed out that preprocessor tokens (#define) don't make it through to their binaries and cannot be used within a debugger.

Below is an alternative for the OP to consider. Using enum nicely sidesteps the subtle problem fixed by a point in the answer by @Chux (unintended consequences of unbracketed tokens.)

Token names have been slightly abbreviated and comments have been streamlined. An intermediate token is used to reduce verbosity and show commonality. (If I've not screwed-things-up, a (presumed) hidden bug is now more discernable. Unchanged here as I have not gone deeper to try to deduce its meaning. Me thinks it's a bug.)

static enum {
    eVF_RESET     = 1,  // on:  flags register set to zero for AND, OR, and XOR opcodes
                        // off: flags register not modified

    eINCREMENT_I  = 2,  // on:  index register increment for save and load opcodes (Fx55 & Fx65)
                        // off: index register not incremented

    eDISPLAY_WAIT = 4,  // on:  each frame a single draw instruction is executed
                        // off: each frame multiple draw instructions may occur

    eSHIFT        = 8,  // on:  shift opcodes (8xy6 and 8xyE) only operate on the Vx regsiter
                        // off: shift opcodes operate on Vy, storing the result in Vx

    eJUMP         = 16, // on:  Bnnn instruction uses Vx
                        // off: Bnn instruction uses V0 // Bnn or Bnnn?

    eRELEASE      = 32, // on:  key is processed when released in Fx0a
                        // off: key is processed when pressed

    eTrio = eINCREMENT_I | eRELEASE | eDISPLAY_WAIT, // trio combo for brevity

    eORIGINAL = ~(  eJUMP | eJUMP ) &  ( eTrio | eVF_RESET ),
//                  ^^^^^   ^^^^^-----!!! Not sure this is right...

    eMODERN   =  ( eSHIFT | eJUMP ) & ~( eTrio )
};

Note: if my bug-hunch is right, using a second intermediate token:
ePair = eSHIFT | eJUMP, // more compaction
could be used:
eORIGINAL = ~( ePair ) & ( eTrio | eVF_RESET ),
eMODERN = ( ePair ) & ~( eTrio ),
drastically reducing the potential for that (presumed) bug to manifest itself...

(Fingers crossed as I have not compiled this to test for syntax errors. Happy to correct any that are spotted and reported!)


For the few tokens/values herein, using familiar decimal values to represent single binary bits is not a big problem. For cases where there may be up to 32 bit flags, I would recommend using hexadecimal (or even octal, if the flags naturally group by 3's) instead of decimal. Most readers would prefer to see 0x20000000, instead of 536870912.

This may become important when writing code to emulate, for instance, a 1970's calculator. The retail price difference between a "four function" calculator and one that had "extended math keys" like [ % ] (show as percentage) and [ x2 ] (square) was significant. The manufacturing price difference of one vs two VLSI chip designs was very significant!. Manufacturers would order, for instance, 100,000 units of the advanced version of the IC. Then, by using a pin, activate (or deactivate) the advanced facilities of the chip when assembling one consumer calculator. Over time, the assembly factory could adjust its output of basic or advanced versions to meet market demand for each version of the calculator (week to week, or month to month). The alternative was to risk running-out-of-stock or to be over-supplied with stock of one-of-two kinds 'n' months later.

Applying this technique to your project (in another universe), presume one bit is needed to "gate" one function, and a second bit to "gate" an advanced version of the function. Consider the effects achievable with:

static enum {
    ...
    eBasicFac = 0x0040, // enable "basic" facility (or disable)
    eAdvanFac = 0x2040, // enable "basic" PLUS "advanced" version
    ...
};

In decimal those two numbers are 64 and 8256.
Can you quickly spot the relationship between 64 and 8256?
Do you see why hexadecimal is the preferred expression of bit flags?


And, for something completely different, how about replacing a ternary operation with a branchless version that achieves the same result:

//  pc = mem + nnn + V[   (opts & OPT_JUMP) ? x : 0x0 ]; // ternary
    pc = mem + nnn + V[ !!(opts & OPT_JUMP) * x ];       // branchless

The & operates as expected.
The !! converts the value to true or false, being 1 or 0.
The * multiplies x by either 1 (identity) or 0 (then it is zero).

It's a few less characters of source code, and, in some circles, it is an easily recognisable pattern that is understood by the reader.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ @Harith I wrote 'jump table', but others call it a dispatch table. A binary value is 'decoded' and a service function executed. Nothing serious. "The machine" state could be conveniently "global", so a bevy of parameters passed to each handler is not required... As simple or complex as desired... Re: "Bye-bye octals (maybe)"... Octal replies, "Fine, but you'll miss me when I'm gone..." :-) (The mathematician's prank of "The Collatz Conjecture" becomes obvious in octal notation...) \$\endgroup\$
    – Fe2O3
    Commented Jun 1 at 3:09
  • 1
    \$\begingroup\$ Hi, @Fe2O3, thank you for expanding and adding these tips which are not present in the other answers, much appreciated :) I see in your other comment you mentioned about commenting <= bounds in loops and here you mentioned about the branchless way, it's good to know about these patterns/ idioms for future. \$\endgroup\$
    – wed1may
    Commented Jun 1 at 7:45
  • 2
    \$\begingroup\$ @wed1may I've been in the business for far too long, and am still learning things about C that I wish I'd known long, long ago. Spend as much time as you can allow just reading code here on "Code Review" or "Stack Overflow"... Two hours busting your brains over something is good because you gain hands-on experience. Two hours skimming other's code to test your comprehension is also good. There's lots of ways to skin a cat, and the more your brain has, at least, seen, the more your brain might be able to use when called upon to do something. (Where to look for those traps.) Best wishes! :-) \$\endgroup\$
    – Fe2O3
    Commented Jun 1 at 9:28
  • 2
    \$\begingroup\$ @wed1may "Suggest time spent reading code"... BLATANT self-promotion here: This post is (was) a hobby project to fulfill two purposes: 1) to roll-my-own Sudoku Solver, and 2) recreational coding for the fun of it... I invite you to grab copies of the 8 files + the 1 file of "puzzles", then, at your leisure, find my bugs for me. :-) (Actually, it seems to work, but who knows if the next Sudoku will trigger a haemorrhage...) Lots of code to skim and critique... Have fun! :-) \$\endgroup\$
    – Fe2O3
    Commented Jun 1 at 10:25
  • 1
    \$\begingroup\$ @Fe2O3 Thanks for that recommendation. I actually have my eyes on another project at the moment, but when I get some free time I'll be sure to look at it :) Sounds like a fun project! \$\endgroup\$
    – wed1may
    Commented Jun 1 at 14:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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