ProDOS HDD Driver return size in X/Y for status call

[peteri]

ProDOS HDD Driver modified to support returning the volume size in the X/Y registers. New command line option to force a size for autoexpanding use. Fixes issue #1264 and #1033 for Pascal 1.3

[tomcw]

Thanks @peteri - I will try to take a look in the coming days.

[peteri]

No problem, my only real questions were if adding GetImageSizeInBlocks should be part of the ImageInfo class and if I'd done the right thing with the using the global CmdLine instance. I think this fixes the 1033 issues, but I haven't extensively tested everything (just Pascal 1.3 and one of the game images for slot 5.

[tomcw]

I have a concern about the HDD firmware and backwards compatiblity:

1. Is it back-compat? ie. the 16-bit value at $C7FC (number of blocks in device)

   • Some s/w must be reading this value to show the device size.
   • Or perhaps all the s/w that reads this value knows that if it's 0x0000 then it needs to issue a STATUS command?

2. Saving the state when the PC=$C7nn (using AppleWin built with the old firmware) and then loading the state (using AppleWin with the new firmware) could result in undefined behaviour, as the running code may've changed.

   • NB. there is some detection for this case in HarddiskInterfaceCard::LoadSnapshot()
   • So we could just bump kUNIT_VERSION to 4, and the version check in HarddiskInterfaceCard::LoadSnapshot() to 3, to throw the error "HDD card: 6502 is running old HDD firmware".
   • But this isn't a great solution, as it prevents some older save-states from being loaded (even if the chance of a snapshot occurring when the 6502 is executing the HDD firmware is small)
   • to do (for me): consider persisting the HDD controller firmware to the state file? (And not just for slot 7, but for other slots that use this card.)

My preference is to retain the current HDD firmware in AppleWin as a fallback measure (eg. selectable via a command line option).

Can you put your new firmware into a new folder "Firmware/HDDv2".

• In Applewin.rc: add a new line: IDR_HDDRVR_V2_FW FIRMWARE "Hddrvr-v2.bin"
• In resource.h: add a new #define IDR_HDDRVR_V2_FW 155 (just add to the end after IDR_BASE64A_VIDEO_ROM)
• And change AppleWin to use this new HDD firmware
• In Harddisk.cpp: bump kUNIT_VERSION to 4 (but leave the version check in HarddiskInterfaceCard::LoadSnapshot() at 2).
• I will make the changes to AppleWin to allow using the old firmware - so no need to update your PR to do this.

[tomcw]

A minor thing about the cmd line switch: "-harddisknumblocks"

Can you add a setter function to Harddisk.h, eg. void SetUserNumBlocks(UINT numBlocks) and a new private member m_userNumBlocks. This way, the HarddiskInterfaceCard class doesn't need to include CmdLine.h, and doesn't need to access g_cmdLine.

Then you'll need to query every slot if it's of type CT_GenericHDD to call this setter function - so just extend this loop to do this too: https://github.com/AppleWin/AppleWin/blob/fcd216bb45e7640bbd3c0c66758bc48e65a38195/source/Windows/AppleWin.cpp#L801

Note the Config UI also allows HDC cards to be inserted. So in CPropertySheetHelper::SetSlot() for cards of type CT_GenericHDD, we should also call this setter function with the cmd line specified value.

[peteri]

Thanks for the comments, I hadn't thought about the save state so I'll get the points you've raised cleaned up.

The block size being zero does mean that code should call the status to get the size and this is the functionality in the original Profile hard disk ROM from Feb 1984 (see page 43 in https://www.bitsavers.org/pdf/apple/disk/profile/appleII_interface/AII_Profile_Boot_Prom_198402.pdf ) so I'd expect most software to make the call.

[tomcw]

...so I'd expect most software to make the call.

Set a watchpoint on $C7FC-FD with bpm c7fc,2

copy_ii+_v9.1_s1.dsk:

• "Y" / Verify option: breaks at 91CD
  • if (C7FC).w=0x0000 then: LDA #0; JSR A1F9
  • function at 91CB returns num blocks in Y:X

91CB:A0 FC                LDY #$FC
91CD:B1 E9                LDA (HGR.SH+1),Y
91CF:AA                   TAX
91D0:C8                   INY
91D1:B1 E9                LDA (HGR.SH+1),Y
91D3:A8                   TAY
91D4:D0 09                BNE $91DF
91D6:E0 00                CPX #$00
91D8:D0 05                BNE $91DF
91DA:A9 00                LDA #$00   ; STATUS command
91DC:20 F9 A1             JSR $A1F9  ; Post: Y:X = num blocks
91DF:18                   CLC
91E0:60                   RTS

So yes, this software from 1990 does make the call.

copy_ii+_v7.2.dsk

• the same code as v9.1 above (just a different location)
• this s/w from 1986

This gives me (some) increased confidence, and lessens my back-compat. concern I raised above. Ideally I'd check with more software, eg. ProDOS, Pascal, CP/M.

[peteri]

I've just had a quick look at the oldest version of ProDOS I can find online (1.0 Nov 83) and it has a similar size check in FILER so I'm happy (at $4250-$429c).

[peteri]

Just a quick comment to say I've not forgotten about this, just been a bit busy at work. I'm hoping to get the suggestions incorporated over the weekend.

[peteri]

Okay, got a bit of time.... I'm not entirely convinced the change to CPropertySheetHelper::SetSlot is needed as adding or removing the controller causes a restart of AppleWin, is there an edge case I'm not aware of? (Or I missed something in the lifecycle of the controller card)

[tomcw]

This PR looks good.

I'm not entirely convinced the change to CPropertySheetHelper::SetSlot is needed as adding or removing the controller causes a restart of AppleWin,

Yes, you're I right - so that code in SetSlot() can go.

Thanks.

[peteri]

Reverted the change to the PropertySheetHelper....

Thanks for being so patient with my changes.

[tomcw]

My notes:

1. When growing an image (ie. appending a block to the end), then CImageBase::WriteBlock() updates the ImageInfo object with the new uImageSize. So subsequent calls to GetImageSizeInBlocks() will return the new size (unless m_userNumBlocks has been set).

2. GetImageSizeInBlocks() returns numberOfBlocks by just dividing by HD_BLOCK_SIZE (512), when perhaps it should round up (ie. add 511) before dividing. Unlikely to be an issue since the image size should (must?) always be a multiple of 512.

Neither are a concern, but just making a note here.

[tomcw]

@peteri - thanks for your PR. It's now merged into the main line.

[tomcw]

@peteri - here's a new AppleWin 1.30.18.0 release that includes your PR work.