keyboardio / Model01-Firmware

The "standard" Keyboardio Model 01 Firmware sketch.
GNU General Public License v3.0
172 stars 302 forks source link

Curly braces don't unshift right away #23

Closed malsyned closed 6 years ago

malsyned commented 7 years ago

Steps to reproduce:

Expected result: Types "{}[]"

Actual result: Types {}{] or {}{}

malsyned commented 7 years ago

This also misbehaves if you try to type }\ too quickly: you get }| instead.

obra commented 7 years ago

Hi Dennis,

This sounds suspiciously like an issue we thought we'd resolved. What OS and version are you testing on?

Thanks!

On Thu, Oct 19, 2017 at 1:20 PM, Dennis Lambe Jr. notifications@github.com wrote:

This also misbehaves if you try to type }\ too quickly: you get }| instead.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Model01-Firmware/issues/23#issuecomment-338026264, or mute the thread https://github.com/notifications/unsubscribe-auth/AACxaOrWultt3SJAccp05DHJpC3-nthMks5st68LgaJpZM4P_zVv .

malsyned commented 7 years ago

Ubuntu 14.04, with the version of the firmware that came pre-installed on my Keyboardio. (S/N 010049 if that is relevant)

obra commented 7 years ago

@algernon @bergey - This is the -opposite- of the problem we had just after PVT, right?

algernon commented 7 years ago

I can reproduce this, and there is a perfectly clear explanation for why this happens. The good news is, this is not necessarily a bug (depends on how you look at it)!

I reproduced with trilling } and [, which sometimes resulted in }{. So I added some key logging, and it turns out that this happens when [ is pressed while } is still releasing, but has not released yet. This means that the Shift is still pressed, so that's why [ gets it too. It's like if you were typing very fast, and you released shift, but the key is travelling up, and you hit another key: shift will still be applied, even though your finger is not on the key anymore.

This is exactly what happens here.

algernon commented 7 years ago

Now, there may be ways to fix this. For example, we could release shift (and the keys it was chorded with), provided they were triggered by a single key (eg, LSHIFT(Key_A) would trigger this, but Shift + A would not), as soon as any other, non-shifted key is pressed.

This would allow { to repeat, pressing [ later would stop } from repeating, and [ would start. Trilling would also work.

The downside of this is that the code required for this may be a tad complicated, at least that's my gut feeling.

algernon commented 7 years ago

@obra What do you think about my idea above? Would this be a desirable feature? If so, I'll have a go at implementing it.

obra commented 7 years ago

I'm a little dubious about the possible complexity, but I'd be happy to review a proposed implementation.

-j

On Fri, Nov 24, 2017 at 5:19 AM, Gergely Nagy notifications@github.com wrote:

@obra https://github.com/obra What do you think about my idea above? Would this be a desirable feature? If so, I'll have a go at implementing it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Model01-Firmware/issues/23#issuecomment-346827059, or mute the thread https://github.com/notifications/unsubscribe-auth/AACxaCeiFuAAe9gnEtp1wmAQDGWztBryks5s5sJLgaJpZM4P_zVv .

malsyned commented 7 years ago

Do you even have to send the shift-on and shift-off scancodes at all? There is a shift bit attached to each character scancode, isn't there? Are there some OSes that won't respect that unless you've sent the shift-on scancode before it?

obra commented 7 years ago

There is not. Shift is a modifier against an entire key report.

On Tue, Nov 28, 2017 at 12:47 PM, Dennis Lambe Jr. <notifications@github.com

wrote:

Do you even have to send the shift-on and shift-off scancodes at all? There is a shift bit attached to each character scancode, isn't there? Are there some OSes that won't respect that unless you've sent the shift-on scancode before it?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Model01-Firmware/issues/23#issuecomment-347658513, or mute the thread https://github.com/notifications/unsubscribe-auth/AACxaDL4lRzoVvLiApYdMNUgpQ4JwSGzks5s7HF5gaJpZM4P_zVv .

gedankenexperimenter commented 6 years ago

Now, there may be ways to fix this. For example, we could release shift (and the keys it was chorded with), provided they were triggered by a single key (eg, LSHIFT(Key_A) would trigger this, but Shift + A would not), as soon as any other, non-shifted key is pressed.

This would allow { to repeat, pressing [ later would stop } from repeating, and [ would start. Trilling would also work.

The downside of this is that the code required for this may be a tad complicated, at least that's my gut feeling.

This is exactly what I have in mind for the "Unshifter" plugin that I have planned, but isn't done. The hard part is that it's impossible to tell for sure where the shift bit in the HID report came from, so I was planning on implementing a keycode mask for the report. It would work the same was as the keyswitch mask – unmasking automatically when the keycode(s) stop appearing in the report on the input side.

This would require (in my opinion) a redesign of the hooks, with a true pre-report hook that gets called whenever a HID keyboard report is about to be sent.