espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.75k stars 739 forks source link

Bangle.js2: Recycled touch event position with long running timeouts #2374

Closed halemmerich closed 1 year ago

halemmerich commented 1 year ago

Bangle.js 2, 2v17.116, Apps fully updated

Touch events can be lost when there are long running timeouts active, but the old position is then used on the next touch. To reproduce you can run the given code and just touch different parts of the screen. busytime=100 starts to show the problem occasionally, with 200 I see it regularly. Upping the wait time to get free slots for event processing does not seem to help, it just ups the probability of hitting a free slot with the touch where it then works fine.

const wait = 0;
const busytime = 200;

function calc(){
  let time = Date.now();
  do {
    Math.sqrt(123);
  } while (Date.now()-time < busytime);
  setTimeout(calc,wait);
}

Bangle.on("touch",(_,xy)=>{
  g.fillCircle(xy.x,xy.y,3);
});

g.clear();
calc();
gfwilliams commented 1 year ago

Thanks - Well, that's a really odd one. Can reproduce here, but I have no idea why it's happening

d3nd3 commented 1 year ago

Isn't this expected behaviour for a loop? Also what is the notion of 'slots'?

halemmerich commented 1 year ago

This is not really a loop but rather a sequence of load and idle. The wait constant adds additional idle time by waiting longer before running the next load sequence. That idle time is what I called a slot for event processing.

halemmerich commented 1 year ago

I think I found a fix. My suspicion is this is caused by skipped drag events under high load so the lastTouchX/Y are not updated correctly.

d3nd3 commented 1 year ago

Is it reproducible on the emulator? I mean the original issue.

d3nd3 commented 1 year ago

In case that isn't the optimal solution, you could also switch the order of if (bangleTasks & JSBT_DRAG) { and if (bangleTasks & JSBT_TOUCH_MASK) { so that if both events occur on the same frame, the lastTouchX is recent.

Just a thought.

By the way , @gfwilliams could you shed light on this: https://github.com/espruino/Espruino/blob/4402d273e9154108108ccd2d85e6f5fc681fbb4b/libs/banglejs/jswrap_bangle.c#L1780-L1790

Does it mean that all touch events will trigger drag, I assume this code intent, because the touch event uses lastTouchX, which is only set after the DRAG job is performed: https://github.com/espruino/Espruino/blob/4402d273e9154108108ccd2d85e6f5fc681fbb4b/libs/banglejs/jswrap_bangle.c#L4177C1-L4191

#ifdef TOUCH_DEVICE
  if (bangleTasks & JSBT_DRAG) {
    JsVar *o = jsvNewObject();
    jsvObjectSetChildAndUnLock(o, "x", jsvNewFromInteger(touchX));
    jsvObjectSetChildAndUnLock(o, "y", jsvNewFromInteger(touchY));
    jsvObjectSetChildAndUnLock(o, "b", jsvNewFromInteger(touchPts));
    jsvObjectSetChildAndUnLock(o, "dx", jsvNewFromInteger(lastTouchPts ? touchX-lastTouchX : 0));
    jsvObjectSetChildAndUnLock(o, "dy", jsvNewFromInteger(lastTouchPts ? touchY-lastTouchY : 0));
    jsiQueueObjectCallbacks(bangle, JS_EVENT_PREFIX"drag", &o, 1);
    jsvUnLock(o);
    lastTouchX = touchX;
    lastTouchY = touchY;
    lastTouchPts = touchPts;
  }
#endif

If this variable : lastTouchPts = touchPts; represents if a finger is currently touching or not. Does : https://github.com/espruino/Espruino/blob/4402d273e9154108108ccd2d85e6f5fc681fbb4b/libs/banglejs/jswrap_bangle.c#L1796C3-L1802

void touchHandler(bool state, IOEventFlags flags) {
  if (state) return; // only interested in when low
  // Ok, now get touch info
  unsigned char buf[6];
  buf[0]=1;
  jsi2cWrite(TOUCH_I2C, TOUCH_ADDR, 1, buf, false);
  jsi2cRead(TOUCH_I2C, TOUCH_ADDR, 6, buf, true);

"returning when state is low" have anything related to the state of touching? like an ordinary button or is it different because its the touch, so its just saying a data is ready to be read? If it were related to the state of touching then it would interfere negatively with the use of touchPts i believe.

What is your thought on why lastTouchX was used on the touch execute, when its set by the drag execute?

Finally, why is jshHadEvent() only called inside the drag condition area?

I predict that your answer will be that the code inside where DRAG task created is the code for all touch events, that makes the most sense to me. I'm just triple checking.

gfwilliams commented 1 year ago

Thanks for this @halemmerich!

It sure looks like touch should use the current touch coordinates - I'm not sure I understand why it didn't. Maybe it's a hangover from before touchX was added for drag handling.

Does it mean that all touch events will trigger drag

Yes - the idea was that drag was fired basically whenever a finger is on (or lifted) so you don't need two different events - and you can use dx/dy to figure out how much actual dragging there was.

does "returning when state is low" have anything related to the state of touching?

No - that's the state of the IRQ line from the touchscreen controller, so it's just low when data is ready.

Finally, why is jshHadEvent() only called inside the drag condition area?

It is called when the interpreter should wake from sleep and handle an event. If there isn't an event it shouldn't handle it - which jshHadEvent did you mean? The one in https://github.com/espruino/Espruino/blob/4402d273e9154108108ccd2d85e6f5fc681fbb4b/libs/banglejs/jswrap_bangle.c#L1780-L1790 looks correct.

Closing this now as PR is merged

d3nd3 commented 1 year ago

https://github.com/espruino/Espruino/blob/4402d273e9154108108ccd2d85e6f5fc681fbb4b/libs/banglejs/jswrap_bangle.c#L1758-L1791

I still don't understand why the code allows a bangleTasks bit to be set whilst allowing to not get to line 1789 and call jshHadEvent() , what am I missing?

If the jshHadEvent() is not called, do the bits set on bangleTasks get discard/reset?

gfwilliams commented 1 year ago

Ahh, ok, I see what you mean.

Yes, if bangleTasks is set then so should HadEvent, so that looks like a bug - although I'm not sure if it actually shows itself since the touchscreen reports a bunch of events in quick succession, one of which probably fires jshHadEvent anyway. It could do with fixing either way though - I'll get something in for it

d3nd3 commented 1 year ago

Ahh, ok, I see what you mean.

Yes, if bangleTasks is set then so should HadEvent, so that looks like a bug - although I'm not sure if it actually shows itself since the touchscreen reports a bunch of events in quick succession, one of which probably fires jshHadEvent anyway. It could do with fixing either way though - I'll get something in for it

Yes its confusing to read, but has little effect on operation, possibly even 0. If it works, it works, but is always nice to have code that makes more intuitive sense.

edit: ye so the bug would be when single tap in same location, 2nd finger maybe + jshEvent() somewhere else, some rare bug, very small. possibly some timing issue created, with a delayed touch somewhere, if allowed to sleep with a task.