kervinck / gigatron-rom

System, apps and tooling for the Gigatron TTL microcomputer
BSD 2-Clause "Simplified" License
231 stars 80 forks source link

gtemuAT67 : Implement 128K RAM and IO extension board. #189

Closed leonbottou closed 3 years ago

leonbottou commented 3 years ago

This pull request implements basic support for the banked ram extension board. This is achieved by replacing _RAM with a C++ class that does what the extension board does. I also added a couple utility function in namespace Cpu.

My next step is to add a SD card emulation code, so that the emulator can boot the Gigatron from a SD card. The hope is to enable the development of native SD support. Another next step is to produce an actual memory expansion board with a SD card slot.

p.s. -- the Gigatron is a time sink.

at67 commented 3 years ago

Thanks for the pull request, the code looks good and having the emulator be able to emulate the RAM and IO expansion board will be a handy addition.

Just a couple of queries:

1) You seem to have removed the out of bounds address masks, these were intended to stop wayward vCPU, (and even wayward C++ code), from trying to access illegal host memory. Having the emulation crash within the emulator due to bad vCPU code is acceptable, but having the emulator crash is not.

2) A lot of the C++ code that uses Cpu::swapMemoryModel() assumes that it is a toggle between 32K and 64K, adding the 128K option will definitely break logic that assumes that. I have no problem with adding 128K as an additional option, so maybe Cpu::swapMemoryModel() can be re-written to accept an enumeration, (rather than just cycling through a list), and the logic that references Cpu::swapMemoryModel() can be refactored. If that's non trivial, then maybe just a separate function to activate the 128K address space.

3) There seems to be a few & 0xFFFF masks happening in the code on uint16_t variables, is this intended as it seems to be superfluous?

P.S. If it's just strictly Contrib/at67 code that you will be modifying, then it's probably better to do the pull requests here: https://github.com/at67/gigatron-rom, this will make the merge's simpler and also keep Marcel's codebase cleaner.

leonbottou commented 3 years ago

1) The _RAM object now makes sure one does not write out of bounds. It just takes the low 16 bits of the provided address and ignores the rest. 2) I checked that swapMemoryModel() is called in only one place (in editor.cpp) and I found convenient to keep the same user interface. But I can understand you might have other plans. Not knowing what they are, I am not sure what to do. 3) I have to review the code to see what can be removed....

leonbottou commented 3 years ago

About your point (3). You're correct. I remove two useless &0xffff for uint16_t.