kareltucek / firmware

This is fork of UHK's firmware featuring extended macro engine.
Other
82 stars 9 forks source link

unwanted concurrent macro execution despite postponeKeys #23

Closed orthoceros closed 5 years ago

orthoceros commented 5 years ago

Hi, I tested some macro ideas towards fast&reliable secondaries for alphabetic keys without having to move my fingers far away from the base row. I tried implementing a 2-finger gesture for a mod key, but I failed to make it reliable, even when using low level commands like $postponeKeys ifPending.... I narrowed the problem down and found that with fast/simultaneous key presses, sometimes the second key press is not postponed, although this is the first command of the macro for the first key press. Instead, the second key press triggers a concurrently running macro (broken mutex). (In hindsight, this might also explain a certain type of unreliable behavior I had with rocker gestures discussed in an older issue.)

These are two minimal macros to reproduce the problem:

kareltucek commented 5 years ago

The update cycle functions in phases - first (all) macroes are resumed, then keystates are preprocessed (including application of postponing), then key actions are activated. Specially start-macro action only loads the macro into memory and the first action of the macro is executed in the next update cycle.

Still, this is an interesting ingisht which might help explaining some issues I've been seeing lately.

kareltucek commented 5 years ago

The funny part is that all I can reproduce is Mutex 210 broken despite postponeKeys - apparently, my OS is filtering out the duplicite scancode reports :-).

Let me think about it for some time... I can theoretically make the postpone kick in by one iteration of the update cycle earlier, and I could rework the state handling so that preprocessing happens on per-key basis. Unfortunately, when you manage to activate both keys in the same update cycle, the information about which key was first is lost anyway.

orthoceros commented 5 years ago

Cool :) Yes, if the two key presses are indeed received within the exactly same update cycle, their order would be ambiguous. But this would have no impact when using symmetric macros on both possible first keys; and it would probably never occur during normal typing... Sure, no hurry; happy Easter weekend!

kareltucek commented 5 years ago

Here we go... I've changed the order of event-loop processing. Surprisingly it seems that nothing went terribly wrong and that the firmware is at least as stable as the previous one.

Also, you should be now able to use //comments at ends of lines - however, I didn't test it much.

kareltucek commented 5 years ago

Ok, I take it back, the postponer is broken...

kareltucek commented 5 years ago

I think that this should be solved now.

orthoceros commented 5 years ago

Thanks for your efforts! :) I imagined what it could/would be hard to get the mutex logic working.

Unfortunately, we are not there, yet. I see that you added some debug info like @K1 that does not come from my macro pair. I do not know what this means, so here is the raw output of some samples I just generated in Notepad++ by repeatedly hitting cv, cv, cv, ... with two fingers:

cvvccvcvcvcvcvcMutex 20 broken deMSUPtiex  2p1o s0btrpooknee deyspite postponeKys
vccvcvcMutex 20 broken dEMUStpiext 21p os0brtopken MDUEymspxixt2 1 0pborsrtkpeoen ndCKSDpyitp p ostsonoeKKyss
vccMutex 20 brokevn DMeutspeix te2 1pobsrtpkcoenE MDUKeseypxit2e0  pborsvtkpeONMun teDKxs pyi21t ebrpoosketnpo dneSKpieyts postponeKeys
cMutex 20 broken dEMUStpiext 21p os0brtopken dEYMSUPTieutx 2xp1o sb0trpokrne Dyspitie p ostsonoeKKyss
cvcvcvcMutex 20 broken despiMTUE epxo s2t1p o0bnreOKenys despite postponeKys
cvcvcvcMutex 20 broken deSMPUITeex p2o 1s t0bp ronkeo ndyssepitie p ostsonoeKKyss
vccvcvvccvcvcMutex 20 broken despIMUTE xp os2 1t po0b rnoeKKny sdesepitie p ostsonoeKKyss
cvcvcMutex 20 broken DMEUTSpeix t2 1 po0bsrotpkoen DMuTSYpix ti2x 1pob0srtskroenoe eDCKSypdtpi peo stosnpeoKYKsys
cvcMutex 20 broken DMEUTSpeix t2 1 po0bsrotpkoenn DMUsypixti21 0p borstskeono edKSDKPyiti p ostsonoeKKyss
cMutex 20 broken MDUEtsepxi t21 0pborstkpeoNM uDKmusxpyi2xt1 b0proskrtepnoc kendnKSP iyt peo stosnpeoKYKsys
vccvcMutex 20 broken deSMPUITeex p2o 1s t0bp ronkeo ndyssepitie p ostsonoeKKyss
cMutex 20 broken vdeSMpuittex  p2o1s tbproonkeeC EDMysutpeix te2 0pobsrtpkoene DKesypite postponeKeys
cvvcvccvccMutex 20 broken despIMUTe xp os21t po0brnoeKny sdespite postponeKys
cMutex 20 broken despIMUTE xp os2 1t po0b rnoeKKny sdeSEPMIUTie xpo2s1 t0s borneky dsdspiite  posstooneKKyss
vccvvccvvccMutex 20 broken despite poSMUTPoexn @K1 ey0bsrokoen ndesepitie p ostsonoeKKyss
cvcvcMutex 20 broken dEMUStpiext 21p os0brtopken deeYSMpuIteux p2xo1stb0pronkre dysdpitpei peo stosnpeoKYKsys
vccvcvcvcvcvcvcvvccvcvcvcvcvcMutex 20 broken deSMPUItex p2o1s 0tbpronke dysSPMiUTeu xpo2xs1 tpb0ornekr ydsdpeitpei peo stosnpeoKYKsys
cvcvvccvcvvccvcMutex 20 broken despite pOMSUTpeox n@ !K e0by rsokoen ndesepitie p ostsonoeKKyss
cMutex 20 broken deSMPUItex p2o1s 0tbpronke DYMSMUTspeix t2 1p0obsrtpkoenc DKDsypptie p ostsonoeKKyscsMutex 20 broken despite postponeKeys
vccvcMutex 20 brokenv dEMsuptietx 2p1o sbtrpokneCK DEMyusspiext  20p osbrtopokenn KDyspite postponeKeys
cvcMutex 20 broken DMEuTSPeiuxt 2 x1pobs0r otpkoren DMUSYDpimuxtp i 2x1pe osb0rtoskrenpno DYKSpiyt p ostsonoeKKyss
cvvcvccvcMutex 20 broken desPMIUTTe x po2s1 tp0boroneKO yndssepitie p ostsonoeKKyss
vccvcvcMutex 20 broken deSMPUItex p2o1s 0tbpronke dysspite postponeKys
vccvcMutex 20 broken deMSUPtiex 2p1o s0btrpookne deyspitce postponeKys
cvcMutex 20 broken deMSUPtiex  2p1o s0btrpookne deySMPUIutetx xp2o1s0tb prornke dysdpitie p ostsonoeKKyss
cMutex 20 broken despite postPMOUTNeTXK 2y 1s 0b rokoen ndesepitie p ostsonoeKKyss
cMutex 20 broken despite postponeKeys
vccvcMutex 20 broken despite postponeKeys
vccvcvcvcMutex 20 broken despite posvtpOMnuetKx ys21 broken despite postponeKeys
kareltucek commented 5 years ago

