llvm-mos / llvm-mos-sdk

SDK for developing with the llvm-mos compiler
https://www.llvm-mos.org
Other
266 stars 53 forks source link

NES: Rework CNROM, UNROM and UNROM-512 to properly handle NMIs. #198

Closed asiekierka closed 11 months ago

asiekierka commented 11 months ago

Fixes https://github.com/llvm-mos/llvm-mos-sdk/issues/195 for these three mappers. I think this leaves only MMC3 as the remaining mapper without NMI handling (MMC1 and Action53 already had their own; NROM does not require it).

I've implemented my own approach to handling NMIs:

For CNROM and UNROM-512, I've extended this with additional functionality:

As this may not be immediately intuitive to someone tinkering with the code in the future, I have added a test for these two mappers which automatically validates it.

This required removing get_chr_bank on CNROM - well, it didn't strictly require it, but then it would require two RAM bytes instead of one; I expect the functionality to be sufficiently unnecessary as to not be worth the added cost. Likewise, the public API of UNROM-512 has changed; as neither API has yet shipped in a production build of the SDK, this is currently not a breaking change.

mysterymath commented 11 months ago

EDIT: I withdraw most of this; I noticed this was based off of jroweboy's work w/ Action53. I need to read that more carefully and fold this into my opinion.

asiekierka commented 11 months ago

Perhaps it'd help to get more eyeballs on the PR? There are a few NES LLVM-MOS-SDK users by now; they might have their own opinions to contribute.

mysterymath commented 11 months ago

Yeah, paging @jroweboy , if you get the chance.