simonowen / samdisk

A portable disk image utility, specialising in copy-protected PC-compatible formats.
http://simonowen.com/samdisk
MIT License
86 stars 11 forks source link

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

Closed TomDDG closed 2 years ago

TomDDG commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 583bae7ce5315d7af4794767d9b6dcd444fc3d39, 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Brilliant thanks for fixing.