#228 Proper dual drive support

[Andre Fachat]

Currently the dual drives are emulated by re-using the disk image slots of unit 9 for unit 8 drive 1, and unit 11 for unit 10 drive 1. This restricts the number of dual disk drives, and results in code problems throughout the drive and image management.

Proper dual drive support should introduce two image slots per disk unit, to support up to 8 disk images, for four dual drives.

Please see the attached patch (svn diff against r35538) for an initial try.

[gpz]

i second the need for this being done - however, its not my area of expertise so i wont touch it or apply patches unless its complete with UI changes and everything else thats needed

[Andre Fachat]

Isn't there anyone else with drive emulation expertise?
Anyway, here's an updated patch that compiles for GTK3 and SDL.

Open issues are:

1. I haven't implemented a logic to hide the drive0/1 selection for single drive units
2. in some places in the monitor extra parameters may be needed to select drive 1
3. other points may still ripple through, but serial drives can probably be optimized as they are all drive 0 only.
4. Snapshot format needs to change
5. I marked all places where I think some more work is needed with TODO and/or "drive 1"

I won't have time keeping up with this though for the next time. It was just an attempt at fixing some technical debt I left long ago.

Hope it helps anyway
André

[Andre Fachat]

In the patch there are UI changes included for SDL and GTK3. What else do you think is missing to include the patch? Monitor and snapshots are changes that reach into general topics, like what is the strategy with snaphot changes, or how are (potentially) incompatible changes in the monitor commands handled? So I can't really do these, at least not unless some guidance on what direction to work.

[gpz]

The main guideline is: dont leave non working things behind. because we have had this way too often in the past, and the general experience from this is: it wont get fixed later. thats why we are very reluctant to add incomplete patches that possible break something that worked before.

that said, "incompatible changes in the monitor commands" are fine. if there are good reasons to change them, just do it. BUT keep the documentation in sync :)
same for the snapshots - we dropped the idea of backwards compatible snapshots long ago, and when they change in a way that old snapshots will no more work, so be it. do NOT add fancy magic code that tries to guess what older snapshots would do in the newer code. those things are a nightmare to maintain and test, and we would usually even remove them without notice when we see them in older code.

(another small cosmetic issue: dont use c++ style comments please)

last not least, i really like how this is going - however, i wont have time to look at it in detail right now. perhaps rhialto wants to chime in :)

[gpz]

i totally forgot about this... however, we might pick it up, there is a wiki page here now: https://vice-emu.pokefinder.org/index.php/Improving_dual_disk_drive_handling#Improving_dual_disk_drive_handling

[Olaf Seibert]

I am trying out the patch now (applied to a today's version, r36910 or so.

It seems the files for the new widget are missing: drivenowidget.[ch] ?

[gpz]

it would be really nice to get this merged soonish... so who will look at it? :)

[Olaf Seibert]

The drive widgets are still missing, but here is a diff for svn r36910 "or so". I hope I didn't leave anything further out by accident. It is only partially compile tested: up to the point that make detected it wanted the source files for the new drive widget.

[gpz]

so, i had a look. this patch is plenty broken:

• patch doesnt work against trunk (a dozen rejects or so)
• debug code was left enabled
• printf was used instead of the respective log functions
• various codestyle issues, TABs, indention, variables declared at wrong places, C++ comments
• missing files

so, i have fixed all of the above (please notice i wont do this normally - those are all reasons why a patch would be rejected normally). there are some problems left before it can be merged:

• the GTK stuff prints GTK warnings. i dont know why
• attaching a regular disk image (d64) in x64sc is now broken (WTF?)
• the drive# widget is always there, it should only be shown when a second drive actually exists
• there are a ton of TODOs left in the code. they should be documented seperatly so someone has atleast a chance to look at them without spending a week crawling through the code
• there are various GUI issues/unimplemented features. there should be a list of those too
• last not least it should be tested for regressions. apparently that wasnt the case yet (see D64)

i have not tested the SDL UI.

updated patch against trunk (r37063) is attached

[gpz]

updated patch. just noticed its a few kb less than olafs, i dont know why.

someone else should review and fix

[gpz]

it would be really nice if we could make this happen...

[compyx]

Updated patch to work against r37748. Haven't fixed any Gtk3 warnings yet. Fixed indentation issues in reject hunks. Some please test this, it compiles on Debian with GCC 8.3 without warnings.

