Rockbox

Tasklist

FS#10603 - AMS Sansa: refactor DBOP button reading

Attached to Project: Rockbox
Opened by Bertrik Sikken (bertrik) - Saturday, 12 September 2009, 09:26 GMT
Last edited by Rafaël Carré (funman) - Wednesday, 06 January 2010, 23:41 GMT
Task Type Patches
Category LCD
Status Closed
Assigned To No-one
Operating System Another
Severity Low
Priority Normal
Reported Version Version 3.3
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Some 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.
This task depends upon

Closed by  Rafaël Carré (funman)
Wednesday, 06 January 2010, 23:41 GMT
Reason for closing:  Accepted
Additional comments about closing:  r24192
Comment by Thomas Martitz (kugel.) - Saturday, 12 September 2009, 14:35 GMT
This one actually builds. It doesn't work on my fuze. Screen goes dark after the bootlogo. I'm assming that the hold button gives false positives.

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.
Comment by Michael Chicoine (mc2739) - Saturday, 12 September 2009, 16:37 GMT
I'm not having any luck building on e200v2 with this patch. I get this error:

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

Comment by Bertrik Sikken (bertrik) - Saturday, 12 September 2009, 16:39 GMT
Indeed, the hold button seems a bit different on the e200v2 (but does work IIRC), I hope we get that one working fine too eventually.

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.
Comment by Thomas Martitz (kugel.) - Saturday, 12 September 2009, 16:54 GMT
I'm not sure what's bad with the increased polling rate. What's your issue with it?

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.
Comment by Michael Chicoine (mc2739) - Saturday, 12 September 2009, 17:10 GMT
Latest patch now builds without errors. All e200v2 buttons work properly. Scrollwheel very responsive even with fast scrolling.
Comment by Thomas Martitz (kugel.) - Saturday, 12 September 2009, 17:12 GMT
It isn't at all on my fuze.

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.
Comment by Michael Chicoine (mc2739) - Saturday, 12 September 2009, 17:48 GMT
Reverted kernel-as3525.c and changed line 47 from button_read_dbop(); to button_read_device();

The scrollwheel seems smoother with bertrik's patch. The most notable difference was better scroll acceleration when scrolling fast through a long list.
Comment by Thomas Martitz (kugel.) - Saturday, 12 September 2009, 18:36 GMT
Are you saying the scrollwheel is worse after reverting kernel-as3525.c? I really can't believe that, as the revert means to poll the wheel for changes twice as often. On the fuze, reverting kernel-as3525.c *vastly* improves the scrollwheel (more smooth and reliable).
Comment by Michael Chicoine (mc2739) - Saturday, 12 September 2009, 18:45 GMT
Yes, that is what I am saying. On the e200v2, the scrollwheel was worse after reverting kernel-as3525.c.
Comment by Thomas Martitz (kugel.) - Sunday, 13 September 2009, 12:29 GMT
I still can't believe this :)
Comment by Jack Halpin (FlynDice) - Sunday, 13 September 2009, 16:52 GMT
I second everything mc2739 sees for my e280v2. This patch works very well as is.
Comment by Thomas Martitz (kugel.) - Wednesday, 30 September 2009, 23:59 GMT
Sync. Still no success with the hold button here :(
Comment by Rafaël Carré (funman) - Friday, 02 October 2009, 17:55 GMT
sync after r22863
Comment by Thomas Martitz (kugel.) - Friday, 09 October 2009, 23:28 GMT
This one works great on my fuze. it fixes the hold button.

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).
Comment by Rafaël Carré (funman) - Saturday, 10 October 2009, 09:42 GMT
works fine on my fuze

i can not tell the difference for the scrollwheel with or without this patch (read wheel HZ times per second)
Comment by Bertrik Sikken (bertrik) - Saturday, 10 October 2009, 10:19 GMT
I think this violates the basic principle of the original patch: making sure that the DBOP port is either used for writing the LCD or reading the buttons, not some mix where we try to use the port for both reading and writing at the same time. The only reason it actually works in practice is that 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.

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.
Comment by Thomas Martitz (kugel.) - Saturday, 10 October 2009, 11:04 GMT
The fuze has 12 clicks per rotation. The wheel value can have 4 values per click. 12*4=48 per rotation.
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?
Comment by Rafaël Carré (funman) - Saturday, 10 October 2009, 11:24 GMT
How much time/ticks does lcd_update() need ? We would see how much reads we miss from the wheel
Comment by Thomas Martitz (kugel.) - Saturday, 10 October 2009, 11:27 GMT
~10ms, we're at 100fps, and we cannot go faster.
Comment by Rafaël Carré (funman) - Saturday, 10 October 2009, 11:45 GMT
Then we can miss 1 wheel read per call to lcd_update(), since there is a read at the start of dbop_lock() call, and the time of lcd_update() is < 1 tick (a bit less than 10ms).

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."
Comment by Thomas Martitz (kugel.) - Saturday, 10 October 2009, 11:52 GMT
Since we're doing no precharging (no write) we don't need to wait for the fifo empty, when doing the read in the blocked state. I just tried it, and it works.

edit: meh, doesn't quite work well, sometimes there are glitches.
Comment by Thomas Martitz (kugel.) - Saturday, 10 October 2009, 11:59 GMT
I just tried without doing that read when blocked again (i.e. take the e200v2 code path).

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.
Comment by Rafaël Carré (funman) - Thursday, 15 October 2009, 05:25 GMT
How does e200v2 wheel perform compared to fuze?
Comment by Bertrik Sikken (bertrik) - Sunday, 25 October 2009, 16:07 GMT
As far as I can tell from the code, the fuze wheel is twice as fast as the e200v2, but I'm not sure if the fuze and e200v2 have been objectively compared.
Comment by Bertrik Sikken (bertrik) - Sunday, 25 October 2009, 16:39 GMT
ok, I took Thomas' patch from 10 October and simplified it a bit further. A nice thing I didn't realise before is that we can now interrupt basically any LCD transfer to do a dbop button read, because the new dbop routine doesn't send commands to the LCD anymore but only manipulates the dbop data lines. So it is possible to remove the dbop_lock's completely.

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.
Comment by Michael Chicoine (mc2739) - Monday, 26 October 2009, 00:11 GMT
e200 scrollwheel is not working with the latest patch. Also seeing some intermittent blue pixels and screen contents that have shifted. Tested on r23352.
Comment by Thomas Martitz (kugel.) - Monday, 26 October 2009, 00:19 GMT
The scrollwheel works great on my fuze, but I can also see what mc2739 reported.

Additionally, I tried to use other precharge values (upto 0x8EFF) to fix the hold button, but no success.
Comment by MichaelGiacomelli (saratoga) - Saturday, 21 November 2009, 18:35 GMT
kugel suggested on IRC that this task be the last one standing between making the Fuze and e200v2 "stable". I think this is reasonable.

Comment by Rafaël Carré (funman) - Wednesday, 30 December 2009, 14:54 GMT
The blue pixel is the 0x80FF precharge value written to DBOP, I'm not sure why you think locking isn't required anymore.

What do you mean by "the new dbop routine doesn't send commands to the LCD anymore but only manipulates the dbop data lines."
Comment by Rafaël Carré (funman) - Wednesday, 30 December 2009, 15:17 GMT
- Sync last patch from betrik
- Restore locking scheme
- Move precharge levels to export/config/*

Tested on fuze:
- no LCD glitches at all
- all buttons work
- scrollwheel works
Comment by Michael Chicoine (mc2739) - Wednesday, 30 December 2009, 15:51 GMT
Tested dbop_buttons4.patch on e200v2 with r24125.

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.
Comment by Bertrik Sikken (bertrik) - Wednesday, 30 December 2009, 16:09 GMT
funman, the idea is that we can manipulate the DBOP data lines without affecting the display, as long as the lcd control lines remain passive.

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.
Comment by Thomas Martitz (kugel.) - Wednesday, 30 December 2009, 16:36 GMT
The shifted screen content shows that this is not working. The DBOP itself is getting confused, since due to the write, the windowing/rect setup is messed up.
Comment by Rafaël Carré (funman) - Wednesday, 30 December 2009, 17:17 GMT
bertrik: how is this pulse generated ?

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)
Comment by Rafaël Carré (funman) - Wednesday, 30 December 2009, 17:19 GMT
mc2739, can you try removing both settings of DBOP_CTRL's bit 19 in dbop_read_input() ?
Comment by Michael Chicoine (mc2739) - Wednesday, 30 December 2009, 21:40 GMT
@funman - I tried your suggestion (DBOP_CTRL's bit 19) and it did not make any difference. The scrollwheel and hold button still did not work.

@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.
Comment by Michael Chicoine (mc2739) - Wednesday, 30 December 2009, 23:00 GMT
After further tests, I have noted the following:

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.
Comment by Thomas Martitz (kugel.) - Thursday, 31 December 2009, 00:41 GMT
Interesting, that is probably related to the high refresh rate of those plugins. This patch should actually ensure that buttons are read by doing a read in dbop_lock(), so I'm not sure.

One thing that should be done is making the variables in the dbop file volatile, maybe that fixes things during frequent lcd updates.
Comment by Michael Chicoine (mc2739) - Thursday, 31 December 2009, 01:52 GMT
Correction

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.
Comment by Bertrik Sikken (bertrik) - Saturday, 02 January 2010, 11:13 GMT
funman, the display control pulse is generated after correctly configuring the DBOP_TIMPOL_01 and DBOP_TIMPOL_23 registers, see paragraph 7.3.12.1 of the datasheet (revision 1.13). These two registers allow generation of 4 display control signals, c0-c4. Within one display write cycle, you can define 3 'phases' each with their own polarity and you can define when each phase starts within the write cycle, this is described in paragraph 7.3.12.3 of the datasheet.

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.
Comment by Thomas Martitz (kugel.) - Saturday, 02 January 2010, 15:57 GMT
The patch seems broken. EDIT: I seem to refer to a deleted comment.
Comment by Rafaël Carré (funman) - Saturday, 02 January 2010, 15:58 GMT
I tried removing the setting of DBOP_TIMPOL_23 in button reading since you said it was not used and I was seeing constant blue pixels being added, so I thought there might be a race condition.

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
Comment by Thomas Martitz (kugel.) - Saturday, 02 January 2010, 16:05 GMT
That's great!
Comment by Thomas Martitz (kugel.) - Saturday, 02 January 2010, 16:15 GMT
I haven't tried but does it actually build? 'delay' seems to be used before it's declared in dbop-as3525.c
Comment by Jack Halpin (FlynDice) - Saturday, 02 January 2010, 17:04 GMT
This seems to work just fine here on my e280v2. When applying the patch git complained that the 2 dbop-as3525.x files already existed but when I checked them everything appeared to be in order. Scrollwheel, hold button, & rec button appear to function normally along with all other buttons. mpegplayer worked fine and various other plugins also.

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.
Comment by Michael Chicoine (mc2739) - Saturday, 02 January 2010, 17:23 GMT
kugel. is correct about 'delay'.

This patch does not fix the problems in mpegplayer, doom and rockboy on the e200v2 that were noted for the previous patch.
Comment by Rafaël Carré (funman) - Saturday, 02 January 2010, 18:04 GMT
- Declare delay properly
- 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 ?
Comment by Michael Chicoine (mc2739) - Saturday, 02 January 2010, 18:29 GMT
funman,

I am still seeing the same problems with the increased delay.
Comment by Rafaël Carré (funman) - Saturday, 02 January 2010, 18:46 GMT
- restore the delay before reading to 50 since it doesn't change anything
- lower the delay after changing DBOP_TIMPOL_23 to 2
Comment by Rafaël Carré (funman) - Sunday, 03 January 2010, 21:28 GMT
- increase delay after changing DBOP_TIMPOL_23 to 5 after seeing problems on my fuze
- 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
Comment by Michael Chicoine (mc2739) - Sunday, 03 January 2010, 22:53 GMT
Tested dbop-buttons8.patch on e200v2 with r24174.

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.
Comment by Rafaël Carré (funman) - Monday, 04 January 2010, 21:45 GMT
sync without modifying e200v2 delays (they are easily visible with diff anyway now)

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 ..
Comment by Michael Chicoine (mc2739) - Tuesday, 05 January 2010, 01:59 GMT
funman,

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.
Comment by Thomas Martitz (kugel.) - Tuesday, 05 January 2010, 02:06 GMT
Good catch. The Fuze needs no precharge, hence it worked (the scrollwheel bits aren't precharged by 0x80FF).

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.
Comment by Michael Chicoine (mc2739) - Tuesday, 05 January 2010, 13:30 GMT
Here is an updated patch with changes for the e200v2. This works well with no button or scrollwheel problems and no screen issues.

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)
Comment by Thomas Martitz (kugel.) - Tuesday, 05 January 2010, 14:07 GMT
Weeh, awesome! I hope we can get this in then.
Comment by Michael Chicoine (mc2739) - Tuesday, 05 January 2010, 15:03 GMT
I ran test_fps with the latest patch compared to svn and got these results:

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
Comment by Jack Halpin (FlynDice) - Tuesday, 05 January 2010, 16:02 GMT
I can confirm hold/rec/scrollwheel all work normally on my e280v2. mpegplayer, pictureflow, various games(didn't check doom) all have no issues.
Comment by Rafaël Carré (funman) - Wednesday, 06 January 2010, 00:11 GMT
Great! I think we just made it.

Do we wait to see if bertrik approves or we commit now ?
Comment by Thomas Martitz (kugel.) - Wednesday, 06 January 2010, 07:09 GMT
I talked with bertrik in IRC yesterday, it looked like he's fine with committing. I managed to test on my fuze, it works great.

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.
Comment by Rafaël Carré (funman) - Wednesday, 06 January 2010, 12:01 GMT
I agree (even if I put it there)
Comment by Thomas Martitz (kugel.) - Wednesday, 06 January 2010, 12:54 GMT
So go for it!

Loading...