10/19/11 Update: Added patches for the recently released GB sources to keyboard_patches-GB.tar.gz.
7/9/11 Update: New patch to export the "column-switch delay", which allows for much decreased CPU utilization/much better efficiency. See below for details.
3/23/11 Update: Add alternate "timer delay" patch to enable runtime configuration of keypad_timer delay. See below for details.
Attached is a kernel source patch that fixes the EB13 hardware keyboard dropped/skipped key press problem, at least for twice-pressed keys. It restores DI18-level responsiveness for the hardware keyboard.
I know dropped keypresses on the hardware keyboard has been a problem for a long time, even with DI18. There are likely multiple factors at play here, and this patch only corrects one of them. However, DK28/EB13 are particularly bad at dropping twice-pressed keys, and hopefully this patch alleviates this issue for most folks who experience it.
Bug details:
Sesquipedalian points out in one of the General threads concerning the issue that EB13 often drops twice-pressed letters on the hardware keyboard. For example, if one types "hello", often "helo" is emitted.
Yesterday I performed some timing tests and discovered that the EB13 keyboard driver delays 1/10th of a second (100 ms) between reporting "key_press" & "key_release" events. Unfortunately this delay is on the order of human muscle response, and when folks press the same letter twice in that 100 ms, the kernel will only be able to acknowledge one of the presses.
Keyboard driver background:
The Epic's keyboard driver is located at drivers/input/keyboard/victory/s3c-keypad.c in the EB13 kernel sources.
On initialization, the driver establishes an interrupt handler (s3c_keypad_isr) for keyboard ("keypad") events, which is executed whenever the kernel is interrupted with a key press. During an interrupt, the handler starts a timer (keypad_timer) to expire after "3 * HZ/100" jiffies (27 ms), and clears the interrupt request. The driver actually figures out which key was pressed/released in a separate routine that executes after the timer expires. Also, the interrupt is not immediately reenabled, so if a second keyboard event (either a key press or release) comes in before the timer expires, nothing additional happens.
When keypad_timer expires, the keypad_timer_handler function is invoked. This routine scans through each of the six "columns" (which roughly corresponds to the rows) of the keyboard looking for key presses & releases. In doing so, the driver delays 0.3 ms to ensure each column switch is accepted by the hardware, so the enitre routine should take rougly 1.8 ms to execute. This is actually quite a long time(but not inappropriately so) relative to the clockspeed of the processor, which is partly why the scanning is done outside the keyboard interrupt where operations "that take a while" are discouraged. So this isn't a bad setup, even if it might appear a bit unusual at first.
The end of the scanning routine (keypad_timer_handler) checks to see if any keys are currently held down, and if so it restarts keypad_timer.It must do this, becuase if a key is pressed & released "quickly", before the timer actually expires, then a release interrupt would never be generated (even if the interrupt was immediately reenabled). Hence, if a key is pressed & released "quickly", it is detected by the scanning routine after the second timer expiration. It's likely that the keybord only generates an interrupt on key press, so the timer is necessary to detect key releases. While a key is held down, the scanning routine keeps resetting the timer. Once a key release is detected, the timer is deactivated and the interrupt reenabled.
The bug:
Now, the problem is that keypad_timer, when restarted by the scanning routine, is set to expire after "HZ/10" jiffies (98 ms). This is an unusually long waiting time--four times as long as the original timer. If the same key is pressed twice in this interval, the scanning routine will pick it up only as a single press/release pair. If multiple, different keys are pressed in this interval, I believe the scanning routine will pick all of them up, but not necessarilly in the right order.
So the fix is to change the keypad_timer's restart expiration time to something more reasonable, like the 27 ms used by the interrupt handler. This decreases the waiting time to one in which it's unlikely that someone will be able to press multiple, or repeatedly press a key. As it turns out, that's exactly what DI18 does, so we know it works. It's totally unclear why Samsung changed it.
Oh well.
Edit: Originally I thought keys were latched, so press events would be detected by the scanning routine even if the key is released before keypad_timer expires. They're most certainly not though, the scanning routine likely polls the instantenous key (up or down) state, so a key press must be held for at least 27 ms so that the scanning may complete after the interrupt is serviced. If a key is held shorter than this, it will not be detected.
3/23/11: Alternate "timer delay" patch:
Also attached is an alternate patch that supercedes the original. It allows for runtime configuration of the keypad_timer delay via a sysfs exported timer_delay variable.
The current delay may be read with:
and a new delay may be set with:
where N is the delay in jiffies.
A delay of "1" yields the shortest timer, and the most responsive keypad possible, whereas a delay of "256" (1 second) (in most kernels) is the longest allowed. At that point the keyboard is already useless since any keypress results in either repeated characters or the "alternate character popup".
In most kernels (where HZ=256), the default delay is "7" (27 ms), which is the same as the original patch and DI18. A delay of "16" (63 ms) approximates the behavior of stock EB13/EC05.
Basically, the delay is the minimum amount of time one must hold a key down for a keypress to register. It's also the maxmimum time the kernel believes a key is "held down" after it's let go. For example, with a delay of "7" one must hold a key down for 27 ms to register as pressed, and no-more-than 55 ms later does the key register as being released.
Or for a better example, with a delay of "128" one must hold a key down for half a second, and once registered, the key is "held down" for an additional half a second. Which almost guarantees an application-level repeated keypress or the alternate character popup.
Ideally one would start with a delay of "7" and reduce by one until the timer is fast enough for your typing rate that you no longer have dropped keypresses. Although many folks will find that a delay of "7" is fast enough for them.
7/9/11: New "column-switch delay" patch:
Attached are two (mutually exclusive) patches to export the "column-switch delay" (column_delay) in addition to the timer_delay variable. The first applies against stock EC05 and supercedes all previous patches. The second (alt patch) applies against stock EC05 with the previous timer_delay patch already applied and should be used for custom kernels already containing that patch.
The column-switch delay is the amount of time, in µs, that the driver delays while switching between the six keyboard "columns" (actually rows) in the keyboard scaning routine. A delay is necessary to ensure that the column switch has been performed by the hardware before scanning a new column, otherwise ghost keypresses result.
However, the 300 µs delay specified by Samsung is very inefficient, particularly for "reasonable" timer_delay values. With theimpaler747's suggested timer_delay of 5, the keyboard driver, while holding a key down, consumes 9.7% of CPU time (empirically tested), at any clock rate. This is quite-likely responsible for folks' previous complaints that use of the hardware keyboard causes audio skips, poor game performance, decreased battery life, etc.
This patch allows this (default) 300 µs delay value to be modified through a sysfs exported column_delay variable. Similar to the timer_delay, it may be read with:
and a new delay may be set with:
where N is the delay in µs.
A delay of "1" is the most efficient (only 0.02% CPU utilization) but results in ghost key presses. In testing, I've found ghosting to disappear at delays of "3" and above. To provide some buffer, I've been using (and recommend) a delay of "5" for both column_delay and timer_delay values for the past week, and I've not run into any issues.
At a delay of "5", the keyboard driver is far more efficient, consuming only 0.2% of CPU time while keys are held. This hopefully translates into less stuttering/performance problems and better battery life.
Testing on other devices is needed (and appreciated!) to find a reasonable delay where ghosting doesn't appear, assuming that "5" is too low on some devices. Even delays of "10" and "50" (comparable to other keyboard drivers) result in ~0.3% and 1.7% CPU utilization respectively, which is much better than the Samsung default.
Mirror links:
GB kernel patches: keyboard_patches-GB.tar.gz
Column-switch & timer delay patch: epic_keypad_delays-EC05.diff (for stock EC05)
Column-switch & timer delay patch: epic_keypad_delays_alt-EC05.diff (for kernels with timer delay patch already applied)
Historical links:
Timer delay patch: epic_keypad_timer_delay-EB13.diff
Original patch: epic_keypad_timer_fix-EB13.diff
7/9/11 Update: New patch to export the "column-switch delay", which allows for much decreased CPU utilization/much better efficiency. See below for details.
3/23/11 Update: Add alternate "timer delay" patch to enable runtime configuration of keypad_timer delay. See below for details.
Attached is a kernel source patch that fixes the EB13 hardware keyboard dropped/skipped key press problem, at least for twice-pressed keys. It restores DI18-level responsiveness for the hardware keyboard.
I know dropped keypresses on the hardware keyboard has been a problem for a long time, even with DI18. There are likely multiple factors at play here, and this patch only corrects one of them. However, DK28/EB13 are particularly bad at dropping twice-pressed keys, and hopefully this patch alleviates this issue for most folks who experience it.
Bug details:
Sesquipedalian points out in one of the General threads concerning the issue that EB13 often drops twice-pressed letters on the hardware keyboard. For example, if one types "hello", often "helo" is emitted.
Yesterday I performed some timing tests and discovered that the EB13 keyboard driver delays 1/10th of a second (100 ms) between reporting "key_press" & "key_release" events. Unfortunately this delay is on the order of human muscle response, and when folks press the same letter twice in that 100 ms, the kernel will only be able to acknowledge one of the presses.
Keyboard driver background:
The Epic's keyboard driver is located at drivers/input/keyboard/victory/s3c-keypad.c in the EB13 kernel sources.
On initialization, the driver establishes an interrupt handler (s3c_keypad_isr) for keyboard ("keypad") events, which is executed whenever the kernel is interrupted with a key press. During an interrupt, the handler starts a timer (keypad_timer) to expire after "3 * HZ/100" jiffies (27 ms), and clears the interrupt request. The driver actually figures out which key was pressed/released in a separate routine that executes after the timer expires. Also, the interrupt is not immediately reenabled, so if a second keyboard event (either a key press or release) comes in before the timer expires, nothing additional happens.
When keypad_timer expires, the keypad_timer_handler function is invoked. This routine scans through each of the six "columns" (which roughly corresponds to the rows) of the keyboard looking for key presses & releases. In doing so, the driver delays 0.3 ms to ensure each column switch is accepted by the hardware, so the enitre routine should take rougly 1.8 ms to execute. This is actually quite a long time
The end of the scanning routine (keypad_timer_handler) checks to see if any keys are currently held down, and if so it restarts keypad_timer.
The bug:
Now, the problem is that keypad_timer, when restarted by the scanning routine, is set to expire after "HZ/10" jiffies (98 ms). This is an unusually long waiting time--four times as long as the original timer. If the same key is pressed twice in this interval, the scanning routine will pick it up only as a single press/release pair. If multiple, different keys are pressed in this interval, I believe the scanning routine will pick all of them up, but not necessarilly in the right order.
So the fix is to change the keypad_timer's restart expiration time to something more reasonable, like the 27 ms used by the interrupt handler. This decreases the waiting time to one in which it's unlikely that someone will be able to press multiple, or repeatedly press a key. As it turns out, that's exactly what DI18 does, so we know it works. It's totally unclear why Samsung changed it.
Oh well.
Edit: Originally I thought keys were latched, so press events would be detected by the scanning routine even if the key is released before keypad_timer expires. They're most certainly not though, the scanning routine likely polls the instantenous key (up or down) state, so a key press must be held for at least 27 ms so that the scanning may complete after the interrupt is serviced. If a key is held shorter than this, it will not be detected.
3/23/11: Alternate "timer delay" patch:
Also attached is an alternate patch that supercedes the original. It allows for runtime configuration of the keypad_timer delay via a sysfs exported timer_delay variable.
The current delay may be read with:
Code:
cat /sys/devices/platform/s3c-keypad/timer_delay
Code:
echo N > /sys/devices/platform/s3c-keypad/timer_delayA delay of "1" yields the shortest timer, and the most responsive keypad possible, whereas a delay of "256" (1 second) (in most kernels) is the longest allowed. At that point the keyboard is already useless since any keypress results in either repeated characters or the "alternate character popup".
In most kernels (where HZ=256), the default delay is "7" (27 ms), which is the same as the original patch and DI18. A delay of "16" (63 ms) approximates the behavior of stock EB13/EC05.
Basically, the delay is the minimum amount of time one must hold a key down for a keypress to register. It's also the maxmimum time the kernel believes a key is "held down" after it's let go. For example, with a delay of "7" one must hold a key down for 27 ms to register as pressed, and no-more-than 55 ms later does the key register as being released.
Or for a better example, with a delay of "128" one must hold a key down for half a second, and once registered, the key is "held down" for an additional half a second. Which almost guarantees an application-level repeated keypress or the alternate character popup.
Ideally one would start with a delay of "7" and reduce by one until the timer is fast enough for your typing rate that you no longer have dropped keypresses. Although many folks will find that a delay of "7" is fast enough for them.
7/9/11: New "column-switch delay" patch:
Attached are two (mutually exclusive) patches to export the "column-switch delay" (column_delay) in addition to the timer_delay variable. The first applies against stock EC05 and supercedes all previous patches. The second (alt patch) applies against stock EC05 with the previous timer_delay patch already applied and should be used for custom kernels already containing that patch.
The column-switch delay is the amount of time, in µs, that the driver delays while switching between the six keyboard "columns" (actually rows) in the keyboard scaning routine. A delay is necessary to ensure that the column switch has been performed by the hardware before scanning a new column, otherwise ghost keypresses result.
However, the 300 µs delay specified by Samsung is very inefficient, particularly for "reasonable" timer_delay values. With theimpaler747's suggested timer_delay of 5, the keyboard driver, while holding a key down, consumes 9.7% of CPU time (empirically tested), at any clock rate. This is quite-likely responsible for folks' previous complaints that use of the hardware keyboard causes audio skips, poor game performance, decreased battery life, etc.
This patch allows this (default) 300 µs delay value to be modified through a sysfs exported column_delay variable. Similar to the timer_delay, it may be read with:
Code:
cat /sys/devices/platform/s3c-keypad/column_delay
Code:
echo N > /sys/devices/platform/s3c-keypad/column_delayA delay of "1" is the most efficient (only 0.02% CPU utilization) but results in ghost key presses. In testing, I've found ghosting to disappear at delays of "3" and above. To provide some buffer, I've been using (and recommend) a delay of "5" for both column_delay and timer_delay values for the past week, and I've not run into any issues.
At a delay of "5", the keyboard driver is far more efficient, consuming only 0.2% of CPU time while keys are held. This hopefully translates into less stuttering/performance problems and better battery life.
Testing on other devices is needed (and appreciated!) to find a reasonable delay where ghosting doesn't appear, assuming that "5" is too low on some devices. Even delays of "10" and "50" (comparable to other keyboard drivers) result in ~0.3% and 1.7% CPU utilization respectively, which is much better than the Samsung default.
Mirror links:
GB kernel patches: keyboard_patches-GB.tar.gz
Column-switch & timer delay patch: epic_keypad_delays-EC05.diff (for stock EC05)
Column-switch & timer delay patch: epic_keypad_delays_alt-EC05.diff (for kernels with timer delay patch already applied)
Historical links:
Timer delay patch: epic_keypad_timer_delay-EB13.diff
Original patch: epic_keypad_timer_fix-EB13.diff
Last edited:
