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

feat: Add implementation of INY and DEY opcode #21

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

Prabhat1308
Copy link
Contributor

@Prabhat1308 Prabhat1308 commented Mar 21, 2024

fixes the issue #20 and #25
checks out the INY and DEY opcode from #6 master instruction tracker

@goblinoats
Copy link
Collaborator

On the face it, this looks good to me. Assuming the tests pass?

@Prabhat1308
Copy link
Contributor Author

I did run nargo test on both inx and iny but the helpers directory code throws errors which #22 is supposed to fix.
I also ran

cargo run --release test_roms/cpu/nestest.nes --trace --instr_name iny

and I am successfully getting a trace.

@Prabhat1308 Prabhat1308 changed the title feat: Add implementation of INY opcode feat: Add implementation of INY and DEY opcode Mar 23, 2024
@Prabhat1308
Copy link
Contributor Author

Hi . @goblinoats I have checked and tested the code and and both opcodes are working once #22 gets merged.
There was an error in the tests in inx example where its was passing even when zero flag was not set when X = 0

fn test_0() -> Field {
    main(
        1,
        [343432, 343433, 343434, 343435, 343436, 343437, 343438, 343439, 343440],
        [8203, 79, 8203, 8203, 80, 8202, 8202, 8205, 8205],
         [49231, 200, 49232, 49232, 224, 0, 1, 36, 36], //incorrect test , since zero flag is not set before .
        [49231, 200, 49232, 49232, 224, 0, 1, 38, 36],  //corrected test
        [0, 0, 1, 0, 0, 0, 1, 0, 1]
    )
}

This has been corrected by @matthieuauger in his PR #24 .

@matthieuauger
Copy link
Contributor

Oops @Prabhat1308 just noticed we developed the same DEY opcode, I pushed it this morning following implementation of DEX yesterday evening. My bad I should have created the related issue

@goblinoats
Copy link
Collaborator

There's a small conflict for your PRs @Prabhat1308 following #22 but once you resolve that and double check it's all good I will merge them in.

@Prabhat1308
Copy link
Contributor Author

@goblinoats I have resolved the merge conflicts.

@matthieuauger
Copy link
Contributor

@Prabhat1308 in order to keep git history clean you should rebase on main instead of merging main in your branch. I can help on this if you need to

@Prabhat1308
Copy link
Contributor Author

@matthieuauger sure , would like some help

Signed-off-by: Prabhat1308 <[email protected]>

change opcode in tests

Signed-off-by: Prabhat1308 <[email protected]>
Signed-off-by: Prabhat1308 <[email protected]>

changes to dey fix

Signed-off-by: Prabhat1308 <[email protected]>
@goblinoats
Copy link
Collaborator

👍

@goblinoats goblinoats merged commit 59b0638 into tonk-labs:main Mar 25, 2024
Prabhat1308 pushed a commit to Prabhat1308/dappicom that referenced this pull request Apr 1, 2024
feat: Add implementation of INY and DEY opcode

changes to dey fix

Signed-off-by: Prabhat1308 <[email protected]>

feat: add opcodes clc cld cli clv

Signed-off-by: Prabhat1308 <[email protected]>

reolve merge conflicts

Signed-off-by: Prabhat1308 <[email protected]>

fix: merge issues

Signed-off-by: Prabhat1308 <[email protected]>
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.

3 participants