Firngrod/key chords by firngrod · Pull Request #1624 · UltimateHackingKeyboard/firmware

firngrod · 2026-09-13T19:15:47Z

Added key chords as a macro accessible feature.

I have left a self-review with some additional context which may not be readily available from the code, along with some questions about implementation strategy preferences.

There are also a lot of openers for discussions in the code comments, so don't skip those, by the way.

Allows effect to last until the last key Lacks support for macro checks for key release Also removed an unused parameter

…plication

firngrod

Conversation starters

firngrod · 2026-09-13T19:21:11Z

+  - `set chord[.LAYERID_BASIC] ACTION` Configures a chord with an action.  If a layer is specified, the chord will be available when that layer is active, if not, it will be available on any layer.  The same chord can be defined for different actions for different layers, including the `any` layer.  If action is `none`, the chord is removed and the keys will work as individual keys if the chord is pressed, not as an empty action.
+  - `set chordTimeout <ms, 0-255 (INT)>` sets the interval within which all keys of a chord must be pressed in order for the chord to be activated.
+  - `set chordPriorIdleTime <ms, 0-65535 (INT)>` sets a limit to how soon a chord can be activated after a key has been pressed.  This helps prevent accidental activations while typing rapidly.
+  - `set chordLifetime { leadingKey | allKeys }` sets how long a chord activation lasts.  With `leadingKey`, the chord is considered released when earliest key pressed in the activation is released, with `allKeys`, the chord is considered released when all keys in the chord is released.


The main differentiator is that with allKeys, the release is deterministically when all keys are released, not while with leadingKey, it can realistically be any of the keys. This can allow for, for example, overloading of a mod key.

firngrod · 2026-09-13T19:22:28Z

+        bool ErrorBefore = Macros_ParserError;
+        Macros_ParserError = false;
        uint8_t num = Macros_ConsumeInt(ctx);
