Rework io #310

rhalkyard · 2020-12-17T20:54:27Z

Here's a proposal to close #252 and improve the flexibility of the IO words as discussed in that issue:

I have removed OPENW and CLOSEW and replaced them with two source files:

open.fs contains 'raw' open and close routines without error handling, for use with V
io.fs contains IO words for all Kernal routines, with error handling

The reason for splitting them up is twofold:

V only uses the un-error-checked (OPEN) and (CLOSE), so by breaking them out into a separate file, we avoid cluttering the default dictionary and don't take up any extra cartridge space.
The Commodore 128 port that I'm working on (see C128 version? #95 and Commodore 128 Port #305) needs to do some additional setup as part of (OPEN), so it will become a platform-specific component anyway.

Additionally, I have added a small DOS-command and drive-status utility in dos.fs, which also serves as an example of using IO words for both input and output.

Finally, I've written docs for the new words, and added a small printer example to the tutorial.

Removed old `OPENW` and `CLOSEW` words from disk.asm that were undocumented, and only used by `V`. Added new words corresponding more closely to their underlying kernal calls. `open.fs` contains the minimal set of IO words needed by `V`, and is included in the default dictionary, `io.fs` adds the remaining words and provides error checking.

rhalkyard · 2020-12-17T21:01:16Z

@jkotlinski do you have any strong preference about OPEN and CLOSE staying in disk.asm? I figured that this way, people could strip them out if they didn't need them.

Let me know what you think about the (OPEN) vs OPEN naming scheme - I wanted to make it possible for programs to handle errors themselves, without forcing everybody to have to bother with that.

jkotlinski

Added a lot of random comments. Thanks for the awesome contribution, I'm impressed!

forth_src/dos.fs

CHANGELOG.md

disk.asm

docs/tutorial.tex

forth_src/dos.fs

forth_src/open.fs

docs/tutorial.tex

docs/words.tex

Whammo · 2020-12-19T00:49:49Z

If DEVICE were to fall into _errorchread in disk.asm, it could indicate whether the choice is valid or not.

rhalkyard · 2020-12-19T01:18:17Z

If DEVICE were to fall into _errorchread in disk.asm, it could indicate whether the choice is valid or not.

Unfortunately, trying to read the error channel of a nonexistent device causes the kernal to go into a deadlock, so it isn't quite that straightforward: https://codebase64.org/doku.php?id=base:reading_the_error_channel_of_a_disk_drive

It is possible to work around that issue, but a check like that wouldn't work with devices like printers and plotters, and I'm cautious about adding too much special-case logic to the IO words - ideally I think they should map very closely to the underlying Kernal calls.

Whammo · 2020-12-19T01:41:43Z

Very true! Thanks! feature creep <> lean

rhalkyard · 2020-12-19T02:35:33Z

Very true! Thanks! feature creep <> lean

I like the potential of building up a more 'friendly' IO library on top of this base, though. I'm imagining a library that programs could include if they wanted and leave out if they didn't.

I'm not sure how well it would map to Forth, but it would be nice to have some one-word equivalents to BASIC's INPUT#, GET# and PRINT# statements so you don't have to keep juggling chkin and clrchn - that's where I'd put more thorough error checking and input validation.

Whammo · 2020-12-19T03:00:48Z

Or at least these equates:

$ffb7 constant readst
$ffba constant setlfs
$ffbd constant setnam
$ffc0 constant open
$ffc3 constant close
$ffc6 constant chkin
$ffc9 constant chkout
$ffcc constant clrchn
$ffcf constant chrin
$ffd2 constant chrout

- (open), (chkin) and (chkout) renamed to open?, chkin?, chkout? and return file number and detailed error code - clrch renamed to clrchn - ioerr prints meaningful error message - made dos command utilities less hackish - fixes to docs, comments and changelog

rhalkyard · 2020-12-19T03:49:41Z

Or at least these equates:

Yeah, I've throught about making an include-able file with the addresses of useful Kernal routines and variables - especially because on the C128, some of them are in different places, so referring to them by name rather than by address makes it easier to write code that runs on both systems.

