Rockbox

Tasklist

FS#9025 - Gigabeat's keypad unusable

Attached to Project: Rockbox
Opened by Clément Pit--Claudel (CFP) - Wednesday, 21 May 2008, 20:52 GMT
Last edited by Michael Sevakis (MikeS) - Thursday, 29 May 2008, 21:29 GMT
Task Type Bugs
Category User Interface
Status Closed
Assigned To No-one
Operating System Gigabeat F/X
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Hello,
r17549 and 17559 change gigabeat's handling of button inputs. Installing this new versions causes my gigabeat to turn unusable. I hard-reseted it, tried again, but encountered the same problem. Sometimes pressing a key has an actual effect, but sometimes it considers the key is still being pressed though it's been released, and sometimes it just does not react at all. Furthermore, reverting the firmware to previous versions fixed the problem. Is anybody experiencing the same problem ? Shouldn't these changes just be reverted ? Or am I doing something wrong ?
This task depends upon

Closed by  Michael Sevakis (MikeS)
Thursday, 29 May 2008, 21:29 GMT
Reason for closing:  Accepted
Additional comments about closing:  Mulled it over in IRC and other's felt the patch as-is was the best way to handle it.
Comment by David Maher (Sunookitsune) - Thursday, 22 May 2008, 18:24 GMT
I've noticed the same problem with my Gigabeat F. I had been running the daily build from 5/14 (r17500) without issue, than switched to the current build (r17608) and had the exact problem described above. Rockbox would start randomly flipping through menus, and ignoring most of my keypresses. Reverting to r17500 fixed the problem. I'm at work right now, so I can't test further.
Comment by Clément Pit--Claudel (CFP) - Friday, 23 May 2008, 06:15 GMT
Same here (rockbox flips through menus)
The firmware is actually quite unusable. Reverted to r17548.
CFP
Comment by Clément Pit--Claudel (CFP) - Friday, 23 May 2008, 06:22 GMT
Did a little more testing. Bug created in r17559 : building with current svn and button-meg-fx.c from r17549 works perfectly.
Comment by Clément Pit--Claudel (CFP) - Saturday, 24 May 2008, 13:20 GMT
I had a look at the actual code : Although r17549 introduced double entries for each key, values actually read were not modified until r17559 : GPJDAT was compared to 1000011001001 [0x10C9], excluding values 1<<11, 1<<8, 1<<5, 1<<1. r17559 changed 0x10C9 to 1100111101011 [0x19EB] (0x19EB = 0x10C9 | 100100100010). Definitely adding values 1<<(1, 5, 8, 11) makes keypad too sensitive. I suggest we revert to the version preceding r17549 (keycodes might however be useful to keep as a record, I think).
CFP.
Comment by Michael Sevakis (MikeS) - Wednesday, 28 May 2008, 07:06 GMT
I don't think a full revert is a workable solution since I and others rather like the increased sensitivity. Though I commented rather unconstructively in the  FS#9045  but I've thought about a bit and so will try to suggest something better.

I'm not one really to like adding settings for things but in this case I propose the following addition which will be workable for everyone (so I'll kick it off right here since I'm not inclined to produce a patch atm):

