stardot / b-em

An opensource BBC Micro emulator for Win32 and Linux
http://stardot.org.uk/forums/viewtopic.php?f=4&t=10823
GNU General Public License v2.0
112 stars 57 forks source link

Shadow RAM selection bug #156

Closed hoglet67 closed 3 years ago

hoglet67 commented 3 years ago

Steve,

In the couse of debugging Beeb816 I've written a test program for the Master's shadow RAM access logic.

ShadowTest.ssd.zip

Running on a Master I get: capture5

Some notes:

Running on B-Em I get: Screenshot from 2021-05-15 11-13-09

I think there are two issues:

  1. ACCCON bit 3 (Y) needs to be taken into account to determine if an instruction fetch was done by the VDU driver (i.e. it's not sufficient just to treat all accesses from 0xC000 to 0xDFFF as being VDU driver). Code running in Hazel is not considered VDU driver.

  2. ACCCON bit 2 (X) is needs to be interpreted more selectively. In beeb-em is forces the mapping in of the shadow region for both non-VDU and VDU code. It should only be used for non-VDU code.

My understanding of the logic is:

  1. On every instruction fetch, the vdu_op state bit is updated as follows:

vdu_op = (address >= 0xC000 && address <= 0xE000 && !acccon_Y)

  1. On every memory access in the range 0x3000-0x7FFF the shadow bank is assigned as follows:

shadow_bank = vdu_op ? acccon_E : acccon_X

There is one further corner case to consider.

If the VDU driver (&C000-&DFFF ROM) made a JMP to, say, &3000, is the bank for instruction fetch from &3000 controlled by acccon_E or acccon_X? I haven't been able to test this on the real hardware. As the VDU driver will never do this, it's likely not important.

For reference, here's a nice diagram of &FE34 from the New Advanced User Guide (remastered): acccon

And here's the test code:

   20 P%=&2100
   30[OPT I%
   40 .test
   50 SEI
   60 LDA &FE34
   70 PHA
   80 LDX #&05
   90.cloop
  100 LDY #&00
  110 STY &FE34
  120 LDA &C000,X
  130 LDY #&08
  140 STY &FE34
  150 STA &C000,X
  160 STA &2000,X
  170 DEX
  180 BPL cloop
  190:
  200 LDY #&00
  210 STY &FE34
  220 LDA #&55
  230 STA &3000
  240 LDY #&04
  250 STY &FE34
  260 LDA #&AA
  270 STA &3000
  280:
  290 LDA #&00
  300 STA &D6
  310 LDA #&30
  320 STA &D7
  330:
  340 LDY #&00
  350]
  360 FOR Z%=0 TO 1
  370[OPT I%
  380 LDX #&00
  390.tloop
  400 STX &FE34
  410 JSR &2000+Z%*(&C000-&2000)
  420 STA &2040+Z%*&10,X
  430 INX
  440 CPX #&10
  450 BNE tloop
  460]
  470 NEXT
  480[OPT I%
  490 PLA
  500 STA &FE34
  510 CLI
  520 RTS
  530]
  540 NEXT
  550 FOR I%=&2000 TO &20FF
  560 ?I%=0
  570 NEXT
  580 CALL test
  590 PRINT~&FE34,~&2000,~&C000
  600 FOR I%=0 TO 15
  610 PRINT ~I%,~I%?&2040,~I%?&2050
  620 NEXT
  630 PRINT "55 = NORMAL RAM"
  640 PRINT "AA = SHADOW RAM"

Dave

SteveFosdick commented 3 years ago

Ok, I have a version that passes that test. I am just checking if there is a simpler way but I'll push a branch with the fix.

SteveFosdick commented 3 years ago

See https://github.com/stardot/b-em/commit/a911999997ffe81a6e0d375d210b11d0501a17c5

hoglet67 commented 3 years ago

a911999 doesn't get case 4 and 5 correct for accesses from &C000: Screenshot from 2021-05-16 07-40-45

I think the problem in those cases, memlook[0][0x30] and memlook[1][0x30] both point to the shadow bank.

Could you update shadow_mem() to also update memlook[1]?

Dave

SteveFosdick commented 3 years ago

Dave, are you sure you're running the version I pushed - the version number in the tile bar doesn't match but then I am not sure that is updated when it should be unless it is a completely clean build, i.e. at least as far back as running configure.

I did push an issue156 branch and then made some mistakes in git that also pushed other stuff to it so I deleted it and had another go. The current issue156 branch should be right and the write_acccon_master function isn't calling shadow_mem any more but basically swapping the two banks. As you say what I initially struggled with is that shadow_mem was leaving the shadow memory mapped to both memlook[0][0x30] and memlook[1][0x30] so there was no way to get to the normal RAM but I think doing the swap works - it certainly passed the test when I tried it locally.

SteveFosdick commented 3 years ago

P.S. I didn't just update shadow_mem because this is also called by the integra B code.

SteveFosdick commented 3 years ago

I have just had another look and it seems I was just having a bad day with git and the version I pushed wasn't the version I had tested. Try https://github.com/stardot/b-em/commit/51fd87726ccd6a8813eded4ca10680acedea96cb

hoglet67 commented 3 years ago

OK, that looks better: Screenshot from 2021-05-16 13-02-20

I'm still a bit confused as to when the version in the title bar is updated.

I've tried to do a clean build on the sf/issue156 branch, i.e.

git checkout sf/issue156
./autogen.sh
./configure
make clean
make

But the version is incorrect (and unchanged from before).

SteveFosdick commented 3 years ago

I get a yet different version. Configure says: Configured version: e5adcaf. Looking at git log, that corresponds to origin/master, origin/HEAD, master but not HEAD -> sf/issue156, origin/sf/issue156. I think that's another bug, though.

SteveFosdick commented 3 years ago

I have opened Issue 157 for the version number problem.