Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for NMOS quirks and CMOS extensions #38

Merged
merged 5 commits into from
Nov 23, 2023
Merged

Conversation

breqdev
Copy link
Owner

@breqdev breqdev commented Nov 23, 2023

Previously, we effectively implemented a hybrid chip with the features (and some undocumented opcodes, per #17) of the original NMOS 6502 but without some of the quirks. While most machines of the era used the NMOS chip, later models of the Apple II lineup used the 65C02 (as did other later machines) for its compatibility and increased featureset.

This PR adds a flag to the Mos6502 implementation to allow it to only run NMOS quirks (including undocumented opcodes and unintended reads) when set to behave as an NMOS chip and to enable the extra CMOS instructions if selected.

The new quirks are tested using the Klaus Dormann test suite for 65C02.

@ava-silver
Copy link
Collaborator

This PR adds a flag to the Mos6502 implementation to allow it to only run NMOS quirks (including undocumented opcodes and unintended reads) when set to behave as an NMOS chip and to enable the extra CMOS instructions if selected.

omg two chips for the price of one (feature flag) :3 😱

Copy link
Collaborator

@ava-silver ava-silver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we wanna add unit tests for this? i trust youve tested it manually but could also be neat :3

@@ -133,6 +135,10 @@ impl Execute for Mos6502 {
// ASL
let (address, cycles) = self.fetch_operand_address(opcode);
let value = self.read(address);

if let Mos6502Variant::NMOS = self.variant {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make sense to make this a march statement instead? in case there are other variants we wanna add in the future 👀 :3

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe thats premature optimization? 🍃

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait thoughts on matches!(thing, pattern) instead of the if let? probably just a stylistic thing, especially since you dont need to match on any parameters if that makes sense

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't aware of matches!() but seems like exactly what i want! i alternated between if let and == in this and wasn't rly happy with it

im a bit against a full match statement bc that would kind of imply that quirks are likely to have varying behaviors for every version, where these quirks are more just special cases for one version at a time -- e.g. if NMOS has a quirk and it was fixed in CMOS, it's unlikely to show again up in e.g. the 65816 or something later

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. you'd never really have a situation like this

match version {
  NMOS => do one thing
  CMOS => do another thing
  65816 => do a secret third thing
}

its more like, the little if statements sprinkled throughout account for each chip's separate set of errata, so something special-cased in for an NMOS quirk but skipped on CMOS wouldn't really be applicable if we added in a third generation

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeep, my 🍃 ass only realized that after i left the comment, makes total sense :3

@@ -198,6 +214,11 @@ impl Execute for Mos6502 {
// ROR
let (address, cycles) = self.fetch_operand_address(opcode);
let value = self.read(address);

if let Mos6502Variant::NMOS = self.variant {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nmos just got a lil quirky with it 🤪

@breqdev
Copy link
Owner Author

breqdev commented Nov 23, 2023

do we wanna add unit tests for this? i trust youve tested it manually but could also be neat :3

codecov looks like it lost the PR somehow :( will investigate

@breqdev
Copy link
Owner Author

breqdev commented Nov 23, 2023

fine, i'll do it myself 😼
codecov link: https://app.codecov.io/github/breqdev/noentiendo/commit/ad5810e09c9874d63843189014c2f5d4dbb1d4b6/blob/src/cpu/execute.rs

so the 65C02 extensions are tested as part of an existing test suite that gets loaded in and ran here:

#[test]
fn test_klaus_65c02() {
let roms = RomFile::from_file("bin/klaus_65C02.bin");
let platform = TextPlatform::new();
let pc = Rc::new(Cell::new(0));
let mut system = KlausSystemBuilder::build(
roms,
KlausSystemConfig {
pc_report: Some(pc.clone()),
variant: Mos6502Variant::CMOS,
},
platform.provider(),
);
for _ in 0..=100000000 {
system.tick();
}
assert_eq!(pc.get(), 0x24f1);
}

the main limitations of using someone else's test suite like this is that it doesn't cover the undocumented/illegal opcodes for the NMOS version (which has been a limitation since #17) and we can't really poke at it to see what's going on as well. but from what i can tell it's a pretty standard correctness test, and verifies that status flags etc all work fine in addition to the core instruction behavior

not opposed to writing our own unit tests for opcode implementations but IMO that engineering time could probably be better spent elsewhere on e.g. memory banking logic, which in many cases is completely untested rn -- systems module sitting at <15% coverage rn 🤪

@ava-silver
Copy link
Collaborator

makes sense! good to hear it has some tests already, the tradeoff makes total sense :3

gonna approve now!

Copy link
Collaborator

@ava-silver ava-silver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meow meow oomfie

@breqdev breqdev merged commit 382f14b into main Nov 23, 2023
5 checks passed
@ava-silver ava-silver deleted the bc/6502-variants branch December 30, 2023 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants