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

Add support for AppleSingle files #2

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Mechazawa
Copy link

CC65 outputs AppleSingle files which are currently not supported by a2kit. This merge adds the option to put AppleSingle formatted files onto disks.

Example:

a2kit mkdsk -bo dos33 -v 254 -t do -k 5.25in -d TEST.dsk
a2kit put -f TEST -d TEST.dsk -t as < tests/test.as
Screenshot 2024-09-18 at 00 48 02

@dfgordon
Copy link
Owner

This looks great. Since we are adding public interfaces I may want to think more about how to organize it in terms of modules, whatever we do gets baked in for as long as we are on v3.x. For testing I usually like to compare the disk images that a2kit creates with the same disk image created by some other software, can we do that here? It looks like the code ran, but a byte-for-byte comparison can sometimes reveal a difference nevertheless.

@dfgordon
Copy link
Owner

It turns out merging this PR requires a major version increment, try cargo semver-checks on the fork. I would rather not do a major increment at present.

However there is a way to get this capability merged in short order using the auto type. Right now a2kit put will always return an error if -t auto is chosen, but nothing stops us from letting it succeed if it is handed an AppleSingle. In general the idea would be to use some sort of heuristics to determine what we want to put given this or that data stream. In the case of AppleSingle it may be sufficient to simply check the magic number.

If this makes sense, can you update the fork such that

  • In pack_primitive, if ItemType::Automatic matches then run some code that checks to see if the data is AppleSingle, if yes proceed as before, if no return an error. Also return an error if fimg.file_system is some file system we are not handling.
  • Move the general parsing from bios into commands::put and make it private. This is a strategy to get the specific capability for CC65 now without committing to a more general interface. Generalizations can happen later.
  • If I haven't missed anything the only file with a diff should be commands::put.rs.

One thing my research has not turned up is whether this CC65 way of packing the data and resource forks into a ProDOS 8 binary is universal, e.g., might some other software expect us to write them in reverse order?

@Mechazawa Mechazawa marked this pull request as draft September 23, 2024 08:20
@Mechazawa
Copy link
Author

I'll add the tests and make the required changes. I'll un-draft the merge when it's ready for another review

@Mechazawa
Copy link
Author

Mechazawa commented Sep 23, 2024

It turns out merging this PR requires a major version increment, try cargo semver-checks on the fork. I would rather not do a major increment at present.

I'll revert the increment, not really sure why it snuck in there.

However there is a way to get this capability merged in short order using the auto type. Right now a2kit put will always return an error if -t auto is chosen, but nothing stops us from letting it succeed if it is handed an AppleSingle. In general the idea would be to use some sort of heuristics to determine what we want to put given this or that data stream. In the case of AppleSingle it may be sufficient to simply check the magic number.

Yeah it'll be pretty trivial to check if the file is an AppleSingle based on the magic number. I'll make sure to add it.

Move the general parsing from bios into commands::put and make it private. This is a strategy to get the specific capability for CC65 now without committing to a more general interface. Generalizations can happen later.

I'd prefer if we keep the AppleSingle parsing in a seperate file so we can expand it in the future if needed. This would also make maintaining it a bit easier. We can create a new directory containing file parsers. If you still prefer placing it in commands::put then I'll just place it there.

One thing my research has not turned up is whether this CC65 way of packing the data and resource forks into a ProDOS 8 binary is universal, e.g., might some other software expect us to write them in reverse order?

From what I've gathered resource forks are rarely used and documentation on them is pretty sparse. I'll have a look if I can find some more info about them.

@dfgordon
Copy link
Owner

I agree the main parsing should be in a separate file in the long run, I just wanted to keep it private for now. If you can arrange to put it in a private module that would also do. My thinking is to put it eventually as a public root module in fs, but I didn't want to commit to that just yet. In this vision it would be handled similarly to FileImage, maybe implementing the Packing trait, but this needs more thought.

@dfgordon
Copy link
Owner

dfgordon commented Oct 20, 2024

I compiled something with CC65 and used the fork to copy the binary to a disk using -t auto. It worked.

First about the actual workings from an end-user perspective. Right now if you put an AppleSingle to an MS-DOS disk it will copy data over, but will ignore the MS-DOS attributes silently. For ProDOS we are also not copying over all the "ProDOS File Info." In the general case we should retain that information. Luckily it is all representable in FileImage so this should present little trouble. If the user tries to write an AppleSingle to Pascal or CP/M, my thinking is that should be an error.

Next how to organize. We are allowed to add a "defaulted trait item." So what can be done is to add the following to fs::Packing trait:

fn pack_apple_single(&self, fimg: &mut FileImage, dat: &[u8]) -> STDRESULT {
  Err(Box::new(FileSystemMismatch))
}

You have to also add a dispatch function to FileImage:

pub fn pack_apple_single(&mut self, dat: &[u8]) -> STDRESULT {
  self.packer().pack_apple_single(self, dat)
}

This funny thing calls the appropriate Packer delegate per file system (the effect is like inheritance).

The implementation can then be done per file system, at whatever pace we like, by adding this trait implementation in the appropriate fs::<specific_fs>::pack module. The action that is supposed to happen is simple in concept: convert the AppleSingle data to a FileImage. Obviously this calls into the as module to work its magic.

N.b. the pack modules usually have functions to help with just this task, such as taking chrono times and putting them in the native file system format.

The new module, currently bios::as, can go as fs::as. I think it will have to be public to be visible from the various pack modules.

Finally in commands::put::pack_primitive you will have something like

  if let Ok(()) = fimg.pack_apple_single(&dat) {
    Ok(())
  } else if some_other_condition() {
    // pack some other thing
  } // etc

@Mechazawa
Copy link
Author

Adding support for the different filesystems in that way sounds like a good idea. We can also make an unpack per filesystem which would make sharing individual files from a filesystem easier.

Filesystems like CP/M etc can still be supported but showing a warning when no file attributes for the target fs could be found/applied. I think this would allow users to still write files if they want to.

Currently I haven't been able to put a lot of time into the MR because I managed to fry my Apple2 and kill ~70% of the chips. So most of my free time has been going into testing everything and finding replacement parts.

@dfgordon
Copy link
Owner

Oh dear sorry to hear that!

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