Secondary role issue - Githubissues

cbucknell commented 5 years ago

Firmware: 8.5.4
Agent: 1.2.12
OS: Windows 10

To replicate this issue, assign the "Mod" layer as a secondary role to the right spacebar, using Space as scancode (default). This enables the use of the arrow cluster with one hand, while still being able to use the right spacebar as Space when tapping.

Issue: Whenever space and another key are quickly pressed alternately, most spaces are not registered (e.g., alternating "a" and space: aa a a aa a a aaaaaa). Issue can be also replicated when typing very fast since some spaces are not registered between words. This issue is solved when the secondary role is deactivated (e.g. a a a a a a a a a).

Additionally, when enabling the secondary role through a third party software (TouchCursor: https://martin-stone.github.io/touchcursor/) and disabling the secondary role in the UHK, the issue disappears. This suggests that the issue is firmware related.

mondalaci commented 5 years ago

@kareltucek @eltang Can you guys reproduce this, or have an idea about the root cause? This hasn't ever been reported yet.

kareltucek commented 5 years ago

If I understand the issue correctly, it's the same old good one which has been discussed between me and @p4elkin ( https://github.com/UltimateHackingKeyboard/firmware/pull/193 and https://github.com/UltimateHackingKeyboard/firmware/issues/187 ).

Basically, when writing fast, the keypresses overlap in time. I.e., when writing " a", the a is pressed prior release of the spacebar which therefore acts as a layer switch instead.

So yes and yes. Unfortunately, I do not have a good idea about a proper solution...

/E: removed the no-longer-relevant paragraph

@cbucknell, would you mind investigating and describing here how the touchcursor solves the resolution? I didn't manage to get it running in a virtual machine...

/E: sorry for the edit mess... I hope that email notifications are not fired at every update?

cbucknell commented 5 years ago

@kareltucek managed to find the source code of TouchCursor: https://github.com/martin-stone/touchcursor -- is it useful?

kareltucek commented 5 years ago

Ok, once again, seems like I've let my expectations mislead me into a terribly bad analysis.

I went through the source code and tried the program on an old windows box. The touchcursor solution does not contain any explicit delays at all - it simply resolves the situation depending on the first key up event (if the "layer switcher" is released first, it is taken for primary action, otherwise for secondary). Activation of the "action key" seems to be postponed until its release. If both keys are held without release, it seems that windows repetition hook kicks in on its own.

The touchcursor handles it via a relatively simple state machine, which is defined at 493 in https://github.com/martin-stone/touchcursor/blob/master/touchcursordll/dllmain.cpp . The postponing mechanism uses a single register. Nice & clean.

This strategy sounds very reasonable to me. The only imperfection is a small delay before activation of "held" keys (like mouse movement).

As a result I gave one more though to the mechanics of my modified macro engine - this time I've found a way how to integrate a proper solution without much pain. Lets view it as a proof-of-concept experimental implementation. Here is a prepared configuration so you can try out easily: UserConfiguration.json.txt https://github.com/kareltucek/firmware/releases/tag/v8.5.4.kt.4

After flashing the firmware and loading the configuration, you shoul be in an "INI" keymap. Then, hitting spacebar will initialize internal variables and switch you into M3A - a regular US qwerty layout. Here, S D F act as secondary switchers - S will activate mouse mode on H J K L; D will activate arrow keys on the same cluster.

@mondalaci what do you think?

mondalaci commented 5 years ago

@kareltucek Very sorry for the late follow-up, and thanks for delving into this! I'm not sure when will I be able to dedicate sufficient time for this issue.

@cbucknell Would you give a try to @kareltucek's firmware to see if it resolves the issue?

cbucknell commented 5 years ago

@mondalaci tried @kareltucek firmware and it solves the space issue. Even if I type very fast, spaces are indeed recorded every time! Did not understand though how the SDF secondary switches work though.

Edit: did not load the config correctly, see comment below

kareltucek commented 5 years ago

@cbucknell , now I am not sure what exactly you have tested if you write that you did not understand the SDF switches. Did you not understand how to use the bindings or how is the macro composed or how it internally works? (The config/firmware above does not contain modified space, as you must have noticed, but contains secondaries on the three keys, so that if you press s+j, the cursor should move down, etc..)

If you wish to modify your own maps to use the spacebar, there's a (simpler) example an some documentation in the repository at https://github.com/kareltucek/firmware - just search for resolveSecondary. If you need to tailor the macro for different function or explain anything, simply ask ;-) .

/E: refined the explanation in the readme a bit.

cbucknell commented 5 years ago

@kareltucek @mondalaci I didn't load the config correctly and had touch cursor enabled. Sorry!

Fixed it and now the SDF switches are working! I tried alternating quickly S and J to check if sometimes the cursor would get activated. It rarely does, however it seems that it happens a bit more often compared to the touch cursor. I imagine it depends on the listen time? How complex is it to implement touch cursor's implementation so we can compare?

p4elkin commented 5 years ago

@cbucknell fwiw, I guess months ago I had a similar issue (@kareltucek mentioned the issues like #187). I have created my own fork which is spiritually not far from what @kareltucek has done but is much less feature rich: it only re-implements the way first/second roles are handled. Been living with it for quite some time and experience almost no troubles whatsoever. Unfortunatelly, I don't have time to actively participate on this research right now, but would like to return in future.

Here's how my typical config looks like, if that's something you are aiming for (as you can see - all the layer toggles are on alphabetic keys, as well as alt and cmd) - feel free to give it a shot.

This fork should be compatible with agent and the normal configuration flow should be applied. The latest release is here: https://github.com/p4elkin/firmware/releases/tag/8.5.3.187-4

cheers, HTH

kareltucek commented 5 years ago

I imagine it depends on the listen time?

It should not if the time is long enough. You can increase the time to e.g., 600ms to make sure.

How complex is it to implement touch cursor's implementation so we can compare?

This should be (more-or-less) the touch cursor version as far as I understand the code correctly. I have to admit that I am a bit confused by the waitMappedDownSpaceUp state and by the fact that the code contains no timing checks at all. I think that I can see one more parameter which could be possibly added to the solution and which might be present in the touch cursor solution by means of internal mechanisms of the windows hooks. I will think about it a bit more.

Unfortunately, I cannot take the thing from touch cursor directly and put it into UHK, because the two things take entirelly different sort of input - UHK provides raw key states, while touch cursor takes preprocessed windows hooks (I.e., KeyDown, KeyUp and KeyTap actions or something similar) and hacks around them.

The latest release is here: https://github.com/p4elkin/firmware/releases/tag/8.5.3.187-4

@p4elkin Cool! I will give it a shot tomorrow!

kareltucek commented 5 years ago

My conclusion after one day of testing is that @p4elkin's firmware works like a charm.

Regarding my solution, I agree that there still is a difference between me and touch cursor, but I am afraid that I am not capable of implementing it fully without a heavy refactor of key state handling.

cbucknell commented 5 years ago

@p4elkin will give it a shot! Will provide impressions soon.

cbucknell commented 5 years ago

After two weeks of testing @p4elkin's firmware, I can say it works really well. It would be great to see this functionality implemented in UHK's firmware by default. Maybe even having an advanced option to modify the timeouts. cc @mondalaci

mondalaci commented 5 years ago

@cbucknell We'll definitely consider it. Thanks for your feedback, and @p4elkin's great work!

kareltucek commented 5 years ago

Btw, there's a new release of my version of the feature. (Finally, I have refactored the state handling mechanism so that it still follows the official code 1:1, but decouples activation of keys from hardware and debouncing states.) Any feedback will be appreciated.

p4elkin commented 5 years ago

Hey @mondalaci,

Sry to distract with this topic once again, I've a small question: I still experience intermittent chattiness of the keys with my fork. It doens't happen consistently (which makes it super hard to trace) and only affects the left side. Do you maybe have any pointers to pay attention to (like halfs state synchronisation or whatnot)?

It must be somehow related to the fact that I continiously track the pressed keys and sometimes include them into the report in a delayed fashion (i.e. detect the pressed key, but actually push it into the report in the next update cycle), but I am a bit clueless, cause it never ever happens with the right half (which makes me think that the algo is mostly fine, but violates some underlying communication logic).

I know that helping with forks isn't the teams responsibility, just maybe if you have anything from the top of the head, thanks!

mondalaci commented 5 years ago

@p4elkin Can you reproduce this issue with the official UHK firmware? This might be due to a hardware issue.

p4elkin commented 5 years ago

wow, looks like I indeed totally mis-read the symptoms and after I took care of the most problematic keys mechanically, the situation improved dramatically.

So far no occurrence of chattiness 🎊 👍 (could save me some time spent on banging against the wall 🤕 😀 )

kareltucek commented 5 years ago

Sorry for being pessimistic, but if this was a switch-only issue, it should not have had occured on left half only. Of course, cleaning switches is one way to fix a misbehaving debouncer :-).

My suggestion is to look at the way the key states are rewritten into state matrix - as far as i remember, right half is synchronized synchronously ( from usb_report_updater.c 463 ), while left half is synchronized asynchronously ( similar snippet in slave_drivers/uhk_module_driver.c ) - from an interrupt/callback.

If the synchronisation occurs in the middle of processing the key in the updateActiveUsbReports function (I.e., between debouncing and between updating last to current.), it can mess things up. I am no longer sure whether the problem affects official firmware too - if it does, then only with very rare occurences since the problematic timespan is very short within official firmware. Either way, even an innocent change into the code can trigger this...

p4elkin commented 5 years ago

my "usual suspects" were 'b' and 't' (most of the issues). I was almost sure it was a mechanical issue, but then I had several cases of other keys on the left half mis-behaivng, which made me think of the programmatic source of the problem. Anyhow, so far it seems surely bearable, but I'll keep in mind your pointers, will be the first thing to dig if the problem comes back in full power, thanks.

p4elkin commented 5 years ago

@kareltucek your pessimism was prophetal - the issue kept coming back randomly (sometimes - annoyingly).

However, in my mind I had ruled out the potential "race" condition of key state matrix update: didn't know at all how interrupts work and thought that the asynchronity achieved in a similar way to e.g. JS frameworks (you have some sort of a loop which may trigger an async callback only after the user code returns control, which lead me to think that while another iteration of usb_report_updater is running - the key state matrix is intact). Looks like I was wrong and indeed what you've described appears to be a plausible scenario.

I did apply the synchronisation: whenever the communication happens over i2c (in uhk_module_driver) - update not the KeyStates itself, but a dummy copy (luckily memory can fit it in :)). Then in usb report updater read the left half's current states from the dummy, pretty much the same way like the right half's keys are updated.