Keypad Sensitivity
Low (default, reads one line per direction [++] works like before)
Medium (same as SVN, reads two lines per direction [+, ++])
High (reads all three lines per direction [ , +, ++)

Setting the sensitivity will only require changing of the master bitmask dynamically to one of three values and no change to the code inside the if(touchpad) clause.

void button_read_device(void)
{
...
touchpad = GPJDAT & touchpad_mask;

if (touchpad)
{
/* Keep direction-desensitization code for center-press */
...
/* Each direction check may now simply bit-test all three lines unconditionally */
...
}
...
}

void set_keypad_sensitivity(int level)
{
static const uint32_t masks[3] =
{
0x10c9, 0x19eb, 0x1fff
};

if ((unsigned)level >= 3)
level = 0; /* Wrong...use default */

touchpad_mask = masks[level];
}
Comment by David Maher (Sunookitsune) - Wednesday, 28 May 2008, 12:18 GMT
If a setting for keypad sensitivity is added, it should default to the old level of sensitivity ("Low" in your description). Otherwise there would be no way of getting to that setting without manually editing the config file.
Comment by David Maher (Sunookitsune) - Wednesday, 28 May 2008, 15:28 GMT
Maybe it's just me, but it seems to make the most sense to revert the code. The average user running Rockbox would see the option to change sensitivity. Perhaps they think it would be easier with higher sensitivity, or perhaps they just want to try out this new setting. Suddenly, their player starts doing random things, and they can no longer control it even enough to fix the setting they changed. It's highly unlikely that they'd know enough (or want) to use bootloader USB mode to edit the config file to remove the setting they set.

Also, given the choice between the current revision, which some people prefer, but also appears to break many people, and the old revision, which apparently worked for everyone, it seems like an easy decision.
Comment by Clément Pit--Claudel (CFP) - Wednesday, 28 May 2008, 17:27 GMT
I agree with you about reverting the code. The problem, as you said, with provinding users with a setting is that they might not be able to revert the setting to a previous one. There might be a solution to this though, wich would consist in coding a sort of keypad tester that would be shown after setting modification : the change would only be taken in account if the user could press a certain sequence of keys (eg left, top, right, bottom) : after, say, 5 seconds, or after a wrong input, the keypad sensitivity would be reverted to default. This still might be hard to code. However, as of current revision, I can say that it is quite annoying having to use my own build rather than rockbox default. Still, I understand that some users might want to keep the setting. I'd like to code a plugin that would do the switch, but I must say I don't really know how to access rb settings from code...
CFP
Comment by Michael Sevakis (MikeS) - Thursday, 29 May 2008, 00:42 GMT
Of course the safest setting would be the default. If worse comes to worse a settings reset can be done if navigation is beyond just difficult. Too elaborate and it's bloaty and/or awkward to use and doesn't fit nicely into the settings scheme and the plugin system isn't voiceable yet.

The change was made because the old version didn't work so well for everyone (it wasn't even my idea) or it would never have been done. The old worked for me but not nearly as well as current SVN which just requires a light touch - the direction would require making sure I always pressed toward the extreme ends or it would miss.

Perhaps just use "Low" and "High" meaning 0x10c9 or 0x19eb and forget about 0x1fff since I have a feeling that's too much for anything and triggers inner directions before the center.
Comment by David Maher (Sunookitsune) - Thursday, 29 May 2008, 01:17 GMT
I'm more than happy to test things, but I don't have enough time to be much help writing code at the moment. But again it seems to me that slightly annoying to use beats completely unusable.

Also, it seems to me that the version I'm currently running (r17500) works better with the touchpad than it used to, though I can't remember when it improved. Looking at the changelog it may have been r17219, since that change seems to be about the correct amount of time ago for the improvement. I can try to test a bit and see if I can track down when it was if you want.
Comment by Brett (Wrathernaut) - Thursday, 29 May 2008, 08:34 GMT
Any chance this is related to #9035? http://www.rockbox.org/tracker/task/9035

I'll update to latest again and see if similar issues are apparent.
Comment by Michael Sevakis (MikeS) - Thursday, 29 May 2008, 09:26 GMT
David,

I can bring previous functionality back by default and throw together whatever later so everyone can have the touchpad useable to their liking.

Brett,
 FS#9035  doesn't in any way sound related to this.
Comment by Michael Sevakis (MikeS) - Thursday, 29 May 2008, 11:31 GMT
Bah, too easy to do so here's the whole patch as I envisioned this working.
Comment by Clément Pit--Claudel (CFP) - Thursday, 29 May 2008, 15:12 GMT
Good job, although I'm still unsure of how users who will encounter problem after changing the setting will get back to default ?
Comment by Michael Sevakis (MikeS) - Thursday, 29 May 2008, 17:27 GMT
Simple, reset the settings with the "A" button on boot if it's completely unworkable.
Comment by Clément Pit--Claudel (CFP) - Thursday, 29 May 2008, 17:46 GMT
Still, having to reset the settings is a quite heavy procedure, implying to lose any previously set settings. Still, I like the possibility of switching back to low sensitivity.
Comment by Michael Sevakis (MikeS) - Thursday, 29 May 2008, 20:19 GMT
I think if one is experienced enough to setup the device in such detail one may have also saved a .cfg file as backup and probably can do a quick edit. It also sounds as though it doesn't render it so useless you'd have no chance of getting to the menu item. Besides, if it's wrong for you I suspect you'll never put it on "High" again. :) It also strongly defaults to "Normal". A button push to only reset the touchpad at boot could be done cheaply as well.
Comment by Alex Parker (BigBambi) - Thursday, 29 May 2008, 20:33 GMT
Personally, I very much prefer the new setting, I find it much easier to control. However, if it makes some players unusable, then there is clearly an issue - a setting would work for me. I found the old one would sometimes not recognise a direction for me, and now it works all the time, with no false pushes on centre. However, me missing a direction from time to time isn't as bad as people having false centres.
Comment by David Maher (Sunookitsune) - Thursday, 29 May 2008, 20:57 GMT
At least on my unit, the controls are completely useless with the current revision, so much so that I would have no chance of navigating to that menu to turn it back off.

Loading...