Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRV_TOUCH_ADA_SIMPLE: Multiple touch events for component #96

Closed
rrroonn opened this issue Jan 9, 2019 · 16 comments
Closed

DRV_TOUCH_ADA_SIMPLE: Multiple touch events for component #96

rrroonn opened this issue Jan 9, 2019 · 16 comments
Labels
device_arduino Devices: Arduino compatible
Milestone

Comments

@rrroonn
Copy link

rrroonn commented Jan 9, 2019

I hope its ok to seek advice here as I could not find a suitable alternate contact point or forum?

On arduino using simple touch I am getting repeating events. I am trying to just handle the release event for a touch, but that is confounded by some repeating mechanism. Try as I might, I can't find how to disable or the code responsible.

I have this code (pretty much std) to handle events:

bool CbBtnCommon(void* pvGui,void *pvElemRef,gslc_teTouch eTouch,int16_t nX,int16_t nY)
{
 gslc_tsElemRef* pElemRef = (gslc_tsElemRef*)(pvElemRef);
 gslc_tsElem* pElem = pElemRef->pElem;

 if ( eTouch == GSLC_TOUCH_UP_IN ) {
   // From the element's ID we can determine which button was pressed.
   switch (pElem->nId) {
//<Button Enums !Start!>
     case E_B0:
     case E_B1:
     case E_B2:
     case E_B3:
     case E_B4:
     case E_B5:
     case E_B6:
     case E_B7:
     case E_B8:
     case E_B9:
         {
         addToPassword('0'+(pElem->nId-E_B0)); 
         // need to redraw PIN/PASSWD
         gslc_tsElemRef *pwd =  gslc_PageFindElemById(pvGui, E_PG_PIN, E_PASSWORD);
         if (pwd) gslc_ElemSetRedraw(pvGui, pwd, GSLC_REDRAW_FULL );
         }
         break;     

       [snip]

//<Button Enums !End!>
     default:
       break;
   }
 }
 return true;
}

This is a sample of a single touch & release event on a button:

DBG: Touch Press=109 Raw[788,469] Out[45,251]
 TS : (45,251) Pressure=109 : TouchDown
DBG: Touch Press=25 Raw[786,460] Out[45,256]
 TS : (45,256) Pressure=25 : TouchMove
DBG: Touch Press=0 Raw[786,460] Out[45,256]
 TS : (45,256) Pressure=0 : TouchUp
DBG: Touch Press=224 Raw[788,459] Out[45,256]
 TS : (45,256) Pressure=224 : TouchDown
DBG: Touch Press=224 Raw[787,459] Out[45,256]
DBG: Touch Press=0 Raw[787,459] Out[45,256]
 TS : (45,256) Pressure=0 : TouchUp

The repeating is not as bad when debugging touch, but I guess thats just because of the serial delays.

Is there a bug, or am I missing something?

thank you,

Ron

@ImpulseAdventure
Copy link
Owner

Hi Ron -- thanks for documenting this.

What display and touch controller are you using in your setup?

I have observed that the 4-wire resistive Adafruit_TouchScreen library (used in the DRV_TOUCH_SIMPLE mode) occasionally returns a "0" for touch pressure (Z) during my testing of mcufriend displays, which appears to be similar to the output you are reporting. A momentary result of 0 would be interpreted by GUIslice as an intermittent release.

I would be interested to know if you are using this particular touch mode.

In the case of the Adafruit_TouchScreen, I was intending to debug the getPoint() call to determine why these brief 0 results are returned. In the interim I was thinking it may be worth trying to set ADATOUCH_RX (the rxplate resistance setting) to 0 which should disable some of the Adafruit getPoint() calculations.

@rrroonn
Copy link
Author

rrroonn commented Jan 9, 2019

Yes, its exactly that problem. Just writing up as your comment came in.

Its an issue with TouchScreen.cpp believing that a minor change between two samples invalidates the measurement!
This is how to fix with a hammer (which I did for testing):

#if NUMSAMPLES > 2
   insert_sort(samples, NUMSAMPLES);
#endif
#if NUMSAMPLES == 2
   // Allow small amount of measurement noise, because capacitive
   // coupling to a TFT display's signals can induce some noise.
 //  if (samples[0] - samples[1] < - || samples[0] - samples[1] > 4) {
 //    valid = 0;
//   } else {
     samples[1] = (samples[0] + samples[1]) >> 1; // average 2 samples
//   }
#endif

I really dont like that library, and I previously written my own code to bypass it. I guess the simple fix is to ensure NUMSAMPLES is NOT 2. This is a classic example of a library where you have to modify to fix this behaviour as they have coded in there .cpp file:

#define NUMSAMPLES 2

when something like

#ifndef AF_TOUCH_NUM_SAMPLES
    #define  AF_TOUCH_NUM_SAMPLES 2
#endif 

would allow you to override by defining this before including their library! Sigh.

I did try setting RXPLATE to 0 but this did not change behaviour. I don't know where they get the default 300 value from. I have left it as 0 for now.

Feel free to close, thanks for your input.

thanks,

Ron

@Pconti31
Copy link
Contributor

Pconti31 commented Jan 9, 2019

The 300 represents the resistence in ohms between the two touch pins. If you have a multimeter you can get the actual value, which are most likely higher than 300.

@ImpulseAdventure
Copy link
Owner

ImpulseAdventure commented Jan 9, 2019

Ron -- yes, you are correct in that the touch filtering logic you point out in the Adafruit library is very likely contributing to the issue.

I had a couple concerns with the Adafruit_TouchScreen implementation:

  • The touch release (ie. z=0) actually appears to depend on an integer overflow (div/0) as a result of the rtouch /= z1 line.
  • The noisy read filtering logic (looking for two coordinates within a small deadband) is also returning z=0.
  • The net result is that the return value signature from getPoint() appears to be ambiguous: both the noisy filtered result (during a touch press) and a touch release both give the same return value!
  • Libraries that track changes in getPoint() z value may detect a spurious touch release event, which leads to your observation above.

It doesn't appear that pull requests for Adafruit_TouchScreen are being integrated that often, so we might have to accept it as-is, though I still plan to submit a PR for the library for consideration.

Workaround

Thankfully, I have determined that the library also offers a pressure() call that does not include the filtering logic. This means that upon detecting a getPoint(): z=0, I can perform an additional verification via pressure() to see if there is a continued press. It looks like this might work.

I have pushed a new branch https://github.com/ImpulseAdventure/GUIslice/tree/WIP96-Touch that makes a change to GUIslice to work around the Adafruit_TouchScreen issue. If you have an opportunity to try it out, that would be great.

RX Plate

As for rxplate, yes, Paul is correct -- one can measure resistance across the X+ and X- pins. In my particular mcufriend shield I measured 240 ohms, so clearly these displays have some variability.

Note that when rxplate=0, the Adafruit code would avoid the div/0 issue I noted above, so perhaps one could infer a different return signature from the function. Nonetheless, I think the above workaround might be better, if it works.

@rrroonn
Copy link
Author

rrroonn commented Jan 9, 2019 via email

@rrroonn
Copy link
Author

rrroonn commented Jan 9, 2019

I noticed that you have:

  // ----------------------------------------------------------------
  #elif defined(DRV_TOUCH_ADA_SIMPLE)

  uint16_t  nRawX,nRawY;
  uint8_t  nRawPress;

nRawPress can easily overflow and cause problems, I think it needs to be 16 bits.

Also, have you considered adding a debounce algorithm to your touch events? I may just have a crappy test display, but it was a common one on eBay. It definitively needs some debouncing to make it useful with touch (button) input.
something like ..

is button being touched?
  has it been touched continuously for DEBOUNCE_TIME?
    then fire button TOUCH event
  else wait
else was button TOUCH event fired
  then fire button RELEASE event
end

I did a bit of experimentation and found that a debounce delay of 35-50 mSec is effective - at least for my test display.

It may be difficult in your implementation though.

Something to consider anyway.

thanks.

@ImpulseAdventure
Copy link
Owner

ImpulseAdventure commented Jan 10, 2019

Thanks Ron -- I realized just now that I had forgotten to push my changes to the branch! They should be visible now in WIP96-Touch for you to try with the original Adafruit library if you like. I also fixed the return pressure width that you noted above (though there is still a minor discrepancy b/w uint16/int16).

As for debouncing, my mcufriend displays appeared to function OK without it, but I haven’t specifically tested light touch presses. Nonetheless it seems like a good idea to integrate support in the 4-wire resistive controller mode. I believe I should be able to write a small filter layer that sits between the Adafruit_TouchScreen and the rest of the GUIslice TDrvGetTouch event handler. I’m tempted to write a new simple resistive controller library that integrates basic filtering, debouncing and perhaps calibration mapping... though it seems likely that someone would have already coded up something similar elsewhere.