Never had a single occurrence of chattiness ever since (around a day of active work so far)!

Thanks again for the suggestions so far, will update if I notice the problems later.

p.s. @mondalaci seems to me that the official firmware might also be vulnerable to this problem at least in theory, if it might give you some food for thought, here's the commit: https://github.com/p4elkin/firmware/commit/771a9511f9000cc6e4a75f0240647e77aa10d32a (obviously far from an optimal solution, but does the trick just fine).

kareltucek commented 5 years ago

Well, what if we assume that race and bounce happen both at the same time? I think that it could actually trigger the problem in official firmware.

Some ideas about testing this:

Set debouncing period to absurdly high value (5 seconds?) and simule bounce by actual doubletap - the longer the release time between keys the better.
Move debouncer into its own loop before the internal loop begins in order to increase the probability of race.

p4elkin commented 5 years ago

The second bullet point is exactly what I've been doing in my fork: before emitting any report updates - mitigate bouncing, populate the active state, see the situation with the pressed keys vs their previous state etc, only then - process the state and emit the actions.

During one of my investigations of the matter I've been stripping out all the logic I had to barely this: populate a simple state and emit report records re: currently pressed keys. And I was occasionally experiencing chattiness (now makes sense kinda!).

Re: first point - nice suggestion, was also thinking to give it a shot that way. I went ahead and even though it performs a tad weirdly, there's a clear difference between how the halves perform: I never managed to screw up the right one, whereas with the left one I can often make it type several identical chars during the large debounce period.

