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

Releasing `fn` before key enters 2 characters #150

Closed
obra opened this Issue Jul 28, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@obra
Member

obra commented Jul 28, 2017

@bergey writes:

On the PVT with default firmware, on Windows 10, releasing the fn key while still holding one of uiopprints both characters - {u, [o- and so forth. I find I roll my hand in a way that I do this frequently. Does anyone want this behavior

@obra

This comment has been minimized.

Show comment
Hide comment
@obra

obra Jul 28, 2017

Member

I think I agree that it's not desirable, but it does seem to "work like Shift does"

I feel like we can probably do better on this.

Member

obra commented Jul 28, 2017

I think I agree that it's not desirable, but it does seem to "work like Shift does"

I feel like we can probably do better on this.

@bergey

This comment has been minimized.

Show comment
Hide comment
@bergey

bergey Jul 29, 2017

It seems to me that when I release Shift while still holding a letter, the lower-case letter begins to repeat after a pause. I'd guess the same pause time as when I press & hold a letter without involving Shift. This is the same with the Model 01 & with other keyboards; and on Linux as well as Windows.

With the Fn pairs, I can't detect a pause, and haven't managed to release the letter fast enough after the Fn key that I don't get a second character. I think I'd be happy with either behavior:

  • After releasing Fn, don't send keycodes for currently held keys until they are released
  • After releasing Fn, pause briefly before sending keycodes

Thanks for moving this issue to the appropriate repo.

bergey commented Jul 29, 2017

It seems to me that when I release Shift while still holding a letter, the lower-case letter begins to repeat after a pause. I'd guess the same pause time as when I press & hold a letter without involving Shift. This is the same with the Model 01 & with other keyboards; and on Linux as well as Windows.

With the Fn pairs, I can't detect a pause, and haven't managed to release the letter fast enough after the Fn key that I don't get a second character. I think I'd be happy with either behavior:

  • After releasing Fn, don't send keycodes for currently held keys until they are released
  • After releasing Fn, pause briefly before sending keycodes

Thanks for moving this issue to the appropriate repo.

@algernon algernon added the bug label Jul 30, 2017

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 30, 2017

Member

I too agree, that the current behaviour is undesired, and a similar issues is plaguing TapDance (keyboardio/Kaleidoscope-TapDance#9) - or at least I feel it is similar. The easiest would be to mask out keys when releasing Fn, and make that a generic thing, so it can be used by other plugins.

Something like maskHeldKeys(), which would temporarily ignore all key events for keys that are held, and remove them from the mask when they are released. This can be done with a pair of uint32_t bitmasks. OneShot already implements something similar, I can pull that out into core.

I'll see if I can reproduce the problem, and if the suggested masking helps - if it does, I'll prep a PR.

Member

algernon commented Jul 30, 2017

I too agree, that the current behaviour is undesired, and a similar issues is plaguing TapDance (keyboardio/Kaleidoscope-TapDance#9) - or at least I feel it is similar. The easiest would be to mask out keys when releasing Fn, and make that a generic thing, so it can be used by other plugins.

Something like maskHeldKeys(), which would temporarily ignore all key events for keys that are held, and remove them from the mask when they are released. This can be done with a pair of uint32_t bitmasks. OneShot already implements something similar, I can pull that out into core.

I'll see if I can reproduce the problem, and if the suggested masking helps - if it does, I'll prep a PR.

algernon added a commit to keyboardio/Kaleidoscope-Hardware-Model01 that referenced this issue Jul 30, 2017

Add helpers to aid in implementing key masking
There are situations when one wants to ignore key events for a while, and mask
them out. These newly introduced functions help do that.

They are in the Hardware plugin, because this is where it is most efficient to
implement the masks: the hardware library knows how many bits it needs, and how
best to represent the masks. We use a 32-bit bitmap here, other keyboards may
use a different size, or an entirely different approach too.

This is one part of the fix to address keyboardio/Kaleidoscope#150.

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

algernon added a commit that referenced this issue Jul 30, 2017

Implement key masking for momentary layer keys
The goal is to ignore key events from still-held keys in a situation where we
just turned a layer off. Thus, if one holds a momentary layer key, then presses
and holds another key, releases the layer key, we want to ignore the other held
keys until they are released.

This is accomplished by masking all held keys when a momentary layer has been
turned off, and ignoring all masked key events in `handleKeyswitchEvent` until
they are released, when we unmask them.

This should address #150, but requires
keyboardio/Kaleidoscope-Hardware-Model01#9.

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

This comment has been minimized.

Show comment
Hide comment
@obra

obra Aug 4, 2017

Member

@bergey I've merged @algernon's two proposed patches to deal with this issue. I'd love to know if a fresh update and build resolves this issue.

Thanks!
Jesse

Member

obra commented Aug 4, 2017

@bergey I've merged @algernon's two proposed patches to deal with this issue. I'd love to know if a fresh update and build resolves this issue.

Thanks!
Jesse

@ToyKeeper

This comment has been minimized.

Show comment
Hide comment
@ToyKeeper

ToyKeeper Aug 5, 2017

Just leaving a +1. I was going to report this issue, but it seems I'm a week late. :)
I tend to end up with extra unwanted characters after releasing Fn, so it's nice to see that a fix is already in progress.

ToyKeeper commented Aug 5, 2017

Just leaving a +1. I was going to report this issue, but it seems I'm a week late. :)
I tend to end up with extra unwanted characters after releasing Fn, so it's nice to see that a fix is already in progress.

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Aug 5, 2017

Member

I just tested this again, while working on keyboardio/Kaleidoscope-OneShot#10, and the masking did not solve the issue by the looks of it. Fn held, then u held produces repeating {, but as soon as I release Fn, I get {u printed, then u repeated.

Member

algernon commented Aug 5, 2017

I just tested this again, while working on keyboardio/Kaleidoscope-OneShot#10, and the masking did not solve the issue by the looks of it. Fn held, then u held produces repeating {, but as soon as I release Fn, I get {u printed, then u repeated.

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Aug 6, 2017

Member

Right, the problem is that the masking wasn't applied properly, and that part is fixed in on Kaleidoscope-Hardware-Model01 master.

Member

algernon commented Aug 6, 2017

Right, the problem is that the masking wasn't applied properly, and that part is fixed in on Kaleidoscope-Hardware-Model01 master.

@ToyKeeper

This comment has been minimized.

Show comment
Hide comment
@ToyKeeper

ToyKeeper Aug 6, 2017

I don't suppose there's any chance of not changing the layer on Fn release? So, if Fn-u produces repeating {'s, and the user releases Fn but continues to hold u, it would continue to repeat { instead of u?

That's how my HHK works, at least. It seems like a good way to do it.

Other mods switch on release, but not Fn. So, "+Ctrl, +e, -Ctrl" produces repeating 'e'. Same with Shift, or Alt, or Hyper, or any mod handled by the computer's OS. But Fn is special since it's handled by the keyboard's firmware. If the Fn layer has an action defined for a key (instead of falling through), the underlying layer isn't activated on Fn release.

Basically, the active layer doesn't change until a key is released. The event sent to the host for a given key is set on keypress, and doesn't change until the key is released. This would also mean that, if the user presses left Fn for layer 1, presses 'u', then presses right Fn for layer 2, the key would continue to make '{' events even if layer 2 has a different mapping for it.

ToyKeeper commented Aug 6, 2017

I don't suppose there's any chance of not changing the layer on Fn release? So, if Fn-u produces repeating {'s, and the user releases Fn but continues to hold u, it would continue to repeat { instead of u?

That's how my HHK works, at least. It seems like a good way to do it.

Other mods switch on release, but not Fn. So, "+Ctrl, +e, -Ctrl" produces repeating 'e'. Same with Shift, or Alt, or Hyper, or any mod handled by the computer's OS. But Fn is special since it's handled by the keyboard's firmware. If the Fn layer has an action defined for a key (instead of falling through), the underlying layer isn't activated on Fn release.

Basically, the active layer doesn't change until a key is released. The event sent to the host for a given key is set on keypress, and doesn't change until the key is released. This would also mean that, if the user presses left Fn for layer 1, presses 'u', then presses right Fn for layer 2, the key would continue to make '{' events even if layer 2 has a different mapping for it.

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Aug 6, 2017

Member

Repeating { in the scenario you describe would not be trivial to implement, at least at a guess. I suppose we could provide a knob or a plugin to have that behaviour. Can you open an issue about it? :)

Member

algernon commented Aug 6, 2017

Repeating { in the scenario you describe would not be trivial to implement, at least at a guess. I suppose we could provide a knob or a plugin to have that behaviour. Can you open an issue about it? :)

@obra

This comment has been minimized.

Show comment
Hide comment
@obra

obra Aug 7, 2017

Member
Member

obra commented Aug 7, 2017

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Aug 7, 2017

Member

I'll see what I can do.

Member

algernon commented Aug 7, 2017

I'll see what I can do.

@ToyKeeper

This comment has been minimized.

Show comment
Hide comment
@ToyKeeper

ToyKeeper Aug 7, 2017

I'm running the code with Layer.repeat_first_press now, and it seems to be working great so far. I set it to true in my sketch and I haven't encountered any issues with it. It's nice being able to keep repeating something from another layer without holding the layer key, like arrow keys and mouse wheel actions.

ToyKeeper commented Aug 7, 2017

I'm running the code with Layer.repeat_first_press now, and it seems to be working great so far. I set it to true in my sketch and I haven't encountered any issues with it. It's nice being able to keep repeating something from another layer without holding the layer key, like arrow keys and mouse wheel actions.

@gedankenexperimenter

This comment has been minimized.

Show comment
Hide comment
@gedankenexperimenter

gedankenexperimenter Aug 9, 2017

Contributor

It looks like this issue should be closed, just like #158

Contributor

gedankenexperimenter commented Aug 9, 2017

It looks like this issue should be closed, just like #158

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Aug 9, 2017

Member

Closing this - if the issue still persists, or resurfaces, please reopen!

Thanks a lot for reporting it @bergey, it kickstarted a very useful discussion, which ended up with not only fixing the issue, but with a much faster scan cycle, and less surprising momentary layer behaviour too. \o/

Member

algernon commented Aug 9, 2017

Closing this - if the issue still persists, or resurfaces, please reopen!

Thanks a lot for reporting it @bergey, it kickstarted a very useful discussion, which ended up with not only fixing the issue, but with a much faster scan cycle, and less surprising momentary layer behaviour too. \o/

@algernon algernon closed this Aug 9, 2017

@bergey

This comment has been minimized.

Show comment
Hide comment
@bergey

bergey Aug 12, 2017

Sorry for the late response. I confirm that this bug was fixed, and the behavior of 491dca3 works well for me. Thanks for the discussion & code changes.

I am very amused that it's broken again, per #173 =)

bergey commented Aug 12, 2017

Sorry for the late response. I confirm that this bug was fixed, and the behavior of 491dca3 works well for me. Thanks for the discussion & code changes.

I am very amused that it's broken again, per #173 =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment