Review LOADB+SAVEB error handling

[jkotlinski]

Do LOADB+SAVEB have appropriate error handling? If not, fix it.

see discussion in #357

[Whammo]

LOADB has two modes of failure.

1. If the high byte of the destination address is zero, it still executes LOAD but sets load_binary_status to fail, executes _errorchread but zeropage could have been trashed- or loaded to.
2. bcs .disk_io_error after LOAD also sets load_binary_status to fail,

[Whammo]

I'm pretty sure this works like it should.
open.fs

marker foo
: setlfs ( file# sa -- )
$b9 c! $b8 c! ;

: setnam ( nameaddr namelen -- )
$b7 c! $bb ! ;

\ Open a logical file
\ ioresult is 0 on success, kernal
\ error # on failure.
( nameaddr namelen file# sa --
  ioresult )
: open
 setlfs setnam [ w stx,
$ffc0 jsr, \ OPEN
+branch bcs, \ carry set = error
0 lda,# \ A is only valid on error
:+
w ldx,
lsb sta,x
0 lda,# msb sta,x
] ;

\ Close a logical file
code close ( file# -- )
txa, pha,
lsb lda,x \ x = file#
$ffc3 jsr, \ CLOSE
pla, tax, inx,
;code

: errchk  sr c@ 1 and if
ar c@ else
0  then ioabort ;

: load  ( sa dst -- ioresult )
xr ! ar c! $ffd5 sys errchk ;

: save ( start end zeropage -- ioresult
)
dup ar c! swap xr ! !
$ffd8 sys errchk ;

( filenameptr filenamelen dst -- endadd
)
: loadb
1 0 setlfs -rot setnam 0
swap load drop $ae @ ;

: saveb ( start end nameaddr namelen --
)
1 1 setlfs setnam $c1 save drop ;

[jkotlinski]

Some thoughts about existing code:

• I think it would be good if LOADB and SAVEB return an ioresult that can be checked by calling code. If one is too lazy to do proper error handling, at least it's easy to call ioabort
• It might be more straightforward if LOADB returned the number of read bytes.
• It's not so nice that LOADB and SAVEB assume that logical file# 1 is free to use. But it's maybe livable.
• It's not good that LOADB and SAVEB check the error channel in case of failure. If there is no device, it will hang.

EDIT: changed my mind on most of these points.

[jkotlinski]

i discovered now, thanks to $9d, CHKIN to device 0 (keyboard) actually is an error. CLRCHN should be used instead. see ea486d8

[Whammo]

Some thoughts about existing code:

• I think it would be good if LOADB and SAVEB return an ioresult that can be checked by calling code. If one is too lazy to do proper error handling, at least it's easy to call ioabort
• It might be more straightforward if LOADB returned the number of read bytes.
• It's not so nice that LOADB and SAVEB assume that logical file# 1 is free to use. But it's maybe livable.
• It's not good that LOADB and SAVEB check the error channel in case of failure. If there is no device, it will hang.

ioresult could be a variable that is simply overwritten with the next file, stack, or even a FIFO buffer

[jkotlinski]

yes, that could work too, with the advantage that it's not a breaking change.
benefits of returning ioresult is that it encourages error handling, and it is similar to standard word READ-FILE.

[jkotlinski]

it's also a bit of open question if LOADB/SAVEB should be interactive or not.
the existing words are interactive, in that they read and print the command channel.
my gut feel is that the words are more generally useful if they are Not interactive, and instead return ioresult. but I think I could be convinced otherwise.

[jkotlinski]

...I think I change my mind! I now think it's fine if LOADB/SAVEB keep their current signatures, and are "interactive"
but they should be improved so that they always give some nice feedback to user, and never hang or lock up

Two problems to fix:

• 12 device parse-name foo $7000 loadb hangs
• 12 device $7000 $7100 parse-name foo saveb hangs

[Whammo]

ST is supposed to set a timeout show serial bus timeout with Bit 2. I am unaware of the conditions, but it's something to look into.
Some sources say it's bits 1 for read or write and 2 that it occurred. 1541s generally don't do this.
They all agree that bit 7 is device not present.
Something to consider, load and save block. open does not.

[Whammo]

Load/Save uses fast loaders, so that's not going anywhere. so merely opening channel 15 first, then you can readst, and see if the device is present first. abort or close and then load/save.

[jkotlinski]

Oh! I forgot about fastloaders. But don’t they also follow the regular pattern with returning status by carry flag + accumulator? In that case, LOAD failure could be detected and handled before trying to read error channel.

…

On Tue, 31 Aug 2021 at 08:50, Whammo ***@***.***> wrote: Load/Save uses fast loaders, so that's not going anywhere. so merely opening channel 15 first, then you can readst, and see if the device is present first. abort or close and then load/save. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#363 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAY34O3BERHMYYSR2323DP3T7R3SLANCNFSM5DBESVRA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[Whammo]

The point is, LOAD blocks on a device not present error, right? So you can't check, the carry, it hangs.
So if you can open an arbitrary file or the command channel first, you don't have to read it, but you know the device exists.
Then you can safely LOAD

[Whammo]

Anyway, _errorchread definitely hangs if DEVICE not present.

: ecr $1551 sys ; ( Version 3 address of _errorchread )

Try it with different device numbers.

[jkotlinski]

I find that LOAD does in fact return an error in A:

( set filename 'x' )
'x' w c!
1 ar ! w xr ! 0 yr !
$ffbd sys ( setnam )

( logical file 1
  device 10 - non existing
  secondary address 1 )
1 ar ! 10 xr ! 1 yr !
$ffba sys ( setlfs )

( LOAD file to memory )
0 ar ! $ffd5 sys

( prints 5 - DEVICE NOT PRESENT error )
cr
ar c@ .

[Whammo]

I was meaning LOADB and SAVEB hangs, no way to check status.

[jkotlinski]

Oh yes! I’d like to see that fixed

…

On Tue, 31 Aug 2021 at 23:46, Whammo ***@***.***> wrote: I was meaning LOADB and SAVEB hangs, no way to check status. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#363 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAY34O3DYYREC3GSGXMNBHTT7VESFANCNFSM5DBESVRA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[jkotlinski]

It was complicated to fix loadb and saveb for every kind of error, so I'm abandoning this effort for now.

I follow your suggestion to let v check for a good device number: 9a4e8ce

[jkotlinski]

Created follow-up issue for the loadb/saveb hangs.

#499