jon-axon / chipolata Goto Github PK

The current Chipolata GUI is little more than a PoC that demonstrates the use of the egui UI library combined with Rust threading to execute a Chipolata program hard-wired into the application code.

A MVP worthy of a v1.0 label should:

• Renders the Chipolata display to a robust window that is either resizable or can be configured at startup
• Provide a graphical method for loading and executing programs from files
• Allows program execution to be paused and resumed, stopped and restarted from fresh, or for a new program to be loaded
• Provide a graphical method for specifying Chipolata configuration options, and loading/saving these from files
• Provide a mechanism to allow the user to specify and persist key-bindings (ideally GUI-based)
• Allows Chipolata processor speed to be increased and decreased during program execution (either through keys or GUI)
• Gracefully handles errors bubbling up from the Chipolata back-end (i.e. doesn't crash the GUI)

At present both the CI (main) and CI (dev) workflows are using Windows runners - at twice the cost of Linux runners - due to unexpected errors occurring when using ubuntu-latest.

I have not looked into this but am not aware of any reason there should be a problem here; this should be investigate as ideally these jobs should run on Linux.

Although the Chipolata sound timer is fully-implemented and working in the back-end, currently no attempt is made to check for the state of this and play audio accordingly within the Chipolata GUI.

This is likely to require inclusion of a 3rd party crate to play audio.

Error handling is currently handled fairly robustly from a pattern perspective, with the crate-specific Error enum used to differentiate multiple variants of internal error, and all Chipolata back-end functions bubbling-up any such errors to the hosting application. However at present the enum variants contain no data fields that hold details of the actual errors; all that's reported is the type of error e.g. "invalid operands" (but without saying what the operands are. This should be expanded:

• Data pertinent to the specific error should be added to the enum
• Surrounding context (e.g. CPU cycles) should also be added

3rd party test cases are currently failing; this needs investigating and fixing

Current Chipolata has only ever been compiled for the x64 Windows platform, however there should be nothing intrinsic to prevent this from compiling for other common architectures. At the very least it should offer binaries for Linux and MacOS.

The original COSMAC was limited to 2KB memory (not all of which was addressable), although 4KB was also an option. Currently CHIP-8 emulation mode allows 4KB of memory, but the EmulationLevel::Chip8 enum variant could be enhanced with a field to allow toggling between the two when instantiating the Chipolata Processor, for maximum flexibility.

These fields are 16-bit but for the original CHIP-8 interpreter they were 12-bit, so for ideal emulation in CHIP-8 mode we should throw an error if an attempt is made to assign to/increment these registers beyond the 12-bit max.

This will be most easily achieved by replacing direct assignment to the fields with a setter method that specifically checks for emulation mode and checks for 12-bit overflow if appropriate.

It would be interesting to understand how much work is required to create a browser-based WebAssembly version of Chipolata, to complement the current desktop app.

There appear to be two different behaviour in the wild for FX1E, which are mutually exclusive, yet there are examples of ROMs that require each of these to run correctly.

Apparently the original COSMAC did not alter vF during execution of this instruction, but some later interpreters set vF to either 1 or 0 depending on whether I overflows beyond normal addressable memory during the operation.

The game Spacefight 2091! relies on the latter behaviour to function correctly, however the demo Sens8tion relies on the former.

I have not seen this behaviour implemented as an option/quirk in any other interpreter, consequently other common interpreters e.g. Octo seem to only handle one form or the other (e.g. Octo doesn't render Spacefight 2091! correctly). It would be nice to implement and surface this as a configurable compatibility option in Chipolata, to allow user mandate of either behaviour.

3rd party test cases for strict CHIP-8 emulation are currently failing on the DXYN instruction, with regards to waiting for a display interrupt before rendering. This should be investigated and fixed.

While adding SUPER-CHIP high-resolution support, the codebase relating to execution of DXYN instructions and associated manipulation of the bitmapped frame buffer became very ugly and unwieldy; this can and should be greatly simplified. This should ideally rationalise all the following methods (plus any helpers):

Processor::execute_DXYN()
Processor::execute_DXYN_chip8()
Processor::execute_DXYN_superchip11_low_res()
Processor::execute_DXY0_superchip11()
Processor::execute_DXY0_superchip11_low_res()
Display::draw_sprite()

As part of this, associated unit tests should be re-worked and extended, as coverage here is also weaker than in other places in the codebase.

At the moment, the CI workflows are limited to build the development and main codebases, testing them, and - in the case of main only - rebuilding and publishing the rustdoc documentation hosted on GitHub Pages.

The act of creating an actual release, uploading artefacts etc is currently manual. However it should be possible to also automate this and tag these steps on to the end of a push to main.

Investigate how use Github actions (or any other appropriate CI/CD mechanism) to automatically execute unit tests when adding code to the repo.

Currently, the DXYN instruction timing (when in variable instruction cycle mode) is implement just as a random value within the min and max cycle range of the original COSMAC VIP interpreter. However it should be possible to do better than this, as the actual cycles depends specifically on what is being drawn (rather than a random factor). Whether or not this is worth the potentially significant effort to implement is a moot point, but this should be looked at and evaluated as a possible enhancement to improve emulation accuracy.

Tests have shown that std::thread::sleep() is not sufficiently reliable for simulating specific processor speeds, due to its poor resolution and accuracy of sleep timing for very short sleep durations (millisecond to microsecond granularity). Consequently, at present Chipolata's Processor::execute_cycle() method uses an active busy-waiting/spin loop with std::time::Instant::elapsed() time checks to get as close as possible to the desired wait times.

Whilst effective, this is not ideal in that the thread keeps the processor busy while spinning; sleeping would be preferably from an efficiency perspective.

The following 3rd party crate potentially addresses this, and should be investigated as an alternative approach:
https://crates.io/crates/spin_sleep/1.1.1

We must be careful, however that little/no sleep timing resolution is lost with this approach, otherwise processor speed approximation will again become inaccurate (the existing unit test processor::tests::test_processor_speed() should detect this; it is currently configured to allow a 2% deviation of measured simulated processor speed from specified processor speed).

Even the v0.1 pre-release codebase is in pretty good shape in terms of doc comments, but these should be thoroughly reviewed (for both back-end and front-end crates) prior to v1.0 release to ensure consistency, accuracy, and comprehensiveness of code documentation.

In addition to the README and rustdoc html documentation (automatically published here via CI), more general documentation should be produced within the Wiki. This should include, but not necessarily be limited to:

• Codebase organisation (crates, modules, relationships between structs etc)
• 3rd party dependencies
• Fundamental key design/architecture decisions e.g. threading approach
• Possible future extensions/roadmap

The Chipolata emulator library allows persistence of options sets to disk and easy re-loading, however there are also elements of the UI itself that can and should be configurable but currently are not. For example:

• The range of the CPU cycle/s (target) slider is hardcoded to 100-10,000 hz
• The foreground and background colours can be configured at runtime but cannot be saved; they always revert to defaults open re-loading the Chipolata exe
• The keymap is hardcoded into the binary code (and as such is probably unusable on anything but a QWERTY keyboard)

So, the UI should be enhanced to provide a mechanism for the above to all be configured and persisted to disk, with user-defined default values then loaded automatically along with Chipolata.

It should be possible for Option configurations to be stored in files, and for a hosting application to instantiate the struct directly from this by passing the filename to a constructor method.

Similarly, a method for persisting configured Option instances to disk should be included, to allow a user within a hosting application to set up a series of preferred options then persist these for use in later sessions.

Current v0.1 GUI code has functional message passing implemented for communication between the UI and back-end threads, however this is very much proof-of-concept code, and should be reviewed and refined. In particular:

• Can the number of channels be reduced (currently 3 are used)?
• Can the messages passed be made more robust (e.g. inclusion of new types rather than primitives)?

Although sound support is fully-enabled in the Chipolata back-end (via the sound timer and associated methods), the UI currently does nothing to actual play audio, so this functionality must be added.

Unit tests should be added to check for correct flag/result setting for these instructions. Specifically to check the behaviour when VF is passed as the VX argument.

Original CHIP-8 display resolution is 64 x 32 pixels, while SUPER-CHIP is 128 x 64 pixels.

However there is apparently also a "high res" CHIP-8 mode in-between the two, at 64 x 64 pixels.

The origin of this should be investigated, and an assessment made as to whether it is worth adding support for this mode into Chipolata (and how difficult or otherwise that might be).

Currently the variable cycle timing flag is part of the Options struct and as such can be set independently from the CHIP-8/48/SUPER-CHIP EmulationLevel.

However this is problematic, as SUPER-CHIP support will introduce SUPER-CHIP-only instructions, however by definition no original COSMAC VIP timings exist for these. We could choose to simply invent a timing for these, or set them to 0 cycles etc, however it would be more faithful to the original intention of the different emulation level support to ONLY enable the COSMAC VIP variable cycle timings when in CHIP-8 emulation mode.

So, the change should be to move the Options::use_variable_cycle_timings flag inside the EmulationLevel::Chip8 enum variant.

A fairly significant enhancement (although modified SUPER-CHIP 1.1 behaviour of existing CHIP-8 instructions is already handled through the appropriate emulation mode). Required component additions alterations are well-documented e.g. http://johnearnest.github.io/Octo/docs/SuperChip.html and https://chip-8.github.io/extensions/#super-chip-10.

This should be achieved through extension of the existing code branching based around the EmulationLevel enum specified when instantiating the Chipolata Processor.

Note that to correctly handle RPL user flags - which in the original implementation were persisted between program executions - will require refactoring of the design and so may be better handled as a later, separate enhancement.

Instruction timings can be found here: https://jackson-s.me/2019/07/13/Chip-8-Instruction-Scheduling-and-Frequency.html.

Currently all the execute_<opcode>() methods within Processor return unit, but could be modified to calculate and return microseconds timings as per the above link. The execute() method would then bubble this back up to execute_cycle(), which in turn could use these timings within the spin loop at the end of the method rather than basing the wait on the simulated processor speed specified when instantiating Processor.

This is probably most elegantly handled from a config perspective by adding this as a boolean field to the EmulationStatus::Chip8 enum variant; when set to true this would then bypass any specified processor speed option and use the COSMAC timings instead.

3rd party test cases are currently failing in relation to key presses; this whole area should be investigated and fixed as appropriate.

Documentation regarding the original CHIP-8 implementation of opcodes 8XY6 and 8XYE is a little ambiguous with regards to whether Vy is modified by the shift operation or not. Current Chipolata behaviour in EmulationLevel::Chip8 mode assumes that Vy is NOT modified, but the following discussion confirms this is incorrect:

mattmikolay/chip-8#4

This should be fixed for accurate COSMAC VIP CHIP-8 emulation.

It should be possible for a hosting application to pass a filename to Program as part of a constructor method and have the struct instantiated directly from this.

Also currently no use for this is planned, it would also be nice for future use to include a similar file-writing method, which may have application in a fully-fledged editor/debugger at some future point, where a program could be modified on-the-fly and written back to disk.

Chipolata implements all opcodes added in SUPER-CHIP 1.0 and 1.1, including FX75 and FX85 that relate to saving and loading the CHIP-8 general purpose registers to the "RPL user flag" registers on the HP-48 platform for which SUPER-CHIP was originally built.

However, as documented well here, there is an added nuance to this that Chipolata does not currently handle, which is that RPL user flags were persisted on the HP-48 between sessions and even between different programs.

This is an added element of authenticity that Chipolata could also implement, rather than resetting these flags ever time Chipolata is restarted (for example by persisting their values to disk rather than to memory).

This is a placeholder issue and should be replaced by/broken down into a series of smaller enhancements in due course.

Although currently unused, the Chipolata library crate already exposes through the StateSnapshot::ExtendedSnapshot enum variant all the information that would be required for the UI to provide full de-bugging information to the user. So, combined with the existing capability to pause and restart execution and run cycle-by-cycle when required, there should already be sufficient support within the library to facilitate a fully-fledged debugging UI (perhaps without the ability for the user to manipulate state).

Thought should be given to how to best represent this from a UI "real estate" perspective, what the overall scope should be, and whether to incrementally add these capabilities or to fold into a single "debugging update" release.