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

added test and added possible correction for sbc #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rijojohn85
Copy link

@rijojohn85 rijojohn85 commented Nov 6, 2023

Hi,
This is my first time submitting a pull request, so please excuse any errors.
Firstly, thank you for making this guide. It's been an amazing learning journey and I've learnt a lot.

I had written tests for all the opcodes for chapter 3.3 and I thought it would be useful to add to the repo for future users.
While testing with your code, I noticed that tests for SBC was failing. On further inspectioin, I noticed that there MIGHT be a error with your SBC code.

In the book you had mentioned

After ADC is implemented, implementing SBC becomes trivial as A - B = A + (-B). And -B = !B + 1

but I'm not sure if you implemented it in your code:
fn sbc(&mut self, mode: &AddressingMode) { let addr = self.get_operand_address(&mode); let data = self.mem_read(addr); self.add_to_register_a(((data as i8).wrapping_neg().wrapping_sub(1)) as u8); }
My tests were always failing by 1. I tried 10-10. It should have been 0, but I'm getting 255 with your code. I changed the code a bit and I was getting the code to pass the tests. It's below.

fn sbc(&mut self, mode: &AddressingMode) { let addr = self.get_operand_address(mode); let data = self.mem_read(addr); self.add_to_register_a(!data + 1); }

Let me know if this is correct, else I'll have a look again and. But I think the tests will be useful otherwise as well.

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.

1 participant