Fix bug in logging all Catweasel samples when at level 7

[TimothyPMann]

Consider out_file_level too, not just out_level. Fixes a bug in bb140aab.

I just noticed this bug because the latest dump from Rüdiger was done with -v72 and is missing the samples beyond where the DMK buffer fills up. I had carelessly only tested with -v7, not -v7x for x<7.

[TimothyPMann]

I've added the -T feature on this branch too. LMK if you'd rather that I split this into a separate pull request and/or separate branch.

[TimothyPMann]

Yes, a blank line will be better there.

On Sun, May 22, 2022 at 8:07 AM Quentin Barnes @.***> wrote:

@.**** commented on this pull request.

In cw2dmk.man https://github.com/qbarnes/cw2dmk/pull/21#discussion_r878882570:

@@ -201,6 +201,16 @@ copy-protected disks that are formatted with nonstandard track numbers). The initial guess is 2 if the drive/media type (-k option) is set or autodetected to be 1; otherwise the initial guess is 1. .TP +.B -T \fIstep_time[,settling_time]\fP +Time in milliseconds to delay after each step pulse (sometimes called +"step rate"), and additional time to delay after the last step pulse (head +settling time). The defaults are 6 ms step time and 0 ms settling time. +The comma and settling_time value are optional. +If your drive has difficulty stepping, try a slower step rate. If you often

I think you want to have a blank line (new paragraph) before this line.

— Reply to this email directly, view it on GitHub https://github.com/qbarnes/cw2dmk/pull/21#pullrequestreview-980980062, or unsubscribe https://github.com/notifications/unsubscribe-auth/AILDEHIJJ3ULKHS3QFP6A2LVLJERJANCNFSM5VPPQVBA . You are receiving this because you authored the thread.Message ID: @.***>

[TimothyPMann]

Changing to unsigned int is fine. I didn't remember that that was the prevailing convention here.

On Sun, May 22, 2022 at 8:47 AM Quentin Barnes @.***> wrote:

@.**** commented on this pull request.

In cw2dmk.c https://github.com/qbarnes/cw2dmk/pull/21#discussion_r878890617:

@@ -134,6 +134,8 @@ int accum_sectors = 0; int menu_err_enabled = 0; volatile int menu_intr_enabled = 0; volatile int menu_requested = 0; +unsigned step_ms = 6;

Although the use of the type unsigned by itself is a perfectly legal type under ISO C, so it is purely a stylistic convention, I would still prefer it stated the implicit int. Prior to this commit the only other time unsigned stood alone was with the recent quirk commit adding line 140, otherwise the unsigned type was previously expressed as unsigned int in the code base. Without the int my brain has to reparse reading the solo unsigned type twice to convince itself that it didn't miss the integer width of the type and that it's intentionally missing. You don't have to change the definitions. I just wanted to explain my take.

— Reply to this email directly, view it on GitHub https://github.com/qbarnes/cw2dmk/pull/21#pullrequestreview-980990186, or unsubscribe https://github.com/notifications/unsubscribe-auth/AILDEHKEZQPMOIG2BEF4FKTVLJJH3ANCNFSM5VPPQVBA . You are receiving this because you authored the thread.Message ID: @.***>

[TimothyPMann]

On Sun, May 22, 2022 at 8:25 AM Quentin Barnes @.***> wrote:

@.**** commented on this pull request.

In catweasl.c https://github.com/qbarnes/cw2dmk/pull/21#discussion_r878887376:

@@ -326,9 +326,9 @@ static void CWTriggerStep(catweasel_contr *c)

This function confuses me on a few points. I'm not sure if it's working around limitations in the CW hardware and/or is taking some coding shortcuts.

My first point is why it is enforcing a time delay between turning on then off the step bit in the controller. Is this a bug with the controller where if turning the bit off too quickly it'll fail?

On why the step time is split evenly in two: That seems to me like it should be unneeded too. In fact while working on this change, I initially tried putting all (except 10 us just to be careful) of the delay after flipping the bit back to zero. I looked at some other Catweasel driver code people have open-sourced, and the others mostly didn't split it. But I thought I was getting some odd results in testing, where a restore to track 0 seemed to be requiring more steps than it should, so I changed it back just so that the default behavior would be exactly the same as before. However, I think I was probably just confused about it taking too many steps... I think I see how that happened. I'll change it again and do some more testing.

My other is that the fixed, enforced delay in the function seems kludgy. I would have instead have a "time of last step timestamp" member in the catweasel_contr structure. On entry to this function, read the struct's timestamp and take the difference between the timestamp and "now". If the difference is less than the step delay, delay for the remaining difference, otherwise, don't wait at all. After the possible delay check, update the current timestamp then do the step operation. But this approach also depends on what's up with the reason for the delay between enabling and disabling the controller's step bit (and why it's split evenly).

If I understand your suggestion correctly, it seems to me like overengineering to some extent. More importantly, it risks causing a regression if some drives actually do need up to 6ms of head settling delay, which previously they got implicitly because we always waited 6ms after initiating the step pulse, but now they won't get it.

Changing to use a timestamp isn't necessary for this commit, but it would be nice to be a future enhancement and have the reason stated behind the split delay operation.

— Reply to this email directly, view it on GitHub https://github.com/qbarnes/cw2dmk/pull/21#pullrequestreview-980987724, or unsubscribe https://github.com/notifications/unsubscribe-auth/AILDEHINZJT6KQKBIGFRLDTVLJGVXANCNFSM5VPPQVBA . You are receiving this because you authored the thread.Message ID: @.***>

[TimothyPMann]

Not splitting the step time is working fine now. I didn't even put in the 10us delay between CWSetCReg(c, CBIT(c, CatStep), 0) and CWSetCReg(c, 0, CBIT(c, CatStep)). So I was just confused about it possibly not working right.

I've done a lot more testing now than the first time, including reading and writing disks with both a 5.25" and 8" drive. I temporarily instrumented catweasel_detect_drive to check that it takes the expected number of steps to reach track 0, to make sure it's not missing steps.

Unrelatedly, my 8" drive's side 1 (back side) head has stopped working. Ugh! I don't have experience fixing this beast or even cleaning the heads, so that will be an un-fun project.

On Sun, May 22, 2022 at 3:34 PM Tim Mann @.***> wrote:

On Sun, May 22, 2022 at 8:25 AM Quentin Barnes @.***> wrote:

@.**** commented on this pull request.

In catweasl.c https://github.com/qbarnes/cw2dmk/pull/21#discussion_r878887376:

@@ -326,9 +326,9 @@ static void CWTriggerStep(catweasel_contr *c)

This function confuses me on a few points. I'm not sure if it's working around limitations in the CW hardware and/or is taking some coding shortcuts.

My first point is why it is enforcing a time delay between turning on then off the step bit in the controller. Is this a bug with the controller where if turning the bit off too quickly it'll fail?

On why the step time is split evenly in two: That seems to me like it should be unneeded too. In fact while working on this change, I initially tried putting all (except 10 us just to be careful) of the delay after flipping the bit back to zero. I looked at some other Catweasel driver code people have open-sourced, and the others mostly didn't split it. But I thought I was getting some odd results in testing, where a restore to track 0 seemed to be requiring more steps than it should, so I changed it back just so that the default behavior would be exactly the same as before. However, I think I was probably just confused about it taking too many steps... I think I see how that happened. I'll change it again and do some more testing.

My other is that the fixed, enforced delay in the function seems kludgy. I would have instead have a "time of last step timestamp" member in the catweasel_contr structure. On entry to this function, read the struct's timestamp and take the difference between the timestamp and "now". If the difference is less than the step delay, delay for the remaining difference, otherwise, don't wait at all. After the possible delay check, update the current timestamp then do the step operation. But this approach also depends on what's up with the reason for the delay between enabling and disabling the controller's step bit (and why it's split evenly).

If I understand your suggestion correctly, it seems to me like overengineering to some extent. More importantly, it risks causing a regression if some drives actually do need up to 6ms of head settling delay, which previously they got implicitly because we always waited 6ms after initiating the step pulse, but now they won't get it.

Changing to use a timestamp isn't necessary for this commit, but it would be nice to be a future enhancement and have the reason stated behind the split delay operation.

— Reply to this email directly, view it on GitHub https://github.com/qbarnes/cw2dmk/pull/21#pullrequestreview-980987724, or unsubscribe https://github.com/notifications/unsubscribe-auth/AILDEHINZJT6KQKBIGFRLDTVLJGVXANCNFSM5VPPQVBA . You are receiving this because you authored the thread.Message ID: @.***>

[TimothyPMann]

I think Rüdiger could use a new release. Both -T and the logging fix are for him.

On Sun, May 22, 2022, 7:11 PM Quentin Barnes @.***> wrote:

@.**** approved this pull request.

Looks great! Let me know if you need me to cut a new release with the changes.

— Reply to this email directly, view it on GitHub https://github.com/qbarnes/cw2dmk/pull/21#pullrequestreview-981072045, or unsubscribe https://github.com/notifications/unsubscribe-auth/AILDEHK34KXOCDQB3L7RKQ3VLLSNBANCNFSM5VPPQVBA . You are receiving this because you authored the thread.Message ID: @.***>

[qbarnes]

Ok. Wanted to make sure you didn't have anything else coming up quickly to be included. I'll bake something soon.

[TimothyPMann]

Nothing in the short term. I am kind of itching to refactor and clean up code so that it will be easier to add new things, but haven't touched actually doing it at all. I also have a branch where I hacked in some HP250-style MMFM support for cw2dmk that seems to work, or mostly work. But it has a number of loose ends and kludges, and there is no support in dmk2cw yet. The only example I have to test with is a partial -v7 log of one disk from Rüdiger. So I'm not sure if I'll pursue this further. It's one of the things that might be easier to add after some refactoring...

On Mon, May 23, 2022 at 8:08 AM Quentin Barnes @.***> wrote:

Ok. Wanted to make sure you didn't have anything else coming up quickly to be included. I'll bake something soon.

— Reply to this email directly, view it on GitHub https://github.com/qbarnes/cw2dmk/pull/21#issuecomment-1134796058, or unsubscribe https://github.com/notifications/unsubscribe-auth/AILDEHO2L5STRYTUGHPW3ATVLONNVANCNFSM5VPPQVBA . You are receiving this because you authored the thread.Message ID: @.***>