SergioLuis / CHIPEIT

A CHIP-8 emulator for Android, written in Kotlin
GNU General Public License v3.0
3 stars 1 forks source link

PR Task 002 #20

Closed SergioLuis closed 6 years ago

SergioLuis commented 6 years ago

The general execution infrastructure is in place. The code is pretty self-explanatory, but requires the reader to be familiar with how the original system worked. You can check out the online available documentation.

Right now, the emulator is able to load a ROM and start the execution (single-threaded). The JP addr, CLS and LD Vx, Byte instructions are already implemented and tested as an example on how to do so.

@jhvargas3112 @ugedo please get familiar with the code and ask any doubts you have. I'll divide the instruction set soon and everybody will get to work on this core.

I created the Pull Request in order to get a formal review on the code. Some work is still to be done (for example, loading an snapshot), but I think we should leave that for later, as this task has gotten out of hand in invested hours terms.

SergioLuis commented 6 years ago

First, my responses to some of the comments:

IMemory uses an Integer as the index of the get and set operations.

The PaddedMemory class is only to avoid other less elegant solutions such as changing the PC on each call. The specialized classes are there to take advantage of ByteArray and ShortArray, which are converted to byte[] and short[] instad to a generic Array<Byte> or Array<Short>. If we didn't use specialized classes, we could get rid of ByteMemory and ArrayMemory and just use a generic Memory<T> : IMemory<T>. Because we rely on the interface instead of on the implementation, we can later change this effortlessly.

Fixed the EOF of IMemory.kt and README.md at bc87cce

A renderer is not a window/screen/surface. Is a renderer.

Correct.

Clear should be called ever before calling any rendering functions.

Incorrect. There is an specific instruction to clear the display, is none of the emulator bussiness.

This allows implementing things like double buffering, vsync and such.

That is none of the emulator bussiness. The client can implement a double-buffer just with the render method.

Maybe we don't need a renderer interface in first place

What do you suggest to issue draw commands in a platform-agnostic way?

Converted the function getMsLeft to the read-only property msLeft at 81d0c04.
Renamed the function init to reset at eb56498

About this:

...should be stated if calling init/reset before using the object is recommended or required.

I think it should be REQUIRED before entering the trigger-sleep while-true loop. BUT, as it is an internal part, I don't want to add a way to enforce it.

I would prefer to rename every ms appearance to milliseconds (for consistency), but I complied with the review comment at 7c3cd2b

Applied review comments at 76431e2

Simplified instruction reading at 9b9c4d8

Renamed the byte val to b at a1b6015


Also: