This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#10603 - AMS Sansa: refactor DBOP button reading
Attached to Project:
Rockbox
Opened by Bertrik Sikken (bertrik) - Saturday, 12 September 2009, 11:26 GMT+2
Last edited by Rafaël Carré (funman) - Thursday, 07 January 2010, 00:41 GMT+2
Opened by Bertrik Sikken (bertrik) - Saturday, 12 September 2009, 11:26 GMT+2
Last edited by Rafaël Carré (funman) - Thursday, 07 January 2010, 00:41 GMT+2
|
DetailsSome of the AMS Sansas use the same set of pins (DBOP data pins) both for reading buttons and for writing data to the display. This patch updates the way the reads and writes are coordinated and unifies the way the button reads are done for the c200v2, e200v2 and fuze.
Current issues: * there are now *two* mechanisms for the fuze and e200v2 to read the buttons: 1) doing a DBOP read and 2) doing a GPIO read. * as far as I can tell, there is no coordination between LCD writing and doing a GPIO read. This is bad because the GPIO read switches the function of the DBOP data pins from DBOP to GPIO while an LCD write is possibly in progress. I think this is the reason for the sporadic "blue bars" on the fuze/e200v2 display. * the mechanism to prevent contention between DBOP read (buttons) and DBOP write (lcd) is quite primitive: it just skips a DBOP read when a DBOP write is busy, so there is no guarantee how often a DBOP read is actually done. * the kernel tick interval has been decreased to 5 ms on the fuze and e200v2 presumably to get more DBOP readings for smooth scrollwheel readout, yet a full LCD update takes about 10 ms on fuze/e200v2 and blocks DBOP reads during that time. * the DBOP read procedure is to first do a "red-pixel" write to the DBOP/LCD then read the DBOP data pin value back. There is a simpler way to read the DBOP data pins, without involving the LCD. This patch addresses these issues as follows: A new dbop-as3525.c modules is added with three functions: * dbop_lock is called by the LCD functions when they want to do an LCD write (e.g. lcd update, which takes up to 10 ms). The dbop_lock function first takes an input sample from the DBOP data pins. * dbop_unlock is called by the LCD functions when they are done writing for the time being. * dbop_input is called by the button driver and either reads the DBOP input directly (when not locked) or returns the most recent DBOP input sample taken in dbop_lock. This way it is guaranteed that the sample is never more than 10 ms old (fuze/e200v2), even when the LCD is very busy. The button code and the lcd code now both refer to this new dbop-as3525 module, so the button code no longer needs to refer to the lcd code. The dbop_input function reads the DBOP data pins by "pre-charging" the data pins to a defined level and reading them back after a fixed delay to see if they changed (when a button was pressed). This delay (31 PCLKs) is independent of CPU boost status, unlike the GPIO delays currently used. The LCD doesn't "see" this write/read cycle because the LCD write strobe line is temporarily disabled in dbop_input. This mechanism is simpler (and even faster) than the "red-pixel" method. The c200v2 can not even read all buttons with the red-pixel method. The GPIO method of reading the DBOP data pins is completely removed. The kernel tick rate is restored to the default 100 Hz. This code has been tested to work on a c200v2 and e200v2 (briefly), not tested yet on a fuze. 
dbop_buttons.patch
(22.4 KiB)
firmware/SOURCES | 3 firmware/target/arm/lcd-c200_c200v2.c | 32 -- firmware/target/arm/as3525/sansa-c200v2/button-c200v2.c | 11 firmware/target/arm/as3525/sansa-e200v2/lcd-e200v2.c | 34 -- firmware/target/arm/as3525/kernel-as3525.c | 26 -- firmware/target/arm/as3525/dbop-as3525.c | 83 ++++++ firmware/target/arm/as3525/dbop-as3525.h | 25 + firmware/target/arm/as3525/button-e200v2-fuze.c | 207 +++------------- firmware/target/arm/as3525/sansa-fuze/lcd-fuze.c | 38 -- 9 files changed, 189 insertions(+), 270 deletions(-) |
This task depends upon
Closed by Rafaël Carré (funman)
Thursday, 07 January 2010, 00:41 GMT+2
Reason for closing: Accepted
Additional comments about closing: r24192
Thursday, 07 January 2010, 00:41 GMT+2
Reason for closing: Accepted
Additional comments about closing: r24192
What do you plan to gain by increasing the timer interrupt? Polling less often gives more missed reads no matter of writing red pixels or GPIO reads or whatever. The kernel tick is 100Hz in SVN too...
EDIT: I deactivated hold button (I tried "pre-charging" with E0FF since hold is high on bit 12, but that didn't work). The scrollwheel feels a lot worse. After reverting the change to the kernel-as3525.c the scrollwheel is *much* better. Apart from that, it seems to work nicely, good job! even the sporadic lcd corruption seems to be gone.
firmware/target/arm/as3525/dbop-as3525.c: In function ‘dbop_lock’:
firmware/target/arm/as3525/dbop-as3525.c:75: error: wrong type argument to increment
I changed the button poll rate back to the default because I thought it would be good to start with a clean slate after such a big change.
So, if it turns out to be indeed insufficient we can always increase it again, or maybe investigate alternative solutions. Things we could do:
* Increase button poll rate like you suggest. We still have a problem with the scroll wheel if the LCD is very busy, since the still dbop stays locked for 10 ms during an LCD update.
* Currently we return BUTTON_NONE if both bits of the wheel are different of the previous value (so more than one step were done since the last poll). Maybe we could cheat a bit here and just assume the previous scroll direction.
I'm a bit skeptical about your second idea (a) if it will work nicely at all and b) if it can replace the increased polling rate). We could also try doing both of course.
Can you test if reverting the change to kernel-as3525.c changes anything? change button_read_dbop() to button_read_device() after reverting if you do so.
The scrollwheel seems smoother with bertrik's patch. The most notable difference was better scroll acceleration when scrolling fast through a long list.
it also applies the increased polling rate, and reading the dbop while being blocked (the scrollwheel bits on the fuze can be read during lcd updates).
i can not tell the difference for the scrollwheel with or without this patch (read wheel HZ times per second)
Regarding the polling rate, can we somehow calculate how fast we really need to poll? For example by considering a maximum reasonably expected speed of changes in the scrollwheel bits?
For example, my e200v1 has about 20 'ticks' per revolution (and I think 2 scrollwheel value changes per tick). So for example assuming a max scroll speed of two revolutions per second gives a max number of changes/s of 80. If this is indeed larger than 100 changes/s (for the fuze for example) that would be a reason to indeed increase the polling rate.
You can easily do 3-4 rotations per second (and I do this), since the wheel isn't just a thin one but it covers most of the front. That means 192 changes per second if you want 4 rotations (and again, I do).
Why shouldn't we read the dbop if we can, even if blocked? That means intentionally ignoring wheel changes. What if I tell you that reading in that state greatly improves the wheel?
The miss happen on heavy load only, and if we don't read the wheel twice per tick there is no miss since we read every tick (at the start of lcd_update())
"Why shouldn't we read the dbop if we can, even if blocked?"
"dbop_read_input waits for the output FIFO to drain before doing the read. Since dbop_read_input can be called from the tick interrupt, this means the interrupt wastes unneeded time."
edit: meh, doesn't quite work well, sometimes there are glitches.
The wheel is much worse when above 1 rotation per second. I'm not going to accept this.
funman: I tried your patch, and I notice a huge difference. It's obvious that that 10ms isn't enough, acceleration breaks if you scroll quickly (say 2 rotations per second). That seems to confirm my calculation.
Unfortunately the c200v2 and e200v2/fuze use a different level for the hold pin, so they may also need a different pre-charge level. I'm curious if this patch still works on both an e200v2 and a fuze.
Also I wonder if it would be possible to get rid of the extra delay on the fuze for the hold button, by using a different pre-charge level for the hold button, please experiment.
Additionally, I tried to use other precharge values (upto 0x8EFF) to fix the hold button, but no success.
What do you mean by "the new dbop routine doesn't send commands to the LCD anymore but only manipulates the dbop data lines."
- Restore locking scheme
- Move precharge levels to export/config/*
Tested on fuze:
- no LCD glitches at all
- all buttons work
- scrollwheel works
Scrollwheel and hold switch do not work, all other buttons appear to work properly. Display looks good in menus and WPS, could not test mpegplayer due to lack of scrollwheel.
Normally, during a word transfer from the cpu to the display, the DBOP generates a pulse on one of its lcd control lines (same pin as GPIO B3). It is this pulse that make the lcd aware that the cpu wants to transfer data.
During the button read, my idea is to configure the DBOP controller to leave this pulse out on the lcd control lines during the DBOP write , so the data lines are changed, but the lcd does not notice it.
I couldn't find a related bit in DBOP_CTRL, and effectively button & lcd dbop write code set the same bits (safe for tri-state and output data width)
@kugel. - I removed the #ifdef SANSA_FUZE so that the same delay is used for the e200v2. The scrollwheel and hold buttons do work. (DBOP_CTRL's bit 19 used in dbop_read_input())
I wiil do some further tests when I get home to determine if there is any screen corruption and whether the scrollwheel is better or worse than SVN with this delay.
1. Scrollwheel works at least as good as SVN.
2. No screen problems detected.
3. While playing a video using mpegplayer, the scrollwheel does not change volume unless the video is paused. Volume change works while video plays with SVN.
4. Doom and Rockboy plugins - none of the buttons or scrollwheel appear to work. Other plugins do work, but I did not test all of them.
One thing that should be done is making the variables in the dbop file volatile, maybe that fixes things during frequent lcd updates.
4. Doom and Rockboy plugins - the buttons work in the menu, but the scrollwheel does not work. Other plugins do work, but I did not test all of them.
It seems all ams sansas use the same configuration for the display control signals: c0 is connected to the LCD input that switches between data and command, c1 is connected to the LCD write strobe input (active low), display control outputs c2 and c3 (configured by DBOP_TIMPOL_23) are not used as far as I can see.
I added a small delay (50 nops) after setting DBOP_TIMPOL_23 and blue pixels / shifting are gone (no locking required)
I tested doom & mpegplayer and the wheel works fine there (FUZE).
EDIT: sorry the patch had color codes in it so I made another one with git diff --no-color and added the delay for E200v2 as well
EDIT: Please disregard this. I must have had the dbop-as3525.x files left over from previous patches. The build breaks on an undeclared delay.
This patch does not fix the problems in mpegplayer, doom and rockboy on the e200v2 that were noted for the previous patch.
- increase delay before reading to 100 nops
I think wheel acceleration is much more noticeable but I would like others to verify since my fuze' wheel is a bit broken (I replaced it)
mc2739 does it fix the wheel on doom/mpegplayer ?
I am still seeing the same problems with the increased delay.
- lower the delay after changing DBOP_TIMPOL_23 to 2
- modify fuze & e200v2 drivers
- remaining differences: window code (lcd_window() on e200v2 versus lcd_window_x()/lcd_window_y() on fuze), and initialisation code
- added invert & flip for fuze
- some cosmetics (comments, indentation, spacing) so diff only reports code difference
- add delays only found in fuze code
I observed the same results as prior patches. I also noticed intermittent blue pixels with this patch.
The Matrix plugin does not have any problems with the scrollwheel (it did not have problems with dbop_buttons7.patch either).
I have now tested all plugins for problems with the scrollwheel, and the only aditional problem is with the Fireworks demo plugin. This plugin also fails with the previous patches.
With further testing, I have noticed that the hold switch is also not working with these same plugins where the scollwheel does not work. This is true with previous versions of the patch.
Since fuze & e200v2 use likely the same controller, we could read the hd66789r datasheet to see differences in lcd initialization / or search better for the correct datasheet
mc2739 could you add some big delay at the start of dbop_read_input() so we know some time has elapsed since the last LCD operation? Perhaps that's the problem in LCD intensive plugins. I have no other idea atm ..
Changing the delay to a larger value did not help.
In looking at differences between Bertrik's original patch (worked well on e200v2) and the current patch, I noticed that the precharge value has changed from 0xF0FF to 0x80FF. I changed that value in the latest patch back to 0xF0FF and the scrollwheel works in all plugins, but the display started having problems (jittery). A value of 0xE0FF appeared to have the same results as 0xF0FF. Values of 0xD0FF through 0x80FF all caused the scrollwheel the have problems in the plugins.
I did not try different delay values with the precharge change. The original patch from Bertrik did not have a delay for the e200v2. A delay was added later to get the patch working on the Fuze.
What kind of problems are you experiencing exactly? Maybe you can try enlarging the delay after writing to DBOP_TIMPOL_23 or other ones, or inserting some at other places.
The original patch from bertrik didn't precharge during lcd updates, that's the main difference so that delays could be now needed.
The following changes were made to the dbop-buttons9.patch:
1. firmware/export/config/sansae200v2.h - changed DBOP_PRECHARGE to 0xF0FF (was 0X80FF)
2. firmware/target/arm/as3525/dbop-as3525.c - changed delay after DBOP_TIMPOL_23 = 0xe007e007; to 10 (was 5)
r24186 svn
Main LCD Update
1/1: 100.0 fps
1/4: 346.5 fps
Main LCD YUV
1/1: 28.8 fps
1/4: 108.5 fps
CPU: 62MHz
r24186 dbop-buttons10.patch
Main LCD Update
1/1: 99.5 fps
1/4: 346.5 fps
Main LCD YUV
1/1: 28.8 fps
1/4: 108.5 fps
CPU: 62MHz
Do we wait to see if bertrik approves or we commit now ?
But we both think that #define DBOP_PRECHARGE shouldn't be in the config/<target>.h file. It's a driver detail (it apps/ and non-target tree code doesn't behavor differently on changing it) and changing it breaks things, so it's not selectable.