Incorrect data on dummy peeks which cause unwanted writes

[andrew-davie]

OK, I have a definite bug in Stella - recent build and previous release. I've been bumping my head against this for a few days - the symptom is that I add code (even just a "ds 4" and I get timing problems in my kernel in earlier code in the bank where there is NO access from the kernel TO that shifted part, but there IS access from the shifted part to the kernel.

But before I get into details, the problem does NOT show on Z26, or on Javatari - these work perfectly, and exhibit the correct behaviour. First, let me show an image of what the bug looks like...

Those shapes down the left side of the screen should not be there. They are clearly a timing issue with PF writes. Now, I can turn them on or off by just adding space to code at the very end of the bank just before a routine that modifies the colours in the kernel itself (this is all running in a RAM bank, 3E scheme). Here's "good" code which shows no bug...

    848  03bf              00             ds    1
    849  03c0
    850  03c0
      0  03c0                         DEFINE_SUBROUTINE FixColours
      1  03c0              00 00       BANK_FixColours =    _CURRENT_BANK
      2  03c0                         SUBROUTINE
      3  03c0                  FixColours
    852  03c0                           ; USES OVERLAY "ColourFixer"
    853  03c0
    854  03c0              a4 96              ldy   BoardScrollY
    855  03c2              cc 7b f3           cpy   LastYScroll
    856  03c5              f0 31              beq   BandsNotChanged
    857  03c7              8c 7b f7           sty   LastYScroll+RAM_WRITE
    858  03ca
    859  03ca              a5 80              lda   Platform
    860  03cc              29 02              and   #%10
    861  03ce              a9 00              lda   #0  ;asl
    862  03d0                           ;asl
    863  03d0                           ;asl
    864  03d0                           ;asl
    865  03d0              85 db              sta   PlatformBase
    866  03d2
    867  03d2              ad 79 f3           lda   BandOffset
    868  03d5              65 96              adc   BoardScrollY
    869  03d7              85 dc              sta   BandOffsetTemp
    870  03d9
    871  03d9
    872  03d9              a2 07              ldx   #SCREEN_LINES-1
    873  03db              86 3e       LoopBankLines stx    SET_BANK_RAM
    874  03dd
    875  03dd              8a             txa
    876  03de              18             clc
    877  03df              65 dc              adc   BandOffsetTemp
    878  03e1              29 1f              and   #31
    879  03e3              65 db              adc   PlatformBase
    880  03e5              a8             tay
    881  03e6
    882  03e6              b9 7f f3           lda   ColourBandsGreen,y
    883  03e9              8d 7d f7           sta   Colour_B + RAM_WRITE
    884  03ec
    885  03ec              a4 cd              ldy   FadeComplete
    886  03ee              f0 02              beq   writeActualCol
    887  03f0              a9 00              lda   #0
    888  03f2              8d 86 f4    writeActualCol sta   Colour_B_Actual + RAM_WRITE
    889  03f5
    890  03f5              ca             dex
    891  03f6              10 e3              bpl   LoopBankLines
    892  03f8
    893  03f8              60      BandsNotChanged rts
    894  03f9
    895  03f9
      0  03f9                         CHECK_HALF_BANK_SIZE  "ROM_SHADOW_OF_RAMBANK_CODE -- 1K"

see that 'ds' at the top? I'll change that to "ds 8" and the bug appears. Note, the code shown is the very last code in the bank. There is nothing but a completely blank 2K in the ROM after this (it's unused).

The code is a tad tricky, in that the loop starts executing in bank 0 - and there is identical code in banks 1-7. 8 banks of identical code. So the loop which does the bankswitch is actually switching to exact same code in other banks... and continuing. I have traced with the debugger and confirmed this is all OK. In any case, shifting the code with the "ds" should not make any difference there.

Note that the loop does not cross page boundaries. Nor does any of the variable accesses. And remember this all works just fine without the shift. And it works fine with or without the shift on other emulators.

The whole bank is in the file BANK_ROM_SHADOW_RAMBANK.asm, which I will attach.

BANK_ROM_SHADOW_RAMBANK.asm.zip

