stella-emu / stella

A multi-platform Atari 2600 Emulator
https://stella-emu.github.io
GNU General Public License v2.0
627 stars 111 forks source link

Implement PA7 emulation #108

Open sa666666 opened 7 years ago

sa666666 commented 7 years ago

I'd hoped that the timer issues were fixed for good, but I found some old ROMs that were used for testing timer functionality. Unfortunately, the output they give on a real system doesn't match Stella for the most part.

Hardware available/tested: NTSC Light Sixer w/ svideo mod, NTSC Jr. (RF only). Hardware matches in all tests. Software tested: Latest Stella git, Stella 4.7.3 All tests in Linux.

   ROM              Hardware         Stella git        Stella 4.7.3
test1.bas.bin        192246            128246          Same as git
test2.bas.bin        064249            000249          Same as git
test3.bas.bin        192248            128248          Same as git
test4.bas.bin        192245            128245          Same as git
test5.bas.bin        192245            128245          Same as git

Note that only the first 3 digits differ, which could be a hint.

testTIMINT_flawed.bin    (0/0, same on all platforms)
testTIMINT_withDelay.bin (press joystick up/down to change delay)
The numbers separated by '/' specifies BEFORE/AFTER/DELAY

Hardware         Stella git        Stella 4.7.3
0/80/0            0/0/0            Same as hardware
0/0/(1..254)      0/0/(1..254)     Same as hardware    
0/80/255          0/80/255         Same as hardware    

In this case, Stella 4.7.3 has accurate emulation, and the latest git has regressions.

timer_test.bas.bin
Select an interval with joystick, press Fire to select.
Scroll over each item to see associated value.

Hardware              Stella git/Stella 4.7.3
1024T (0's on FF-FB)  1024T (5121,5122,5123,5124,5125 on FF-FB)

TIMER.zip

sa666666 commented 7 years ago

Results from Stellerator.

First 5 ROMs all match current git, which is wrong compared to real hardware. Also, some numbers seems to be misdrawn a little, particularly '4', which seems to have a bar over it.

testTIMINT_flawed.bin is same as hardware.

testTIMINT_withDelay is the same as Stella 4.7.3 and real hardware. So there is a regression in git.

timer_test.bas.bin: Unable to see any values on 1024T, as the screen is shifted down too far.

sa666666 commented 7 years ago

Results from MAME.

   ROM              Hardware         MAME
test1.bas.bin        192246         128245
test2.bas.bin        064249         128248
test3.bas.bin        192248         128247
test4.bas.bin        192245         128244
test5.bas.bin        192245         128244

First 3 digits always the same, possibly indicating that nothing is being emulated there. Second 3 digits off by 1 (1 too low) in all cases.

testTIMINT_flawed.bin    Same as real hardware
testTIMINT_withDelay.bin

Hardware         MAME
0/80/0          0/80/0         Same as hardware, but only after a few seconds pass
0/0/(1..254)    0/0/(1..254)   Same as hardware    
0/80/255        0/0/255        Error, doesn't have 80 at 255.
timer_test.bas.bin

Hardware              MAME (same as Stella, incorrect)
1024T (0's on FF-FB)  1024T (5121,5122,5123,5124,5125 on FF-FB)
sa666666 commented 7 years ago

Results from Javatari.

   ROM              Hardware         Javatari
test1.bas.bin        192246         192245
test2.bas.bin        064249         192248
test3.bas.bin        192248         192247
test4.bas.bin        192245         192244
test5.bas.bin        192245         192244

First 3 digits always the same, possibly indicating that nothing is being emulated there. Second 3 digits off by 1 (1 too low) in all cases.

testTIMINT_flawed.bin    0/80

After is 80, indicating TIMINT probably not emulated at all.

testTIMINT_withDelay.bin

Hardware     Javatari
0/80/0        0/80/x

After is always 80 no matter the delay, indicating TIMINT probably not emulated at all.

timer_test.bas.bin

Many values incorrect, but almost always off by 1.

sa666666 commented 7 years ago

Results from z26.

   ROM              Hardware         z26
test1.bas.bin        192246         000246
test2.bas.bin        064249         000249
test3.bas.bin        192248         000248
test4.bas.bin        192245         000245
test5.bas.bin        192245         000245

First 3 digits always the same, possibly indicating that nothing is being emulated there. Second 3 digits same as hardware.

testTIMINT_flawed.bin    0/80

After is 80, indicating TIMINT probably not emulated at all.

testTIMINT_withDelay.bin

Hardware     z26
0/80/0      0/80/x

After is always 80 no matter the delay, indicating TIMINT probably not emulated at all.

timer_test.bas.bin

Same as Stella git, wrong in the same places compared to hardware.

thrust26 commented 7 years ago

Where are those coming from? The code is from Omegamatrix, did he explain what he is doing?

sa666666 commented 7 years ago

He probably did, but I don't remember now. I only happened to find the ROMs at all because I was going through an old SDHC card I found. Maybe if we can track down where they came from, we can add documentation to this thread. I know it was something on AtariAge ...

thrust26 commented 7 years ago

Found the thread: http://atariage.com/forums/topic/202736-create-roms-to-determine-correct-intimtimint-behaviour/#entry2597871

DirtyHairy commented 7 years ago

Interesting stuff, seems I messed something up with TIMINT emulation in Stella :smirk: The thread contains a wealth of information, too tired today, I'll go through it later these days. Some quick observations:

sa666666 commented 7 years ago

Well, I only see one regression from Stella 4.7.3 to 5; some of the other stuff has been broken in Stella for some time.

thrust26 commented 7 years ago
   ROM              Hardware         Stella git        Stella 4.7.3
test1.bas.bin        192246            128246          Same as git
test2.bas.bin        064249            000249          Same as git
test3.bas.bin        192248            128248          Same as git
test4.bas.bin        192245            128245          Same as git
test5.bas.bin        192245            128245          Same as git

It seems that bit 6 of TIMINT is either simply reversed or not emulated at all.

I doubt any existing code (besides the test programs) ever checks this flag. Therefore I wonder if emulating PA7 etc. correctly should have high priority. If so, then we should extend vcs.h too. Various addresses ($286, $287, $28c, $29c..$29f) are missing.

sa666666 commented 7 years ago

I do have some code that does something with bit 6 of TIMINT (aka PA7), and it does have some effect. In particular, as mentioned in the thread, moving the joystick right seems to cause the digits to 'wiggle' in one of the ROMs, which we determined was related to PA7. None of the other emulators would do that, but Stella did once I added the PA7 code. Now, it may still be incomplete, but it is at least partially working.

As for the priority, well it can probably be low for now. I just wanted to have everything documented in github instead of spread all over AtariAge. You notice that I forgot about the issue completely until I happened across the test ROMs.

Actually, I'm much more concerned about the last test, where it displays numbers 5121, 5122, ... , instead of 0's. This seems to be an INTIM issue, not TIMINT.

thrust26 commented 7 years ago

OK, then lets concentrate on INTIM first.

Unfortunately I have no clue how Michael's Basic code works (yet?). The logic is indeed "a bit difficult to follow". :(

DirtyHairy commented 7 years ago

I haven taken the time to read through the thread now. I think we can tick off test1 - test5. @sa666666 , in this thread you already arrived at the conclusion that, on real harware, PA7 varies after power on, so sometimes you get 128, and sometimes you get 192. I'll test for myself later, but that sounds very reasonable. Actually, thinking about this, it may even be that PA7 is always raised if starting from the harmony menu... :smirk:

As for the 1024T stuff, I have no clue. I, too, took a look at the Basic code and also failed to understand what is actually happening. As SeaGtGruff mentions, this has the smell of something going wrong in the test code itself, but of course, the difference still indicates an emulation failure somewhere. I just am not convinced that it actually is the timer that is wrong: it may be something entirely different (like an edge case in an undocumented opcode).

sa666666 commented 7 years ago

The PA7 code I added was strictly based on the documentation I had (which we've recently found to be incomplete at times). So there's that. Plus, it was only to get the 'wiggling' digit working (hmm, that sounds worse that it really is :smiling_imp:). I may look into this further if I find time, but as you say, it might also be an artifact of using the Harmony.

Agreed on the last test ROM as well; it was wonky from the start, but still indicates incorrect emulation of something, somewhere.

However, there's still another issue with testTIMINT_withDelay.bin, where it worked in 4.7.3 and not in 5.0. I think the code I added was the following, which was basically a hack that emulated observed behaviour, not the underlying reason:

// Return at 'divide by 1' rate
uInt8 divByOne = timer & 0xff;

// Timer flag has been updated; don't update it again on TIMINT read
if(divByOne != 0 && divByOne != 255)
  myTimerFlagValid = true;
thrust26 commented 7 years ago

Since I don't understand the code, I watched the test for 1024T in the debugger. I notice that it takes several 10,000 cycles to detect each value until 5121. The following 4 values are detected within just a few 100 cycles. So it seems something goes wrong here.

DirtyHairy commented 7 years ago

Hah, at least for the regression: nailed it :smile: Seems I forgot to reimplement the

However, the reading of the timer at the same time the interrupt occurs will not reset the interrupt flag"

part in the datasheet. It was hidden in the snipped Stephen quoted above, but I failed to see it and subsequently forgot to reimplement it. Omegamatrix' test is checking precisely that: an INTIM read is performed at the very clock the timer wraps (plus an eventual delay, that's why the flag reads 0x00 if the delay is nonzero).

Fixed in 317169c . In 6502.ts, I properly implemented this from the start, that explains the difference.

thrust26 commented 7 years ago

Cool. One less to go.

Coming back to the 1024T test: The test runs ~3 sec in Stella and ~11 sec on real hardware. This fits to the (too) fast detection of the last 4 values in Stella.

BTW: Stellerator delivers the same values as Stella.

sa666666 commented 7 years ago

How are you able to see the results from Stellerator? For me, the screen shifts down and I can't see any of the output values at all.

thrust26 commented 7 years ago

Define the cart as PAL. Then you get a rolling result screen.

DirtyHairy commented 7 years ago

Define the cart as PAL. Then you get a rolling result screen.

I have been long planning to add an option for configuring frame start manually :smiley:

sa666666 commented 7 years ago

Posting the batari basic code for the ROM with the 1024T issue, since it may be missed: timer_test.bas.zip

thrust26 commented 7 years ago

The only code that I was able to identify which may(!) cause a problem is: lda INTIM-255,y ; with Y=255

Maybe there is a timing problem when doing an indexed read?

DirtyHairy commented 7 years ago

@thrust26 Do you perchance know the instruction sequence that this was assembled to?

thrust26 commented 7 years ago

b9 85 01

Stella disassembles this into: LDA ram_85|$100,Y

Both look fine to me. It executes in 5 cycles, but there is an intentional page penalty, maybe this gets missed and the timer is read one cycle to early. Though in the debugger it seems OK.

DirtyHairy commented 7 years ago

Hmmm, that's absolute indexed addressing, and there is an invalid read involved. It's a long stretch, but this could be related.

DirtyHairy commented 7 years ago

Crossing the page boundary takes an extra read cycle, during which the CPU does an unintented read (it's usually called invalid, but unintented would be more precise :smirk:). I'll look up the specifics later this evening, but I am sure that this implemented in 6502.ts. However, I don't remember where I got the exact mechanics from, and if this is not documented in the MOS docs, then I might have peeked at Stella for the read address and replicated a possible bug. As RIOT reads can modify state, this might be a source for bugs.

thrust26 commented 7 years ago

If I get the documentation right, in our case that invalid read would be at $184. That's only a mirror of ZP RAM.

DirtyHairy commented 7 years ago

@thrust26 Yeah, you're right. I just checked, the behavior is described on page 80 of the MOS Programming Manual, and the address is indeed 0x0184. Too bad, but that's not the culprit.

sa666666 commented 7 years ago

Since I don't have batari basic installed, and you guys are probably more familiar with 6502 ASM rather than basic, here's the disassembly of the last test ROM (generated by Stella):

timer_test.bas.asm.zip

ale-79 commented 7 years ago

I do have some code that does something with bit 6 of TIMINT (aka PA7), and it does have some effect. In particular, as mentioned in the thread, moving the joystick right seems to cause the digits to 'wiggle' in one of the ROMs, which we determined was related to PA7. None of the other emulators would do that, but Stella did once I added the PA7 code. Now, it may still be incomplete, but it is at least partially working.

Here are a couple more test roms for the PA7 flag:

Both display colored squares indicating a logic 1, or a thin horizontal line indicating a 0.

The first one is for SWCHA set as input: pa7_test_input

The right difficulty switch is used to set the polarity of the active transition: A=positive (write to $284), B=negative (write to $285). The top square shows PA7 (joystick right direction), the bottom one indicates if the PA7 interrupt flag has been set. This one can be cleared by pressing the joystick button. Unfortunately, testing this rom on real hardware is not possible with a joystick, because of the inevitable contact bouncing of mechanical switches. If you try, the flag is apparently set on both negative and positive transitions no matter the position of the difficulty switch. You'd need an electronic circuit connected to the joystick port that generates clean transitions. Anyway, testing the rom in Stella behaves as described in the RIOT documentation (depending on the position of the difficulty switch, the flag is set on a positive or negative transition), so I believe emulation is correct here.

The second rom sets SWCHA as ouptut and therefore can be tested reliably on real hardware too. The test rom first sets the active PA7 transition to negative polarity, then flips PA7 8 times by writing to SWCHA and reads the value of the PA7 interrupt flag after each change. If the right difficulty switch is set to A, the polarity is also inverted after each write to SWCHA, but before reading TIMINT. The same test is then repeated starting with positive polarity. The results are shown on 3 lines: the first shows the values of PA7, the other two the corresponding values of the pa7 interrupt flag.

Results on real hardware: R_DIF=B: img_0118 R_DIF=A: img_0119

Stella (it's the same with both settings): pa7_test_output

So Stella only emulates the edge detection with SWCHA set as input, while on real hardware it works also when set as output (this is also in the 6532 datasheet).

The RIOT documentation also states that "the interrupt flag might be set by changing the polarity of the active interrupt", but this doesn't seem to happen in my tests. The results with the difficulty switch in A position show that changes of polarity don't alter the flag. The two roms only use addresses $284 and $285, but I also tried the other mirrors and the results are the same (as expected).

P.S. Maybe an indication of the current polarity should be added somewhere in the I/O tab of the debugger.

pa7_test.zip

thrust26 commented 7 years ago

This is interesting and I am all for correct emulation.

But for what kind of development could this be useful? Serial interfaces via joystick port?

ale-79 commented 7 years ago

But for what kind of development could this be useful?

Honestly, I can't think of any example where it could be useful. :smile: Maybe, as you say, someone could find some use for it when interfacing with external hardware through the joystick port.

sa666666 commented 7 years ago

I agree that it is perhaps a dubious feature at the moment and is low priority, but if we have ROMs that show a difference between emulation and real hardware, we should emulate the behaviour correctly. I'd like to get to it at some point, but there are a few more things that are more important ATM. So we should leave this issue open, and come back to it after 5.0.

Unless of course someone wants to take a stab at it now, in which case I won't argue :smile:

thrust26 commented 4 years ago

In reference to this thread at AtariAge I add my own code here too, so that it does not get lost.

Test_Roll_01.zip

ale-79 commented 4 years ago

It seems that the difference in the PA7 flag in the "test[1-5].bas.bin" roms in the first post is indeed due to loading the roms from the Harmony menu, rather than to bug/incomplete emulation. Navigating the files in the SD card triggers the PA7 interrupt whenever you move the stick to the right.

I retested those roms while avoiding moving the stick to the right and the result matches Stella emulation on my console.

thrust26 commented 4 years ago

And what could set the flag?

sa666666 commented 4 years ago

From M6532::update()

  // PA7 Flag is set on active transition in appropriate direction
  if((!myEdgeDetectPositive && prevPA7 && !currPA7) ||
     (myEdgeDetectPositive && !prevPA7 && currPA7))
    myInterruptFlag |= PA7Bit;
sa666666 commented 4 years ago

I'm going to take a stab at the PA7 stuff above; wish me luck.

sa666666 commented 4 years ago

The weirdness in timer_test.bas.bin from above (should be 0's on FF-FB) is now fixed because of https://github.com/stella-emu/stella/commit/f1998c761c47e5618a3a826d8caef141fb408095! So scratch one more off the list.