New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quickly tapping two different tap dance keys results in unexpected behavior #527

Closed
djl opened this Issue Jul 16, 2016 · 16 comments

Comments

Projects
None yet
4 participants
@djl

djl commented Jul 16, 2016

Tapping two different tap dance keys in quick succession results in the second key always being tapped twice. Here's an example:

enum {
    TD_BRL = 0,
    TD_BRR,
};

const qk_tap_dance_action_t tap_dance_actions[] = {
    [TD_BRL]  = ACTION_TAP_DANCE_DOUBLE(KC_LBRC, KC_LCBR),
    [TD_BRR]  = ACTION_TAP_DANCE_DOUBLE(KC_RBRC, KC_RCBR),
}

When hitting the above two keys I expect to see [], instead I only get }[*]. Is this intentional or a bug? If the former, is there a way to disable it?

[*] Actually shifted/modified keys don't work with tap dance functions. Modifiers seem to be ignored. But that's for another issue :)

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 16, 2016

Contributor

This sounds like a bug, the rapid tap of two tap-dance keys is something I tested only very little. Out of curiosity, which hardware is it happening on?

As for mods being ignored, that's a known issue, a limitation of register_code(), called when using ACTION_TAP_DANCE_DOUBLE... definitely worth another issue, there may be an easy way to fix it.

Contributor

algernon commented Jul 16, 2016

This sounds like a bug, the rapid tap of two tap-dance keys is something I tested only very little. Out of curiosity, which hardware is it happening on?

As for mods being ignored, that's a known issue, a limitation of register_code(), called when using ACTION_TAP_DANCE_DOUBLE... definitely worth another issue, there may be an easy way to fix it.

@djl

This comment has been minimized.

Show comment
Hide comment
@djl

djl Jul 16, 2016

This sounds like a bug, the rapid tap of two tap-dance keys is something I tested only very little. Out of curiosity, which hardware is it happening on?

Ergodox EZ. If I find some time I can test on the Infinity Ergodox now that support for it is merged.

djl commented Jul 16, 2016

This sounds like a bug, the rapid tap of two tap-dance keys is something I tested only very little. Out of curiosity, which hardware is it happening on?

Ergodox EZ. If I find some time I can test on the Infinity Ergodox now that support for it is merged.

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 16, 2016

Contributor

EZ is best, I can then try to reproduce it myself, too :)

Contributor

algernon commented Jul 16, 2016

EZ is best, I can then try to reproduce it myself, too :)

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 19, 2016

Contributor

I'm having trouble reproducing this. Do you have the two tap dance keys on the same hand, or different sides?

Contributor

algernon commented Jul 19, 2016

I'm having trouble reproducing this. Do you have the two tap dance keys on the same hand, or different sides?

@djl

This comment has been minimized.

Show comment
Hide comment
@djl

djl Jul 19, 2016

They are on separate hands.

Here's an example keymap where the problem exists (it's just the default layout with the two tap dance keys added).

djl commented Jul 19, 2016

They are on separate hands.

Here's an example keymap where the problem exists (it's just the default layout with the two tap dance keys added).

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 19, 2016

Contributor

Thanks, will look into it tomorrow!

Contributor

algernon commented Jul 19, 2016

Thanks, will look into it tomorrow!

@hia3

This comment has been minimized.

Show comment
Hide comment
@hia3

hia3 Jul 20, 2016

I've tried to use dancing keys too, but I didn't like the delay and lack of auto-repeat.
Here is the variant that sends all keys without delays and supports auto-repeat. But at cost of sending backspace, so it is not applicable everywhere (like for shortcuts for example). I think it would be nice to have something like this as an option in dancing keys.

hia3 commented Jul 20, 2016

I've tried to use dancing keys too, but I didn't like the delay and lack of auto-repeat.
Here is the variant that sends all keys without delays and supports auto-repeat. But at cost of sending backspace, so it is not applicable everywhere (like for shortcuts for example). I think it would be nice to have something like this as an option in dancing keys.

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 20, 2016

Contributor

I am working on a version that supports repeat, without backspace. Will be built on top of #516.

Contributor

algernon commented Jul 20, 2016

I am working on a version that supports repeat, without backspace. Will be built on top of #516.

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 27, 2016

Contributor

Reproduced. No clue yet why it happens, but I can reproduce it, even when they're on the same hand. And with the holding stuff, it produces even stranger results.

I suppose that there may be a race condition between a timer and the scanner, but I'm yet to dive into the issue. The underlying reason may be the same as in #563, but that's a gut feeling at the moment.