I've confirmed that merely padding space so that the routine is shifted causes the kernel (which is at the very top of the file) to glitch. But only on Stella. It appears to me to be the write at 03F2 which is actually causing the problem, but I am not sure. That write is changing a colour inside the kernel - so obviously a candidate for changes in kernel visuals. But not timing.

    167  0085
    168  0085              f0 86       Colour_B_Actual =    * + 1
    169  0085                  SELFMOD_GREEN
    170  0085              a9 00              lda   #0  ; 2
    171  0087              85 48              sta   COLUPF  ; 3 =   5    @08
    172  0089

Note that F486 (the write address) corresponds to 0086 in the listing above. If I take out the write and replace with "lda Colour_B_Actual" instead... no glitch, so that pretty much confirms the write is changing the kernel timing. I really have no idea how/why.

But, as I said, I think it's a Stella bug. Happy to share the whole code base with anyone who needs to test/confirm.

[DirtyHairy]

@andrew-davie Can you test on Stellerator? This might help narrowing down the issue.

[andrew-davie]

@andrew-davie Can you test on Stellerator? This might help narrowing down the issue.

Have done so. Interesting, the game only runs if I set the timing to "instruction" and there's a lot of screen jitter - but ignoring that, the display is basically correct - no glitches seen. Here's the binary for testing...

sokoboo_glitch_stella.bin.zip

[andrew-davie]

Just in case it's significant - the rts when the bug exists is at the VERY LAST BYTE of the RAM bank.

[andrew-davie]

Well, lo and behold... it IS significant. If I setup the system so that the RTS is at (say 3FD) it works fine. If I put a "nop" before the RTS, no problem. Put another "nop" there, pushing the RTS to the end of the bank.... glitch.

[DirtyHairy]

Have done so. Interesting, the game only runs if I set the timing to "instruction" and there's a lot of screen jitter - but ignoring that, the display is basically correct - no glitches seen. Here's the binary for testing...

The jitter goes away if you set the game to PAL or configure the difficulty switches to NTSC 😏 (or if you disable frame autodetect by fixing the first visible line). The issue with CPU timing: cycle is interesting... I'll have to investigate that one.

Anyway, this means that TIA emulation is not the culprit.

[DirtyHairy]

Are you sure that this is working correctly on hardware? If you put a RTS at the end of the RAM bank, then there will be a dummy access to 0x0400 before the 6502 starts reading from the stack. This will trigger an unwanted write to the first byte in the RAM bank. This is also a difference between cycle and instruction timing in 6502.ts: cycle timing emulates the dummy access correctly, instruction timing doesn't.

[andrew-davie]

No, I do not have hardware to test. Your info is news to me!

[andrew-davie]

I guess that explains things, and this "bug" can be closed. Yay for Stella accuracy :P

[DirtyHairy]

I can confirm that the issue with cycle accurate emulation in 6502.ts is caused by what I described above 😏 If I comment out the handling of "writes caused by reads to the RAM bank" in 3E emulation, it works.

So, indeed, "case closed" I guess. We should have a second look at 3E emulation in Stella, though, I am still a bit surprised that 6502.ts and Stella behave differently here.

That's a screenshot from the relevant part of the 6502 manual:

[andrew-davie]

Well spotted by the way. I claim a tiny bit of credit for noticing the RTS was at the end, though to be honest it never occurred to me there was a bogus access to 0x400 as a result.

[DirtyHairy]

OK, I see the difference between 6502.ts and Stella now. 6502.ts writes the last value on the data bus (which is what real hardware should be doing), while Stella writes random data. @thrust26 @sa666666 I wonder: maybe we want to make this optional.

Well spotted by the way. I claim a tiny bit of credit for noticing the RTS was at the end, though to be honest it never occurred to me there was a bogus access to 0x400 as a result.

😏 The difference between cycle and instruction accuracy together with the RTS was the clue.

[thrust26]

@DirtyHairy I suppose that random data is not only RTS related?

And why should we make this optional? If we are sure how the CPU behaves, we need no option.

[thrust26]

Yay for Stella accuracy :P

Even though it might no be obvious to the "normal" user, Stella has seen quite some improvements in accuracy lately. Which is especially important for developers. And we also added a number of developer settings, which you can find in the 'Developer' dialog.

[DirtyHairy]

To me, this constellation is similar to reading from a write register: the lines of the bus that are not driven retain their value from the last cycle, and I suppose this is what ends up stored in RAM. This is not specific to RTS, but every read from a write port in cartridge space is affected.

