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

Various Toolbox fixes #420

Merged
merged 6 commits into from
May 16, 2024
Merged

Various Toolbox fixes #420

merged 6 commits into from
May 16, 2024

Conversation

nielsmh
Copy link
Contributor

@nielsmh nielsmh commented May 9, 2024

This PR addresses some issues I found with the Toolbox implementation from #406, while implementing my own client for DOS/ASPI.

Note that these changes are only tested with my own ZuluSCSI RP2040 based hardware, on a single machine with an Adaptec host controller using ASPI8DOS.sys driver. The client software I have developed/tested with is published at https://github.com/nielsmh/escsitoolbox.
I do not own a Mac capable of running the original Toolbox software, so I don't know if these changes affect compatibility with that.

Use constants for more magic numbers.
Ensure filename buffer is zeroed.
Avoid overrun of SCSI output buffer when max number of files is reached.
Only report as many bytes to write as actually filled in output.
Print additional debug messages.
Tested with MSCDEX and SHSUCDX, without this change they do not detect the media change, and keep reporting the file system of the first loaded CD image.
If after changing image the user then attempts to navigate the new CD regardless, eventually the OS driver will get into a weird state and be unable to use the drive again.
@aperezbios
Copy link
Collaborator

@nielsmh thank you for this valuable contribution. This is awesome :) We will merge it after testing on our end has concluded.

@morio morio merged commit 546f1f6 into ZuluSCSI:main May 16, 2024
1 check passed
@morio
Copy link
Collaborator

morio commented May 16, 2024

Other 0xD8 vendor commands pass testing
Thanks for the PR

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.

3 participants