Closed spjewkes closed 6 months ago
A git bisect shows this commit to be the start of the issue:
c3c4aa1c49edbb9ca0887437d70b062dfbff7206 is the first bad commit commit c3c4aa1c49edbb9ca0887437d70b062dfbff7206 Author: Steve Jewkes steve@mulus.net Date: Thu Mar 28 18:28:37 2024 +0000
Found issue with sbc was commented out TODO - doh
src/z80/instructions.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
I believe this change was correct to some degree as we were channelling the SBC instruction to the SUB command. So it shows that the SBC command is still not right. We might be able to debug the ROM to get this fixed. We just need to find the point in the code where we write out the copyright message.
I notice in the ROM at address 0x0BA4 there are some SBC instructions around a calculation to use the inverse mask. This might be a good place to start.
Note that the copyright message is displayed at address: 0x1299 (message itself is stored at address 0x1539)
Yes, it seems to happen at 0x0BA6. We have SBC A, A. It looks like A is set to 0 before this call. The Fuse emulator produces the result 0 but our emulator results in 0xFF.
The flags might be key. The Fuse emulator has F set to 0x40. Our emulator is set to 0x60.
I seem to have bit 5 set for some reason but it is classed as 'not used'. Both emulators have the zero bit set so I don't think there's an issue with carry when using the SBC instruction.
The reason is the implementation of impl_sub(). This inverts the src element in an attempt to negate it and then applies addition. This would explain why it doesn't work.
This would work if you properly 2s complemented 0. Which would result in 0. We could use addition in this case.
However, I noticed we're using a function called 'add_carry()' from storage element. Now we have code written for add and subtract that should properly handle the carry and overflow, I wonder if we're better off just using those.
Invert is fixed when I use sub_carry() instead. However, our unit tests are now broken. I think our subtract/addition still needs some care.
Right carry is now resolved for the subtraction case. I updated the borrow function and the z80doc tests now pass and our unit tests now pass. I'm hoping this is now finally resolved properly.
Just as a note, I ended up creating this table based on what I could gather from online source (including fuse emulator):
|Result |RHS |LHS |Overflow (add) |Overflow (sub) |Carry |Borrow |
|0 | 0 | 0 | | | | |
|0 | 0 | 1 | | Yes | Yes | |
|0 | 1 | 0 | | | Yes | Yes |
|0 | 1 | 1 | Yes | | Yes | |
|1 | 0 | 0 | Yes | | | Yes |
|1 | 0 | 1 | | | | |
|1 | 1 | 0 | | Yes | | Yes |
|1 | 1 | 1 | | | Yes | Yes |
At some point during development I've started seeing the copyright title inverted. This has happened fairly recently and we might be able to bisect the issue.