I though the random values were there for similar reasons like the "tiadriven" option: to allow developers to more easily identify bugs. However, looking at the code, I may be wrong, and this is just the way it was implemented. Of course, I would be fine with just killing off the random values in favour of the last value on the bus, too.

[thrust26]

Yes, for the TIA bits we have a randomization option to make the coding errors more obvious, else lda F0 instead of lda #F0 will not be noticed. But quite a lot of original ROMs have this bug, so Stella needs an option to disable it.

AFAIK this is not the case for this situation and random values might be better noticeable for developers. But for accuracy the bus values (this needs research) might be the better choice.

BTW: Where in the code do I find the data randomization for this case?

[ale-79]

As you might remember, the results of few tests done on Atari "Superchip" carts a while ago were different from the expected behavior. With the console used for the tests, the value written to ram wasn't affected by the last value on the data bus:

https://atariage.com/forums/topic/285759-stella-getting-into-details-help-wanted/

So the behavior might depend on the actual hardware implementation, and for new homebrews, bankswitching schemes using ram on cart are actually emulated by the arm chip on Harmony/Melody/Unocart, so the definition of "hardware" becomes a bit ambiguous...

[sa666666]

BTW: Where in the code do I find the data randomization for this case?

In System.hxx:

    /**
      Get the current state of the data bus in the system.  The current
      state is the last data that was accessed by the system.

      @return  The data bus state
    */
    uInt8 getDataBusState() const { return myDataBusState; }

    /**
      Get the current state of the data bus in the system, taking into
      account that certain bits are in Z-state (undriven).  In those
      cases, the bits are floating, but will usually be the same as the
      last data bus value (the 'usually' is emulated by randomly driving
      certain bits high).

      However, some CMOS EPROM chips always drive Z-state bits high.
      This is emulated by hmask, which specifies to push a specific
      Z-state bit high.

      @param zmask  The bits which are in Z-state
      @param hmask  The bits which should always be driven high
      @return  The data bus state
    */
    uInt8 getDataBusState(uInt8 zmask, uInt8 hmask = 0x00) const
    {
      // For the pins that are floating, randomly decide which are high or low
      // Otherwise, they're specifically driven high
      return (myDataBusState | (randGenerator().next() | hmask)) & zmask;
    }

[thrust26]

BTW: Where in the code do I find the data randomization for this case?

In System.hxx:

That's for the TIA peeks. But where does the randomization for the unwanted writes data happen?

[sa666666]

Oh yes, for Cart it's in a different place: Cart.cxx

// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
uInt8 Cartridge::peekRAM(uInt8& dest, uInt16 address)
{
  uInt8 value = myRWPRandomValues[address & 0xFF];

  // Reading from the write port triggers an unwanted write
  // But this only happens when in normal emulation mode
#ifdef DEBUGGER_SUPPORT
  if(!bankLocked() && !mySystem->autodetectMode())
  {
    // Record access here; final determination will happen in ::pokeRAM()
    myRAMAccesses.push_back(address);
    dest = value;
  }
#else
  if(!mySystem->autodetectMode())
    dest = value;
#endif
  return value;
}

// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void Cartridge::pokeRAM(uInt8& dest, uInt16 address, uInt8 value)
{
#ifdef DEBUGGER_SUPPORT
  for(auto i = myRAMAccesses.begin(); i != myRAMAccesses.end(); ++i)
  {
    if(*i == address)
    {
      myRAMAccesses.erase(i);
      break;
    }
  }
#endif
  dest = value;
}

[thrust26]

Ah, myRWPRandomValues. I really should have seen this. Thanks!

[sa666666]

And there are a pair of methods, since sometimes a read is an error, and sometimes it's a dummy. And we don't want to break on the dummy reads. We put it in an array to cache the randomization, so that the ROM would always start with the 'same' randomization. This was documented in another thread/issue, but I don't remember which :confused:

[thrust26]

As you might remember, the results of few tests done on Atari "Superchip" carts a while ago were different from the expected behavior. With the console used for the tests, the value written to ram wasn't affected by the last value on the data bus:

https://atariage.com/forums/topic/285759-stella-getting-into-details-help-wanted/

Thanks for reminding me, I had completely forgotten this. So we have covered the issue already and myRWPRandomValues is the result of the fix.

[DirtyHairy]

Thanks for reminding me, I had completely forgotten this

And so did I 😏 Thanks for the reminder, Alex!