imagedev/floppy.cpp: Re-add hard-sector floppy support

[ejona86]

This reverts commit 4d56a31, rolling forward the original hard-sector support commit b2c399c and the workaround commit cf510ad which fixes random Mac freezes while reading floppies as seen at #11585 . This has the additional change of not increasing the precision for 60/rpm from float to double, which broke reading disks for Commodore.

Because of those regressions, care has been taken in floppy.cpp for soft-sectored media to use exactly the same computations of the same types using the same values. The changes in those paths only go so far as adding/subtracting zero. get_pos() is changed, but nothing calls it.

---

As I mentioned in #11150 (comment) , the issues plaguing this change were rounding/precision issues. I tried more encompassing changes to make the precision/rounding predictable and more rigorous, but it never fixed the 60.0/rpm issue. Since such changes couldn't be shown to fix anything and are impossible to verify as safe, I ditched the effort and instead "did what we know works." That was likely the only thing that would get through review, but I had wanted to fully understand the 60.0/rpm issue.

I've tested reading the Chwat d64 image with c64p, booting System 1.1 with mac128k, and formating and booting a boot disk with vector4. The mac128k issue was a non-deterministic hang, so I tried multiple times and opened some programs.

I won't be the least bothered if this is not merged until after 0.261, so that there'd be a guaranteed recent version with non-broken floppy.

CC @cuavas @balr0g @Davidian1024 @ajrhacker

[cuavas]

I don’t think I’m qualified to properly review this myself. @galibert and @ajrhacker can you please review the code changes? In particular, @ajrhacker you mentioned after this feature was merged last time that the changes to floppy_image_device::index_resync looked like they couldn’t be right. Can you please analyse it on the pull request this time?

If this is going to be merged, I’d like it to go in at the start of a release cycle (i.e. definitely not before the freeze next weekend), and for people to test a representative sample of floppy formats after it goes in. I only do minimal testing of floppy support before releases – usually only one or two randomly selected home computers, and only testing read support. I don’t have time to do more extensive media support testing for every release.

[mgarlanger]

With 2 month before the next release, it would be great to get this merge early in this cycle so it has as much time as possible for testing. @galibert @ajrhacker @cuavas can we get this reviewed/merged?

[cuavas]

Well like last time, I don’t feel qualified to review it, and I don’t have time to test a significant number of floppy formats. I don’t want a repeat of a situation where floppy drive emulation was broken for multiple consecutive releases. I’m not in a position to merge it.

[mgarlanger]

Hopefully ajr and/or galibert will have time to review early in the cycle to allow as much time as possible for testing before the next release. Several systems use hard-sectored disks, and this PR needed before other work can be done.

[cuavas]

Yes, but many systems use floppies that are not hard-sectored, and breaking them is not a good look.

[ajrhacker]

Though my review was requested, I don't really have much to say about this change except to note that one piece of software I tested was mac_flop_orig:macwars. With the broken previous version, it died halfway through booting (softlocking MAME, I recall).

[simzy39]

If this works, then will it be considered or is #11952 preferred by the team? I like point 2 of the referenced PR because it better mirrors real hardware. @ejona86 Is this something you could edit your PR to be, but I guess you can't if @galibert made your approach a requirement.

[ejona86]

@galibert, I've merged with master to absorb the changes that prefix members fields with m_. Again, all the bugs caused by the earlier commit were due to different floating point rounding behavior, and this PR exactly preserves the calculations for soft sectored disks.

I'd be happy to use a separate timer like done in #11952 if it made review easier/reduced risk, but I think it exchanges one set of risks/concerns for a different set so I'd do whatever is your personal preference.

@simzy39, I'd prefer it as well, but given our goals having the drive aware is practical and the deficiencies are unlikely to be problems in practice. For example, while having a separate drive for each type of sectoring is unfortunate, very few drive types were used in hard sectored systems so it is unlikely to become a problem. The alternatives look much more invasive. It is also something that can be changed in the future if the tradeoffs here turned out to be poor.

[simzy39]

@ejona86 Can you please test this thoroughly? The Mame devs seem afraid to touch this, and the previous negative experience has been brought up again.

[ejona86]

Can you please test this thoroughly?

I don't know of a way to test it further that would increase others' comfort. If there is, I'd love to know. I get the feeling it really doesn't matter what I do, because it's really an issue of trust.

And I don't trust this change because I tested it; I only trust it myself because static analysis showed "it performs the same identical mathematical operations." Since exhaustive testing is not available here, testing was just confirming I didn't make a logic error. It really says little about the floating point.

the previous negative experience has been brought up again.

Where was that expressed?

[galibert]

On the shoutbox, which is currently down. In any case, I plan to use this 3-day weekend to study your patch and Mark Garlanger's to end up with something I'm confident with. I really want hard-sector support in Mame, just need to convince myself it's not gonna blow in my face again. Probably won't, but once burned twice shy :-) OG.

…

On Wed, Mar 27, 2024 at 7:47 AM Eric Anderson ***@***.***> wrote: Can you please test this thoroughly? I don't know of a way to test it further that would increase others' comfort. If there is, I'd love to know. I get the feeling it really doesn't matter what I do, because it's really an issue of trust. And I don't trust this change because I tested it; I only trust it myself because static analysis showed "it performs the same identical mathematical operations." Since exhaustive testing is not available here, testing was just confirming I didn't make a *logic* error. It really says little about the floating point. the previous negative experience has been brought up again. Where was that expressed? — Reply to this email directly, view it on GitHub <#11705 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGSF4MY5QUXY7WUULVPDXTY2JTPLAVCNFSM6AAAAAA66BJKO6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRSGA3TCMRZGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mgarlanger]

On the shoutbox, which is currently down. In any case, I plan to use this 3-day weekend to study your patch and Mark Garlanger's to end up with something I'm confident with. I really want hard-sector support in Mame, just need to convince myself it's not gonna blow in my face again. Probably won't, but once burned twice shy :-) OG.

Let me know if there is anything I can help with. I think it is crazy that a breakage is considered so problematic. It not like this software is in life-supporting systems. If it is broken for a release, people just have to use the previous month's release. Not the end of the world.

And it when a change impacts a important part of the code, it seems like more of the team could help in testing. People could just spend 30 mins or so, testing the systems they are familiar with. It would go a long ways in make sure there are not regressions. Not everyone is knowledgeable on how to use every system, or are setup to test those systems.

[ejona86]

I'm still willing to put time into this or alternatives. I'll reiterate that the bugs from the previous iteration were due to floating point brittleness. So the change to floppy.cpp in this PR currently uses the exact same operands in the same order for soft-sectored disks. When find_index_hole() uses last_index = 0, next_index = 200000000 the floating point operations and operands (not just the result) should be identical.

I tested (when I created the PR) and the previously failing cases of soft-sector disks did not fail with this change. If I can run tests or restructure the code to increase confidence or make it easier to review, I'm very willing.

[simzy39]

Did this bug affect your implementation at all?
#12439

[ejona86]

No, it didn't impact this implementation, because this aligns the start of the track to the first hard sector. (IDX in soft sectors is the start of the track. IDX for hard sectors is an indirection, and indicates the next pulse is the start of the track.)

[galibert]

Ok damn it. I have your branch updated with some stuff I wanted to do ready (and it has been for a while). I wanted to compare with Mark's but I never find the energy. So I'm going to push that one, and HC feel freer to resync/add whatever changes you find useful. Once the support is in evolving it is way easier.

Commited a9eeca3