whscullin / apple2js

An Apple II emulator originally written in Javascript, now being converted to TypeScript
http://www.scullinsteel.com/apple2/
MIT License
438 stars 57 forks source link

Issue in bank 1 writing #187

Closed univta0001 closed 1 year ago

univta0001 commented 1 year ago

In the current Apple 2JSE, there are rare cases that the memory bank writing is not functioning like the real Apple 2e.

Shown below is the snippet on the code that shows the problem. Expected value in $06 should be #$A1

    LDA $C08B
    LDA $C08B ; Enable Read and Write to Bank 1
    STA $C089 ; Reset the prewrite
    LDA $C089 ; Set the prewrite
    LDA #$A1
    STA $D000 ; Store A1 to Bank 1
    LDA $C08B
    LDA $C08B ; Enable Read and Write to Bank 1
    LDA $D000 ; Read D000 value in Bank 1
    STA $06   ; Store value into $06
    STA $C08A ; Read ROM and no write to bank RAM`
    RTS

The fix to the issue is proposed below

  private _accessLangCard(off: byte, val?: byte) {
        const readMode = val === undefined;
        const result = readMode ? 0 : undefined;

        const writeSwitch = off & 0x01;
        const offSwitch = off & 0x02;
        const bank1Switch = off & 0x08;

        let bankStr;
        let rwStr;

        if (writeSwitch) { // 0xC081, 0xC083
            if (readMode) {
        if (this._prewrite) {
                this._writebsr = this._prewrite;
                    this._prewrite = false;
        } else {
                    this._prewrite = true;
                }
            } else {
                this._prewrite = false;
            }

            if (offSwitch) { // 0xC08B
                this._readbsr = true;
                rwStr = 'Read/Write';
            } else {
                this._readbsr = false;
                rwStr = 'Write';
            }
        } else { // 0xC080, 0xC082
            this._writebsr = false;
            this._prewrite = false;

            if (offSwitch) { // 0xC082
                this._readbsr = false;
                rwStr = 'Off';
            } else { // 0xC080
                this._readbsr = true;
                rwStr = 'Read';
            }
        }

        if (bank1Switch) {
            this._bank1 = true;
            bankStr = 'Bank 1';
        } else {
            this._bank1 = false;
            bankStr = 'Bank 2';
        }

        this._debug(bankStr, rwStr);
        this._updateBanks();

        return result;
    }
iflan commented 1 year ago

Thanks for the bug report! There's definitely a bug because MMU._prewrite is never actually read.

From Understanding the Apple IIe by Jim Sather pp. 5-23 to 5-24:

Writing to high RAM is enabled when the HRAMWRT' soft switch is reset. The controlling MPU program must set the PRE-WRITE soft switch before it can reset HRAMWRT'. PRE-WRITE is set by odd read access in the *C08X range. It is reset by even read access or any write access in the $C08X range. HRAMWRT' is reset by odd read access in the $C08X range when PRE-WRITE is set. It is set by even access in the $C08X range. Any other type of access causes HRAMWRT' to hold its current state. ... PRE-WRITE and WRITE can be thought of as a write counter which counts odd read accesses in the $C08X range. The counter is set to zero by even or write access in the $C08X range. If the write counter reaches the count of 2, writing to high RAM becomes enabled. From that point, writing will stay enabled until an even access is made in the $C08X range. This means there is a feature of RAM card control not documented by Apple: write access to an odd address in the $C08X range controls HRAMRD without affecting the state of HRAMWRT'.

Let's break that down into more manageable chunks:

PRE-WRITE

PRE-WRITE is set by odd read access in the *C08X range. It is reset by even read access or any write access in the $C08X range.

In the emulator, PRE-WRITE is MMU._prewrite. So, MMU._prewrite should be set when writeSwitch && readMode and reset when (!writeSwitch && readMode) || !readMode. The suggested code captures this intent with:

        if (writeSwitch) { // 0xC081, 0xC083
            if (readMode) {
                if (this._prewrite) {
                    ...
                    this._prewrite = false;  // <-- incorrect ???
        } else {
                    this._prewrite = true;
                }
            } else {
                this._prewrite = false;
            }

            ...
        } else { // 0xC080, 0xC082
            ...
            this._prewrite = false;
            ...
        }

The one thing that I'm not sure about is the reset on read when PRE-WRITE is already set (commented line above). This appears to be an error if Sather is correct. According to figure 5.13a on p. 5-30, PRE-WRITE does not depend on anything but A0 and R/W', which supports not resetting PRE-WRITE when it's enabled. If you can find other evidence, one way or the other, I would be interested.

HRAMWRT'

HRAMWRT' is reset by odd read access in the $C08X range when PRE-WRITE is set. It is set by even access in the $C08X range. Any other type of access causes HRAMWRT' to hold its current state.

In the emulator, HRAMWRT' is MMU._writebsr, but the emulator uses regular logic. So, MMU._writebsr should be set when writeSwitch && readMode && MMU._prewrite. It should be reset when !writeSwitch. The suggested code captures this intent with:

        if (writeSwitch) { // 0xC081, 0xC083
            if (readMode) {
                if (this._prewrite) {
                this._writebsr = true;
                    ...
                }
            ...
        } else { // 0xC080, 0xC082
           this._writebsr = false;
           ...
        }

This seems like it works fine.

I'll try to make a change tonight to fix this bug otherwise I'm sure that @whscullin will get to it before long.

univta0001 commented 1 year ago

Probably need to verify that the fix do not cause any regression using the a2audit test from https://github.com/zellyn/a2audit

univta0001 commented 1 year ago

Reopening the issue as only realized it is fixed in forked version at iflan/apple2js