Firngrod/global doubletap handling #1477

firngrod · 2026-02-04T20:05:19Z

Running this now, seems to work fine.
I want to run it for a bit to make sure.

I only propagated the press as far as it needed to go down the tree in usb_report_updater.c.
I think maybe the sequence number for macros should go into the key press info.
Do you have a procedure for remembering to remove removed config values in the future?

Also switched detection of same press from a time-base to a key press count base

…meout

right/src/config_parser/parse_config.c

firngrod · 2026-02-04T20:08:09Z

right/src/macros/commands.c

-    }
-
-    return doubletapFound != negate;
+    return S->ms.keyPress.isDoubletap != negate;


Felt good. The reason for this whole endeavor.

right/src/macros/core.c

right/src/macros/set_command.c

right/src/key_history.h

right/src/key_states.h

firngrod · 2026-02-04T20:15:12Z

right/src/key_states.h

        bool previous : 1;
        bool debouncing : 1;
        secondary_role_state_t secondaryState : 2;
+        bool padding : 1; // This allows the KeyState_NoActivity() function to not trigger false because of sequence


Doing this was just too tempting.

Since we have already allocated another byte, shouldn't we move the secondary role bits there too?

right/src/secondary_role_driver.c

right/src/usb_report_updater.c

kareltucek

I have a good feeling about this refactor (thanks for it! I think it really benefits from your clean plate perspective).

We could merge it as is I think, but at the same time, to get the most benefit from it, we should make one more iteration of refactors.

right/src/config_parser/parse_config.c

right/src/key_history.h

kareltucek · 2026-02-05T12:04:15Z

right/src/key_states.h

+    typedef struct {
+        key_state_t *keyState;
+        bool isDoubletap: 1;
+    } key_press_info_t;


I feel like uint32_t activationTime should become part of this structure.

As well as the sequence number and secondary state.

Secondary state does not make sense on the press info. It is only resolved later. See my other comment on those.

right/src/macros/core.h

kareltucek · 2026-02-05T12:10:45Z

right/src/key_states.h

        bool previous : 1;
        bool debouncing : 1;
        secondary_role_state_t secondaryState : 2;
+        bool padding : 1; // This allows the KeyState_NoActivity() function to not trigger false because of sequence


Since we have already allocated another byte, shouldn't we move the secondary role bits there too?

kareltucek · 2026-02-05T12:19:46Z

right/src/macros/core.c

+                          uint8_t keyActivationsSeq,
+                          uint8_t parentMacroSlot,
+                          bool runFirstAction,
+                          const char *inlineText)


[philosophical nitpick]

I have very mixed feelings about this kind of formatting.

My scala/functional background screems loudly for uniform parenthesis formatting handling and says it should be:

uint8_t Macros_StartMacro( uint8_t index, const key_press_info_t *keyPress, uint16_t argumentOffset, uint8_t keyActivationsSeq, uint8_t parentMacroSlot, bool runFirstAction, const char *inlineText ) {

But I do recognize that this is not a C conventional style, and the clang format we provide isn't configured properly, so this is more of an opinion exchange than any request for any kind of reformat.

If you have your opinion on this, you are welcome to share 😊. Otherwise this is a resolve-without-any-action case ;-).

I have no strong feelings other than lines should be short enough to fit in my editor, and once the line breaks, I want only one argument on each line.

I wasn't even consistent in my breaking of lines.

right/src/macros/set_command.c

right/src/key_history.c

kareltucek · 2026-02-05T12:33:29Z

right/src/usb_report_updater.c

            key_action_t *actionBase;