[compyx]

Updated patch with Gtk3 NULL warning fix.

[compyx]

Updated author of drivenowidget.c. Clever, pretending it was me who wrote it, so I get blamed when it breaks. =)

[gpz]

patch applied in r37793 - thanks for the patch! some GUI things are still missing, we'll look at that...

[gpz]

reopening. this still needs more work than it should have to implement the UI indicators (drive status bar, LEDs etc).

[gpz]

r37795 should fix the statusbar api at least for showing the current track

now to fix the LEDs there are some open ends.... first, we need to handle TWO LEDs per drive apparently, one for "activity" (all drives have that) and one for "error" (only the IEEE and the FD4000 seem to have this). for FD4000 it seems to be implemented, so we may be able to use this to test the api partially. for the IEEE drives the implementation is missing, see drive/ieee/riot2d.c

[gpz]

just as a reminder, leftover TODOs from the above patch are in:

src/attach.c
src/autostart-prg.c
src/autostart.c
src/drive/drive-resources.c
src/drive/drive-snapshot.c
src/drive/drive.c
src/drive/driveimage.c
src/drive/ieee/fdc.c
src/drive/ieee/riot2d.c
src/drive/ieee/via1d2031.c
src/event.c
src/imagecontents/diskcontents.c
src/monitor/mon_drive.c
src/monitor/mon_file.c
src/network.c
src/parallel/parallel-trap.c
src/serial/fsdrive.c
src/vdrive/vdrive.c

also:

src/c1541.c

... so plenty things to fix for someone who is actually interested in those dual drives hint

[gpz]

now i am seeing "fdc: Could not attach image type 1541 to disk #8 without type." when attaching a d64 (to 1541) - which totally doesnt make sense (it comes from src/drive/ieee/fdc.c) args
ok i silenced that, its just a debug thing (confusing!)

the SDL port should now have mostly working implementation of the statusbar. although it doesnt actually really work yet, because of the reasons mentioned above :)

[Querino]

there are issues with this patch.

open 15,8,15,"n1:test,zz"
close 15

doesn't work at all. i get a corrupt disk image.

also, every now and then a

load"$",8

gives me a "?file not found error" if 2 images are loaded.

all that worked just fine before.

[gpz]

please be more specific. what is your config etc
(i had random problems with the PET drives even before the patch for that matter)
perhaps try disabling the virtual device traps, which are enabled by default for some days (unrelated to this patch)

[Querino]

no virtual device traps. SDL version.

x64sc.exe -default -ieee488image ieee.488.eprom.bin -ieee488 -drive8type 8050 -8 "8050_d1.d80" -8d1 "8050_d2.d80"
open 15,8,15,"n1:test,zz"
close 15

now you get "READY."
but if you do a

load "$",8

you get is a "?FILE NOT FOUND ERROR"

now load the $ again, it works, but the disk in drive1, it's broken with 0 blocks free.

[gpz]

OH, its on the c64 even... thats not a code path many ppl use, i guess :) i'll have a look if i can see an obvious problem there

edit: can you confirm this problem is specific to formatting? loading programs/directory from existing images works fine for me

edit++: i can confirm the issue with formatting also in xpet

[gpz]

i cant find an obvious problem that would cause this kind of behaviour... would be good if andre could look at it :)

[Olaf Seibert]

It is not even a 64-specific problem, it also occurs with a PET. That's a bit easier since you have Basic 4.0 commands, including DS$:

$ ./gtk3-xpet -default -drive8type 8050 -8 "8050_d1.d80" -8d1 "8050_d2.d80"

*** commodore basic 4.0 ***

 31743 bytes free

ready.
cA
 0 "kjdhkd          " yy 2c
 2052 blocks free.
 1 "drive1          " 11 2c
 2052 blocks free.

ready.
open 1,8,15,"n1:sdhldg,nn

ready.
?ds$
29, disk id mismatch,38,00,1

ready.

*** commodore basic 4.0 ***

 31743 bytes free

ready.
cA

ready.
?ds$
66,illegal track or sector,00,00,1

ready.
cA
 0 "kjdhkd          " yy 2c
 2052 blocks free.
 1 "
 0
 0
 0
 0
 8224
 0 blocks free.

ready.