Chip8 emulator memory map

I am making a Chip8 emulator and I started out with making a class for handling the memory map. So main the execution will read from this memory object and write to it.

/include/Chip8.h

#ifndef CHIP8_H
#define CHIP8_H
// File for keeping holding Chip8 system variables

#include <iostream>
#include <exception>

namespace chip8{
  constexpr size_t MEM_SPACE = 0xFFF;   // Const for denoting size of memory map
  constexpr long  PROG_START = 0x200;   // Const for denoting start of Chip8 program
}

#endif // CHIP8_H

/include/Memory.h

#ifndef CHIP8_MEMORY_H
#define CHIP8_MEMORY_H

#include <array>
#include <vector>
#include "Chip8.h"

namespace chip8{
  class Memory{
  public:

    // Default constructor repurposed to initialize mem_ptr and ram values
    Memory();

    // Read a single byte starting at index location 'adr'
    uint8_t read_byte(uint16_t adr);

    // Read a two bytes, returned as a short, starting at index location 'adr'
    uint16_t read_short(uint16_t adr);

    // Read 'count' amount of bytes, returned as a vector of uint8_t, starting at index location adr
    std::vector<uint8_t> read_bulk(uint16_t adr, uint16_t count);

    // Store a single byte at the next available index location
    void store_byte(uint8_t data);

    // Store 'n' bytes, starting at the next available index location
    // 'n' is the size of the input vector
    void store_bulk(uint16_t adr, std::vector<uint8_t> data);

    // Function to get mem pointer location
    uint16_t get_memory_pointer();

  protected:
  private:
    // Internal function used to validate addresses before accessing ram
    void check_adr_(uint16_t adr);

    // Internal function to set the memory pointer at an index in ram
    // Protects the class from having a user modify the mem_ptr without reason
    void set_memory_pointer_(uint16_t mem_ptr);

    // Array of bytes to defined the memory map
    std::array<uint8_t, MEM_SPACE> ram_;  

    // Used to keep track of where we are in the memory map
    uint16_t mem_ptr_;                    
  };
}

#endif // CHIP8_MEMORY_H

/src/Memory.cpp

#include <vector>
#include <string>
#include <exception>
#include "../include/Memory.h"
#include "../include/Logger.h"

namespace chip8{

  // Default constructor repurposed to initialize mem_ptr and ram values
  Memory::Memory(){
    ram_.fill(0);
    mem_ptr_ = 0;
  }

  // Used to validate address range. Valid memory is only between 0 and size-1
  void Memory::check_adr_(uint16_t adr){
    if(adr < 0 || adr > MEM_SPACE - 1){
      throw std::out_of_range("Address undefined in memory space");
    }
  }

  // Internal function to set the memory pointer at an index in ram
  // Protects the class from having a user modify the mem_ptr without reason
  void Memory::set_memory_pointer_(uint16_t mem_ptr){
    // Validate address range
    check_adr_(mem_ptr);
    mem_ptr_ = mem_ptr;
  }

  // Function to get mem pointer location
  uint16_t Memory::get_memory_pointer(){
    return mem_ptr_;
  }

  // Read a single byte starting at index location 'adr'
  uint8_t Memory::read_byte(uint16_t adr){
    // Validate address range
    check_adr_(adr);

    uint8_t data = ram_[adr];
    // Debug message
    util::LOG(LOGTYPE::DEBUG, "Read " + std::to_string(data) + " from address " + std::to_string(adr));

    // Return byte
    return data;
  }

// Read a two bytes, returned as a short, starting at index location 'adr'
  uint16_t Memory::read_short(uint16_t adr){
    // Validate address range
    check_adr_(adr);

    // High byte is the one with the smaller address value (closer to 0)
    uint16_t data = ( (ram_[adr] << 8)) | (ram_[adr+1] & 0xFF00);

    util::LOG(LOGTYPE::DEBUG, "Read " + std::to_string(data) + " from address " + std::to_string(adr));

    return data;
  }

  // Read 'count' amount of bytes, returned as a vector of uint8_t, starting at index location adr
  std::vector<uint8_t> Memory::read_bulk(uint16_t adr, uint16_t count){
    // Validate address range. Need to check start address and end address
    // end address is start address + count
    check_adr_(adr);
    check_adr_(adr+count);

    // Store 'count' amount of bytes for return starting at 'adr' in vector v
    std::vector<uint8_t> data_list;
    for(auto i = adr; i < count; ++i){
      uint8_t data = this->read_byte(adr);
      data_list.push_back(data);
    }

    return data_list;
  }

  // Store a single byte at the next available index location
  void Memory::store_byte(uint8_t data){
    // Validate address range so we can store a byte in RAM
    check_adr_(mem_ptr_+1);

    util::LOG(LOGTYPE::DEBUG, "Storing " + std::to_string(data) + " from address " + std::to_string(mem_ptr_));

    ram_[mem_ptr_++] = data;
    return;
  }

  // Store 'n' bytes, starting at the next available index location
  // 'n' is the size of the input vector
  void Memory::store_bulk(uint16_t adr, std::vector<uint8_t> data){
    // Validate address range. Need to check start address and end address
    // end address is start address + vector size
    check_adr_(adr);
    check_adr_(adr+data.size());

    // Set the memory pointer to the start address and iterate through input vector, storing each element in Memory
    this->set_memory_pointer_(adr);
    for(auto it = data.begin(); it != data.end(); ++it){
      this->store_byte(*it);
    }
  }
}

I have compiled it, unit tested basic functions, and confirmed it works but any advice on how to improve the quality of this code is greatly appreciated. My goal is to use this as a small piece of my portfolio so I want to make sure everything is done as well as possible.

• in the Memory c’tor, use the member initialization list to initialize mem_ptr_
• in check_adr you can skip the check for adr < 0

• Chip8.h doesn’t make use of it’s included headers. I’d move them to another file. I suspect <iostream> is used by the logger, why not putting the include there and save compilation units that don’t require iostream from the effort including the header.

• in read_short you’re calculating ram_[adr+1] & 0xFF00, which is 0 as ram_[] is uint8_t

• check which functions don’t modify the internal state of Memory and make them const, e.g. get_memory_pointer or read_byte.

• There is a curious asymmetry between Memory::store_* and Memory::read_*. The former uses mem_ptr_, while the latter does not.

• I understand that mem_ptr_ is intended to model the I register. Please realize that it belongs to the CPU class, rather than Memory.

• read_bulk returning a vector (and store_bulk taking a vector) does not look correct in this context. It seems that their purpose is emulating Fx65 (and Fx55 respectively) family of instructions. Yet again, these methods will only be called by CPU, which already knows where the data shall be read into (or written from). Just pass the pointer to the CPU regfile.