ImpulseAdventure added a commit that referenced this issue Jan 10, 2019
4wire touch release workaround for #96
@rrroonn
Copy link
Author

rrroonn commented Jan 10, 2019

I have copied down new files and now the display is not working. If I disable touchscreen using
#include "../configs/ard-adagfx-mcufriend-notouch.h"
it works.

Not sure if its something that I have done, so I will investigate further.

@ImpulseAdventure
Copy link
Owner

ImpulseAdventure commented Jan 11, 2019

Thanks for the followup, Ron.

It is a bit odd to hear that the display was no longer working with the WIP96 test branch.

I will retest it in the next hour or two but in the meantime my guess is that you might be observing that the touch controller initialization is returning an error, which currently prevents the display from proceeding.

If you have a chance, can you confirm:

  1. You are using the original / unmodified Adafruit_TouchScreen library (I understand you had previously been using a modified version)

  2. Ensure DEBUG_ERR is enabled and the Serial Monitor is active. Confirm ard-adagfx-mcufriend-simple results in no display and that no error messages are reported on the Serial Monitor?

Thanks!

UPDATE: I have retested ard-adagfx-mcufriend-simple with my display and can't reproduce the issue, so hopefully we can narrow it down to something else that may have changed. thx.

ImpulseAdventure added a commit that referenced this issue Jan 11, 2019
- Can re-enable workaround with FIX_4WIRE
@ImpulseAdventure
Copy link
Owner

@rrroonn -- I should have probably clarified earlier: when you indicated that the mcufriend "display is not working", did you mean that the display was visible but that the touch presses were not registering correctly, or that nothing was displayed (ie. white screen)? If nothing showed on the display, then my previous comments apply. If you were seeing the display but the touch releases weren't working then it might match what I'm observing:

With the 4-wire workaround in place, I have noticed on one of my mcufriend displays that the touch releases didn't always seem to render as expected (sometimes it left controls visibly selected), but it could be due to the fact I haven't recalibrated my display yet.

Fix Reverted

Until I have had a chance to recalibrate and confirm that it is working correctly, I have now reverted the 4-wire touch pressure workaround in the latest repository, so it should behave as before. Perhaps you could retry with the latest if/when you have an opportunity.

The 4-wire workaround can still be applied in this release with the temporary config flag: #define FIX_4WIRE

Pin Cleanup

One thing that came to mind is that the Adafruit_TouchScreen sometimes leaves pins in a bad state, impacting displays that share pins (like some mcufriend TFTs). Therefore, the FIX_4WIRE additional pressure() call may need some additional pin cleanup for it to work cleanly.

Thanks!

@ImpulseAdventure ImpulseAdventure changed the title Multiple touch events for component DRV_TOUCH_ADA_SIMPLE: Multiple touch events for component Jan 11, 2019
@rrroonn
Copy link
Author

rrroonn commented Jan 11, 2019 via email

@rrroonn
Copy link
Author

rrroonn commented Jan 11, 2019 via email

@ImpulseAdventure
Copy link
Owner

Excellent... thank you very much for looking into it, Ron! That matches what I anticipated (re: the Pin Cleanup comment above). I don’t think I noticed your attachment in the issue, so I may try to save/restore the pin states as you note and push those changes later (unless you see this in time and can re-attach the code you tested). For now, I’m OK with the simple workaround, but later may revisit it when I investigate the debouncing feature.

@rrroonn
Copy link
Author

rrroonn commented Jan 12, 2019

Re-added to comment.

ImpulseAdventure added a commit that referenced this issue Jan 12, 2019
- Re-enabled touch press workaround #96 (FIX_4WIRE)
@ImpulseAdventure
Copy link
Owner

Thank you Ron. I have now integrated your code (and only applied minor changes for consistency). Looks good in my testing so far.

@ImpulseAdventure
Copy link
Owner

Closing as I believe the original issue (premature touch-release from Adafruit_Touchscreen filtering) has been resolved. Feature suggestion for debounce filtering has been moved to #102

@ImpulseAdventure ImpulseAdventure added this to the 0.11.0 milestone Feb 12, 2019
@ImpulseAdventure ImpulseAdventure added the device_arduino Devices: Arduino compatible label Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
device_arduino Devices: Arduino compatible
Projects
None yet
Development

No branches or pull requests

3 participants