Timendus / chip8-test-suite

A collection of ROM images with tests that will aid you in developing your own CHIP-8, SUPER-CHIP or XO-CHIP interpreter (or "emulator")
GNU General Public License v3.0
359 stars 7 forks source link

Revised: added extra carry checks to the Flags UI #9

Closed jon-axon closed 1 year ago

jon-axon commented 1 year ago

This is a revision to my previous pull request: #8 , which I closed yesterday as I wanted to take a different approach, and resolves #7. I've had another stab at it now and am happier with the new implementation. Please see the previous link for the background to the issue.

This new revision is more consistent and thorough, as it applies the 4th checkmark to both the "Happy" and the "Carry" paths, rather than arbitrarily to just the latter. In order to do this it was necessary to free up extra space in the UI. When I looked at how this could be achieved, I realised that the use of the "8" prefix on all of the opcodes was quite wasteful (13 sprites, each 4 pixels tall) and largely redundant given that all the opcodes on this page except for the final FE are from the 8xxx family. So, I have removed this prefix to make room for the new flags, and leave it to the README file to make clear what the opcode family is. This has also created enough space to reinstate the full "CARRY" word and also the same opcode-to-line distribution of the original UI, which I think is preferable. It now looks like this:

image

In order to free up sufficient program memory for these changes I have:

Timendus commented 1 year ago

Okay, I've thought about this a while, and here's what I think we should do:

  1. If you can remove all the changes from this PR that are not code-related (the builds, images, readme) then I'll merge this PR. I try to keep main as valid documentation for the current release, so people don't get confused
  2. I was working on some unrelated changes to the flags test, I'll merge those into main after yours
  3. It's time to split this project into smaller binaries, and let go of the menu. Having separate binaries is easier to work with for interpreter developers and frees us from having to squeeze every new feature into the same binary size
  4. I'll then release a version 4 that's split up and contains these changes as well as my own

Does that sound alright to you?

jon-axon commented 1 year ago

Great, all sounds good to me! Quick question, then: which branch in your repo do you want me to be raising the PR against? The main reason I included all the other updated files originally was because I only saw a main branch and didn't like the idea of merging anything other than a full release into that. I see there are a couple more now though, too. So if you let me know where you want the PR to go, I'll redo it and only include the following updated files:

I think it may be the case that we can include an additional fourth check for the 8XY6 and 8XYE opcodes, which would only apply to original VIP CHIP-8 emulators where vY was shifted by these instructions: in this scenario we should probably also include a test where vF is passed as the vY argument, to ensure it's correctly mutated. This actually applies a nice uniformity across the tests, as the third and fourth checkmarks can then be consistently used for every 8xxx opcode that sets vF as a side-effect for the cases where vF is passed as the X and the Y arguments respectively. This will also still fit into the new UI design space, wise, which is good too. I would include this extra check now however will hit program size issues again I think, so if you're planning to now split up the binaries (which I agree makes sense) then it's probably more convenient to wait and add the final additional tests after that point, before the final v4 release, if you agree?

Timendus commented 1 year ago

That sounds alright! If you make a PR against main, I'll take it from there. I don't mind having newer code in main, I do mind having the built files, the release and the docs diverge.