From d704f0ca556f309c45673ac9b1c7a64237eb44b9 Mon Sep 17 00:00:00 2001 From: patacongo Date: Fri, 11 May 2012 22:07:06 +0000 Subject: [PATCH] Fix a few STMPE11 touchscreen and NxWM touchscreen calibration bugs git-svn-id: svn://svn.code.sf.net/p/nuttx/code/trunk@4723 42af7a65-404d-4744-a932-0658087f49c3 --- ChangeLog | 7 +++- drivers/input/stmpe11_base.c | 2 +- drivers/input/stmpe11_tsc.c | 62 ++++++++++++++++++++---------------- include/nuttx/wqueue.h | 20 ++++++++++-- 4 files changed, 59 insertions(+), 32 deletions(-) diff --git a/ChangeLog b/ChangeLog index b33ad446f6..4b5ab66d09 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2735,4 +2735,9 @@ * drivers/input/stmpe11_tsc.c: Fix some status checks so that the touchscreen interrupt handling logic does not read data if the fifo is not at the threshold level. - + * include/nuttx/wqueue.h: Add macro work_available() to determine if the + previously scheduled work has completed. + * drivers/stmpe11_tsc.c: Correct errors: (1) Since all interrupt logic is done on + the worker thread, disabling interrupts does not provide protected; Need to + disable pre-emption. (2) Fix handling of touch ID and (2) add some logic to + prevent certain kinds of data overrun. diff --git a/drivers/input/stmpe11_base.c b/drivers/input/stmpe11_base.c index 34d13aeac7..546f1d9016 100644 --- a/drivers/input/stmpe11_base.c +++ b/drivers/input/stmpe11_base.c @@ -188,7 +188,7 @@ static int stmpe11_interrupt(int irq, FAR void *context) * to protected the work queue. */ - DEBUGASSERT(priv->work.worker == NULL); + DEBUGASSERT(work_available(&priv->work)); ret = work_queue(&priv->work, stmpe11_worker, priv, 0); if (ret != 0) { diff --git a/drivers/input/stmpe11_tsc.c b/drivers/input/stmpe11_tsc.c index 09b06d497e..c5aae38305 100644 --- a/drivers/input/stmpe11_tsc.c +++ b/drivers/input/stmpe11_tsc.c @@ -210,22 +210,18 @@ static void stmpe11_notify(FAR struct stmpe11_dev_s *priv) * Check if touchscreen sample data is available now and, if so, return * the sample data. This is part of the stmpe11_read logic. * + * Assumption: + * Pre-emption is disable to prevent the worker thread from running. + * Otherwise, sampled data may continue to change. + * ****************************************************************************/ static int stmpe11_sample(FAR struct stmpe11_dev_s *priv, FAR struct stmpe11_sample_s *sample) { - irqstate_t flags; int ret = -EAGAIN; - /* Interrupts me be disabled when this is called to (1) prevent posting - * of semphores from interrupt handlers, and (2) to prevent sampled data - * from changing until it has been reported. - */ - - flags = irqsave(); - - /* Is there new TSC2007 sample data available? */ + /* Is there new STMPE11 sample data available? */ if (priv->penchange) { @@ -263,7 +259,6 @@ static int stmpe11_sample(FAR struct stmpe11_dev_s *priv, ret = OK; } - irqrestore(flags); return ret; } @@ -279,19 +274,13 @@ static int stmpe11_sample(FAR struct stmpe11_dev_s *priv, static inline int stmpe11_waitsample(FAR struct stmpe11_dev_s *priv, FAR struct stmpe11_sample_s *sample) { - irqstate_t flags; int ret; - /* Interrupts me be disabled when this is called to (1) prevent posting - * of semphores from interrupt handlers, and (2) to prevent sampled data - * from changing until it has been reported. - * - * In addition, we will also disable pre-emption to prevent other threads - * from getting control while we muck with the semaphores. + /* Disable pre-emption to prevent the worker thread from running + * asynchronously. */ sched_lock(); - flags = irqsave(); /* Now release the semaphore that manages mutually exclusive access to * the device structure. This may cause other tasks to become ready to @@ -312,8 +301,11 @@ static inline int stmpe11_waitsample(FAR struct stmpe11_dev_s *priv, ret = sem_wait(&priv->waitsem); priv->nwaiters--; + /* When we are re-awakened, pre-emption will again be disabled */ + if (ret < 0) { +#ifdef CONFIG_DEBUG // Sample the errno (debug output could change it) int errval = errno; @@ -324,6 +316,7 @@ static inline int stmpe11_waitsample(FAR struct stmpe11_dev_s *priv, idbg("sem_wait failed: %d\n", errval); DEBUGASSERT(errval == EINTR); +#endif ret = -EINTR; goto errout; } @@ -337,13 +330,6 @@ static inline int stmpe11_waitsample(FAR struct stmpe11_dev_s *priv, ret = sem_wait(&priv->exclsem); errout: - /* Then re-enable interrupts. We might get interrupt here and there - * could be a new sample. But no new threads will run because we still - * have pre-emption disabled. - */ - - irqrestore(flags); - /* Restore pre-emption. We might get suspended here but that is okay * because we already have our sample. Note: this means that if there * were two threads reading from the STMPE11 for some reason, the data @@ -542,7 +528,7 @@ static ssize_t stmpe11_read(FAR struct file *filep, FAR char *buffer, size_t len report = (FAR struct touch_sample_s *)buffer; memset(report, 0, SIZEOF_TOUCH_SAMPLE_S(1)); report->npoints = 1; - report->point[0].id = priv->id; + report->point[0].id = sample.id; report->point[0].x = sample.x; report->point[0].y = sample.y; report->point[0].pressure = sample.z; @@ -962,7 +948,7 @@ void stmpe11_tscworker(FAR struct stmpe11_dev_s *priv, uint8_t intsta) else if ((intsta & (INT_FIFO_TH|INT_FIFO_OFLOW)) != 0) { - /* Read the next x and y positions. */ + /* Read the next x and y positions from the FIFO. */ #ifdef CONFIG_STMPE11_SWAPXY x = stmpe11_getreg16(priv, STMPE11_TSC_DATAX); @@ -972,6 +958,25 @@ void stmpe11_tscworker(FAR struct stmpe11_dev_s *priv, uint8_t intsta) y = stmpe11_getreg16(priv, STMPE11_TSC_DATAX); #endif + /* If we have not yet processed the last pen up event, then we + * cannot handle this pen down event. We will have to discard it. That + * should be okay because there will be another FIFO event right behind + * this one. Other kinds of data overruns are not harmful. + * + * Hmm.. a better design might be to disable FIFO interrupts when we + * detect pen up. Then re-enable them when CONTACT_UP is reported. + * That would save processing interrupts just to discard the data. + */ + + if (priv->sample.contact == CONTACT_UP) + { + /* We have not closed the loop on the last touch ... don't report + * anything. + */ + + goto ignored; + } + /* Perform a thresholding operation so that the results will be more stable */ xdiff = x > priv->threshx ? (x - priv->threshx) : (priv->threshx - x); @@ -983,7 +988,8 @@ void stmpe11_tscworker(FAR struct stmpe11_dev_s *priv, uint8_t intsta) if (xdiff + ydiff < 6) { - /* Little or no change in position... don't report */ + /* Little or no change in position ... don't report anything. + */ goto ignored; } diff --git a/include/nuttx/wqueue.h b/include/nuttx/wqueue.h index 5f5fecef89..dfd424c8dc 100644 --- a/include/nuttx/wqueue.h +++ b/include/nuttx/wqueue.h @@ -1,8 +1,8 @@ /**************************************************************************** * include/nuttx/wqueue.h * - * Copyright (C) 2009, 2011 Gregory Nutt. All rights reserved. - * Author: Gregory Nutt + * Copyright (C) 2009, 2011-2012 Gregory Nutt. All rights reserved. + * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -160,6 +160,22 @@ EXTERN int work_cancel(struct work_s *work); #define work_signal() kill(g_worker, SIGWORK) +/**************************************************************************** + * Name: work_available + * + * Description: + * Check if the work structure is available. + * + * Input parameters: + * None + * + * Returned Value: + * true if available; false if busy (i.e., there is still pending work). + * + ****************************************************************************/ + +#define work_available(work) ((work)->worker == NULL) + #undef EXTERN #ifdef __cplusplus }