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

Implement support for *.trd images #2

Merged
merged 1 commit into from
Aug 17, 2020
Merged

Implement support for *.trd images #2

merged 1 commit into from
Aug 17, 2020

Conversation

morozov
Copy link
Contributor

@morozov morozov commented Jul 22, 2020

This patch adds support for displaying the geometry of *.trd TR-DOS disk images for ZX-Spectrum. The support for displaying BASIC programs is not added since it's more complicated than for tape images and doesn't seem to be implemented for the disk images of other platforms.

Example output using Dizzy and the Mystical Letter:

$ rio spectrum geometry game_en.trd

DISK INFORMATION:
Type:          80 tracks, double side
Label:                 
Total files:   16
Deleted files: 0
Free sectors:  2122

FILES:
#001 BASIC Program
 - Filename:    boot    
 - Sectors:     1
 - Bytes:       82

#002 Code (bytes)
 - Filename:    code    
 - Sectors:     82
 - Bytes:       20928
 - Address:     24576

#003 Code (bytes)
 - Filename:    scripts 
 - Sectors:     18
 - Bytes:       4535
 - Address:     24576

#004 Code (bytes)
 - Filename:    sndlib  
 - Sectors:     10
 - Bytes:       2487
 - Address:     24576

#005 Code (bytes)
 - Filename:    sounds  
 - Sectors:     4
 - Bytes:       940
 - Address:     24576

#006 Code (bytes)
 - Filename:    title1  
 - Sectors:     27
 - Bytes:       6912
 - Address:     24576

#007 Code (bytes)
 - Filename:    title2  
 - Sectors:     27
 - Bytes:       6912
 - Address:     24576

...

@morozov
Copy link
Contributor Author

morozov commented Jul 23, 2020

As the implementation of TRD#Read() shows, the usage of a buffio.Reader may be suboptimal for disk images which unlike the tape ones allow seeking. Maybe the disk formats should depend on some other reader that implements io.Seeker?

@morozov
Copy link
Contributor Author

morozov commented Jul 23, 2020

Additionally, it might be nice to have the bits describing the file types extracted form the tap package into a separate one that could be reused by TZX and other Spectrum formats like TRD. Right now, TRD has to duplicate the subset of the file properties like name, type, start address for code blocks, etc.

@mrcook
Copy link
Owner

mrcook commented Aug 10, 2020

Hi @morozov, I'm very sorry for not responding earlier. For some reason github didn't send me a notification email and I've only just noticed your PR now!

I'm going to be very busy in the evenings this week so please give me a few days to take a proper look at this.

@morozov
Copy link
Contributor Author

morozov commented Aug 10, 2020

@mrcook no worries. Please take your time.

)

// DiskInformation stores the disk information obtained from the 9th sector of the system track
type DiskInformation struct {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you consider updating the struct and reader to also include the unused atributes? This would help both for documentation, and especially for when I eventually get round to implemeting the writers for the media formats. For example:

type DiskInformation struct {
	EndOfCatalogueInfo uint8      // :=0 - Signals end of catalogue (?)
	Unused1            [224]uint8 // Unused (filled with zero - 0x00)
	NextFreeSector     uint8      // First free sector address: sec
	NextFreeTrack      uint8      // First free sector address: track
	DiskType           DiskType   // Disk type (0x16..0x19)
	NumFiles           uint8      // Number of files on disk
	NumFreeSectors     uint16     // Number of free sectors on disk
	TrdId              uint8      // :=0x10 - TR-DOS ID
	Unused2            uint8      // Unused: 0x00
	Unused3            [9]uint8   // Unused: 0x20 (space)
	Unused4            uint8      // Unused: 0x00
	NumDeletedFiles    uint8      // Number of deleted files on disk
	Label              [8]byte    // Disk label
	EndOfDiskInfo      [3]uint8   // End of disk info, filled with 0x00 (Unused?)
}

Copy link
Contributor Author

@morozov morozov Aug 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree that it's better from the documentation standpoint, using this approach will block the further adoption of the SCL format (if you're interested). It effectively describes the same resulting TRD image but in a more compact format that omits irrelevant some details.

Additionally, if/when it comes to unit testing of the presentation-level code, having to mock these reader-related details in model-level code would also create unnecessary burden.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, what you documented above, is the structure of the system sector (not disk info). It indeed can be documented and implemented like this and then produce a "pure" disk information struct.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only briefly looked at the documentation you linked, so I thought the DiskInformation you used was the same.

Can not the SCL images be considered a different disk format? As the file begins with a 8 byte 'magic': SINCLAIR I guess it can be handled so no?

If you feel it's better so leave it as-is please do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can not the SCL images be considered a different disk format?

I believe it's fair to say that the difference between TRD and SCL is similar to the difference between TZX and TAP respectively. The former describes more physical details of the medium but in most they are irrelevant. Therefore, while both TRD and SCL are different formats, they represent the same underlying model (TR-DOS disk).

Actually, if sourced from an SCL image, the disk information would look like the following:

type DiskInformation struct {
	NumFiles uint8
}

If you feel it's better so leave it as-is please do so.

I believe that Unused1..4 represent the DiskInformation struct, and it's better to leave it as is for now. Often, the need for refactoring becomes more obvious when new requirements get introduced (e.g. convert tape to disk), and at that time there's more information to make the right choice.

@mrcook
Copy link
Owner

mrcook commented Aug 16, 2020

Maybe the disk formats should depend on some other reader that implements io.Seeker?

I'd never even looked into this previously! I guess the whole storage implementation needs an overhaul really. We'll soon be heading in to autumn so maybe I'll take a look at this once the summer is over :)

...it might be nice to have the bits describing the file types extracted form the tap package...

I'm not sure how viable that is really. The tapes don't hold files in the same way disks to, and they certainly don't use those FileType bytes (B, C, D, #) like this TRD format does. Still, I will put it on the list of things to take a look at when I get back to this project properly.

@morozov
Copy link
Contributor Author

morozov commented Aug 16, 2020

I'm not sure how viable that is really. The tapes don't hold files in the same way disks to, and they certainly don't use those FileType bytes (B, C, D, #) like this TRD format does.

You're right. I was thinking about other properties of the file like length in bytes, start address for code files, autostart line for basic files, and so on. The notion of the B (Program) and C (Bytes) file types exists in ZX Spectrum regardless of the storage medium.

@mrcook
Copy link
Owner

mrcook commented Aug 16, 2020

You make good points. I'll certainly have a think about this.

@mrcook mrcook merged commit 06b5812 into mrcook:master Aug 17, 2020
@mrcook
Copy link
Owner

mrcook commented Aug 17, 2020

I agree @morozov, let's leave it like this for now. Many thanks for your time in adding this disk format!

@morozov morozov deleted the trd branch August 17, 2020 20:55
@morozov
Copy link
Contributor Author

morozov commented Aug 18, 2020

My pleasure. Thank you for this awesome tool!

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.

2 participants