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

hfe: Process opcodes when writing #825

Closed
wants to merge 5 commits into from
Closed

Conversation

ejona86
Copy link
Collaborator

@ejona86 ejona86 commented Sep 9, 2023

No description provided.

@ejona86 ejona86 requested a review from keirf September 9, 2023 22:38
src/image/hfe.c Show resolved Hide resolved
src/image/hfe.c Outdated Show resolved Hide resolved
src/image/hfe.c Outdated Show resolved Hide resolved
inc/floppy.h Outdated Show resolved Hide resolved
@keirf
Copy link
Owner

keirf commented Sep 10, 2023

Also what's the test case? Is it particularly hard-sector index opcodes, or are other opcodes also tested?

@ejona86
Copy link
Collaborator Author

ejona86 commented Sep 10, 2023

Also what's the test case?

The start of a write corrupting OP_Bitrate applies to v3. That's hard to reliably trigger, but disasterous. Writing over OP_Bitrate in other cases is asking for confusion, but could probably be done a bunch without being noticed. OP_Index is important for v4, as otherwise writing to the last sector would break the track. Overwriting OP_Index could impact v3, but only sofar as you corrupt the image so it no longer works in HxC.

src/image/hfe.c Outdated Show resolved Hide resolved
src/image/hfe.c Outdated Show resolved Hide resolved
src/image/hfe.c Outdated Show resolved Hide resolved
src/image/hfe.c Outdated Show resolved Hide resolved
@keirf
Copy link
Owner

keirf commented Oct 10, 2023

Thanks. I added some more review comments. Hopefully the ones regarding w and i are pretty clear and agreeable. The one in hfe_setup_track does complicate the code there, but simplifies hfe_write_track and better handles track start I think... :)

Copy link
Collaborator Author

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

The w[i++] stuff worked out well.

I've made the changes, but it is essentially untested (I ran FF_TestBed, but that doesn't use hfev3). I'm going on vacation and not bringing Goteks, so it'll probably be two weeks before I can do that. I can rebase my file-cache branch on top of this while on vacation though, so it is ready when I get back.

src/image/hfe.c Outdated Show resolved Hide resolved
src/image/hfe.c Outdated Show resolved Hide resolved
src/image/hfe.c Outdated Show resolved Hide resolved
@keirf
Copy link
Owner

keirf commented Oct 10, 2023

Looking really good. Two minor comments. This can all wait until after vacation as I'm not making a v3.x release in the immediate future.

I also want to rework the DSK, IMG streaming write code a little bit anyway, as I realised that in getting you to check for 128 bytes available in the main loop, I at least theoretically broke the Data Field CRC check (which needs only 2 bytes). I may end up doing more and if so I may make it a PR for you to review.

@keirf
Copy link
Owner

keirf commented Oct 29, 2023

Thanks for iterating on this. I've squashed it all together and committed to master.

@keirf keirf closed this Oct 29, 2023
@ejona86 ejona86 deleted the hfe-write-ops branch October 29, 2023 18:17
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