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

Latest nightly release (20200607 Build) fails to convert 3" SCP copy protected disks #24

Closed
TomDDG opened this issue Jul 22, 2022 · 10 comments

Comments

@TomDDG
Copy link

TomDDG commented Jul 22, 2022

Previous nightly dated 20200429 Build works fine. Not sure if the change to read single sided SCP has broken this. Also the last stable release doesn't convert some 3" SCP disks.

I test the conversions using RetroVirtualMachine and FUSE. FUSE just crashes and RVM just fails to load. Those converted with nightly build work fine in both.

Using a hex editor it seems to be adding 2 sides to the converted DSK which should only have 1

@simonowen
Copy link
Owner

By default it creates the output with the same geometry as the input, which suggests it thinks there are two sides in the source image. Is a sample disk image available that demonstrates it?

I do remember the SCP specification was changed to add single-sided support at some point, and PR change between the nightly builds should add support for that. Though that should mean the later version is better behaved with single-sided SCP images, and your bug report say otherwise!

If you add -h0 it'll limt the copy to just the first side, even if the source is double-sided. I have seen some double-sided DSK images on the CPC, with an emulator option to flip the disk an access the second side. Though as long as the contents of the first side are correct I'm surprised it would be enough to crash FUSE.

@TomDDG
Copy link
Author

TomDDG commented Jul 24, 2022

Thanks for the quick reply. I tried -h0 and unfortunately it didn't work. I've attached the SCP I captured via Greasweazle and the resultant dsk fiiles. In summary:

v3.8.11 doesn't work even with -h0 option. Adding -h0 option does stop FUSE crashing.
v4alpha dated 29th April works fine
v4alpha dated 7th June doesn't work even with -h0 option, again without this option FUSE crashes

I think you are correct in that the SCP geometry is causing these issues. I did however load the SCP into HxCFloppyEmulator and it only showed 1 side?

Robocop_A.zip

robocop_dsk.zip

@simonowen
Copy link
Owner

Thanks for the disk images! With them I can see the difference between the two SAMdisk builds:

image

It also explains why -h0 was not working with the newer version. The 40 tracks were being split into two sides of 20 tracks, and limiting to head 0 would just keep half of them. I was expecting it to be seeing 2 sides of 40 tracks, with the second side either blank or duplicated.

The bug was introduced in 583bae7, which was attempting to handle images with more than 80 stored sides. Though it was ignoring the stored head count and that broke support for single-sided images.

I've got a fix for it but I'm getting an error reported when testing the 32-bit build, which doesn't show with the 64-bit build. It's also fine with either version when using VS2022, but the offical builds use VS2019 to keep compatibility with Windows XP. Once I understand that I'll push the fix and update the builds.

@simonowen
Copy link
Owner

One more thing is that when scanning the SCP image it's finding an unwanted data error on cyl 11:

image

The error on cyl 0 is the Speedlock partial weak sector, which has the correct difference pattern to work. However the error on cyl 11 is unwanted and will still crash during loading when it can't be read successfully.

This can sometimes be corrected by adjusting the command-line parameters for the PLL, which performs the analogue to digital conversion of the flux reversals. The code in SAMdisk v3 is slightly different to SAMdisk v4, though both are Keir Fraser's code, who is the GreaseWeazle creator.

If it can't be corrected it might need re-dumping. I'm pretty sure I have the +3 version of Robocop and a good EDSK dump of it somewhere, if it's needed.

@simonowen
Copy link
Owner

simonowen commented Jul 25, 2022

Having looked over the SCP specification, I'm now suspecting the SCP image is invalid for a single-sided 40-track disk.

The spec says:

When imaging a single side only, the track data header entry will skip every other entry.
So, images containing just the bottom head with have even TDH entries (0,2,4, etc.) and
images containing just the top head will have odd TDG entries (1,3,5, etc.).

Your sample SCP image is using consecutive entries in the TDH index, with the end track set to 39. That would be what a double-sided 20 track disk would use, which is what the latest nightly build of SAMdisk was showing.

There is a heads field in the file header, which is zero for double-sided, and 1 or 2 for head 0 or head 1 only. Though that doesn't change the format of the TDH index, just which entries are used. Unused/skipped entries hold a zero offset to say they're not present in the image.

If the SCP image was created with GreaseWeazle it may be worth raising an issue with them to query that?

