Starting on implementing different variants #88

omarandlorraine · 2023-08-27T20:19:12Z

No description provided.

omarandlorraine · 2023-08-27T20:51:51Z

From what I can see, the failing CI/CD process has to do with linting skeptic, a build time dependency. How then shall I proceed?

mre · 2023-08-27T20:56:35Z

Skeptic checks that the examples in the README.md are up-to-date. If these get fixed, then it should work again.

omarandlorraine · 2023-08-28T07:05:05Z

Skeptic checks that the examples in the README.md are up-to-date. If these get fixed, then it should work again.

Can you read the output of the CI / lint (pull request)? The error is not coming from the README.

error: unnecessary hashes around raw string literal
  --> /home/runner/work/mos6502/mos6502/target/debug/build/mos6502-5d795f23ad8c7099/out/skeptic-tests.rs:53:10
   |
53 |   {}"####, r####"use mos6502::memory::Bus;
   |  __________^
54 | | use mos6502::memory::Memory;
55 | | use mos6502::instruction::Nmos6502;
56 | | use mos6502::cpu;
...  |
83 | | }
84 | | "####);
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_raw_string_hashes
   = note: `-D clippy::needless-raw-string-hashes` implied by `-D warnings`

omarandlorraine · 2023-08-28T08:26:35Z

Okay, I've sussed it.

Clippy is being run with --all-targets, which gets it to check all targets. But skeptic is unmmaintained, so it's beginning to fall behind. I suggest we remove either skeptic or --all-targets.

Which do you prefer?

omarandlorraine · 2023-08-28T10:38:57Z

I have disabled --all-targets for the time being, the tests are passing now

mre · 2023-08-28T12:11:39Z

Thanks. We can remove skeptic then. I hope there's a replacement of some sort.

omarandlorraine · 2023-08-28T15:24:57Z

We can remove skeptic then. I hope there's a replacement of some sort.

Do you want to find one? Or shall we remove the example(s) from the README and refer readers to the examples/ folder?

omarandlorraine · 2023-09-08T10:41:33Z

Thanks. We can remove skeptic then. I hope there's a replacement of some sort.

There is a replacement; cargo doctest can do it now. I'll open another pull request to bring the compile times down by removing skeptic and num, which should close #89 as well.

mre · 2023-09-08T11:06:22Z

Perfect. Thanks!

omarandlorraine · 2023-09-08T11:14:55Z

Perfect. Thanks!

You ready to merge the variety branch then, or? Still want to do the review?

mre · 2023-09-08T12:47:04Z

Oh, sorry, I was referring to your comment about removing skeptic and nom. 😅
Still have to do the review of this PR.

omarandlorraine · 2023-10-29T14:26:10Z

We should revert c87975e and then rebase this pull-request after #93 is in

This reverts commit c87975e.

mre · 2023-10-30T01:23:12Z

We should revert c87975e and then rebase this pull-request after #93 is in

It's in now. Feel free to revert. 👍

omarandlorraine · 2023-10-30T06:40:12Z

It's in now. Feel free to revert. 👍

That commit has been reverted and the PR has been rebased; so this is ready for merging as far as I am concerned

mre · 2023-10-30T14:18:33Z

src/instruction.rs

