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 56 forks source link

The musical version of Elite has broken ship-drawing, but only on b-em #228

Closed markmoxon closed 1 day ago

markmoxon commented 3 days ago

I'm investigating an issue with b-em and the BBC Master version of the Elite Compendium, in which the ships do not render properly:

https://www.stardot.org.uk/forums/viewtopic.php?p=429373#p429373

This version of Elite works fine for me on BeebEm, beebjit, b2, JSBeeb and on real hardware, on both MOS 3.20 and 3.50. Only b-em is having problems. I've tested this on v-24f41ba and v-TOHv3.2-rc3 on Windows, and can confirm that the issue is happening to me too.

The issue is down to the music, as the exact same problem can be found in the musical version of Elite. The issue breaks all ship-drawing, which you can see on the title screen.

The musical aspect of Elite is based on the Bitshifters ROM at https://github.com/kieranhj/elite-music. The music lives in sideways RAM and the game is patched to call the ROM every vertical sync via the interrupt handler).

I've been investigating, and there are two parts of the code that break the game on b-em. Weirdly, both bits of code need to be disabled to get ship-drawing working again; disabling one bit of code on its own doesn't fix the ships, but disabling both does. I have no idea why they are connected, as they are in totally different parts of the codebase.

If I disable both of these bits of code with NOPs, then the ships draw correctly, though the music is disabled as the music ROM never gets switched in.

I'm having trouble tracking down the exact cause of the broken ships, but I suspect that it might be because the game code in sideways RAM bank 6 isn't being remapped to &8000 properly after switching back from the music ROM. This is a guess, but the ship blueprints are at locations &8000 to &9D95 in bank 6, so if they aren't being paged back into &8000 correctly, then this would cause the ships to break in this way.

If I do a m 8000 command in the debugger, then memory at &8000 to &8FFF is always shown as containing nothing but &FF, which can't be right (it shows 6: at the start of each line, so it looks like it's showing the correct bank number). The correct ship blueprint content is shown from &9000 onwards. Strangely, &FF is shown as the content both when the ships are working and when they aren't, so this might just be an issue with the debugger not showing that part of memory correctly? It's hard to tell.

Attached is a zip containing two disc images of musical Elite.

SSDs.zip

Specifically, elite-fixed has the two STA &FE30 instructions in the PlayMusic routine NOP'd out, and the JSR &8006 instruction changed to BIT &8006 (so the music ROM isn't actually called):

https://github.com/markmoxon/master-elite-beebasm/blob/music/1-source-files/main-sources/elite-source.asm#L10680-L10711

Also, the JSR OSBYTE at the start of the game has been replaced with NOP:LDA #%10 to skip the OS call and simulate QUIET being configured:

https://github.com/markmoxon/master-elite-beebasm/blob/music/1-source-files/main-sources/elite-source.asm#L37767

It doesn't matter what value of A gets set here - the fix is to comment out the OSBYTE call.

I'm a bit stuck on what to do next, but hopefully there are some clues in this! I can try tweaking the code if you'd like me to try various things, or you can build musical Elite yourself from the music branch of BBC Master Elite:

https://github.com/markmoxon/master-elite-beebasm/tree/music

If you do make any code changes, then it's best to keep the number of instruction bytes the same, so the encryption doesn't break.

Anyway, suggestions are welcome, as it would be good to get this working on b-em, if possible!

Mark

SteveFosdick commented 2 days ago

This is certainly intriguing.

If I use the File->Save State feature to save the state of b-em to a file, both for the broken and fixed versions, and then use the bsnapdump program from the b-em src directory to examine each, I also find that in both cases the first 4K of ROM 6 seems to be all FF. The code to save state to a file is simpler than the debugger and I don't think that would be reading the wrong memory.

So, what I think is happening here is that both in the broken and the fixed version, the 4K of RAM that can overlay the sideways ROM area (VDU workspace? described on page F.2-1 as MOS RAM) is paged in at the time the ship data is being copied into bank 6. On the Master, this is done by setting bit 7 of the value written to FE30. Then, when the switch to bank 4 is done to play the music, the value from &F4 saved to the stack doesn't have bit 7 set so when the switch is made back to bank 6, the VDU workspace is paged out and the ship data is not there. Here are some excerpts from the snapdump output:

 VDU workspace
FF808000 - A5 80 A3 81 BF 82 13 83  53 83 FB 83 9D 84 73 85  ........S.....s.
FF808010 - AF 85 E1 86 C3 88 4B 8A  3D 8B 33 8C 35 8D 7F 8D  ......K.=.3.5...
FF808020 - 59 8E 01 8F 2F 90 15 91  45 92 09 93 CF 93 7F 94  Y.../...E.......
FF808030 - 3B 95 65 96 BB 96 D5 97  AD 98 C9 99 35 9A 37 9B  ;.e.........5.7.
FF808040 - 39 9C 00 00 01 00 00 00  00 00 21 61 A0 A0 A0 A1  9.........!a....
FF808050 - A1 C2 0C 8C 8C 8C 0C 8C  05 8C 8C 8C 82 0C 0C 04  ................
  ROM 6
