CHIP-8 delay timer in C++

I'm writing a CHIP-8 emulator in C++, which has two timers. If these timers are non-zero, they are decremented by 1 at 60Hz. I'm still getting the hang of C++, so I wanted to see if I'm doing this the Right Way.

#include <chrono>

struct TimedRegister
{
unsigned char value;
std::chrono::time_point<std::chrono::high_resolution_clock> last_write;

TimedRegister();
void set(const unsigned char new_value);
void decrement();
};


And this is the the implementation:

#include "chip8.hpp"

#include <chrono>

TimedRegister::TimedRegister()
: value(0)
{}

void
TimedRegister::set(const unsigned char new_value)
{
value = new_value;
last_write = std::chrono::high_resolution_clock::now();
}

void
TimedRegister::decrement()
{
if (value == 0)
return;

const auto now = std::chrono::high_resolution_clock::now();
if (std::chrono::duration_cast<std::chrono::milliseconds>(now - last_write)
.count() >= 1000 / 60) {
--value;
last_write = now;
}
}


The decrement function is called from the main emulator loop like this:

while (!chip8.halted) {
chip8.delay_timer.decrement();
}


They seem to work fine, but I'm not sure if I'm getting away with using a heavy-handed, brute-force approach.

• This is somewhat unrelated, but ideally you should be updating the timers while refreshing the screen, like the original interpreter does. – 12Me21 Mar 14 '18 at 19:39

When doing emulation you should disconnect from the real world and only provide a abstract view to a world.

In other words don't use a real time clock instead simulate the timer by linking it to the emulated clockspeed. For example if you pick 8 MHz you would have 133 333 instructions executed per decrement of each timer.

This will allow you to pause and fast forward when debugging programs.

• For Chip8, the timer should be cued off display refresh. If memory serves, vintage Chip8 implementations run roughly 1800 cycles (900 two-cycle instructions) between frames, but some operations like draw sprite wait for the next refresh. – supercat Mar 14 '18 at 22:01

Since you keep a note of last_write, you don't need to keep updating the actual value.

Instead, implement a read() overload that computes the value as seen by the emulated system:

unsigned char TimedRegister::read()
{
if (!value)
return 0;

auto now = std::chrono::high_resolution_clock::now();
auto elapsed = std::chrono::duration_cast<Chip8::Tick>(now - last_write);
if (elapsed.count() > value)
// assign to value, so we don't look at clock next time
return value = 0;

return value - elapsed.count();
}


I'd consider moving away from std::chrono::high_resolution_clock for this - you really want your own emulated-system clock, which you can advance under program control. That will make both unit-testing and debugging easier.

Some minor observations:

• Make the code easier to read
Long lines including std::chrono::high_resolution_clock and std::chrono::milliseconds might be a good choice for applying the using directive:
using high_res_clock = std::chrono::high_resolution_clock;
You can then use high_res_clock instead of the unwieldy old term.

• Use compiler warnings
Always enable as many warning flags as possible. That way the compiler can help you eliminate possible errors. In this case you should initialize last_write just like you do with value.

• class VS. struct
I'm unsure how this ties into the rest of your code but I was taught to use struct for PODs and class if you have methods. Just remember that in classes everything is private by default unlike in structs where it's the opposite.