-            if(((uint8_t*)keyState)[2] == 0) {
+            if(KeyState_NoActivity(keyState)) {


Please use the dirty way here. Feel free to add a comment about it.

Reasoning:

This loop is a performance-critical place.

If I am not mistaken, an inline function is still a function, which means that it introduces memory barriers which may hinder compiler optimizations.

I'll find a different kind of dirt.

kareltucek · 2026-02-05T12:42:26Z

right/src/usb_report_updater.c

-                    keyState->secondaryState = SecondaryRoleState_DontKnowYet;
+                    ++keyState->activationSeq;
+                    KeyHistory_RecordPress(keyState);
+                    pressInfo.isDoubletap = KeyHistory_WasLastDoubletap();


(I find this place intuitively a bit surprising, but probably correct. I would expect these to be evaluated closer to commitKeystate)

It means that the isDoubletap that we pass around in any future evaluation is wrong, isn't it?

The isDoubletap flag should be kept in the key_state_t structure I think.

Passing the key_press_info_t around the usb report updater is clumsy, as we are loosing the activation time and the doubletap flag. I mean, it is correct to have them in there, but not to pass it around in this context.

My idea was to use the key_press_info_t to pass the information through postponer and then from activatedNow to macro engine and keep it in macro state, but accept loosing the information for "normal actions", which would mean to not pass it around in the native action (usb report updater) call tree.

Evaluating it earlier and putting it through the postponer gains us nothing, I think. Especially as the user can, at any time, modify the postponer queue, possibly requiring us to rewrite history. I think this is the best place to evaluate doubletaps.

Good point.

kareltucek · 2026-02-05T12:49:21Z

If my perfectionistic nitpicks and requests are too much, please let me know.

Even if merged as is, this definitely is a step forward.

firngrod · 2026-02-05T14:29:56Z

No problem, I do this for fun, and obsessing over details is fun.

firngrod · 2026-02-05T18:00:49Z

More to come, putting kids to bed.

firngrod · 2026-02-05T20:16:38Z

I lean in a very opposing direction from you on key state vs key press info, but the more I think about it, the more I'm certain that we should go all the way one way or all the way the other.

But before that, I actually think that secondary role state cache should be moved. I think that secondary role driver should own the state, not key state. Put 8 or 16 slots in there for caching resolutions and clear them on release. It can be done pretty cleanly. Macros should always manage their own memory of secondary roles.

Back to key state vs key event. I think that if we keep key events, the key state should only contain information about the key state. Basically what it contains now, minus secondary role. Everything else should live in events. Ideally, there should be no need to pass around key state pointers, key ID should be enough for the purpose of differentiating keys, nothing else. Everything that the logic needs in order to execute should be encapsulated in the event type: Is it a new press (ActivatedNow), new release (DeactivatedNow), is it doubletap, what was the ID of the press for macros to check back if that ID has been released, so on. Macro engine is the only place where any of this information needs to live past the event, so it can store what it needs to store. Realistically, nothing else needs the keyState really, does it? To be clear, I don't want to get rid of passing key state around (yet), but it should theoretically be straightforward to do.

The alternative to this would be to drop the event idea and instead letting the places which need the information extract that information when needed. This requires that these things are not changed during ApplyKeyAction, which I'm pretty sure is already true. For example, as things are, there is no reason that Secondary Role driver and macro engine could not just call KeyHistory_WasLastDoubletap.

The upside of the former approach is that it's formalized and clear, but wastes resources on operations which are not always needed. The upside of the latter is that it's leaner and more efficient, but the flow of information is less clear.

I'm ambivalent. Now that I've thought about it, dropping the event type is probably my preferred way to go. The macro engine needs to be tuned a bit for it, it needs to gather the information it needs on start, then pass it on to spin-offs.

The main thing about the event approach is that one can construct an event and inject it. If Secondary Role calls WasLastDoubletap, I cannot easily execute something as a doubletap, say from a macro. Now I lean that way again! Argh!

firngrod · 2026-02-05T20:24:52Z

I'm going to move secondary role state cache. That one I really like. I'll make it one commit which can be reverted if you don't like it.

firngrod · 2026-02-06T07:44:01Z

Now that I've slept on it, I think I still like the idea of passing a key event around rather than the key state, for the following reasons:

I like the idea of having access to things centralized. Having one place where we determine "This is the event that we're going to process now!" makes it a lot easier to reason about as a human.
Writing testable code for my job has given me a dislike of functions which depend on other things than what they are provided as parameters.
I like the idea of processing events rather than starting a process which then inspects state. It conceptually simplifies the mouse caret handling, for instance.

I think the additional processing to prepare this state is worth it, but I will let you be the judge.

kareltucek · 2026-02-06T11:40:58Z

But before that, I actually think that secondary role state cache should be moved. I think that secondary role driver should own the state, not key state. Put 8 or 16 slots in there for caching resolutions and clear them on release. It can be done pretty cleanly. Macros should always manage their own memory of secondary roles.

I like about the key_state_t solution that it guarantees a full nkey rollover for native actions.

I do realize that it is more of an emotional fondness than an rational reasoning though.

Writing testable code for my job has given me a dislike of functions which depend on other things than what they are provided as parameters.

Well, I have been doing pure functional programming for my other jobs... but this is not a pure functional language and codebase.

I like the idea of processing events rather than starting a process which then inspects state. It conceptually simplifies the mouse caret handling, for instance.

I think the additional processing to prepare this state is worth it, but I will let you be the judge.

I think our ideas about what this refactor should be have been diverging in two directions which are not incompatible, just not one and the same thing (probably falling apart into two press info structures - one event-oriented for the native actions logic, and one state-oriented for macro states), so I am dropping my branch for another time and lets focus on your branch.

I am not sure what changes your idea would imply. Let's say that refactors that objectively improve state of affairs are welcome. On the other hand, if it is not broken, don't fix it. I don't think I am open to reconstructing the entire codebase just to get code that looks nicer.

I think I have stated my case and will look forward to whatever implementation you come up with.

right/src/secondary_role_driver.c

kareltucek · 2026-02-06T11:50:48Z

This is getting a bit messy (🙈), but I’ve read through all your comments and I don’t think I have any objections.

firngrod · 2026-02-06T17:47:08Z

Well, I have been doing pure functional programming for my other jobs... but this is not a pure functional language and codebase.

I get it, and I'm not really advocating for making everything injectable, it's just a reasoning for why I lean that way.

I don't think I am open to reconstructing the entire codebase just to get code that looks nicer.

Absolutely! I don't want to overhaul the code, I just want to settle on which approach to take for the flow of information to the handling of an event.

This is getting a bit messy (🙈), but I’ve read through all your comments and I don’t think I have any objections.

Agreed. Let's clean this up to be only about the global doubletap handling. I will make a minimally disruptive version of global double check, without the press event concept. Regardless of where any discussion of the event concept goes, it will do so elsewhere. This implementation is not good enough, so I will remove it.

I will also split the secondary role state thing to another PR where we can discuss the merits of it in isolation, and later, I will make a PR about macro initial state snapshot handling, I have some ideas in that regard.

mhantsch · 2026-02-06T19:51:05Z

macro initial state snapshot handling

Now you got me curious. Please keep me in the loop on this (when you are starting the PR).

firngrod · 2026-02-06T20:23:10Z

It's just that when the macro starts, it should gather the information it might need during it's lifetime, but which might not be available later, such as doubletap, keytap id and so on. It does so now, but not in a very formalized way. I also want it to remember secondary role resolution and pass all of these things on to macros it might call.

firngrod · 2026-02-06T21:38:06Z

It builds, haven't tested it, still needs code cleanup.

firngrod · 2026-02-06T21:39:32Z

Also some of your comments are unaddressed.

Also, macros remember secondary role resolution once it has one

…s error

right/src/usb_report_updater.c

firngrod · 2026-02-07T21:17:03Z

This now only has the global doubletap and a couple of, I think, objective improvements and/or fixes.

Since doubletap evaluation went on-site, I just made it use individual timeout values again.

By the way, you mentioned that you had a branch with a key event type implementation. I would love to take a look at that when it's ready.

kareltucek · 2026-02-08T10:33:52Z

By the way, you mentioned that you had a branch with a key event type implementation. I would love to take a look at that when it's ready.

Have I?

More context please?

I guess it is deprecated by this PR.

kareltucek · 2026-02-08T10:38:08Z

Since doubletap evaluation went on-site, I just made it use individual timeout values again.

I don't understand this part either.

What do you mean "went on-site"?

Which one is the relevant commit?

firngrod · 2026-02-08T12:34:01Z

By the way, you mentioned that you had a branch with a key event type implementation. I would love to take a look at that when it's ready.

so I am dropping my branch for another time

Maybe you meant dropping your idea of a press event implementation?

What do you mean "went on-site"?

I no longer call KeyHistory_WasLastDoubletap centrally and pass the result around, it is called where it's used. That's what I mean by on-site. It strikes me by the way that there is now a condition where secondary role evaluation in macros may be wrong about doubletap if there has been key action in the time from macro started until secondary role evaluation started. I am personally willing to live with that, but I guess that it's not impossible that someone might make an ifGesture followed by ifSecondary. sigh I'll see if I can do something about that. Maybe a key press info type might help.

firngrod · 2026-02-08T13:19:40Z

Okay, now that we're not in any way bound by my previous implementation of the key press event concept, if I was to do it again, what do you think it should look like?
I still believe that the proper place to evaluate such a structure is when a keypress is being irrevocably applied, in updateActionStates. It would have to make it's way down to macro engine, secondary role driver and layer switch logic at least, for the doubletap to be used there. Is there a better way to do it, you think, than what I did last time, wrapping it around the keyState?

kareltucek · 2026-02-08T15:04:08Z

This now only has the global doubletap and a couple of, I think, objective improvements and/or fixes.

Since doubletap evaluation went on-site, I just made it use individual timeout values again.

I think that unifying doubletap timeouts into a single one was a good thing.

I still believe that the proper place to evaluate such a structure is when a keypress is being irrevocably applied, in updateActionStates.

Totally agreed.

Is there a better way to do it, you think, than what I did last time, wrapping it around the keyState?

No, I think it is fine.

Okay, now that we're not in any way bound by my previous implementation of the key press event concept, if I was to do it again, what do you think it should look like?

I believe you have better idea what to do and how to do it.

If I am to sum up my understanding of what has been going on:

My original idea was there to be a new struct to store the info in the macro state. It would probably look something like this:

struct macro_press_info_t {
    keyId or keystate pointer
    uint32_t activationTime
    ... secondaryRoleState
    ... isDoubletap
    ... seqId
}

But that is basically just gathering stuff that is already lying around and putting it at one more centralized place.

The concept which you had in mind was about passing things inside report updater, which kind of sounds similar, but turns out to be a very different thing.

I think that if we keep key events, the key state should only contain information about the key state. Basically what it contains now, minus secondary role. Everything else should live in events.

Ideally, there should be no need to pass around key state pointers, key ID should be enough for the purpose of differentiating keys, nothing else.

Sounds fine, except that there is a number of states that don't have keyid assigned - the navigation mode states for instance. These would have to be identified and dealt with.

Everything that the logic needs in order to execute should be encapsulated in the event type: Is it a new press (ActivatedNow), new release (DeactivatedNow), is it doubletap, what was the ID of the press for macros to check back if that ID has been released, so on. Macro engine is the only place where any of this information needs to live past the event, so it can store what it needs to store. Realistically, nothing else needs the keyState really, does it? To be clear, I don't want to get rid of passing key state around (yet), but it should theoretically be straightforward to do.

If I understand correctly, you are suggesting to create a new structure similar to this and pass it around the native action processing (usb_report_updater):

struct key_press_info {
    keyId or keystate pointer; 
    eventType (activatedNow, active, deactivatedNow, nonActive);
    isDoubletap
    seqId
}

I would add:

    uint32_t activationTime; // (I prefer to pass it around like this instead of those occasional cryptic readings of CurrentPostponedTime.)

In any case, you seem to have a pretty good understanding of the keystate handling as well as an opinion on how it should be done.

As for whether to do it or how or not, you be the judge. I trust the sanity of your jugdment and would prefer to not have to get too engaged into this.

firngrod · 2026-02-08T20:37:03Z

I have re-deprecated secondary role specific doubletap timeout.

For the issue with macros doing secondary role evaluation after other key actions, I personally am fine with that for now for the following reasons:

People are rarely going to do a bunch of stuff and then do an ifPrimary/ifSecondary
In the case I referred to previously with ifGesture followed by ifPrimary, postpone should still be in effect from the ifGesture, right?
In other cases, even if we handle doubletap correctly, if the key order is already broken by macro delays, it's broken, even if we patch doubletap specifically. This is always going to be a fundamentally broken scenario
It's only the doubletap within secondary role resolution which may be off. It's a pretty specific case which is almost certainly not coming into play

I think this branch is now at a stage where it is ready for a final review and, hopefully, merge, without key press events.
I will look into a key event implementation in another PR where nothing else is on the line, both in terms of communication clarity and risk of loss if PR is rejected. 🙂

That might not be for at least a week as I will be visiting the in-laws for some days this coming week.

Thank you for your patience, and sorry for all the mess.

kareltucek · 2026-02-08T20:45:33Z

I think everything said is correct.

I think this branch is now at a stage where it is ready for a final review

Alright, will give it another look and probably merge soon!

Thank you for your patience, and sorry for all the mess.

No, no, no, thank you for all the "mess". 😊

kareltucek · 2026-02-09T11:28:51Z

right/src/macros/core.c

    S->ms.currentMacroStartTime = CurrentPostponedTime;
    S->ms.currentMacroArgumentOffset = argumentOffset;
    S->ms.parentMacroSlot = parentMacroSlot;
+    S->ms.isDoubletap = KeyHistory_WasLastDoubletap();


Some macros run without owning a key - those started from macroEvents or via usb, or those bound in navigation modes.

Iinm, this line can make them think they were a doubletap. It would be safer to ask for a "keystate" rather than "last key".

Of course, such macros shouldn't be asking for doubletaps in the first place, so this shouldn't be a problem in practice.

kareltucek · 2026-02-09T11:33:34Z

right/src/mouse_controller.c

        // some systems do debouncing because mouse switches are unreliable
-        uint16_t delay = 20;
-        PostponerCore_TrackKeyEvent(singleTap, false, 0xff, Timer_GetCurrentTime());
+        const int16_t delay = 20;


Why signed? Is this a typo?

kareltucek · 2026-02-09T11:43:58Z

right/src/key_states.h

        bool debouncing : 1;
        secondary_role_state_t secondaryState : 2;
+        bool padding : 1; // This allows the KEY_INACTIVE() macro to not trigger false because of sequence
+        uint8_t activationId: 3;


I would feel safer with 4 bits.

kareltucek · 2026-02-09T11:44:26Z

Let's merge this.

If you want to solve any of the nitpicks, please do so in a new PR.

Christian Dirksen added 5 commits February 3, 2026 21:49

Global doubletap detection: Just need layer switching now

702336b

Also switched detection of same press from a time-base to a key press count base

Global doubletap: Included the layer switch doubletap

e45535a

Global doubletap: Deprecated the secondary role specific doubletap ti…

17ce397

…meout

Global doubletap: Updated docs

6cf8005

Global doubletap: Better logic

9ec6850

cla-sign-bot bot added the cla:signed label Feb 4, 2026