r39 patched Kernal to fix LOAD into HiRAM functionality

[ZeroByte]

One of the annoying things about using SD card mode is that LOAD fails when loading into banked RAM.

As many know, when using the emulator without an SD image, it performs a hyperload directly into memory w/o emulating the actual file transfer. It does this by skipping the kernal routine entirely. When using SD card images, the Kernal runs as normal.

Using hyperload, you can issue a command to BASIC like this:

LOAD "MYFILE",8,3,$A000

This loads MYFILE into bank 3 of Hi Ram, and it is automatically continued into banks 4,5,6... as necessary. This operation fails when using Kernal (SD card attached). The same works/fails conditions apply when calling SETNAM,SETLFS,LOAD from within programs.

I got really really tired of this, so I decided to fix it in the Kernal. As I'm not a code god, I'd like to see if anyone experiences any instabilities or crashes with it before I do a pull request to include the fix on the main repo. The ROM image is attached below. Use it with emulator R39 or Box16. It will not work in R38.

I tested the performance with my additional code and the impact is negligible. I haven't checked what the return values look like after a LOAD into Hi RAM. The Kernal's output message gives the memory address where the load finished, and the final bank remains active, so I presume the return values in X/Y are similar. There's not much space available in the ROM so I'm trying to keep from adding too much to it.

Let me know if you experience any crashes with it that don't happen using the current repo version of R39.

Thanks.

EDIT: Dec 05, 21:17 UTC  (3:17pm Central time): Updated the attached ROM to fix a bug reported by @desertfish

EDIT2:

UPDATE - I've re-done the patch with a much smaller code size, saving over 60 bytes vs. my previous patch. The new one is MUCH easier code to read and follow, and it's only slightly slower. For a 96KB file I'm testing with, the previous patch took 60 jiffies (frames) to load, while the new routine took 63 jiffies. This speed penalty only happens on loads into banked memory, so I call that a win. I have updated the file here again.

If you downloaded the previous ones, then I recommend that you re-download this one, as it's the one I'm using for the pull request into the Kernal repo.

 

 
rom-r39-fixedload.bin

[ZeroByte]

Some questions from the Discord channel:

The X16-docs repo is not up-to-date regarding the behavior of load. It doesn't document the VLOAD command, and explains loading into VRAM using old LOAD syntax (and old VERA banking schemes which no longer exist).

Do not be misled by this errata. The intended behavior of LOAD (according to emulator behavior, and comments in the BASIC ROM source)  is that it should have 2 forms:

• 
  LOAD "FILENAME"[,device[,redirect]]
  
  
  
  • 
    redirect is the familiar ,1 which means LOAD will use the 2-byte file header as the load address, not start-of-BASIC
    
  
  
  
  


• 
  LOAD "FILENAME"[,device[,bank,address]]
  
  
  
  • 
    This is the way to specify a target memory location for LOAD - the 2-byte header is simply skipped and the remainder of the file is loaded to the specified address
    
  
  
  
  

It is this second form which I've fixed. It previously worked for destinations in main memory, but would result in bank0 being overwritten insted of the intended bank. Bank overflow was also not supported - the LOAD would just continue up into the ROM region.

EDIT: I've tested the first form with my patch, and I'm happy to report that this version is also fixed. If the file's 2-byte header results in a LOAD into banked memory, the loading behavior is the same as if you had specified the load address in the LOAD command itself. The one difference for this form is that the active bank will become the first one used during the LOAD - i.e. if bank 6 is active, then the file will LOAD into 6,7,8....

[ZeroByte]

One more note: my patch has a corner case that is not fixed: if a LOAD begins in low memory and proceeds up to the BANK area, crossing over IO space, it will usually write some overflow data into RAM bank0 (the Kernal's space) instead of the selected bank.

Fixing this would require one of the following:

1. 
   adding more code to the patch (the DOS bank is almost full so this is undesirable) to handle this case
   


2. 
   changing the algorithm a bit to reduce the code size and handle the case, but adding ~1000 CPU cycles per page of data transferred into HiRam
   

I think it's best to let the write spill into Kernal's RAM bank, as it's already a bad day if a LOAD is writing data into IO space. Go ahead and crash hard so developers/users figure out not to do whatever they've done.

What say ye, community? Do you agree with this line of reasoning?

Note that the ROM I've posted has one difference from my current revision. Current revision allows the $9Fxx write to corrupt the Kernal ram bank. The one posted here tried to fix that, but it still has a bug where the first byte of a write to $9Fxx will appear at $BFxx in bank-1. The rest go to IO space, or the beginning of the correct bank.

EDIT - this bug has been fixed.

[Edmond D]

On 12/3/2021 at 12:34 PM, ZeroByte said:

I think it's best to let the write spill into Kernal's RAM bank, as it's already a bad day if a LOAD is writing data into IO space. Go ahead and crash hard so developers/users figure out not to do whatever they've done.

I'm not for this approach.  I worked in the automation control industry and randomly writing into I/O space was just wrong and could cause permanent damage to man & machine. While I don't expect that anyone would have an X16 hooked up to an industrial welding robot, they many have their own hardware connected and not have designed in failsafes for random events. 



I'd suggest an approach that prevented a load crossing into I/O space or beyond. Just reject the load, or fail just at point of crossing into danger.

It's just safer & easier to prevent a crash then try to pick up the pieces afterwords.  

[desertfish]

the current LOAD routine already walks right over the I/O area I believe, so it didn't get worse (but not better either in this regard).

[Edmond D]

On 12/3/2021 at 3:26 PM, desertfish said:

the current LOAD routine already walks right over the I/O area I believe, so it didn't get worse (but not better either in this regard).

Thanks for letting me know. I'm glad I've only got an emulator running. I'd hate to have hardware with this design flaw in it. ?

[kelli217]

There's a limit to how sophisticated these things can be, given speed and size constraints of retro or retro-like systems.

[rje]

On 12/3/2021 at 5:34 PM, Edmond D said:

Thanks for letting me know. I'm glad I've only got an emulator running. I'd hate to have hardware with this design flaw in it. ?

A: "It's not a bug, it's a feature."

B: "But it's caught on fire..."

 

[ZeroByte]

I did find a way to fix my modifications to the copy routine such that it now correctly handles writes to the $9Fxx page, sending any offset bytes that reach the bank window into the intended bank. The fix only added 2 bytes to the size in doing so. The DOS ROM has virtually no free space left in it, so I'm trying to be as frugal with space as possible. The normal non-bank-area copy routine could be shrunk by a few bytes but would add ~500 clock cycles per page transferred, so I opted to leave it alone.

I've updated my fork with this latest fix.

As for making LOAD throw errors if it reaches IO space: Adding error conditions to LOAD is beyond the scope of this patch. Do keep in mind that for microcomputers like this, it's assumed that everything works a certain way and expects you to do things properly w/o any safety nets due to resource constraints. There just isn't enough time or space to cover everything that might go wrong for a general-purpose system. Anyone using a 6502 to control critical systems would be writing bespoke code and running it on bare metal and thoroughly testing it. There's also the case where a LOAD into HIRAM which walks off the end of the installed memory chips could happen, but the code doesn't check that either. There's also no way to know you aren't overwriting something else important like other data you didn't intend to overwrite or zeropage, or the stack, or the kernal/basic area of memory which also happens to include the IRQ/BRK/NMI vectors...

EDIT: I've updated the ROM file on this post to include this bugfix.

[BruceMcF]

On 12/3/2021 at 6:58 PM, ZeroByte said:

... As for making LOAD throw errors if it reaches IO space ...

The more period-correct way to avoid that kind of overwrite would be to end the load if it hits $9F00 ... maybe set carry to indicate the load was abandoned early.

If that is built into the binary page adjustment, then you need the binary page adjustment to always take place on a true binary page boundary ... so if, eg, the start address was $8080, then the destination page would be $8000 with $80 in the index register, and when the index register hits zero, increment the high byte of the page address and check if it equals $9F ... if so, jump to the "load finished" handling.

If the START address was in the I/O page, that wouldn't change anything. Which is also period-correct. If you start with an I/O page address, you better know what you are doing.