Contributor

algernon commented Jul 27, 2016

Reproduced. No clue yet why it happens, but I can reproduce it, even when they're on the same hand. And with the holding stuff, it produces even stranger results.

I suppose that there may be a race condition between a timer and the scanner, but I'm yet to dive into the issue. The underlying reason may be the same as in #563, but that's a gut feeling at the moment.

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 27, 2016

Contributor

Aha! I see the issue... the state appears to be stale at times, which leads to.. interesting behaviour.

Contributor

algernon commented Jul 27, 2016

Aha! I see the issue... the state appears to be stale at times, which leads to.. interesting behaviour.

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 28, 2016

Contributor

The problem is that we only keep state for one key, while there may be other things in flight. As in, one tap-key is finished, and is going up (but the keyup has not registered yet), while another registers as going down. When the first registers a keyup, we already messed with the state.

To counter this, I think we'll need some kind of buffer, to keep track of the in-flight tap keys. That makes things a bit more complicated, but once figured out, that should cure #563 too. I'll need a few days to let the problem sink in, and then to come up with a reasonable solution.

Contributor

algernon commented Jul 28, 2016

The problem is that we only keep state for one key, while there may be other things in flight. As in, one tap-key is finished, and is going up (but the keyup has not registered yet), while another registers as going down. When the first registers a keyup, we already messed with the state.

To counter this, I think we'll need some kind of buffer, to keep track of the in-flight tap keys. That makes things a bit more complicated, but once figured out, that should cure #563 too. I'll need a few days to let the problem sink in, and then to come up with a reasonable solution.

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 28, 2016

Contributor

Just had an epiphany: instead of having one global state, have a state for every tap dance key, where we can store the counter, timer, pressed and interrupted state. This requires slightly more memory, but I believe this will make things a LOT simpler, including the code. Keymaps using the tap-dance functionality may need updates, but I'm not sure about that yet.

I'll give this a go tomorrow or on Monday. (And apologies for the noise, but I have to note this down before I forget)

Contributor

algernon commented Jul 28, 2016

Just had an epiphany: instead of having one global state, have a state for every tap dance key, where we can store the counter, timer, pressed and interrupted state. This requires slightly more memory, but I believe this will make things a LOT simpler, including the code. Keymaps using the tap-dance functionality may need updates, but I'm not sure about that yet.

I'll give this a go tomorrow or on Monday. (And apologies for the noise, but I have to note this down before I forget)

@pvinis

This comment has been minimized.

Show comment
Hide comment
@pvinis

pvinis Jul 28, 2016

Contributor

of course. for the memory, what about a list, so you have dynamic allocation? but I guess if someone has less than maaaaaany tap dance keys, it should be fine

On Thu, Jul 28, 2016 at 22:28, Gergely Nagy notifications@github.com wrote:
Just had an epiphany: instead of having one global state, have a state for every tap dance key, where we can store the counter, timer, pressed and interrupted state. This requires slightly more memory, but I believe this will make things a LOT simpler, including the code. Keymaps using the tap-dance functionality may need updates, but I'm not sure about that yet.

I'll give this a go tomorrow or on Monday. (And apologies for the noise, but I have to note this down before I forget)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub [https://github.com//issues/527#issuecomment-236015260] , or mute the thread [https://github.com/notifications/unsubscribe-auth/AAGHiVP1oIqBIgL-Zg1kP12yo444w6zcks5qaRD5gaJpZM4JN-Vj] .

Contributor

pvinis commented Jul 28, 2016

of course. for the memory, what about a list, so you have dynamic allocation? but I guess if someone has less than maaaaaany tap dance keys, it should be fine

On Thu, Jul 28, 2016 at 22:28, Gergely Nagy notifications@github.com wrote:
Just had an epiphany: instead of having one global state, have a state for every tap dance key, where we can store the counter, timer, pressed and interrupted state. This requires slightly more memory, but I believe this will make things a LOT simpler, including the code. Keymaps using the tap-dance functionality may need updates, but I'm not sure about that yet.

I'll give this a go tomorrow or on Monday. (And apologies for the noise, but I have to note this down before I forget)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub [https://github.com//issues/527#issuecomment-236015260] , or mute the thread [https://github.com/notifications/unsubscribe-auth/AAGHiVP1oIqBIgL-Zg1kP12yo444w6zcks5qaRD5gaJpZM4JN-Vj] .

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 28, 2016

Contributor

