Remove All Ads from XDA

[PATCH] Hardware keyboard dropped key press fix (10/19/11, GB support)

470 posts
Thanks Meter: 833
By mkasick, Retired Recognized Developer on 1st March 2011, 06:22 PM
Post Reply Subscribe to Thread Email Thread
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:
cat /sys/devices/platform/s3c-keypad/timer_delay
and a new delay may be set with:
echo N > /sys/devices/platform/s3c-keypad/timer_delay
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:
cat /sys/devices/platform/s3c-keypad/column_delay
and a new delay may be set with:
echo N > /sys/devices/platform/s3c-keypad/column_delay
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
Last edited by mkasick; 19th October 2011 at 09:43 PM. Reason: Added patches to support GB kernels.
The Following 24 Users Say Thank You to mkasick For This Useful Post: [ View ] Gift mkasick Ad-Free
1st March 2011, 06:33 PM |#2  
Retired Recognized Developer
Thanks Meter: 148
Donate to Me
Nice work!
Last edited by TheDub; 1st March 2011 at 10:44 PM.
1st March 2011, 06:45 PM |#3  
Senior Member
Thanks Meter: 6
Wow , nice work! I wish we had people as smart as you working @ Samsung or Sprint.

Sent from my SPH-D700 using XDA App
1st March 2011, 06:53 PM |#4  
BeerChameleon's Avatar
Senior Member
Flag Tucson,Arizona.
Thanks Meter: 1,206
Donate to Me
Nice WORK!!!

Since you have this fix, this will definetly improve it a little, now u just need to find a fix to fix all missed letters.

Great Job, cant wait for an updated kernal with this in it.

Our poor developers on here work so hard to clean up manufactures phones issues and bugs and do an amazing job. They should fire everyone at samsung and hire all the devlopers here. lol
1st March 2011, 07:42 PM |#5  
Account currently disabled
Flag Los Angeles, CA
Thanks Meter: 321
great job! so i take it this is both not necessary and not even compatible with DI18?
1st March 2011, 08:10 PM |#6  
Retired Recognized Developer
Thanks Meter: 148
Donate to Me
Well... downloaded Genocides kernel, was able to compile it after getting the toolchain and everything. Worked great. Loaded it on my zip, flashed it..

Kernel doesn't boot and suddenly my SD card is unreadable.. LOL! Guess I'll leave it to the pro's as I try to recover some data off my SD card...

Tried Genocides kernel flash zip, still didn't work, dang.
1st March 2011, 08:14 PM |#7  
Senior Member
Thanks Meter: 6
Lol , sorry I dont know why that is funny to me. Hope you get it working!

Sent from my SPH-D700 using XDA App
1st March 2011, 08:18 PM |#8  
Top Nurse's Avatar
Senior Member
Flag Palm Bay, FL
Thanks Meter: 59
This fix seems to be worse as the special key popup is even more noticeable than before. Perhaps a change in timing would be better. Maybe Samsung changed this in response to complaints of the popup coming up when unwanted.

Sent from Bonsai 6.0.3
1st March 2011, 08:33 PM |#9  
MeetFace's Avatar
Senior Member
Thanks Meter: 423
Originally Posted by TheDub

Nice work!

Building Genocide with this patch applied for all interested testers...

Rodderick will need to be the one to officially include it in HIS! Kernel but now we all have something we can test... will post link when it's done compiling.

Rodderik is already all over it broham. =]
1st March 2011, 08:35 PM |#10  
Rodderik's Avatar
Recognized Developer
Thanks Meter: 1,318
Donate to Me
Originally Posted by TheDub

Well... downloaded Genocides kernel, was able to compile it after getting the toolchain and everything. Worked great. Loaded it on my zip, flashed it..

Kernel doesn't boot and suddenly my SD card is unreadable.. LOL! Guess I'll leave it to the pro's as I try to recover some data off my SD card...

Tried Genocides kernel flash zip, still didn't work, dang.

thats my bad your sdcard got nuked it has to do with the i9000 having and internal and external sdcard storage...for now use my modified initramfs and not the new ported one on my git (says not to use i9000 initramfs in README on github)

mkasick thx for the info and i will put it in the kernel immediately and test
Last edited by Rodderik; 1st March 2011 at 08:42 PM.
1st March 2011, 08:46 PM |#11  
Senior Member
Thanks Meter: 10
Now fix the QAOL bug

Sent from my SPH-D700 using Tapatalk

Read More
Post Reply Subscribe to Thread

Guest Quick Reply (no urls or BBcode)
Previous Thread Next Thread
Thread Tools Search this Thread
Search this Thread:

Advanced Search
Display Modes