Here's a branch in my fork that simulates the situation: https://github.com/p4elkin/firmware/commits/simulate-sync-issues, here's the commit: https://github.com/p4elkin/firmware/commit/f70cd1e18133cac79362f0a2b4694cfc0c32f561

p4elkin commented 5 years ago

What is also interesting, this commit: https://github.com/p4elkin/firmware/commit/573a2ef1dcbd0a9b60fa8e9531005c6a1988fd72 (the same trick that I applied to my fork) gets the situation back to normal in my case and the halves start to behave in the same way!

I'll probably go ahead and file a separate ticket for this.

control-freak commented 3 years ago

can anyone please tell me how to flash the firmware and load the configuration of kareltucek? i am fairly new to this and googling didn't help at all.

kareltucek commented 3 years ago

Sure!

1) Note that this thread is pretty old and therefore most of the information are no longer relevant. 2) Flashing:

First, download new release from https://github.com/kareltucek/firmware
Start up your agent - any version should do. Of course, preferably a newer one (if I remember correctly, compression format was changed at some point, and I am not sure if the backward compatibility applies).
Click firmware in the left pane.
Click 'Choose firmware file and flash it' (and select the file) 3) Writing your first macro should be covered by 'Getting started' at https://github.com/kareltucek/firmware. If not, please create a new ticket in that repo and tell me there what can be improved. 4) Regarding secondary roles, I see that there is no relevant example, but something like this should do (each line has to be pasted as a separate "write text action"):
```
$ifSecondary final holdLayer mod
$tapKey escape
```