I'd be surprised if we could dynamically alloc in the firmware, to be honest. But even if we could, that'd be much harder to manage, and we'd lose more space with the bookkeeping and management code, than just preallocating ~32 bytes / tap-dance key. And that 32 bytes is a very, very pessimistic prediction: We need a pressed state (1 bit), an interrupt state (1 bit), a counter (8 bits, or 16, but that'd be an overkill), a timer (16 bits). That's 26 bits, not even 4 bytes per key (on top of the keycode that we already have, anyway, but even with that, we are still under 32 bits). I think we can live with that. I'm pretty sure we'll save a key or two worth of code by simplification :)

Contributor

algernon commented Jul 28, 2016

I'd be surprised if we could dynamically alloc in the firmware, to be honest. But even if we could, that'd be much harder to manage, and we'd lose more space with the bookkeeping and management code, than just preallocating ~32 bytes / tap-dance key. And that 32 bytes is a very, very pessimistic prediction: We need a pressed state (1 bit), an interrupt state (1 bit), a counter (8 bits, or 16, but that'd be an overkill), a timer (16 bits). That's 26 bits, not even 4 bytes per key (on top of the keycode that we already have, anyway, but even with that, we are still under 32 bits). I think we can live with that. I'm pretty sure we'll save a key or two worth of code by simplification :)

@pvinis

This comment has been minimized.

Show comment
Hide comment
@pvinis

pvinis Jul 28, 2016

Contributor

sounds good then :) cant wait. let me know if you need something.

On Thu, Jul 28, 2016 at 23:02, Gergely Nagy notifications@github.com wrote:
I'd be surprised if we could dynamically alloc in the firmware, to be honest. But even if we could, that'd be much harder to manage, and we'd lose more space with the bookkeeping and management code, than just preallocating ~32 bytes / tap-dance key. And that 32 bytes is a very, very pessimistic prediction: We need a pressed state (1 bit), an interrupt state (1 bit), a counter (8 bits, or 16, but that'd be an overkill), a timer (16 bits). That's 26 bits, not even 4 bytes per key (on top of the keycode that we already have, anyway, but even with that, we are still under 32 bits). I think we can live with that. I'm pretty sure we'll save a key or two worth of code by simplification :)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub [https://github.com//issues/527#issuecomment-236024132] , or mute the thread [https://github.com/notifications/unsubscribe-auth/AAGHiVS50bG6grpewPiniX4MNYeAXiYoks5qaRjIgaJpZM4JN-Vj] .

Contributor

pvinis commented Jul 28, 2016

sounds good then :) cant wait. let me know if you need something.

On Thu, Jul 28, 2016 at 23:02, Gergely Nagy notifications@github.com wrote:
I'd be surprised if we could dynamically alloc in the firmware, to be honest. But even if we could, that'd be much harder to manage, and we'd lose more space with the bookkeeping and management code, than just preallocating ~32 bytes / tap-dance key. And that 32 bytes is a very, very pessimistic prediction: We need a pressed state (1 bit), an interrupt state (1 bit), a counter (8 bits, or 16, but that'd be an overkill), a timer (16 bits). That's 26 bits, not even 4 bytes per key (on top of the keycode that we already have, anyway, but even with that, we are still under 32 bits). I think we can live with that. I'm pretty sure we'll save a key or two worth of code by simplification :)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub [https://github.com//issues/527#issuecomment-236024132] , or mute the thread [https://github.com/notifications/unsubscribe-auth/AAGHiVS50bG6grpewPiniX4MNYeAXiYoks5qaRjIgaJpZM4JN-Vj] .

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Aug 1, 2016

Contributor

I had a go at implementing a new version of the tap-dance functionality last week, but it's not done yet, and my first attempt was a failure. I'm dumping my ideas here, that I came up with during the weekend, so I won't forget.

The basic idea is to have a state for each and every tap-dance key: the current 'qk_tap_dance_state_t` struct will do, as-is, so we can keep the interfaces for the callbacks. Everything else changes, though, and here's how I imagine it would work:

Whenever a key is pressed, we go into process_tap_dance. If it is not a tap-dance key, and last_td_key is 0, we don't do anything. If last_td_key is set, which means that there is a tap-dance sequence in-flight, we cancel that sequence by setting its pressed state to false, and calling the on_reset callback, and clearing last_td_key.

If it is a tap-dance key, and last_td_key is not set, then we start the sequence, by recording last_td_key, and incrementing the counter, and starting a timer for the key.

If last_td_key is set, and is the same, we increment the counter, and restart the timer.

If last_td_key is not the same, we cancel the previous sequence (see above), and start a new one.