ouch

orthoceros commented 5 years ago

Good news!:

cvcvcvcvcvcvcvvcvccvcvcvcv
vccvcvvccvvccvvccvcvvccvcvcvvccvcvcv
vvccvcvcvcvcvvccvvccvvcvccvcvcvcvcv
cvcvcvcvcvcvvccvvccvvccvcvcvcvcvcvcvcvcv
cvcvcvcvcvcvcvcvcvcvvccvcvcvcvcvcvcvcvvccvvccv
vcvcvcvcvvccvcvvcvccvcvvcvccvcvvccvcvcvcv
cvvccvcvcvcvcvcvcvcvcvcvc

It works after adding postponeKeys also in the last two lines of the macro! :) Obviously, I hit the next cv so fast that it ran concurrently with the unprotected last lines of the previous/postponed macro. So, I think, this was a logic error in my test macros, not a firmware issue! Sorry for the ouch ;)

kareltucek commented 5 years ago

That might even explain that garbled output :-). I figured that one of the macros would be triggered by the other one in the unprotected lines, but did not realize that you might be smashing before the previous macro had a chance to finish its output.

I still don't like what happens when you run the two concurrent writes...

kareltucek commented 5 years ago

I think that the garbled text is after all really caused only by the two macros interleaving their output - because order of newly added keys is not determined and because modifier modifies all keys of a report and not only the one for which it was added. I will be adding a mutex on the text dispatch mechanism (i.e., if it is used by one macro, all other macros will wait).

kareltucek commented 5 years ago

Anyway, thanks for report and collaboration, this was a very good catch!

orthoceros commented 5 years ago

You are welcome. Thank you for making my UHK better! :-) I will now have to turn back to uni (busy research weeks ahead...). But I will come back, trying to implement macro ideas like a resolve2KeyModUntilBothAreReleased for alphabetic key pairs in the weekends to come...

kareltucek commented 5 years ago

What is exactly the idea? That two keys together would act as one secondary-role switch?

orthoceros commented 5 years ago

Yes; I just explained the details in a new issue.

kareltucek commented 5 years ago

@orthoceros by chance, haven't you noticed some key chatter in the last few releases (especially the today one)?

I am afraid that the postponer-related changes might be the culprit...

orthoceros commented 5 years ago

The only thing I noticed recently was that when I pressed the right case key (in front of Space), which I mapped to Del, I sometimes got Del two times instead of just a single Del. I have not typed much with the last firmware, yet. If I notice something, I will report it here.

luteijn commented 5 years ago

I did see some chatter (two keys for one attempted keystroke), and when switching back to the official firmware it seems to have gone. So it might be something to look into.

kareltucek commented 5 years ago

@luteijn can you provide some more details?

luteijn commented 5 years ago

Some sample text with the duplication left in:

