search

MEGA65 / mega65-rom-public

MEGA65 ROM public issue reporting
4 stars 0 forks source link

Monitor's Load and Save Ignore Upper Bytes of Addresses #97

Closed johnwayner closed 1 year ago

johnwayner commented 1 year ago

Test Environment (required)

Example

S"TEST",8 58000 58010

This will actually save the values from 8000 to 800F. Likewise:

L"TEST",8 48010

This will load those same values into 8010.

Expectation

Either the full addresses should be honored, or an error should be shown to the user and no action taken.

Analysis

The ROM monitor (bsmon) uses the Kernal's Load and Save functions to implement its L and S commands. However, those functions require the setting of additional zero page values to work with addresses beyond 16-bit (eal+2/3, sal+2/3).

On Discord, BitShifter talked about the change to the Kernal routines:

The kernal load routine has been expanded to accept a 32 bit load address in the direct page 32 bit pointer eal ($ad,$ae,$af,$b0). The routine has no free registers for parameter passing of the high word eal+2 and eal+3, so you have to set them in your code before calling the load routine. See for example this code fragment from the BLOAD command. The change to 32 bit load address was necessary for loading to the attic RAM, which was not possible with the bank parameter.

https://discord.com/channels/719326990221574164/782757495180361778/1028680048204587068

I originally opened this issue in the mega65-rom project instead of this one. I'm including all these possible fixes I had in that original issue, but it has been decided to fix it for now with the first option: Do as BASIC does and modify EAL/SAL directly.

Possible Fixes

Set the zero page values directly as BASIC65 does

The values are read into Long_AC and Long_PC and could very easily be copied to EAL+2/3 and SAL+2/3 before the Load and Save calls. This is how Basic is using addresses greater than 16-bit with those Kernal calls.

Make Loading and Saving into addresses larger than 16-bit an error

Boooo :)

Modify the Kernal's SETBNK function and use it in bsmon

SETBNK (which was added for the C128) could be modified to allow callers to specify the upper part of the 28-bit addresses. The way the 128 uses this function doesn't really make sense on the Mega65, but it's intention is similar. On the 128 it is used to determine the MMU configuration to be used when performing Loads and Saves (setting symbol ba [zp: $C6]) and also when reading the filename. On the Mega65, the bank value (ba [zp: $BD]) is used directly in the address (but only by Basic!, see ldbank), the Kernal never uses it that I can see.

In addition, the jmp table locations for most of the calls added for the 128 have been shifted a bit so that they are at different locations -- including SETBNK ($FF68 -> $FF6B).

So, SETBNK probably needs some attention regardless -- mainly to decide if it should exist and what it should be used for.

An option is to use it to pass these required values. There are likely several ways to do this. One is to have specific, non-sensical (very high), values for the bank parameter (A) that indicate that the starting or ending higher address bytes are being set.

Add new Kernal functions: Far_Load and Far_Save, and use them in bsmon

Hopefully @dansanderson can elaborate on this option. Having them would indicate to the Kernal that the caller definitely wants to use longer addresses, but those address would still have to be passed in somehow. Perhaps via zero (or base) page quad pointers mimicing what Save uses for the start address?

Register usage there tries to match existing calls, but just an idea.

@dansanderson responded to the original task with:

To elaborate on Far_Load and Far_Save: we can't change the behavior of existing routines without breaking existing callers, so new routines are warranted. I'm not comfortable with leaving the existing routines to honor whatever is sitting in undocumented zero page variables: the existing routines are documented to work with 16-bit addresses, and if they're not zeroing out the new internal high bytes then they should be. Far_Load and Far_Save can accept additional parameters via SETBNK or on the stack.

The monitor is considered part of the KERNAL so it could just do what BASIC does and modify internal variables directly. I prefer the other solutions that also provide a stable API for doing this from third-party programs. I don't see a need for SETBNK to be compatible with the C128 definition given the other ways the KERNAL API has changed. From code comments in the ROM, it does seem like Commodore intended a new definition to serve this role of setting higher address bytes for disk operations. It's merely unfinished.

dansanderson commented 1 year ago

@johnwayner I granted you write access to the repo. Go ahead and make a branch in the repo (from master HEAD), and start the PR from there. Thanks again!