@keirf
Copy link

keirf commented Jul 25, 2022

The image which @TomDDG uploaded is probably generated with GW's legacy_ss option. This was necessary for correct operation of single-sided SCP images with older versions of SAMdisk.

If you are now following the revised spec, the legacy_ss option is no longer needed. It can be got rid of by running:

gw convert Robocop_A.scp fixed_Robocop_A.scp

And I attach the result of that here:
fixed_Robocop_A.scp.zip

Note that, by my understanding of the revised spec (and images from latest Supercard Pro software) the behaviour of the start/end track fields in the SCP header is unaffected by the single-sided header flag.

Note also that greaseweazle can consume both old- and new-style single-sided SCP images. The simple heuristic checks whether the image is marked single-sided, yet has non-zero TDH index entries on both sides. If so, it's "legacy single sided" using consecutive TDH index entries, and greaseweazle acts appropriately.

@simonowen
Copy link
Owner

Thanks @keirf! I can't remember if it was just the wrong implementation in SAMdisk or if the early specification was unclear on the handling of single-sided images. I've got a memory that the SCP software perhaps didn't support it so no sample images were available.

I'll also add support for SAMdisk to read the legacy_ss style image, with a warning.

(@TomDDG unrelated, but if you're dumping +3/CPC disks I'd also recommend dumping 42 tracks instead of 40, to be sure you catch the extra tracks on a few copy-protected disks).

@keirf
Copy link

keirf commented Jul 25, 2022

Don't worry, it's how all third parties I know of (including me and HxC) interpreted it before the spec got updated. I don't know whether Supercard Pro itself has always done it this way, or if it got updated when the spec did. It was all very unhelpful anyway!

simonowen added a commit that referenced this issue Jul 25, 2022
Added support for legacy single-sided images, from before the specification
clarified their structure. This allows an image using consecutive TDH
entries to still be read correctly (#24).

Changed to trust the max track value in the header, used to determine the
TDH index size. Also validate the heads value is in the expected range.

Added support for new flags from the SCP file specification. At present we
reject non-floppy image types but this could probably be relaxed if needed.
@simonowen
Copy link
Owner

I've created an updated build that supports the legacy single-sided disk images, which now support the attached image.

Thanks to you both for your help!

@TomDDG
Copy link
Author

TomDDG commented Jul 25, 2022

Brilliant thanks for fixing.

gnaray added a commit to gnaray/samdisk_plus_forked that referenced this issue Jun 25, 2024
Added support for legacy single-sided images, from before the specification
clarified their structure. This allows an image using consecutive TDH
entries to still be read correctly (simonowen#24).

Changed to trust the max track value in the header, used to determine the
TDH index size. Also validate the heads value is in the expected range.

Added support for new flags from the SCP file specification. At present we
reject non-floppy image types but this could probably be relaxed if needed.
gnaray added a commit to gnaray/samdisk_plus_forked that referenced this issue Jun 25, 2024
Added support for legacy single-sided images, from before the specification
clarified their structure. This allows an image using consecutive TDH
entries to still be read correctly (simonowen#24).

Changed to trust the max track value in the header, used to determine the
TDH index size. Also validate the heads value is in the expected range.

Added support for new flags from the SCP file specification. At present we
reject non-floppy image types but this could probably be relaxed if needed.
gnaray pushed a commit to gnaray/samdisk_plus_forked that referenced this issue Jun 27, 2024
Added support for legacy single-sided images, from before the specification
clarified their structure. This allows an image using consecutive TDH
entries to still be read correctly (simonowen#24).

Changed to trust the max track value in the header, used to determine the
TDH index size. Also validate the heads value is in the expected range.

Added support for new flags from the SCP file specification. At present we
reject non-floppy image types but this could probably be relaxed if needed.
gnaray pushed a commit to gnaray/samdisk_plus_forked that referenced this issue Jun 27, 2024
Added support for legacy single-sided images, from before the specification
clarified their structure. This allows an image using consecutive TDH
entries to still be read correctly (simonowen#24).

Changed to trust the max track value in the header, used to determine the
TDH index size. Also validate the heads value is in the expected range.

Added support for new flags from the SCP file specification. At present we
reject non-floppy image types but this could probably be relaxed if needed.
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

No branches or pull requests

3 participants