Rockbox

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.

   dbop_buttons.patch (20.9 KiB)

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

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.

   dbop_buttons.patch (23.7 KiB)

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.

Latest patch now builds without errors. All e200v2 buttons work properly. Scrollwheel very responsive even with fast scrolling.

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.

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.

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).

Yes, that is what I am saying. On the e200v2, the scrollwheel was worse after reverting kernel-as3525.c.

I still can’t believe this :)

I second everything mc2739 sees for my e280v2. This patch works very well as is.

Sync. Still no success with the hold button here :(

   dbop_buttons.patch (24 KiB)

sync after r22863

   dbop_buttons.patch (23.9 KiB)

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).

   dbop_buttons.patch (24.5 KiB)

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)

   sansa_fuze_normal_tick.diff (1.5 KiB)

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.

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?

How much time/ticks does lcd_update() need ? We would see how much reads we miss from the wheel

~10ms, we’re at 100fps, and we cannot go faster.

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.”

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.

   dbop_buttons2.patch (0.9 KiB)

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.

How does e200v2 wheel perform compared to fuze?

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.

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.

   dbop_buttons3.patch (24.1 KiB)

e200 scrollwheel is not working with the latest patch. Also seeing some intermittent blue pixels and screen contents that have shifted. Tested on r23352.

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.

kugel suggested on IRC that this task be the last one standing between making the Fuze and e200v2 “stable”. I think this is reasonable.

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.”

- 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

   dbop_buttons4.patch (28.3 KiB)

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.

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.

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.

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)

mc2739, can you try removing both settings of DBOP_CTRL’s bit 19 in dbop_read_input() ?

@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.

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.

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.

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.

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.

The patch seems broken. EDIT: I seem to refer to a deleted comment.

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

   dbop_buttons5.patch (27.4 KiB)

That’s great!

I haven’t tried but does it actually build? ‘delay’ seems to be used before it’s declared in dbop-as3525.c

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.

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.

- 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 ?

   dbop_buttons6.patch (27.4 KiB)

funman,

I am still seeing the same problems with the increased delay.

- restore the delay before reading to 50 since it doesn’t change anything
- lower the delay after changing DBOP_TIMPOL_23 to 2

   dbop_buttons7.patch (27.4 KiB)

- increase delay after changing DBOP_TIMPOL_23 to 5 after seeing problems on my fuze
- modify fuze & e200v2 drivers

1. remaining differences: window code (lcd_window() on e200v2 versus lcd_window_x()/lcd_window_y() on fuze), and initialisation code
2. added invert & flip for fuze
3. some cosmetics (comments, indentation, spacing) so diff only reports code difference
4. add delays only found in fuze code

   dbop-buttons8.patch (46.7 KiB)

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.

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 ..

   dbop-buttons9.patch (25.6 KiB)

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.

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.

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)

   dbop-buttons10.patch (24.5 KiB)

Weeh, awesome! I hope we can get this in then.

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

I can confirm hold/rec/scrollwheel all work normally on my e280v2. mpegplayer, pictureflow, various games(didn’t check doom) all have no issues.

Great! I think we just made it.

Do we wait to see if bertrik approves or we commit now ?

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.

I agree (even if I put it there)

So go for it!