The quuick brown foox jumped over the lazy dog. All cows eat grass. Jackdaws something something mmble sphinx of green quartz.. The word mumble in the previous sentence had a double backspace eating the u. The duplicated periods are suppoosed too be only oone periiod. The word duplicated in the previoous sentence actually had a doouble i that got eaten by a double backspace trying to erase the mistyped letter after it, and I know the difference between to, tooo and two, ut the duplicate o is of course not mine there. I mistyped but, and the b got double backspaced.

It's particularly obvious the o is prone to duplication, but that may just be because it is a vowel so has a relatively high frequency. I've switched back to the official firmware and will type the same bit of text again to see if there's any duplication, perhaps caused by my own shakiness:

The quick brown fox jumped over the lazy dog. All cows eat grass. Jackdaws something something mumble sphinx of green quartz. The word mumble in the previous sentence had a double backspace eating the u. The duplicated periods are supposed to be only one period. The word duplicated in the previous sentence actually had a double i that got eaten by a double backspace trying to erase the mistyped letter after it, and I know the difference between to, too and two, but the duplicate o is of course not mine there. I mistyped but, and the b got double backspaced.

No duplication at all... and actually the duplication is a lot worse than I thought, as before I'd only typed a few paragraphs of text in a non-ideal position with a big book in front of the keyboard to type 'around', and I blamed most of the duplication to that, my own clumsiness, humidity changes and/or forgetting what mode in vi I was in...

luteijn commented 5 years ago

From a quick test with a few of the older firmwares I had lying around, the issue starts with firmware version kt.15, I'll flash kt.14 and see if it does turn up there too and I was just not hitting the issue in the short test:

this is written with kt.9 firmware. It looks like there is no chatter of the kind described above present here, although this is a bit of a short sample so I'll write a bit more. hyperloop hyperbole hyperspace. hmm what else uses lots of keys on the right side of the keyboard. it doesn't really matter does it?

this is written with kt.12 firmware. It looks like there is no chatter of the kind described above present here, although this is a bit of a short sample so I'll write a bit more. hyperloop hyperbole hyperspace. hmm what else uses lots of keys on the right side of the keyboard. it doesn't really matter does it?

this is written with kt.14 firmware. It looks like there is no chatter of the kind described above present here although this is a bit of a short sample so I'll write a bit more. hyperloop hyperbole hyperspace. hmm what else uses lots of keys on the right side of the keyboard. it doesn't really matter does it?

this is written with kt.15 firmware.. It lookos like there is no chatter of the kkind described above present here although this is a bit oof a short sample so I'll write a bit moore. hyperloop hyperbole hyperspace. hmm what else uses loots of keys on the right side of the keyboard. it doesn't really matter does iit?

kareltucek commented 5 years ago

Thanks very much! This is all I need (and more actually!) for the time being, I will look into it soon.

The confirmation about right-half-only narrows down the problem pretty much even without knowing the exact firmware version. The thing is that states of the right half and left half get read & copied into the state matrix via different mechanisms. One of them runs synchronously, the other one asynchronously, and the states are being updated in a bit clumsy manner, which is hard to predict...

orthoceros commented 5 years ago

I am still on kt.16 and definitively do not have this double-key chatter, except for the occasional double-backspace on my right case key. As this chatter always involves the same scan code (no "cross-key-chatter"), I wildly guess that it could be worthwhile to test, whether the issue gets any worse/better by setting $setDebounceDelay 0 or $setDebounceDelay 250? Could there be any code interference between the new postponing and debounce-logic, maybe

kareltucek commented 5 years ago

No need for that. There's nothing wrong with postponing and there's nothing wrong with debounce logic. See the previous answer which you have probably missed ;-).

orthoceros commented 5 years ago

I see; sorry for my cross-post earlier. Great that you could already narrow it down with @luteijn's "right-half-only" info! (I just wonder, why it happens so rarely and only on the right case key for me, but much more frequent for him....)

kareltucek commented 5 years ago

That's a good point. It seems like I've accidentally disabled debouncing after all. Fixed in 8.5.4.kt.17.

Now I hope it was the only problem... ...and wonder why right half only.

luteijn commented 5 years ago

This is being typed with kt.17; so far so good, and no more duplication of keys.

As for the differences in being hit by this between us, it probably has to do with differences in typing style. This might even explain why my right hand is the only side affected, I might just be hitting the right keys softer, or more/less decisive, or in quicker/slower succession etc. than the left ones and/or you do. The environmental situation may be worse here too, making debouncing more needed. I'm not a great typist, so I don't type very consistently, which may also make a difference.

And I expect that the right keyboard half is just a bit more vulnerable to bouncing keys due to the differences in reading in of the state. Anyway, things seem fine again now for me, so any bugs left probably are canceling each other out in my case.

kareltucek commented 5 years ago

Thanks for testing! Happy to hear that the thing works for you now.

I think that the assumption that right half's hardware is more vulnerable to bouncing is the most likely so far (e.g., due to having faster update cycles (I make this up, but sounds legit...)). The old key chatter tickets do not mention right half being more prone to chatter, but there always was some debouncing algorithm on key presses... Also, I think it has nothing to do with your writing style since my and Orthoceros's bouncing happened on right half only too.