jkotlinski / durexforth

Modern C64 Forth
Other
230 stars 28 forks source link

LOADB/SAVEB hang on invalid device #499

Closed jkotlinski closed 1 year ago

jkotlinski commented 1 year ago

12 device parse-name foo $7000 loadb <-- hangs

12 device $7000 $7100 parse-name foo saveb <-- hangs

Whammo commented 1 year ago

IEC commands do not freeze on invalid device.

482

 code ds
      w stx,
    $00 lda,#
    $90 sta, \ clear st
    $ba lda,
  $ffb1 jsr, \ listen
    $6f lda,#
  $ff93 jsr, \ second #15
  $ffae jsr, \ unlisten
    $90 lda, \ readst
+branch bne, \ device not present?
\ checking for the device is optional
\ but a good idea.
    $ba lda,
  $ffb4 jsr, \ talk #8
    $6f lda,#
  $ff96 jsr, \ tksa #15
here
    $90 lda, \ readst
+branch bne,
  $ffa5 jsr, \ acptr get byte
  $ffd2 jsr, \ chrout
swap    jmp,  \ loop
:+
  $ffab jsr, \ untalk
:+
      w ldx, ;code
Whammo commented 1 year ago

Which means DOS will also freeze when device is invalid.

Whammo commented 1 year ago

It's not load or save that freezes, it's open.

Whammo commented 1 year ago

It might be simpler to vector kernal open through something like this:


        LDA #$00
        STA $90       ; clear STATUS flags

        LDA $BA       ; device number
        JSR $FFB1     ; call LISTEN
        LDA #$6F      ; secondary address 15 (command channel)
        JSR $FF93     ; call SECLSN (SECOND)
        JSR $FFAE     ; call UNLSN
        LDA $90       ; get STATUS flags
        BNE .devnp    ; device not present
        JMP open
.devnp
        ... device not present handling ...
        RTS
jkotlinski commented 1 year ago

The original code for reading error channel is here: https://codebase64.org/doku.php?id=base:reading_the_error_channel_of_a_disk_drive

" A small warning: Both the BASIC and the Assembler versions will deadlock if the device is not present. "

Edit: It also presents an alternate version which avoids the deadlock.

Edit 2: OK, it looks like you found the same code :-)

jkotlinski commented 1 year ago

OK, I think the above commit might work.

The codebase64 page says:

"In this version the status-byte is accessed directly which reduces the portability of the code." <-- That's not great, but OK...

"This will limit you to IEC-bus devices" I am not sure what practical consequences that has. Would one ever want to LOADB/SAVEB to something else?

Whammo commented 1 year ago

You can use kernal readst, but unlike basic, it doesn't clear after reading. EOF (st=64) isn't really an error is it? Save to cassette??

Whammo commented 1 year ago

We have to reset status before IEC commands because they only change ST on failure.

Whammo commented 1 year ago

Yes, DOS, INCLUDED and OPEN, and anything with kernal open is subject to hang on device not present.

Whammo commented 1 year ago

All the info I used to master kernal IEC

https://commodore.software/downloads/download/215-articles-and-guides/15601-iec-disected

Whammo commented 1 year ago

Another advantage of IEC commands is they don't consume file numbers.

jkotlinski commented 1 year ago

When testing with #501, these cases produce error messages and no hang:

12 device parse-name foo included

12 device dos scratch0:test

Maybe everything is fine with #501? Am I missing something?

Whammo commented 1 year ago

12 device dos no string freezes 12 device dos i okay. ~workaround?~ INCLUDED working correctly is good news.

Whammo commented 1 year ago

The problem is a bug with the Kernal opening a file with no file name. It creates the file entries in the tables so chkin doesn't error , then exits. The command channel doesn't require an (IEC) open or a close, so it doesn't check if the device is there before it goes to chrin, (acptr) then it gets stuck waiting. One solution is to never close 15.

jkotlinski commented 1 year ago

One could add a 0 check to send-cmd, but maybe it is equally fine to let user exit the bad state by pressing RESTORE?

After all, DOS with empty string is not a real use-case?

On Wed, 28 Dec 2022 at 10:56, Whammo @.***> wrote:

The problem is a bug with the Kernal opening a file with no file name. It creates the file entries in the tables so chkin doesn't error , then exits. The command channel doesn't require an open or a close, so it doesn't check if the device is there before it goes to chrin it gets stuck waiting.

— Reply to this email directly, view it on GitHub https://github.com/jkotlinski/durexforth/issues/499#issuecomment-1366518846, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAY34O3CHTE3DBGEVCRIWBTWPQFFXANCNFSM6AAAAAATKIP26I . You are receiving this because you authored the thread.Message ID: @.***>

Whammo commented 1 year ago

DOS with an empty string prints the current device status if it's there, and hangs otherwise. This is why BASIC insists upon device numbers for anything other than tape.

jkotlinski commented 1 year ago

I see, so it is a use case. Maybe send-cmd should be rewritten, too, to use IEC instead of OPEN?

Whammo commented 1 year ago
: send-cmd ( addr len -- )
?dup if
$f $f open ioabort 
clrchn $f chkin ioabort
begin chrin emit readst until
clrchn $f close cr
else drop _errorchread then ;

perhaps?

But yes it's doable.

jkotlinski commented 1 year ago

That seems like a fine solution. Added!

Whammo commented 1 year ago

I think there were some additions to V that can be rolled back now.

jkotlinski commented 1 year ago

Oh really? What V additions?

Whammo commented 1 year ago

added v check for bad device# ?

jkotlinski commented 1 year ago

ah right! great catch. thank you!

Whammo commented 1 year ago

At some point in the future maybe, we can get durexForth off of the filesystem and onto IEC commands so file numbers used by the system would go away.

jkotlinski commented 1 year ago

That stuff is a bit above my head probably :-)

Whammo commented 1 year ago

Serial IEC is certainly arcane. The documentation is sparse. The IEEE parallel PET stuff is well documented, of which the serial stuff is a tiny subset, the overview makes it understandable.

Whammo commented 1 year ago

Wow! Thanks. I'll try not to f- anything up. 🥇