Skip to content

Commit

Permalink
Fix the layer-dependent modifiers handling
Browse files Browse the repository at this point in the history 
Closes qmk#181.
  • Loading branch information
@vifon
vifon committed Mar 5, 2016
1 parent 4e42500 commit c248088
Showing 1 changed file with 21 additions and 1 deletion.
22 changes: 21 additions & 1 deletion tmk_core/common/action.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,26 @@ void action_exec(keyevent_t event)
#endif
}

/*
* Make sure the action triggered when the key is released is the same
* one as the one triggered on press. It's important for the mod keys
* when the layer is switched after the down event but before the up
* event as they may get stuck otherwise.
*/
action_t store_or_get_action(bool pressed, keypos_t key)
{
#ifndef NO_ACTION_LAYER
static action_t pressed_actions[MATRIX_ROWS][MATRIX_COLS];

if (pressed) {
pressed_actions[key.row][key.col] = layer_switch_get_action(key);
}
return pressed_actions[key.row][key.col];
#else
return layer_switch_get_action(key);
#endif
}

void process_action(keyrecord_t *record)
{
keyevent_t event = record->event;
Expand All @@ -62,7 +82,7 @@ void process_action(keyrecord_t *record)

if (IS_NOEVENT(event)) { return; }

action_t action = layer_switch_get_action(event.key);
action_t action = store_or_get_action(event.pressed, event.key);
dprint("ACTION: "); debug_action(action);
#ifndef NO_ACTION_LAYER
dprint(" layer_state: "); layer_debug();
Expand Down

35 comments on commit c248088

@eltang
Copy link

@eltang eltang commented on c248088 Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the benefit-cost ratio of this change is high enough to warrant it. Here are some alternative solutions to your problems.

You can simply make any of your current modifiers that are not transparent across all your layers into weak ones so that they are automatically "released" whenever you switch layers by assigning a variant of the macro below to each of them.

case 0:
    if (record->event.pressed) {
        add_weak_mods(MOD_BIT(/*modifier*/));
        send_keyboard_report();
    }
    else {
        del_weak_mods(MOD_BIT(/*modifier*/));
        send_keyboard_report();
    }
break;

Also, you can solve your problem with changing your default layer by assigning variants of the macro below to your current DF(/*layer*/) keys.

case 0:
    if (record->event.pressed) {
       default_layer_set(/*layer*/);
    }
break;

@eltang
Copy link

@eltang eltang commented on c248088 Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just feel like your patch is a little overkill, especially in the amount of additional memory it consumes.

If you want to cut down on syntax, you can define this function in your keymap file.

void weak_modifier(keyrecord_t *record, uint8_t modifier) {
    if (record->event.pressed) {
        add_weak_mods(MOD_BIT(modifier));
        send_keyboard_report();
    }
    else {
        del_weak_mods(MOD_BIT(modifier));
        send_keyboard_report();
    }
}

Then, you can give each modifier a much shorter definition like this one.

case /*case*/:
    weak_modifier(record, modifier);
break;

@eltang
Copy link

@eltang eltang commented on c248088 Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, my solution only would have worked if each of your modifiers was only visible on one layer. If you want any modifiers to be on more than one layer, you want to define them like this.

case /*case*/:
    if (record->event.pressed) {
        switch(biton32(layer_state)) {
            case 0:
                register_code(/*code*/);
            break;
            case 1:
                register_code(/*code 2*/);
            break;
            case 2:
                register_code(/*code 3*/);
            break;
        }
    }
    else {
        unregister_code(/*code*/);
        unregister_code(/*code 2*/);
        unregister_code(/*code 3*/);
    }
break;

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can quickly become very unmaintainable it seems. And you have to actively think about this issue when creating the keymap. I see that it might be the solution if you only need one or two such keys but I'd still rather have my patch as an option (off by default).

@eltang
Copy link

@eltang eltang commented on c248088 Mar 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just feel like my fix would take up a lot less memory than yours would. I updated it with a switch statement; see if you like it better now.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still need to track each such situation and hand-craft your macros for each such key. A maintenance hell. I think both solutions have their merits depending on the use case. Since I have some unused RAM, I prefer mine but I understand your point.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an idea. Instead of storing the action code for each key when it is pressed, why not just store the highest layer that was active? That way, only five bits (to store the values 0–31) need to be allocated for each key rather than the previous 16.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was one of my ideas for future improvement. I would need to move (or copy but moving would be preferable) some logic from layer_switch_get_action so that I can retrieve the layer number easily.

Using exactly 5 bits would be tricky (aligning and such) and IMHO not worth it but a single byte is not that much worse.

I'll try to implement it in a few days.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a fairly simple way to ensure that not even three bits are wasted for each key. I might have time to implement it in a few weeks.

I imagine that biton32(default_layer_state | layer_state) will return the correct layer number.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a fairly simple way to ensure that not even three bits are wasted for each key. I might have time to implement it in a few weeks.

What is it? Bit fields with __attribute__((packed)) or something like that? Now I'm curious.

I imagine that biton32(default_layer_state | layer_state) will return the correct layer number.

That was my first thought too but it does not take KC_TRNS into account.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, it's an array: uint8_t array[5][(MATRIX_ROWS * MATRIX_COLS) / 8]. Imagine a keyboard has eight columns and one row. The array would be uint8_t array[5][1]. Say that a key is pressed and we need to store the layer from which the action code was read. (key.row * key.col) / 8 gives the correct row of the array to enter, [0]. Then, we need to load each bit of the layer number into separate bytes in that row. In each one, we need to grab the corresponding bit of the layer number, shift it left (key.row * key.col) % 8, and load it in. Does that make sense to you? If not, I can write the code that stores and retrieves the layer numbers.

I realize that too. Perhaps we need to make layer_switch_get_action return it's loop control variable in addition to the action code? Yeah, it might be easier just to move everything in this function to action.c so that all the data that it uses is more accessible to the other code in that file.

Edit: Nobody try to make code based on this. I found some flaws in the logic that I will fix.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it should work (as long as the number of keys is divisible by 8) though I'm beginning to get worried about the performance. Though it probably won't have any noticeable impact anyway.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter whether the number of keys is divisible by eight. If it's not, there will just be some empty space at the left of each byte of the bottommost row of the array.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we need to add 1 for that to work like that? It's a minor problem either way.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

If you really want to boost the performance of the firmware, there are a ton of ways in which it could be optimized. For example, I believe most of the code in keymap_common.c is just adding an extra layer of abstraction for no reason as the code in action.c has the ability to interpret action codes directly.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. I'm probably worrying for no reason.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to optimize keymap_common.c when I have time.

What were you saying about adding one?

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the division by 8, so that we round up and not down.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are referring to the declaration of the array, correct? It seems that [(MATRIX_ROWS * MATRIX_COLS) / 8] does need to be changed to [((MATRIX_ROWS * MATRIX_COLS) / 8) | 1] in order to accommodate keyboards with less than 8 keys.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now I'm no longer sure what I referred to. Too many potential off-by-ones and not enough sleep. ;)

@eltang
Copy link

@eltang eltang commented on c248088 Mar 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it would be good to drop support for layers 16–31 entirely from QMK. In addition to making this feature much easier to implement, it would also allow many rarely-used action codes to be repurposed. I am developing a lot of features, but there just aren't enough action codes to go around. Alternatively, we might just expand the action codes to 32 bits.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also deprecate KC_TRNS. There won't be much use for it if this PR gets merged.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about halving the layer count. That's not much if you consider some special purpose layers (for example I have some layers for macro recording). But maybe it would be enough in practice, hard to tell...

We could also deprecate KC_TRNS. There won't be much use for it if this PR gets merged.

It would be still useful for layers modifying only the selected keys, regardless of the layer below. Additionally, I still consider this change optional.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't thought of those things. I think storing the value of the loop control variable in layer_switch_get_action when the function terminates is the way to go.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read your discussion with @ezuk. When do you want to do the optimization? I think that the memory usage can be cut down enough that this feature can be enabled by default.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just pushed the first half. Now it's 1 byte per key.

I don't think enabling it by default would be a good idea either way. It does change the layer semantics and it may interfere with someone's setup. I know I needed to adjust one or two things in mine.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, enabling this feature would allow the rest of the code to be greatly simplified. For example, clear_keyboard_but_mods wouldn't have to be called every time the layer changes. Also, I have some features that will depend on this feature.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to do your magic with 5 bits?

@eltang
Copy link

@eltang eltang commented on c248088 Mar 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I'll send you a PR in a few days.

@ezuk
Copy link

@ezuk ezuk commented on c248088 Mar 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @vifon - backwards compatibility is crucial, let's keep this an optional feature.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vifon @ezuk Exactly what functionality is lost when this feature is enabled?

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no functionality lost here. The functionality is changed and it will surely break some more complex layouts in a confusing way if it's made a default. Enabling it in some major update would be a good idea though.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I thought. It won't break most common keymaps, correct? I'm quite confident that the programmers who have constructed more complex keymaps will eventually be able to figure things out.

@vifon
Copy link
Owner Author

@vifon vifon commented on c248088 Mar 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't break most common keymaps, correct?

For some definitions of "most" and "common".

Surely they will be able to figure things out but I'd hate to debug my config only because I merged to upstream.

@eltang
Copy link

@eltang eltang commented on c248088 Mar 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some proper documentation should keep frustration to a minimum. I just think that now that this feature can be made to use only ~5 bits per key, its benefit-cost ratio is a little too high for it not to be enabled by default.

Please sign in to comment.