Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DOS1 mode, page 1 DEV_RW error checking #111

Closed
wants to merge 16 commits into from

Conversation

S0urceror
Copy link

@S0urceror S0urceror commented Sep 11, 2022

Hi, I propose this PR to fix the DOS1 mode, page 1, DEV_RW return value error checking. See DRV.MAC.

I made a build of Nextor on my Mac and had to make a couple of changes to make it build. Please ignore these if they are not relevant.

PS. great Makefile does a lot of magic!

Closes #110

@Konamiman
Copy link
Owner

Hi @S0urceror, thanks for your contribution. May I ask for a few changes:

  1. Remove the changes in printf.c, I have already adapted all the C sources to SDCC 4.2 in Migrate sources and build scripts to Nestor80 #115

  2. Remove the export statement in the makefile. This is specific to your machine, and additionally, the makefile for the kernel allows now to specify the location of specific tools via environment variables, e.g. M80=path/to/M80 make.

  3. Remove the "fix"/"end fix" comments. This information belongs in the source control system, not in the source code.

  4. Add some testing instructions to the pull request: what would be the steps to reproduce the issue that you are fixing, and how to verify that it's actually fixed?

@Konamiman Konamiman added this to the v2.1.2 milestone Apr 16, 2023
jr nc,DIO_WR_OK
; fix by S0urceror
and a
jr z,DIO_RD_OK
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this jump to DIO_WR_OK instead?

@S0urceror
Copy link
Author

S0urceror commented Apr 17, 2023 via email

@Konamiman
Copy link
Owner

Konamiman commented Oct 29, 2023

Hi @S0urceror, I'm preparing the release of Nextor 2.1.2. Would you be able to handle the changes I requested in #111 (comment) and #111 (comment) please? Also there are now conflicts in .gitignore and printf.c that need to be solved (feel free to ask me if you don't know how).

@Konamiman
Copy link
Owner

By the way @S0urceror I've set you as "assignee" for the pull request because it makes it easier for me to see who's the author in the list of open pull requests. Not that I'm your boss giving you work to do or anything like that. 😄

@S0urceror
Copy link
Author

Hi @Konamiman, no problem to perform the things you asked. I thought I already did that way back in April and resubmitted. Sorry for the delay. Thanks for chasing this. Will try to do this later this week.

@S0urceror
Copy link
Author

Hi @Konamiman, made the requested changes.

But somehow I can't figure out .gitignore. I copy & pasted your version of this file into my project and resubmitted and somehow Github says it's still not correct.

Since I'm now using your .gitignore all DS_Store are appearing again in my GitHub which is a pain that Mac users have to endure I'm afraid.

@S0urceror
Copy link
Author

Description
According to the Nextor Driver Development Guide a device driver's DEV_RW call should return it's error code via register A.

Problem
If a driver sets the error code via a LD A, nn call the flags register is not set. Upon return of the DEV_RW function we need to test the value of A. A value of non-zero means and error. Zero means all okay.

Solution
Added AND A and JR Z instructions to handle this.

To test on old version of Nextor
Create a device driver, return zero via LD A, 0 and return, depending on the state of the flags an error is triggered while there isn't one.

@Konamiman Konamiman changed the title fix DOS1 mode, page 1 DEV_RW error checking Fix DOS1 mode, page 1 DEV_RW error checking Nov 5, 2023
@Konamiman
Copy link
Owner

Hi @S0urceror. I have fixed the .gitignore file myself using the GitHub web editor.

If you want these .DS_Store files to be ignored by your local git environment without having to add them to .gitignore, you can do so by adding them (as a .DS_Store/ line) in the .git/info/exclude file, see https://stackoverflow.com/questions/1753070/how-do-i-configure-git-to-ignore-some-files-locally

Now, could you please add an additional commit that:

  1. Removes all the existing .DS_Store directories that you pushed.
  2. Restores the source/command/command/command2.com file that you deleted. I agree that this file si not needed, but this pull request is not the place to delete it.

I tried to do these changes myself by cloning your branch locally, but I don't have the permissions to push any changes to your repository.

@Konamiman
Copy link
Owner

Hey @S0urceror, on a second thought, this thing is actually somewhat more complex, since there's a difference in how errors are reported by device-based drivers vs drive-based drivers:

  • In device-based drivers, DEV_RW indeed returns an error code in A and that's it, with a value of 0 meaning no error.
  • But in drive-based drivers (and this is compatible with MSX-DOS drivers), the carry flag must be set when returning an error from DSKIO, because 0 is an actual valid error code! (it means "Write protected disk").

So upon return of either DEV_RW or DSKIO, the Nextor code should check the carry flag or not, depending on the type of driver (while the relevant code is running, the KSLOT variable temporarily holds 0 for drive-based drivers, and 1 for device-based drivers). The equivalent code for normal mode (in val.mac) is doing that already.

I can implement the required change myself if you want. For that we have two options: either you give me write permissions on your branch, or I close your pull request and then create a new one from scratch. Which one do you prefer?

@S0urceror
Copy link
Author

Hi @Konamiman, sounds indeed this is a bit more convoluted. I think it is best you make the change. Please close my original pull request. I think this is the cleanest way to do this.

I did however come up with the following code that might do what we want. But I leave it to you to come up with a better solution.

	ld	ix,DV_DSKIO##	;Or DEV_RW (they are at the same address)
	ex	af,af'
	ld	a,DV_BANK##
	call	CALBNK##
	push af ; preserve A and flags
	ld	a,(KSLOT##)
	and a
	jr z, DIO_RD_DRIVE_BASED
	; device based
	pop af ; restore error code
	and a
	jr z,DIO_RD_OK
	jr DIO_RD_ERR
DIO_RD_DRIVE_BASED:
	pop af ; restore error code and flags
	jr nc, DIO_RD_OK
DIO_RD_ERR:
	pop	hl	;On disk error, just return
	pop	de
	pop	bc
	pop	bc	;Not POP AF, to preserve error info
	jp	CONV_ERR
DIO_RD_OK:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return value of DEV_RW not correctly checked in DOS1 mode
2 participants