We should only consider starting a new sequence on keydown. If we receive a keyup for a tap-dance key that is already cancelled, we ignore that.

Hmm... that wasn't entirely clear, I'm afraid. Anyway, the goal is to make a conscious distinction between a key going up or down, and handle that differently (the current code doesn't really do that), and when another key - tap-dance or not - gets pressed, handle that gracefully too, in case another tap-dance key is on the way up.

The problematic case will be held keys: hold a tap key, and press another key... I suppose we will need a way to figure out whether the sequence timed out, or was forcibly cancelled / interrupted.

Contributor

algernon commented Aug 1, 2016

I had a go at implementing a new version of the tap-dance functionality last week, but it's not done yet, and my first attempt was a failure. I'm dumping my ideas here, that I came up with during the weekend, so I won't forget.

The basic idea is to have a state for each and every tap-dance key: the current 'qk_tap_dance_state_t` struct will do, as-is, so we can keep the interfaces for the callbacks. Everything else changes, though, and here's how I imagine it would work:

Whenever a key is pressed, we go into process_tap_dance. If it is not a tap-dance key, and last_td_key is 0, we don't do anything. If last_td_key is set, which means that there is a tap-dance sequence in-flight, we cancel that sequence by setting its pressed state to false, and calling the on_reset callback, and clearing last_td_key.

If it is a tap-dance key, and last_td_key is not set, then we start the sequence, by recording last_td_key, and incrementing the counter, and starting a timer for the key.

If last_td_key is set, and is the same, we increment the counter, and restart the timer.

If last_td_key is not the same, we cancel the previous sequence (see above), and start a new one.

We should only consider starting a new sequence on keydown. If we receive a keyup for a tap-dance key that is already cancelled, we ignore that.

Hmm... that wasn't entirely clear, I'm afraid. Anyway, the goal is to make a conscious distinction between a key going up or down, and handle that differently (the current code doesn't really do that), and when another key - tap-dance or not - gets pressed, handle that gracefully too, in case another tap-dance key is on the way up.

The problematic case will be held keys: hold a tap key, and press another key... I suppose we will need a way to figure out whether the sequence timed out, or was forcibly cancelled / interrupted.

algernon added a commit to algernon/qmk_firmware that referenced this issue Aug 17, 2016

tap-dance: Major rework, to make it more reliable
This reworks how the tap-dance feature works: instead of one global
state, we have a state for each tap-dance key, so we can cancel them
when another tap-dance key is in flight. This fixes qmk#527.

Since we have a state for each key, we can avoid situation where a keyup
would mess with our global state. This fixes qmk#563.

And while here, we also make sure to fire events only once, and this
fixes qmk#574.

There is one breaking change, though: tap-dance debugging support was
removed, because dumping the whole state would increase the firmware
size too much. Any keymap that made use of this, will have to be
updated (but there's no such keymap in the repo).

Also, there's a nice trick used in this rework: we need to iterate
through tap_dance_actions in a few places, to check for timeouts, and so
on. For this, we'd need to know the size of the array. We can't discover
that at compile-time, because tap-dance gets compiled separately. We'd
like to avoid having to terminate the list with a sentinel value,
because that would require updates to all keymaps that use the feature.
So, we keep track of the highest tap-dance code seen so far, and iterate
until that index.

Signed-off-by: Gergely Nagy <algernon@madhouse-project.org>

algernon added a commit to algernon/qmk_firmware that referenced this issue Aug 17, 2016

tap-dance: Major rework, to make it more reliable
This reworks how the tap-dance feature works: instead of one global
state, we have a state for each tap-dance key, so we can cancel them
when another tap-dance key is in flight. This fixes qmk#527.

Since we have a state for each key, we can avoid situation where a keyup
would mess with our global state. This fixes qmk#563.

And while here, we also make sure to fire events only once, and this
fixes qmk#574.

There is one breaking change, though: tap-dance debugging support was
removed, because dumping the whole state would increase the firmware
size too much. Any keymap that made use of this, will have to be
updated (but there's no such keymap in the repo).

Also, there's a nice trick used in this rework: we need to iterate
through tap_dance_actions in a few places, to check for timeouts, and so
on. For this, we'd need to know the size of the array. We can't discover
that at compile-time, because tap-dance gets compiled separately. We'd
like to avoid having to terminate the list with a sentinel value,
because that would require updates to all keymaps that use the feature.
So, we keep track of the highest tap-dance code seen so far, and iterate
until that index.

Signed-off-by: Gergely Nagy <algernon@madhouse-project.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment