Fixing Nestest failiures

Fixing Nestest failiures
by aphex on 2010-07-20 (#64505)

Hi everyone
Here is a screenshot of the nestest.nes rom completed in my emulator

As you can see, many of the tests fail, but the test numbers that are indicated are mostly all just general failiures, such as "SBC # Failiure", so it's difficult to track down exactly what is wrong. I'll start with the first error, 73. This is listed here as an SBC # Faliure. Here is my SBC instruction as it stands at the moment.

Code:

void NesMainClass::_SBC(u8 toSub, int cycles) 
{
   cpuCycles += cycles;

   // Store the A register 
   u8 beforeSub = regs->A;   

   // Do the subtraction
   regs->A -= toSub;

   // Subtract one more if carry is clear
   if (!TestBit(regs->P, FLAG_C))
      regs->A--;

   // Is the Negative/Sign bit set?
   regs->P = (TestBit(regs->A, 7)) ? 
      SET_N_FLAG : CLR_N_FLAG;

   // Is the result zero?
   regs->P = (regs->A == 0) ? 
      SET_Z_FLAG : CLR_Z_FLAG;

   // Was there a borrow
   regs->P = (beforeSub - toSub >= 0) ? 
      SET_C_FLAG : CLR_C_FLAG;

   // The Overflow flag works by indicating whether there was a sign overflow
   // i.e The result was outside the range of -128 to 127 
   regs->P = (regs->A > 127 || (s8)beforeSub + TestBit(regs->P, FLAG_C) -1 - (s8)toSub < -128) ?
      SET_V_FLAG : CLR_V_FLAG;
}   

Assuming all the various addressing modes are correct, the only problem i can think of is that the overflow being set for a signed underflow as well. (ie should only be set if the result is more that 127 and not less than -128 as well? Im not sure...). I really can't see the problem here, can anyone else spot any problems with this code?

thanks

by pdq on 2010-07-20 (#64516)

It's much easier if you grab the golden nestest log from here and compare your CPU state against it, instruction by instruction.

NOTE: you will need to set your PC to $C000 (ie bypass the reset/power interrupt vectors).

http://wiki.nesdev.com/w/index.php/Emulator_tests

by James on 2010-07-20 (#64524)

This topic comes up every once in a while and I'm always tempted to reply with my take on it. Here goes:

First of all, I don't understand how you're setting the processor flags (looks like regs->P can be overwritten by subsequent tests). In any event, I think your overflow logic is off.

The overflow flag is set when the result is outside of the signed char range (i.e., > 127 or < -128). With SBC, an overflow can only happen when the signs, and hence the high bit, of the operands differ. A positive number subtracted from a positive number, or a negative number subtracted from a negative number, will never overflow. Examples:

5 - 127 = -122 (no overflow)
-5 - -128 = 123 (no overflow)
5 - -128 = 133 (overflow -- signed result is -123, so high bit is 1)
-5 - 127 = -132 (overflow -- signed result is 124, so high bit is 0)

Plug in various numbers and you'll see that you can't make 2 same-signed numbers overflow.

With this knowledge, you can implement the following logic: SBC performs A - M - (carry ? 0 : 1). If the signs (high bit) of A and M differ and the signs of A and the result differ, then overflow has occurred:

overflow = (A^M) & (A^result) & 0x80 ? true : false;

For ADC (A + M + (carry ? 1 : 0)), only 2 positive, or 2 negative, numbers can result in overflow. Therefore, you need to test that the signs of both A and M differ from sign of the result:

overflow = (A^result) & (M^result) & 0x80 ? true : false;

Hope this helps,
James

by aphex on 2010-07-20 (#64533)

@pdq

I have already done this, and I actually have programmed my emulator to output an identicaly formatted log file to the nestest.log file. Although, the contents is different at this point obviously.

@James

To make things clearer, take this for example (but you're right lol)

Code:
regs->P = (TestBit(regs->A, 7)) ? SET_N_FLAG : CLR_N_FLAG;

SET_N_FLAG is a macro which is defined like so.

Code:
(regs->P |= 0x80)

CLR_N_FLAG is a macro which is defined like so.

Code:
(regs->P &= 0x7F)

So, to put another way, i'm simply saying.

Code:
if (TestBit(regs->A, 7))
{
regs->P = (regs->P |= 0x80);
}
else
{
regs->P = (regs->P &= 0x7F);
}

...and you know what? You just made me realise that I am not setting the flags wrong, but i am doing something pointless. The most logical way to do this would be like this.

Code:
(TestBit(regs->A, 7)) ? SET_N_FLAG : CLR_N_FLAG;

Sorry about that (and for going on about it), I just recently re-wrote all the flag logic so i missed that, thanks for pointing it out. I was basically assigning regs->P to itself for no reason...

Anyway, as for my SBC problem, thanks alot, that seemed to fix my problem. Here is my new flag test in SBC

Code:
((beforeSub^toSub) & (beforeSub^regs->A) & 0x80) ?
   SET_V_FLAG : CLR_V_FLAG;

Although, now its replaced with another problem, "035h - CPX # failure (messed up flags)".

Any ideas? I can't figure it out. My flags seem fine, here is my CMP instruction.

Code:
void NesMainClass::_CMP(u8 cmpReg, u8 toCmp, int cycles)
{
cpuCycles += cycles;

cmpReg -= toCmp;

// Is the Negative/Sign bit set?
(TestBit(cmpReg, 7)) ?
   SET_N_FLAG : CLR_N_FLAG;

// Is the result zero?
(cmpReg == 0) ?
   SET_Z_FLAG : CLR_Z_FLAG;

// Should we set the carry?
((s8)cmpReg >= 0) ?
   SET_C_FLAG : CLR_C_FLAG;
}

Here is where the immediate CPX is called.

Code:
case 0xE0: _CMP(regs->X, readMem(regs->PC), 2); regs->PC++; break;

Any ideas? Im sure my flags are correct in my cmp instruction, ill keep checking.

Thanks alot for your help

by aphex on 2010-07-20 (#64534)

Ok, i think i got the CPX flags problem fixed.

Code:
void NesMainClass::_CMP(u8 cmpReg, u8 toCmp, int cycles)
{
cpuCycles += cycles;

// Is the Negative/Sign bit set?
(TestBit(cmpReg-toCmp, 7)) ?
   SET_N_FLAG : CLR_N_FLAG;

// Is the result zero?
(cmpReg-toCmp == 0) ?
   SET_Z_FLAG : CLR_Z_FLAG;

// Should we set the carry?
(cmpReg-toCmp >= 0) ?
   SET_C_FLAG : CLR_C_FLAG;
}

Now im getting an ADC error (my emulator is so buggy lol). Im getting "022h - ADC # failure". Here is my ADC instruction

Code:
void NesMainClass::_ADC(u8 toAdd, int cycles)
{
cpuCycles += cycles;

// Store the A register so we can check if there was a carry
u8 beforeAdd = regs->A;

// A reg = A reg + Carry + some value
regs->A = (regs->A + TestBit(regs->P, FLAG_C) + toAdd);

// Is the Negative/Sign bit set?
(TestBit(regs->A, 7)) ?
   SET_N_FLAG : CLR_N_FLAG;

// Is the result zero?
(regs->A == 0) ?
   SET_Z_FLAG : CLR_Z_FLAG;

// Was there a carry
((beforeAdd + toAdd) > 255) ?
   SET_C_FLAG : CLR_C_FLAG;

// Set overflow?
((beforeAdd^regs->A) & (toAdd^regs->A) & 0x80) ?
   SET_V_FLAG : CLR_V_FLAG;
}

I'll keep seeing if i can fix it, but open to any ideas. Thanks

by pdq on 2010-07-20 (#64537)

If you have your nestest log, then you should just post the failing instruction along with the golden one here. It's much more obvious as to exactly what you got wrong.

Your carry seems borked because a u8 - u8 is always >= 0. Have you tried this instead?

Code:
(cmpReg>=toCmp) ?
SET_C_FLAG : CLR_C_FLAG;

by aphex on 2010-07-20 (#64540)

Quote:
a u8 - u8 is always >= 0

I beg to differ, in C++ if you to the sum 10 - 15, you will get -5, unless you typecast the result of course. For example, this will evaluate as true.

Code:
if (10-15 < 0)

As for the nestest log, I will show you the first part that isn't the same. (ignoring scanline and small cycle differences)

Golden nestest.log, Starting at line 102

Code:
C824 48 PHA A:FF X:00 Y:00 P:AD SP:FB CYC: 86 SL:243
C825 28 PLP A:FF X:00 Y:00 P:AD SP:FA CYC: 95 SL:243
C826 D0 09 BNE $C831 A:FF X:00 Y:00 P:EF SP:FB CYC:107 SL:243

My nestest log, starting at line 102

Code:
C824 48 PHA A:FF X:00 Y:00 P:AD SP:FB CYC:90 SL:1
C825 28 PLP A:FF X:00 Y:00 P:AD SP:FA CYC:102 SL:1
C826 D0 09 BNE $C831 A:FF X:00 Y:00 P:FF SP:FB CYC:108 SL:1

As you can see, A is pushed on to the stack (which contains FF), then it is pulled from the stack in to the flags register, (which should now = FF right?). In the golden nestest log, even though FF is pushed on to the stack (PHA) and that byte is pulled back in to the flags register, EF appears in the flags register. ie the brk flag is unset.

Is this just my incompitance in not handling the brk flag correctly? Should the brk flag only be set by the brk instruction and never be set b any other instruction?

by pdq on 2010-07-20 (#64545)

Quote:
Is this just my incompitance in not handling the brk flag correctly? Should the brk flag only be set by the brk instruction and never be set b any other instruction?

Yes, in fact you shouldn't even implement the B flag in your P reg (it's always zero). However, when P is pushed onto the stack for BRK or PHP, the byte is ORed so that B is set.

by blargg on 2010-07-20 (#64546)

First thing you need to do is put your log and the nestest log in the same format. In your case, I'd strip off all the timing information after the registers. Then use a text file comparison tool that shows mismatching lines. Doing it manually is a very bad idea. You'll miss differences and spend lots of time doing something humans aren't suited for.

EDIT: whoops, they are in the same format. The wrapped lines threw me off.

by aphex on 2010-07-20 (#64547)

Hi Blargg
Both logs are basically in the exact same format now, and I'm actually making some great progress since my first post, I now pass 10 of the 13 tests and lots of games that didn't boot before are starting to work.

Something I realised is that the Indirect Indexed instructions such as LDA (nn,X), suffers from the same bug as JMP Indirect when fetching the high byte of the word addres from past a page boundry. This isn't documented anywhere as far as I'm aware, maybe it should be put on the nesdev wiki?

Anyway, your advice is good and has come to mind to me too, but will be more useful when I have ironed out more bugs as the differences in the two files are fairly easy to track down at the moment. This is mainly due to the fact that they are "fairly" small in size and that the errors are pretty much contiguous at the moment.

I will likely find more issues with my instruction set so i'll keep posting in this thread if need be.

Thanks for all your help everyone

by blargg on 2010-07-20 (#64548)

Whoops, you're right. The line-wrapping of the logs threw me off.

It's documented in most 6502 references that zero-page addressing wraps around. LDX #1 : LDA $FF,X will load from $0000, not $0100, for example.

Once you pass nestest, be sure to run my instruction tests as well (the other ones on the Wiki page).

by aphex on 2010-07-21 (#64582)

Blargg, I tried your instruction tests as well, but something is going very wrong in my emulator. Here is a screenshot of the rom single "01-implied.nes", rom. (same story for all the rom singles)

After a short time, I hit an illegal instruction. I'm positive it's some memory writes to the nametables that are going wrong here and not my scanline rendering routine. I'll have to resolve my final two errors in your nestest.nes rom first, but have you got any ideas about which instruction(s) might be causing this, as this is a common problem with a fair few games. On the bright side, I passed 11 out of the 13 tests now.

by blargg on 2010-07-21 (#64607)

As stated on the Wiki, it's best to get nestest passing first. Since you have an instruction log, it's easy to figure out why it's failing. After that's passing all the official instructions at least, then move on to mine. If your PPU isn't working, take a look at the readme, which explains other ways you can read the result. For one, there's audio beeps. If your APU doesn't work, then you can examine $6000-$7FFF to obtain an ASCII version of the output, that lists failed instructions.

by aphex on 2010-07-21 (#64612)

Quote:
As stated on the Wiki, it's best to get nestest passing first.

Yes, I will get the last two tests fixed (hopefully) first before I move on to other test roms.

As for my PPU emulation, the only problem (optimistically) is that incorrect data is been written to the nametables. As you said though, i would need to make sure that I'm passing all the basic instruction tests before I should go any deeper in to these more specific problems.

I'm just getting into APU emulation, so I'll probably be asking some questions on this soon, although sound emulation isn't as scary as I once thought it was

Thanks Blargg

by aphex on 2010-07-22 (#64630)

My emu finally passed all the tests

Thanks so much for all the help, this is a major mile stone for me as alot of the games are booting and playing fine, and of course, thanks for writing the nestest Blargg.

Now, the other instruction tests are running ok, although, I am getting lots of errors. for example, here is the rom single, "01-implied.nes"

Now all these are showing that there is some sort of problem with those instructions, correct?

by blargg on 2010-07-22 (#64640)

Every instruction failed, so it's some instruction or behavior that's causing them all to fail. I noticed I hadn't uploaded a newer version of the test that's more robust to emulator errors like this. instr_test-v2. This uses a pre-computed CRC table and makes extra sure the APU IRQ is turned off, even if an emulator handles it wrong in some ways.

by Zepper on 2010-07-22 (#64645)

All 15 tests passed. Sweet.

by comegordas on 2010-08-29 (#66927)

Blargg, and what about this?

Code:
DB9E 8D 00 03 STA $0300 = 01 A:A9 X:07 Y:00 P:E5 SP:FB CYC:114 SL:63
DBA1 A9 AA LDA #$AA A:A9 X:07 Y:00 P:E5 SP:FB CYC:126 SL:63
DBA3 8D 01 03 STA $0301 = 00 A:AA X:07 Y:00 P:E5 SP:FB CYC:132 SL:63
DBA6 A9 60 LDA #$60 A:AA X:07 Y:00 P:E5 SP:FB CYC:144 SL:63
DBA8 8D 02 03 STA $0302 = 00 A:60 X:07 Y:00 P:65 SP:FB CYC:150 SL:63
DBAB 20 B5 DB JSR $DBB5 A:60 X:07 Y:00 P:65 SP:FB CYC:162 SL:63
DBB5 6C FF 02 JMP ($02FF) = A900 A:60 X:07 Y:00 P:65 SP:F9 CYC:180 SL:63
0300 A9 AA LDA #$AA A:60 X:07 Y:00 P:65 SP:F9 CYC:195 SL:63

JMP ($02FF) = A900, and afther that the PC is at 0300, and not at A900. did i get wrong the indirect addressing mode?[/code]

by Dwedit on 2010-08-29 (#66928)

Indirect jump is bugged on the 6502, it doesn't add 1 to the full 16-bit value when it reads the second byte, it adds 1 to the low byte only.
So JMP (03FF) reads from 3FF and 300, not 3FF and 400.

by comegordas on 2010-08-29 (#66930)

thanks Dwedit. didn't know that. why there's SO many errors on the doc's!? read severall 6502 docs and never found that :\
is there any errorless doc that you can point me to learn the correct behavior of the addressing modes? at this point there's no doc that i can trust in

by Dwedit on 2010-08-29 (#66932)

Every ZP,X instruction does an 8-bit add, and wraps back to 00 if it exceeds FF. Hence why it's Zeropage,X. Same idea with Indirect,X addressing, it pulls the two address bytes entirely from within the zero page.

Some instructions have a one-cycle penalty for when computed addresses cross a 256-byte boundary (a 'page').
For example, lda 02FF,X takes one more cycle to execute if X >= 1, so it must adjust the high byte of the address.
Branch instructions (BNE, BEQ, etc) also have a penalty if the branch address target is on a different page than the address _after_ the branch instruction.

The document here lists what all the instructions do at each cycle:
http://nesdev.com/6502_cpu.txt

This document is also a nice summary for telling you which instructions have the cycle penalties (but has other errors, for example, is says that PLA doesn't affect flags when it does.)
http://www.masswerk.at/6502/6502_instruction_set.html

by comegordas on 2010-08-29 (#66934)

thanks!

by comegordas on 2010-08-30 (#66950)

BTW, i'm having something strange with this rom. that tile "*" is not getting showed on my emulator, and i can't figure out why.

Nestest on FCEUX:

Nestest on my emulator:

i've loaded the rom in FCEUX and in VirtuaNES and then see what the Name Table Viewer shows, and in both emulators the tile "*" is marked as background data (not as sprite data). but anyway this tile doesn't shows up on my emulator as a background tile, neither as a sprite