control-freak commented 3 years ago

So can this be be achieved with better software? Sorry i probably sound stupid but can you give me a link to download the agent? Google gives too many different results.

kareltucek commented 3 years ago

Agent is the configuration tool for UHK. E.g., you can get it at https://github.com/UltimateHackingKeyboard/agent/releases

control-freak commented 3 years ago

It says Cannot find your UHK Please plug it in! I have a normal keyboard. Will that do?? :|

kareltucek commented 3 years ago

Nope. But do not despair, you can buy your own UHK at https://ultimatehackingkeyboard.com/ :-).

eric-fulfil commented 3 years ago

I desperately desire UHK to be the best keyboard in the world in all ways (99% there!) and the implementation of Secondary Roles has 2 major issues that make it useless for all my use-cases when compared to ZSA's firmware for example, which does this perfectly. I wish to use 'A' and ';' as shift keys (and other modifiers under my fingertips, but we need not get that crazy yet), but typing at even a moderate speed is unusable in this case, typing "back" almost always results in "bCk". The fix here is simple (in theory, the firmware code is a bit beyond my reasoning) and the only thing stopping me from $ on 2 more UHKs! Top of this wishlist is changing or providing options for:

The timeout after a key is released before it's secondary role potential is cancelled and the primary button fires. For all my cases, this timeout must be exactly zero; once the key is up, shift should not be applied. I think the timeout here is 250ms or something like that, but I can't even think of a use-cases where non-zero even makes much sense tbh.
The primary key should not be fired after enough time has elapsed to enter secondary mode. I might want to shift then change my mind after awhile, without wanting to type 'A'.

kareltucek commented 3 years ago

Eric, we are talking about the basic simple implementation which is present in the official firmware, right?

Have you tried the implementation in p4elkin's firmware or mine? If not, you definitely should. (If you have any questions or ideas about improving the algorithm provided on my branch, please create a new ticket there.)

The fix here is simple

The fix is simple just under the assumption that you always release the previous key before pressing the next key when writing. Do you really write that way? I personally overlap presses of two to three consecutive keys when writing fast.

I think the timeout here is 250ms or something like that, but I can't even think of a use-cases where non-zero even makes much sense tbh.

There is no such timeout.

The only present timeout is the debouncing interval, which basically makes every keystroke last at least 50ms, which is however much less than a regular keystroke takes (which is 90-250ms according to my measurements), so it should not affect the behaviour in any way.

Regarding this feature availability on the official firmware, it is basically blocked by UltimateHackingKeyboard/Agent#562, since it needs configuration. (This is because due to its nature, secondary resolution is not suitable for all usecases - namely, because it is less deterministic than the simple version and because it introduces artificial delays in keyboard operation.)

mondalaci commented 3 years ago

Would the artificial delay introduced by the fix of this issue be noticeable by users? If not, changing the user configuration format may not be required.

kareltucek commented 3 years ago

Yes, it would be. (The resolution depends on the release of the previous key, so the delays may be even up to 300-400ms long.)

I am confident that in case of my implementation, leaving the users with my implementation only (assuming that it is a sane default) is not acceptable. The user must have at least a checkbox which will read something like enable alphanumeric-friendly resolution.

mondalaci commented 3 years ago

300-400 ms is significant, indeed. I'm interested in getting this fixed, and we can extend the user configuration accordingly. Waiting for Eric's testing results.

eric-fulfil commented 3 years ago

Hm haven't tried flashing a custom build of the firmware, was only browsing source code, but I'll give it a try, thanks for the tips. To reply to @kareltucek I'm 100% sure that releasing a key still keeps it in secondary mode for some time, I was absolutely sure to release the key before pressing the next key, but no dice. And just to be clear my use-cases would require exactly a zero timeout; rapidly pressing/releasing a key would immediately type the normal char upon release and deactivate secondary mode.