+pub struct Nmos6502;
+
+impl crate::Variant for Nmos6502 {
+    fn decode(opcode: u8) -> Option<(Instruction, AddressingMode)> {


This part is so nice.

mre · 2023-10-30T14:19:07Z

src/lib.rs

@@ -40,3 +40,12 @@ pub mod cpu;
 pub mod instruction;
 pub mod memory;
 pub mod registers;
+
+pub trait Variant {


Some doc comments would help here.

Just a basic description that there are different variants of the CPU, and that they handle operations differently.

mre · 2023-10-30T14:22:05Z

README.md

@@ -52,7 +53,7 @@ fn main() {
        0x4c, 0x10, 0x00, // Jump to .algo
    ];

-    let mut cpu = cpu::CPU::new(Memory::new());
+    let mut cpu = cpu::CPU::new(Memory::new(), Nmos6502);


Maybe we could add a Default impl for that?

// Initializes an Nmos6502 variant let mut cpu = CPU::default();

If we don't want to add a default, we can alternatively provide

let mut cpu = CPU::nmos();

Just an idea.

I couldn't figure out how to do Default because of the generics; do you have a concrete idea?

mre · 2023-10-30T14:22:46Z

Looks good. Just some minor comments. We can merge if you like as there are no blockers. Up to you. 😄

omarandlorraine · 2023-10-30T20:06:06Z

Great, are you happy to merge and do a release on crates.io?

mre · 2023-10-31T15:45:22Z

Sure!

mre · 2023-10-31T15:46:44Z

Done: https://crates.io/crates/mos6502/versions

omarandlorraine force-pushed the variety branch from b685829 to d914687 Compare August 27, 2023 20:46

omarandlorraine force-pushed the variety branch from 67cac0d to fb53ae4 Compare August 28, 2023 07:24

omarandlorraine requested a review from mre August 28, 2023 15:54

omarandlorraine force-pushed the variety branch from 2cfcf60 to 032f80e Compare August 28, 2023 18:09

omarandlorraine mentioned this pull request Oct 29, 2023

use cargo test now instead of skeptic to run tests #93

Merged

omarandlorraine mentioned this pull request Oct 29, 2023

remove num #94

Closed

Sam M W added 7 commits October 29, 2023 15:22

start on separating 6502 variants from cpu itself

4a2d15f

add a single variant: the NMOS one

f4d88c7

get examples & tests running again

1f19384

Add the Revision A variant, one that has no ROR

9358ab8

disable failing lint in build-time dependencies

877f83a

Variant with no decimal mode

030e6c1

Revert "disable failing lint in build-time dependencies"

09b78be

This reverts commit c87975e.

omarandlorraine force-pushed the variety branch from bd57275 to 09b78be Compare October 29, 2023 15:22

mre reviewed Oct 30, 2023

View reviewed changes

Sam M W and others added 3 commits October 30, 2023 19:47

some doc comments

26ea6e0

Merge branch 'mre:master' into variety

4c58753

specify the variant in unit test now the API has changed

73e4896

mre merged commit 11499b6 into mre:master Oct 31, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Starting on implementing different variants #88

Starting on implementing different variants #88

omarandlorraine commented Aug 27, 2023

omarandlorraine commented Aug 27, 2023

mre commented Aug 27, 2023

omarandlorraine commented Aug 28, 2023

omarandlorraine commented Aug 28, 2023

omarandlorraine commented Aug 28, 2023

mre commented Aug 28, 2023

omarandlorraine commented Aug 28, 2023

omarandlorraine commented Sep 8, 2023 •

edited

Loading

mre commented Sep 8, 2023

omarandlorraine commented Sep 8, 2023

mre commented Sep 8, 2023

omarandlorraine commented Oct 29, 2023 •

edited

Loading

mre commented Oct 30, 2023

omarandlorraine commented Oct 30, 2023

mre Oct 30, 2023

mre Oct 30, 2023

mre Oct 30, 2023

mre Oct 30, 2023

omarandlorraine Oct 30, 2023

mre commented Oct 30, 2023

omarandlorraine commented Oct 30, 2023

mre commented Oct 31, 2023

mre commented Oct 31, 2023

Starting on implementing different variants #88

Starting on implementing different variants #88

Conversation

omarandlorraine commented Aug 27, 2023

omarandlorraine commented Aug 27, 2023

mre commented Aug 27, 2023

omarandlorraine commented Aug 28, 2023

omarandlorraine commented Aug 28, 2023

omarandlorraine commented Aug 28, 2023

mre commented Aug 28, 2023

omarandlorraine commented Aug 28, 2023

omarandlorraine commented Sep 8, 2023 • edited Loading

mre commented Sep 8, 2023

omarandlorraine commented Sep 8, 2023

mre commented Sep 8, 2023

omarandlorraine commented Oct 29, 2023 • edited Loading

mre commented Oct 30, 2023

omarandlorraine commented Oct 30, 2023

mre Oct 30, 2023

Choose a reason for hiding this comment

mre Oct 30, 2023

Choose a reason for hiding this comment

mre Oct 30, 2023

Choose a reason for hiding this comment

mre Oct 30, 2023

Choose a reason for hiding this comment

omarandlorraine Oct 30, 2023

Choose a reason for hiding this comment

mre commented Oct 30, 2023

omarandlorraine commented Oct 30, 2023

mre commented Oct 31, 2023

mre commented Oct 31, 2023

omarandlorraine commented Sep 8, 2023 •

edited

Loading

omarandlorraine commented Oct 29, 2023 •

edited

Loading