-        if (Macros_ParserError) {
+        bool ErrorAfter = Macros_ParserError;
+        Macros_ParserError |= ErrorBefore;
+        if (ErrorAfter) {


Pretty sure I did this because I was getting misleading error messages.

firngrod · 2026-09-13T19:23:08Z

-macro_history_t MacroHistory[MACRO_HISTORY_POOL_SIZE];
-uint8_t MacroHistoryPosition = 0;
-


Dead code since doubletap was moved

firngrod · 2026-09-13T19:49:45Z

+    return lastPress.multiTapCount > 1;
+}
+
+uint8_t KeyHistory_GetChordActivationIdOfLastAction()


The implementation here fetches activationId from the chord rather than having a cached value. This would of course have to change if we ever want to have an actual histrory.

firngrod · 2026-09-13T19:50:36Z

 {
    for (int i = 0; i < count; i++) {
-        consumeOneKeypress(suppress);
+        consumeOneKeypress();


It was not even in the function definition any more.

firngrod · 2026-09-13T19:53:11Z

        // If the cached action is the current base role, then hold, otherwise keymap was changed. In that case do nothing just
        // as a well behaved hold action should.
-        if(KeyState_Active(keyState) && action->type == actionBase->type && action->keystroke.secondaryRole == actionBase->keystroke.secondaryRole) {
+        if(KeyState_Active(keyState)) {


The uncached action was only used for this purpose: To disable layer hold for secondary keys on keymap switch if the key was not mapped to the same layer on the new map. I cannot see any other reasonable case for this code. There is the unreasonable case of remapping the key from a macro while it's held.
I decided to remove that to get rid of passing the base action here. Deciding on a base layer action was tricky in the cases where the held key was a chord. Also, pure layer holds, (not secondary keys) are not subject to the same disabling on map change.

firngrod · 2026-09-13T19:55:40Z

        DEBUG_KEY_LIFE(applied);
        KEY_TIMING(KeyTiming_RecordKeystroke(keyState, active, Timer_GetCurrentTime(), Timer_GetCurrentTime()));
        keyState->current = active;
+        EventVector_Set(EventVector_NativeActionsPostponing);


As we discussed in the issue thread, only one key can now get activated per cycle, with the rest being pushed on the queue before actual key evaluation, in order to eliminate cases where one key evaluating for chords or secondary roles would not see when another was pressed within the same cycle.

kareltucek

Just a couple comments from a first, very shallow pass.

It will take a few more days till I get back for a deeper pass.

kareltucek · 2026-09-16T16:07:27Z

 COMMAND = set simulateLowResScrolling BOOL
 COMMAND = set i2cBaudRate <baud rate, default 100000(INT)>
 COMMAND = set diagonalSpeedCompensation BOOL
 COMMAND = set chordingDelay <time in ms (INT)>


(Someone should probably mention somewhere that this is an independent concept.)

Chords have not been released. It's not too late to name them something else, though chord is the most natural name. Unfortunate that key cluster kind of rules out that name for chords.

Maybe a combo?

kareltucek · 2026-09-17T10:15:03Z

+uint8_t NextActivationId = CHORDS_INVALID_ACTIVATION_ID + 1;
+
+// Not sure how nice or not this is.  I like named parameters rather than just true/false in calls.
+// I also don't like the all-caps of macros, this C doesn't have consexpr, and I don't know if global consts will get optimized.


Interesting. I bit cryptic. I kind of like it as long as it doesn't pollute global namespace.

I just like that it's extremely obvious what means pressed and what means released.

kareltucek · 2026-09-17T12:03:25Z

                if (KeyState_DeactivatedNow(keyState)) {
+                    // I kind of want to only trigger chord releases for chord-holding keys.
+                    // It's safe as it is, but it is not exactly free if there are chords.
+                    // Maybe grab a bit in the keyState for isHoldingChord?


I am in favor for grabbing a bit in the keyState for isHoldingChord.

Will do that then. That will make release key handling a bit cheaper if chords are enabled with maintain until all keys released.

kareltucek · 2026-09-17T15:56:51Z

+        return noneVar();
+    }
+
+    if (!Chords_TryAddChord(layerId, keys, keyCount, &chordAction)) {


This is fine.

I would probably formulate the error so that it is not a strict statement - like ending with a question mark instead of a period, to at least give myself a hint to check the path in case it behaves weird.

kareltucek · 2026-09-17T15:59:21Z

+                    // I kind of want to only trigger chord releases for chord-holding keys.
+                    // It's safe as it is, but it is not exactly free if there are chords.
+                    // Maybe grab a bit in the keyState for isHoldingChord?
+                    // I could also get rid of the need to do this by storing activationIds for the holding keys, costing memory per chord.


I don't understand the storing activationIds idea, but I suspect it would take time for lookups at other places.

If active chords record the key activation id for each key which "holds it", release checks for the chord can just compare key activation ids, as per normal key release procedure. That would remove the need for tracking releases like this, but that would cost 5 bytes per chord slot. I like isHoldingChord, unless I find out that it's bad somehow.

kareltucek · 2026-09-17T16:01:04Z

    EventVector_Unset(EventVector_StateMatrix);
    EventVector_Unset(EventVector_NativeActionsPostponing);

    for (uint8_t slotId=0; slotId<SLOT_COUNT; slotId++) {


From what I remember, this loop is quite performance critical.

For that reason, I had been keeping it one loop without separating the preprocessing into a separate one.

Well, we need a solution for when 2 keys are pressed within the same cycle:
a and b are a chord and pressed in one cycle. a has lowest id and runs first, starts chord resolution but does not see b as it hasn't been dealt with yet, starts postponing, waiting for more keys. b is then postponed, but nothing causes a to be handled again until timeout.
Another option could be to always schedule a run the next cycle if postponing has been enabled and a key postponed because of it within the same cycle, though that could get tricky with where secondary keys set postponing. Alternatively, always trigger a cycle if a key is put in postponer?

Just always scheduling another cycle whenever a key is put on postponer might be the best/cleanest idea? It may cost a bit of processing, but only while there's downtime anyway.

kareltucek · 2026-09-17T16:04:31Z

-                applyLayerHolds(keyState, actionBase);
+                // apply base-layer holds, but only if the key is not currenty applying another action
+                // Also, don't apply layers from keys currently undergoing chord evaluation
+                if (chordRes != ChordResolution_Wait && cachedAction->action.type == KeyActionType_None) {


This changes layer hold semantics. See right/src/layer_switcher.c scenario comments.

This is getting a bit convoluted, as I had probably introduced some artifacts too over time.

I should probably back off, write some tests and see where all the discrepancies that I am seeing come from.

Only for keys which have an action associated to them on layers in which they are pressed.
This code change targets the case where key a was pressed on layer mod with action keypress leftArrow, but a has a layer switch to fn on base layer, so when mod is released, a now holds leftArrow and toggles fn. It' an edge case.
I get the need for empty actions to work like that, so key release/press order is not strictly enforced on layer dancing, but I feel that if the key is bound on the first layer, the user has already forefeited clean layer dancing.
I will change it to only skip the layer hold if chord is awaiting resolution if you want a more conservative approach.

What I'm not too fond of is the way layer switches are handled so many places. Would it be possible to recheck pressed keys when switching layer to base and fake an activation for the first pressed with a cached none-action and base layer layer switch action, if any? Then this case of layer handling could be removed, and the one in ApplyKeyAction could be the main one? It's also complicated, in a different way. Also, secondary role layer switches are still different.

firngrod · 2026-09-20T19:09:22Z

+                // Do not activate on the first key, but rather on the last.
+                // This is to prevent the following issues:
+                //  - activateKeyPostponed with prepend, or consumePending modifying the roll-out
+                //  - the rest of the chord waiting on the queue causing problems with ifSecondary in the macro we run
+                // We are still hypothetically vulnerable to other macros using those commands, but that's highly hypothetical as they would
+                // likely be used while postponeKeys is active, meaning we would not be resolving anyway.


I could probably treat the keystrokes the same as the macros and activate them on the last key, but I wasn't entirely sure if it would cause issues to have the first keys hold the action without an ActivatedNow round and then having the last key do an AcitvatedNow. Probably it's fine, didn't take the chance. If it is fine, this logic can be simplified.

firngrod · 2026-09-23T19:25:29Z

I'm still fretting over the complexity of allKeys. I don't like the price for what it provides.
I just thought of another possibility: Use the current allKeys functionality for simple keystrokes, but for macros, make the first key provided in the definition the holding key, always. Then the macro code does not have to know anything about chords, release checks in macros become a single key again, all of the release tracking for chords goes away, but the user can still deterministically know which key keeps the chord/macro alive.
The downside: Behavior is different for keystrokes and macros. A chord mapped to keystroke mouseBtnLeft does not behave exactly like a chord mapped to macro leftClick where leftClick is just holdKey mouseBtnLeft. This could be mitigated through documentation, and macros can stil do ifNotActive a ifNotActive b exit if they really want to maintain for all keys, though that brings back the issues solved through key_state_t.activationId

firngrod · 2026-09-26T18:58:49Z

Something else that hit me - How are we feeling about the key non-carrying chord keys being registered as inactive in macros if they were pressed as part of a chord? For the leadingKey, that's the case (uses PostponerExtended_ConsumePendingKeypresses), and in the case where the user defines the carrying key, it would also be the case for some of the keys. I can solve it, but it's going to make the chord function a bit funky if we also want to ensure that the queue is clean once any macro assigned to the chord runs.

Christian Dirksen added 20 commits July 25, 2026 21:13

Chords: Can now store and fetch chords

71d9932

Chords: It works, but it's not pretty yet

687fdb9

Key Chords: Support for the Any layer

1c30164

Key Chords: Support for chord trigger on hold and on release

e0aebc2

Key Chords: Remove the logic to trigger on hold and release

73abc04

Key Chords: Key history now supports chords

251c7de

Key Chords: Added support for the allKey application type

a79733f

Allows effect to last until the last key Lacks support for macro checks for key release Also removed an unused parameter

Key Chords: Cleanup

b004d90

Key Chords: Support release detection in macros when using allKeys ap…

7c3c6c0

…plication

Key Chords: Minimum idle time since last pressed implemented

d2d0647

Key Chords: Fix after rebase to master

e51201e

Key Chords: Reference manual

65bc11d

Key Chords: Self review

15b461a

Merge remote-tracking branch 'upstream/master' into firngrod/key_chords

7067874

Macro history was unused now

746f297

Merge remote-tracking branch 'upstream/master' into firngrod/key_chords

5e8c153

Merge remote-tracking branch 'upstream/master' into firngrod/key_chords

fba69f2

Merge remote-tracking branch 'upstream/master' into firngrod/key_chords

f37e66c

Unneeded include

4eb6609

Merge remote-tracking branch 'upstream/master' into firngrod/key_chords

ba27397

cla-sign-bot Bot added the cla:signed label Sep 13, 2026

firngrod commented Sep 13, 2026

View reviewed changes

kareltucek reviewed Sep 18, 2026

View reviewed changes

firngrod commented Sep 20, 2026

View reviewed changes

Key Chords: More documentation

d8cc0de

		macro_history_t MacroHistory[MACRO_HISTORY_POOL_SIZE];
		uint8_t MacroHistoryPosition = 0;

Conversation

firngrod commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

firngrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kareltucek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firngrod Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firngrod Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firngrod commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

firngrod commented Sep 26, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

firngrod commented Sep 13, 2026 •

edited

Loading

firngrod Sep 18, 2026 •

edited

Loading

firngrod Sep 18, 2026 •

edited

Loading

firngrod commented Sep 23, 2026 •

edited

Loading