kareltucek commented 3 years ago

but i'll give it a try

(Quite detailed instructions are just a few posts above - the alternative version of secondary roles is available via macros...)

In case you want to play with timeouts (I suspect you will want shorter than default ones), search the README for resolveSecondary.

I was absolutely sure to release the key before pressing the next key, but no dice.

I don't think I am able to reproduce the problem. If I try to coordinate release of one key with hit of the next one, I am getting mixed results. But in that case, I usually manage just to release pressure on the key, but not to lift the finger before the next hit. And that is by definition correct.

Of course, it is theoretically possible to write some machinery which will prevent short enough overlaps... but I do not see much sense in it, since I believe most people are "lazy writers" who release keys lazily just when they need the fingers elsewhere and even not necessarily in correct order...

eric-fulfil commented 3 years ago

I just wrote a small javascript snippet and realized that yes, I am indeed holding down the key when pressing the next key and that there is no "timeout" from releasing the key to deactivating secondary mode, my mistake. This pretty much just leaves an optional timeout for cancelling the primary button if held long enough as a feature request. I guess you've all thought far ahead of me in terms of supporting these more advanced use-cases. Here's the test I used to debug and confirm that I indeed have fat fingers in case anyone's curious, just plop it into a frontend with jQuery on it:

    let lastUpTime = 0;
    jQuery("input").keyup(function(e) {
        lastUpTime = new Date().getTime();
        console.log( `${lastUpTime}: keyup: ${e.key}` );
    });
    jQuery("input").keydown(function(e) {
        const downTime = new Date().getTime();
        console.log( `${downTime}: keydown: ${e.key}` );
        console.log(`ms since last keyup: ${lastUpTime - downTime}`);
    });

kareltucek commented 3 years ago

This pretty much just leaves an optional timeout for cancelling the primary button if held long enough as a feature request.

My implementation will automatically enter secondary state after 350ms, which in most of the use-cases should be equivalent to the desired no-op...

However, if needed, the conditions can be composed, e.g.:

$ifSecondary final ifNotPlaytime 350 holdKey LG-
$holdKey space

Or the other way around, if naive non-blocking secondary role is used:

$holdKey LG-
$ifNotInterrupted ifNotPlaytime 350 holdKeySpace

Still, this both assumes the custom firmware.

This pretty much just leaves an optional ...

Supporting alphanumeric-friendly secondaries is a reasonable request, it is just not as simple as claimed above. Omitting some details, the approach which I am using is basically to wait to see order of release of the keys - if the order is press secondary, press next key, release the next key, or if waiting for this sequence is longer than 350ms, then first key is handled as a secondary, otherwise as a primary. This might need prolonged press to activate secondary, but is pretty reliable to not interfere with writing.

I will try to find time soon to port the functionality to the official branch since Laci states it as desired. Thinking about it again, in this case it might make sense to have both drivers work alongside, and discriminate between them based on the primary action scancode, if Laci insists on non-configured solution.

Still, feedback on the $ifSecondary approch would be very welcome, since my own testing has strong "author-bias".

djrenren commented 3 years ago

I recently switched from ergodox to UHK and noticed this weirdness with dual function keys. FWIW their default is requiring both keys be held for the timeout to get the modified result, and it always did what I expected.

They have a couple different options that describe a lot of the same suggestions in this thread. I don't think we need all of them, but they're useful for discussing what behavior we want:

https://configure.ergodox-ez.com/ergodox-ez/layouts/default/latest/config/tapping

kareltucek commented 3 years ago

their default is requiring both keys be held for the timeout

Mm... Does it restrict order of the presses/releases? Specifically, does the dual-function key have to go first, or can the other key go first too (as long as the "overlap" condition is satisfied)?

djrenren commented 3 years ago

It does restrict the order. The dual-function key has to go first. The sequence must always be: (to use their syntax)

SFT_T(KC_A) Down (on the dual role key)
KC_X Down (on another key)

After that they have a couple of resolution options. This seems super sensible because it mimics how keys like control and shift work on a regular keyboard.

For the life of me I can't tell if release order matters on QMK. I lent my QMK keyboard to a friend but I'm getting it back later today and I can report on some experiments.

I think the big difference between QMK and UHK is that the UHK doesn't require a timeout to count as a hold. It appears to work like so:

SFT_T(KC_A) Down (on the dual role key) ---Some amount of time less than the tap timeout---
KC_X Down (on another key) ---Immediately output X---

which is unforgiving to fast typists (as noted earlier in the thread).

This is similar to the Permissive Hold option (off by default) on QMK which would work like:

SFT_T(KC_A) Down (on the dual role key) ---Some amount of time less than the tap timeout---
KC_X Down (on another key) ---Still less than the timeout---
KC_X Up ---Still less than the timeout, Output X---

In the Permissive Hold case, the keyup ordering does matter. It allows you to skip the waiting so long as the second key is released before the first.

I think Permissive hold could still have misfires for users. Requiring the key be held past some timeout to get the modified behavior seems reasonable. It does means you'd have to buffer key presses until either the timeout is reached or the key released.

Also there's a longer article on the implementation of tap and hold in QMK from their docs. It still has some ambiguities unfortunately but it's better than the blurbs in the configurator that I linked earlier: https://beta.docs.qmk.fm/using-qmk/software-features/tap_hold

djrenren commented 3 years ago

I've created a little fiddle of the sort of thing I'm envisioning. fiddle

It includes a little visualization of how the buffer is working. It also allows playing around with behavior when there are multiple dual-function keys in play. The top the of the TS lets you tweak the timeout and the dual-function mappings. I defaulted the timeout to an absurdly high 2 seconds so that the behavior is easier to see, but lowering it will give a better impression of the typing experience. (i recommend 200ms)

p4elkin commented 3 years ago

hey @djrenren,

Didn't have the time to fully digest all the listed options, but it looks like what you have in mind does resonate with my version of the firmware (linked above). It is quite dated by now, but I am still on it and have no problems using 'a' 's' and 'd' as the mouse/mod/fn keys.

The algorithm aims to switch between several states during the typing process: 1) handleFreeTypeState() free typing mode (ordinary keys), all the keys are emitted already upon key-down detection. 2) handleSecondaryRoleReleaseAwaitState secondary role key "suspicion" mode (key-down doesn't emit, key press is cached). In this state we await other events:

when other key is pressed we look whether the cached secondary key is still pressed and if not - we first emit it retrospectively and then the currently pressed key (to respect the order). If the cached secondary key is still on - we move to state 3. 3) handleActiveSecondaryRoleState secondary key role is on, take it in account for all the emitted events until the key is released.

Effectively the secondary role kicks in iff the respective key is held during a certain threshold (250ms, see SECONDARY_ROLE_ALPHABETIC_KEYS_KICK_IN_THRESHOLD) or if another key was released while the modifier key is still pressed (normally never causes false positives during typing, cause you'd release the key with secondary role before the following key).

See the relevant snippet:

 if (State.modifierCount > 0) {
        // detect if any modifier becomes active
        //
        // the previous conditions have to be met
        // or any of the modifiers may pressed long enough
        for (uint8_t i = 0; i < State.modifierCount; ++i) {
            if (modifier(i)->keyRef.state->current) {
                if (secondaryRoleTimeoutElapsed(modifier(i))) {
                    shouldTriggerSecondaryRoleActivationMode = true;
                    break;
                }
            }
        }

        for (uint8_t i = 0; i < State.actionCount && !shouldTriggerSecondaryRoleActivationMode; ++i) {
            if (!action(i)->keyRef.state->current) {
                shouldTriggerSecondaryRoleActivationMode = true;
            }
        }
    }

You can check out the details here

cheers!

djrenren commented 3 years ago

Hi @p4elkin. Thanks for the awesome writeup! It looks like your scheme is what QMK would call permissive hold. Seems reasonable.

I'm concerned a bit about this state machine because It's not clear how it handles multiple secondary role keys that are pending. It may be totally fine but I'm struggling with the firmware code cuz this is the first time I've really peeked into it.

What happens when you have the following case:

0ms: z/ctrl down (we've pressed a dual function key) 10ms: t down (we've pressed a single action regular key) 20ms: x/shift down (we've pressed a second dual function key) 30ms: y down (we've pressed a single action regular key) 40ms: z/ctrl up (we've released the first dual function key) 50ms: y up (we've released a single action regular key) 60ms: i down 230ms: u down

In theory this should emit: 40ms: z, t 50ms: shift+y 220ms: shift+i 230ms: shift+u

To call out why this case is interesting:

At 40ms, we see the z/ctrl key is released so we can replay it and any intermediate actions UP TO the next unresolved key. That is, we can't do anything with y down event at 30ms because it is blocked by an unresolved secondary role key x/shift which was pressed at 20ms.
At 50ms, y is released so we consider it modified by all the secondary roles currently pressed, even if they're unresolved. (You could make an argument that this should emit ctrl+shift+y because both roles were present on the down press, but i think the state at release will cause fewer typing mistakes)
At 220ms we've reached the timeout for x/shift, meaning we can emit the queued i press (even though it hasn't been lifted yet). Notably, this is the only emit that's occurring as a result of time rather than in response to a key press.
At 230ms: We're past the timeout for x/shift so we know it really means shift. This means that we can emit shift+u immediately on press without waiting for time or the up event.

