search

MEGA65 / mega65-rom-public

MEGA65 ROM public issue reporting
4 stars 0 forks source link

BASIC: DS not reset after DOPEN#1,"...",W #114

Closed johnwayner closed 3 days ago

johnwayner commented 5 months ago

Test Environment (required)

Describe the bug DS and DS$ should be cleared when a new disk operation is executed.

To Reproduce See screen shots.

Expected behavior The following commands are disk operations and should (maybe?) clear DS and DS$:

Whew... so many. Maybe this is why DS was originally cleared each time it was read?

Screenshots ds_not_cleared

Similar program on C128: c128_ds_test

Additional context This previously closed issue changed how DS and DS$ were cleared: https://github.com/MEGA65/mega65-rom-public/issues/85 These disk operations should have been modified to ensure they cleared them.

Related documentation issue: https://github.com/MEGA65/mega65-user-guide/issues/590

Rhialto commented 1 week ago

Some of the commands do not necessarily refer to a disk operation (I presume that like in the older ROM versions, OPEN can open a channel to the keyboard, and certainly to an IEC printer) so maybe DS should not always be cleared...

dansanderson commented 1 week ago

Fair point. From a quick test of C128 in VICE, it indeed looks like opening a channel to a non-disk device does not clear DS/DS$. This could probably happen in BASIC DOS set-up.

OPEN 5,8,5,"NOTPRESENT,S,R"
PRINT DS
OPEN 1,3
PRINT#1,"HI"
PRINT DS

Note that RUN will reset DS as part of start-of-program run state, even though it's not always a disk operation. We should double-check that that still works with whatever fix we decide.

dansanderson commented 6 days ago

Did a quick code review, this isn't definitive, but I'm looking for the fewest number of jsr Clear_DS calls that will accomplish this. My first guess that appears to cover almost all of the list:

OPEN and CLOSE are tricky, unfortunately. Within BASIC, they're just calling KERNAL routines. To replicate the C128 behavior precisely, these would need extra logic to test the device ID for "is a disk." My first guess is that we should punt on getting this exactly right and just have every OPEN and CLOSE clear DS.

dansanderson commented 4 days ago

OK, I audited all the commands.

DOPEN# W sets bit 6 of parsts, which causes sendp to be called with A=8 (vs. A=4 for non-W), where A is "number of bytes in format." It looks like this simply translates to ",W" in the DOS filename string, and everything else is the same. I'm not sure how this call path fails to update DS, so I don't know the appropriate fix yet.

As noted above, MERGE and CHDIR U12 have unique call paths, so that makes sense. Maybe explicit Clear_DS calls are appropriate here.

DLOAD "BAD" while in EDIT mode actually swallows the error incorrectly. It is exiting to list_done when the disk status >= 20, which might be incorrect; list_err might be more correct. It's similar to IMPORT, which works. I'll mess with it.

I can say that blindly adding jsr Clear_DS to all the places I listed caused all kinds of problems. 😅 Cleaning up just these few paths is the more appropriate change.

All the other commands listed are clearing DS correctly, as far as I can tell. These appear to be Read_DS calls that are reading the 0 directly from the disk device, and not Clear_DS calls.

dansanderson commented 4 days ago

Filed a separate bug for EDIT mode DLOAD not reporting errors: https://github.com/MEGA65/mega65-rom-public/issues/142

Submitted a change to fix MERGE and CHDIR U12 to reset DS.

This bug is now specifically about DOPEN#1,W.