FFF68000 - FF FF FF FF FF FF FF FF  FF FF FF FF FF FF FF FF  ................
FFF68010 - FF FF FF FF FF FF FF FF  FF FF FF FF FF FF FF FF  ................
FFF68020 - FF FF FF FF FF FF FF FF  FF FF FF FF FF FF FF FF  ................
...
FFF68FD0 - FF FF FF FF FF FF FF FF  FF FF FF FF FF FF FF FF  ................
FFF68FE0 - FF FF FF FF FF FF FF FF  FF FF FF FF FF FF FF FF  ................
FFF68FF0 - FF FF FF FF FF FF FF FF  FF FF FF FF FF FF FF FF  ................
FFF69000 - 44 50 60 05 44 54 60 07  44 4C 58 05 44 4C 5C 05  DP`.DT`.DLX.DL\.
FFF69010 - 44 58 5C 1E 21 00 08 1E  31 00 0C 5E 00 18 02 1E  DX\.!...1..^....
FFF69020 - 00 18 02 9E 20 40 10 1E  20 40 10 3E 00 00 7F 01  .... @.. @.>....

There is, presumably a bug in b-em here but it may not be in the handling of the writes to FE30 to switch between banks 4 and 6 but something earlier in the loading process. As there is an OSBYTE call that seems to be implicated in this, I wonder what that does that may have a bearing on this.

SteveFosdick commented 2 days ago

Looking at the OSBYTE that you also needed to NOP out to get the ship to draw correctly, this also writes to FE30 because it pages in the Terminal ROM, where much of the MOS code is located, then restores the previous ROM before returning. Just as in the case with swapping between ROMs 4 and 6, it does this with bit 7 clear and thus leaves the VDU workspace paged out and, along with it, the ship data.

markmoxon commented 2 days ago

Interesting! Just to be clear, Elite doesn’t use that 4K of RAM at &8000 - the ship data is in bank 6 of sideways RAM, so if the 4K of RAM is still paged in at &8000 when Elite regains control, then that will break things. Is b-em correctly paging the 4K of RAM back out when bit 7 of &FE30 is clear? If not, then that would leave the 4K of RAM incorrectly paged in instead of the ship data in bank 6, and that would create the ship problem.

SteveFosdick commented 2 days ago

It definitely looks like it is being left paged in too long. Although Elite is trying to load the ship data into bank 6 it looks like it is ending up in this VDU workspace. Looking at all the writes up to the end of this screen: fixed-state-initial-screen

Shows that all the time bit 7 has been set this has been from MOS before calling the VDU driver (once per character on that screen). Saving the state at that point shows the VDU 4K workspace as starting:

  VDU workspace
FF808000 - 22 22 22 22 22 22 22 22  22 22 22 22 22 22 22 22  """"""""""""""""
FF808010 - 22 80 80 80 80 80 80 80  80 80 80 80 80 80 80 80  "...............
FF808020 - 80 80 00 00 00 00 00 00  00 00 00 00 00 00 00 00  ................

and ROM 6 starting:

  ROM 6
FFF68000 - FF FF FF FF FF FF FF FF  FF FF FF FF FF FF FF FF  ................
FFF68010 - FF FF FF FF FF FF FF FF  FF FF FF FF FF FF FF FF  ................
FFF68020 - FF FF FF FF FF FF FF FF  FF FF FF FF FF FF FF FF  ................

But bank 6 does have anything but FF at 9000 onwards either, yet, so presumably the ship data hasn't been loaded yet. Pressing a key to continue gets a few more MOS VDU writes of 8F to FE30, a bunch of writes with bit 7 clear, then at the end:

cpu core6502: E594: write to FE30, value=F
cpu core6502: E594: write to FE30, value=9
cpu core6502: E594: write to FE30, value=F
cpu core6502: E594: write to FE30, value=6
cpu core6502: 0F39: write to FE30, value=F6

and at this point the ship is shown rotating on the screen. The last write of F6 is presumably from the Elite code. This is also the point where a save state shows the first 4K of bank 6 as FF and the 4K VDU workspace with much more in it than shown above. So, at this point, the first 4K of ship data may not be intended to be in the 4K VDU workspace, but it seems it is.

Looking back a bit further, there is:

cpu core6502: E594: write to FE30, value=F
cpu core6502: E594: write to FE30, value=9
cpu core6502: 0EB7: write to FE30, value=F6
cpu core6502: E594: write to FE30, value=F
cpu core6502: E594: write to FE30, value=9
cpu core6502: E594: write to FE30, value=F
cpu core6502: E594: write to FE30, value=9

So this is plausible for some disc activity going on, with paging between terminal (OS code) and DFS, but what is happening with the write of &F6 from RAM? Here's the snippet:

    0EAC: A9 06       LDA #06     
    0EAE: 85 F4       STA F4      
    0EB0: AD 30 FE    LDA FE30    
    0EB3: 29 F0       AND #F0     
    0EB5: 09 06       ORA #06     
    0EB7: 8D 30 FE    STA FE30    

This is expecting to be able to get the state of the 4K RAM by reading FE30. B-Em does not support that - the returned result with almost certainly have bit 7 set even if that 4K RAM is paged out, so the subsequent write to FE30 pages that 4K RAM in!

SteveFosdick commented 2 days ago

This patch fixes the issue.

--- a/src/6502.c
+++ b/src/6502.c
@@ -470,6 +470,9 @@ static uint32_t do_readmem(uint32_t addr)
                         return wd1770_read((uint16_t)addr);
                 break;

+        case 0xFE30:
+            return ram_fe30;
+
         case 0xFE34:
                 if (MASTER)
                         return acccon;
SteveFosdick commented 2 days ago

This fix is now on master in GitHub.

markmoxon commented 1 day ago

Brilliant! Thank you Steve, I will feed this back to the original bug reporter on Stardot.

I just checked and the code snippet containing LDA &FE30 is from the original Elite source. &FE30 is supposed to be read-only (I guess reading from &F4 is more correct), but it does seem to work on real hardware, so thanks for closing the loophole!