jkotlinski · 2020-12-20T14:09:49Z

forth_src/io.fs

+\ Set output device to open file #
+\ 'easy' version, aborts on error
+: chkout ( file# -- )
+chkout? ioerr ;


I think everything looks kind of good, the one thing I'm not sure about are these "easy" versions. Maybe it would make things in practice simpler, to only have one set of open/chkin/chkout words that always return a status?
When looking in C and Forth libraries, this to me seems like the normal thing to do.

I think cc65 is a good reference: https://cc65.github.io/doc/funcref.html#ss2.11

For those who would like an "easy" version with inbuilt error handling, it could always be added later as a separate, optional module?
What do you think?

I also kind of feel the same about ioerr - it's nice but I'm not sure if it's needed.

As a contrary reference, 64Forth OPEN/CHKIN/CHKOUT seem to have in-built error handling - or at least those functions do not return any value to indicate success or failure.

Forth is kind of difficult because the only thing you know, in ease of use and performance it is "supposed" to be somewhere between BASIC and C - and that leaves quite a lot of space for tradeoffs and design choices. 🤷

I think what definitely speaks for having ioerr around is that the printer example now looks simple and good.

Changing printer example to e.g. 0 0 7 47 open ioerr ( open address 7 on device 4 as file 47 ) would make it a bit less nice, but it's still OK.

Leaving all error handling to be implemented by the user, then it feels like a correct printer example will not be nice anymore.

I would consider renaming ioerr to ioabort, to make it easier to understand what it does.

I am sorry for all the "loud thinking" but my current opinion is, as a summary:

drop the "easy" versions, only keep one set of open/chkin/chout

rename ioerr to ioabort and document it

in documentation, rename "result" of above word signatures to "ioresult" to clarify that these words are supposed to be used together

With these changes, I think it will turn out really great!

I like this idea a lot. open ioabort isn't really that much more to type than just open. I think it hits a nice middle ground of 'more expressive than BASIC, but easier than asm or C'.

jkotlinski · 2020-12-20T15:01:47Z

forth_src/open.fs

@@ -0,0 +1,35 @@
+\ Open a file, no error handling
+\ returns 0 on success, file # on error


This comment looks wrong, should it be "\result=0 on success, or OPEN error code" ?

jkotlinski · 2020-12-20T15:02:24Z

forth_src/open.fs

+0 lda,# msb sta,x msb 1+ sta,x
+;code
+
+\ Close a file


For ultimate correctness I guess it should be "close logical file"

jkotlinski · 2020-12-20T15:03:15Z

forth_src/io.fs

+require open
+
+\ Set input device to open file #
+\ returns 0 on success, file # on error


This comment looks wrong, should it be "\ result=0 on success, or CHKIN error code" ?

jkotlinski · 2020-12-20T15:03:47Z

forth_src/io.fs

+
+\ Set output device to open file #
+\ returns 0 on success, file # on error
+code chkout? ( file# -- file# result )


This comment looks wrong, should it be "\ result=0 on success, or CHKOUT error code" ?

jkotlinski · 2020-12-20T15:09:28Z

forth_src/viceutil.fs

@@ -54,6 +56,6 @@ else emit then loop
 $a emit 1 ;

 : dump-labels base @ >r hex
-s" words" 1 openw
+s" words,w" 1 1 open 1 chkout


I think this would be an excellent use case for the new ioerr/ioabort!

jkotlinski · 2020-12-20T15:17:08Z

CHANGELOG.md

- - Made QUIT do CLRCHN instead of CHKIN.
+### Removed
+ - Removed undocumented OPENW and CLOSEW words.
+ - Made QUIT do CLRCHN instead of CHKIN


Merge error. This line should be moved up to ### Fixed section.

jkotlinski · 2020-12-20T15:55:52Z

I like the potential of building up a more 'friendly' IO library on top of this base, though. I'm imagining a library that programs could include if they wanted and leave out if they didn't.

I'm not sure how well it would map to Forth, but it would be nice to have some one-word equivalents to BASIC's INPUT#, GET# and PRINT# statements so you don't have to keep juggling chkin and clrchn - that's where I'd put more thorough error checking and input validation.

Adding BASIC equivalents should be possible. It's nice for beginners, the main problem is that those words would have a lot of overhead. But as long as it is opt-in, I cannot see a problem with adding that.

* Removed 'easy' IO words * 'ioerr' renamed to 'ioabort' and documented * documentation and comment fixes

jkotlinski

Brilliant! Thank you for the contribution. Happy holidays!

rhalkyard added 3 commits December 17, 2020 14:26

Add printer example to docs

e4b9ee0

Utility for DOS commands and drive status

edc3a94

jkotlinski added the enhancement label Dec 17, 2020

jkotlinski reviewed Dec 18, 2020

View reviewed changes

Changes per PR feedback:

3db0bd9

- (open), (chkin) and (chkout) renamed to open?, chkin?, chkout? and return file number and detailed error code - clrch renamed to clrchn - ioerr prints meaningful error message - made dos command utilities less hackish - fixes to docs, comments and changelog

rhalkyard force-pushed the rework-io branch from 4b2f275 to 3db0bd9 Compare December 19, 2020 03:07

rhalkyard and others added 2 commits December 18, 2020 21:07

Added cautionary note about low-level IO words

b2fef04

Merge branch 'master' into rework-io

c59d92a

rhalkyard mentioned this pull request Dec 19, 2020

viceutil.fs is broken #309

Closed

rhalkyard added 2 commits December 19, 2020 21:39

Merge branch 'master' into rework-io

d29b5d8

update dump-labels to use new io words

2705154

jkotlinski reviewed Dec 20, 2020

View reviewed changes

Changes per PR review

17de1cf

* Removed 'easy' IO words * 'ioerr' renamed to 'ioabort' and documented * documentation and comment fixes

jkotlinski approved these changes Dec 25, 2020

View reviewed changes

jkotlinski merged commit eeca09a into jkotlinski:master Dec 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework io #310

Rework io #310

rhalkyard commented Dec 17, 2020

rhalkyard commented Dec 17, 2020

jkotlinski left a comment

Whammo commented Dec 19, 2020

rhalkyard commented Dec 19, 2020

Whammo commented Dec 19, 2020

rhalkyard commented Dec 19, 2020 •

edited

Loading

Whammo commented Dec 19, 2020

rhalkyard commented Dec 19, 2020

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading

rhalkyard Dec 23, 2020

jkotlinski Dec 20, 2020

jkotlinski Dec 20, 2020

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski commented Dec 20, 2020

jkotlinski left a comment

		@@ -0,0 +1,35 @@
		\ Open a file, no error handling
		\ returns 0 on success, file # on error

Rework io #310

Rework io #310

Conversation

rhalkyard commented Dec 17, 2020

rhalkyard commented Dec 17, 2020

jkotlinski left a comment

Choose a reason for hiding this comment

Whammo commented Dec 19, 2020

rhalkyard commented Dec 19, 2020

Whammo commented Dec 19, 2020

rhalkyard commented Dec 19, 2020 • edited Loading

Whammo commented Dec 19, 2020

rhalkyard commented Dec 19, 2020

jkotlinski Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

jkotlinski Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

jkotlinski Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

jkotlinski Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

rhalkyard Dec 23, 2020

Choose a reason for hiding this comment

jkotlinski Dec 20, 2020

Choose a reason for hiding this comment

jkotlinski Dec 20, 2020

Choose a reason for hiding this comment

jkotlinski Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

jkotlinski Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

jkotlinski Dec 20, 2020

Choose a reason for hiding this comment

jkotlinski Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

jkotlinski commented Dec 20, 2020

jkotlinski left a comment

Choose a reason for hiding this comment

rhalkyard commented Dec 19, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading

jkotlinski Dec 20, 2020 •

edited

Loading