FS#10671 - Touchscreen support for newer Cowon D2+ players

Attached to Project: Rockbox
Opened by Jonas Aaberg (GMelchett) - Sunday, 11 October 2009, 17:05 GMT
Last edited by Rob Purchase (shotofadds) - Thursday, 15 October 2009, 20:46 GMT
Task Type Patches
Category Drivers
Status Closed
Assigned To No-one
Operating System Another
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


This patch adds touchscreen support for newer Cowon D2+ players. The touchscreen chip itself is still unindentified.

Applies fine on svn October 11th 2009.
This task depends upon

Closed by  Rob Purchase (shotofadds)
Thursday, 15 October 2009, 20:46 GMT
Reason for closing:  Accepted
Additional comments about closing:  In r23194
Comment by Jonas Aaberg (GMelchett) - Sunday, 11 October 2009, 17:07 GMT
The patch itself.
Comment by Jonas Aaberg (GMelchett) - Sunday, 11 October 2009, 17:13 GMT
Sorry, a typo in the old patch.
Comment by Rob Purchase (shotofadds) - Sunday, 11 October 2009, 17:34 GMT
Thanks for this. There are few things that need to be resolved before this can go in the main build:

- you can get the PMU type by calling get_pmu_type() defined in power-target.h
- all the touchscreen code should probably be moved out of button-cowond2.c into a separate file, since it's turning in to a bit of a monster now
- consider making button_read_touch_* always return 10-bit precision (as per the PCF), this would solve your 'short' issues and not really cause any reduction in accuracy
- it's probably a waste to poll the touchscreen every 100Hz, maybe cache the results and poll every 10 ticks or so?

Also I was thinking of removing the touch_calibration stuff since there's already a calibration screen in the settings menu (which could be set up with sensible defaults). That would remove some of the complexity.
Comment by Jonas Aaberg (GMelchett) - Sunday, 11 October 2009, 18:48 GMT

I agree with all your comments, and I'll update my patch. I'll look into the calibration code as well.

Comment by Jonas Aaberg (GMelchett) - Tuesday, 13 October 2009, 12:26 GMT
Hi Rob, I hope this patch is to your satisfaction.

Comment by Rob Purchase (shotofadds) - Tuesday, 13 October 2009, 16:45 GMT
That's looking pretty good, although if possible I'd like to try and tidy up the FIXME's before it gets committed (although the I2C related names are going to be tricky if we can't identify the device). The reason is simple: most probably no-one will ever get around to fixing them...

A few minor comments:

- it'd be more consistent to use the TIME_AFTER macro to check whether to read the touchscreen or not, rather than a simple counter (see example in touchscreen_read_pcf50606)
- the patch contains a mixture of TABs and spaces, and we don't like TABs ;-)
- the calibration constants you added turn out to be fairly similar to the existing ones (which aren't accurate anyway), maybe the existing ones or a 'halfway house' would work for you?

Apart from the last one, these are things I can fix myself before committing. If the old calibration values work for you (it doesn't need to be accurate - that's what the config option is for...) I will commit this later today. The extra calibration step will probably be removed entirely anyway, as it is a relic from before the config option existed.

Finally one more thing, how good is the data coming from this 'unknown' device? Does it need to be smoothed at all like the data from the PCF50606? You can check quite easily by adding the test_touchscreen plugin to plugins/SOURCES and running it see how much the pointer jitters.....
Comment by Jonas Aaberg (GMelchett) - Tuesday, 13 October 2009, 17:05 GMT

Feel free to do what changes you feel needed. Is there any code-style checking patch, like checkpatch for the linux kernel?
Ah, forgot about the TIME_AFTER macro - of course it should be used.
The old pcf50606 calibration values are nearly the same as mine. Yes, your old ones works fine.

I tested with the test_touchscreen plugin and the pointer isn't steady - or rather I guess my hand isn't. It tend to flicker on a I guess one pixel level, mostly in X direction.

I'm pretty confident that the unknown touchscreen device is an TI TSC2003 - or something very alike. I have not gotten the "Measure" parts of the commands to work yet, but the others seems to respond properly.
Comment by Rob Purchase (shotofadds) - Tuesday, 13 October 2009, 19:57 GMT
If you're only getting jitter of 1px or so then the accuracy is already far better than the PCF50606 equivalent. I would guess the touchscreen controller hardware is doing some smoothing of the data - which would probably make it a TSC2007:

"If the acquired signal source is noisy because of the digital switching circuit, it may necessary to evaluate the
data without noise. In this case, the median value filter operation helps remove the noise. The array of seven
converted results is sorted first. The middle three values are then averaged to produce the output value of the
MAV filter.
The MAV filter is applied to all measurements for all analog inputs including the touch screen inputs, temperature
measurements TEMP1 and TEMP2, and auxiliary input AUX."

(from )
Comment by Rob Purchase (shotofadds) - Tuesday, 13 October 2009, 21:24 GMT
Here's the update I intend to commit, could you check it still works on your D2+ as I have moved a few things around!

ps. to answer your earlier question, we don't have anything as fancy as a patch checking script - just a few basic guidelines in docs/CONTRIBUTING.

EDIT: updated patch to include a missing file
Comment by Jonas Aaberg (GMelchett) - Wednesday, 14 October 2009, 18:24 GMT

You are probably right, it is an TSC2007. Sounds more like it.
Your patch looks fine, but I tested your patch and it didn't work. I haven't had the time to find out what it wrong with it. I'll do that tomorrow.
Ah, the documentation. Sorry forgot about it since you have been so helpful. Thanks!

Comment by Rob Purchase (shotofadds) - Wednesday, 14 October 2009, 19:29 GMT
Strange, I wonder what the problem is.

I thought it might be because we now check the return values from i2c_readmem() - but our implementation (in i2c-telechips.c) always seems to return 0, so it can't be that. I hope it doesn't take too much effort to debug my mistake :/
Comment by Jonas Aaberg (GMelchett) - Thursday, 15 October 2009, 15:14 GMT

After fixing wwo small errors your patch works:

- Should be TIME_BEFORE instead of TIME_AFTER in touchscreen_read_tsc200x(), touchscreen-cowond2.c
- Add a line saying "last_btn = btn" at the end of touchscreen_read_tsc200x(), touchscreen-cowond2.c

Then it works fine.
Comment by Rob Purchase (shotofadds) - Thursday, 15 October 2009, 19:09 GMT
Ah yes, I hadn't noticed the logic was backwards... but regarding 'last_btn = btn', the ver2 of this patch didn't do this - presumably it didn't work very well either ;-)

I'll commit your changes tonight, many thanks for working on this.