I think these 4 cases capture most of the edge cases of the space. My biggest concerns with your state machine is how it handles these overlapping secondary roles (so the 40 and 50ms emits) and how it could handle timeout-based key emits (220ms). The latter might not be worth the effort or you might not agree it's the even a good idea which is totally fair, but it's the most responsive while still preventing mistakes which is why I'm throwing it out there.

kareltucek commented 3 years ago

220ms: shift+i

This does not seem right - isn't x/shift already in shift mode because of the y up?

kareltucek commented 3 years ago

I've created a little fiddle of the sort of thing I'm envisioning. fiddle

@djrenren what about trying out your fiddling directly on the UHK? O:-)

Here's some code which implements the strategy as you describe it https://github.com/UltimateHackingKeyboard/firmware/pull/326 . All important logic is placed in resolveCurrentKeyRoleIfDontKnowTimeout. If you return SecondaryRoleState_DontKnowYet, it will be called again in next refresh cycle. When the event (press/release) pointer is NULL, it means it didn't take place yet. If it is non-null, it contains .time which can be used to determine behaviour.

Personally, I am hitting false positives a lot (with 200ms and permissive mode = off).

djrenren commented 3 years ago

This does not seem right - isn't x/shift already in shift mode because of the y up?

Ah I was thinking per-key as in "if an entire key down/up cycle occurs while an unresolved dual function key is held, treat that keypress as modified, but don't resolve the dual function key until the timeout", but you're suggesting that that should change the mode of the key immediately. Makes sense. I'll have to fiddle and see what works!

@djrenren what about trying out your fiddling directly on the UHK? O:-)

For sure! Thanks for the quick primer on the code. I'll definitely start playing with it!

djrenren commented 3 years ago

Sorry for the slow response, I was having issues with the windows setup, so I had to get my linux environment back in order.

@kareltucek, are the false positives you're hitting cases where the tap behavior is selected but the held behavior was intended? Because I've noticed your implementation does something I didn't expect. At the end of the timeout, if the key has not been used as a modifier, it treats it as the tap behavior. (This gave me so many false positives that the keyboard was hardly usable). This means you have to be quick lest you get extra input. I was thinking that if you held the key past the timeout, it would behave like the secondary role.

The following diff fixed all my false positives:

This has the downside of making it impossible to hold the key down for repeated entry. QMK "solves" this by allowing a tap then hold to behave as a held down key.

kareltucek commented 3 years ago

are the false positives you're hitting cases where the tap behavior is selected but the held behavior was intended?

Nope - the other way around. I.e., secondary role (hold behaviour) often activates when writing. I am a relatively decent touch typist (~55wpm), but I am also a "lazy writer", meaning that I tend to release keys quite slowly, so my problem is not much of a surprise. For instance, in the word "star", I tend to first press the first three letters before I even start releasing first.

Still I am happy to hear that it indeed works fine for you. Means we are on the right track.

At the end of the timeout, if the key has not been used as a modifier, it treats it as the tap behavior.

Yes, this is always the question, since the "correct" timeout behaviour depends on the key in question.

But as you figured out, it is trivial to fix/"fix" :-).

This has the downside of making it impossible to hold the key down for repeated entry. QMK "solves" this by allowing a tap then hold to behave as a held down key.

This should be quite easy to add too - suffices to remember the previous resolution key and time and compare it against a timeout.

Regarding my problems, it will be quite easy to fix by lenghtening the timeout and adding one more "safeguard" timeout at the end (which relies on the fact that am quite reliable at releasing keys in almost the right order), just will have to find some time to implement it. In the end, the same algorithm should work for both, just will have one more parameter...

UltimateHackingKeyboard / firmware

Secondary role issue #207