espruino / Espruino

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

Missing `'accel'` events #2319

Closed notEvil closed 1 year ago

notEvil commented 1 year ago

Hi,

I found that 'accel' events are sometimes skipped. On my Bangle.js 1, after "Resetting without loading any code", the following prints 16ms at (semi-)regular intervals (cnt ~= 60).

var pret = 0;
var cnt = 0;

//Bangle.setCompassPower(1);

Bangle.on('accel', (a) => {
  var t = getTime();
  if (0.088 < (t - pret)) {
    Terminal.println((t - pret) + ' ' + cnt);
    cnt = 0;
  }
  pret = t;
  cnt++;
});

'mag' is fine.

gfwilliams commented 1 year ago

I don't think that's anything to worry about. As I understand it, we poll the accelerometer for accelerometer events (rather than responding to an IRQ line). The accelerometer and Bangle clocks aren't in exact sync so every so often there is drift and when the Bangle checks for an event there isn't one there (hence the slightly longer than normal gap).

Is this causing you any problems?

notEvil commented 1 year ago

Atm I'm just recording and hoped for consistent intervals so aligning the sequence with other sequences (e.g. mag and from Android) would be trivial (no time, insignificant offset/drift, ...).

So assuming its just a clock difference, all I need is a poll interval that is slightly faster than the acc so it wouldn't miss by chance. Took me a while to realize that, thanks :)

notEvil commented 1 year ago

Almost ... while peripheralPollHandler is triggered at almost repeatable intervals, jswrap_banglejs_idle isn't.

I've changed peripheralPollHandler to call a native function if defined, and added a function to set the native function (using jsvIsNativeFunction and jsvGetNativeFunctionPtr), to confirm this and get to the values. When JS is "busy", 'accel' will skip some measurements for certain. Would you consider adding a native callback to poll? I would prepare a PR

notEvil commented 1 year ago

Hi again, sry for the spam

I feel close to a working solution, and it does work for many minutes (~25) before it breaks. @fanoush helped me with a strange issue at https://forum.espruino.com/conversations/383232/ which might still be the reason. But I can't rule out the changes I did for this issue.

This is what I've changed:

based on fa34cf78f381572f39b0ff8200eb0f5ef2999d30

```diff diff --git a/libs/banglejs/jswrap_bangle.c b/libs/banglejs/jswrap_bangle.c index f8e3bf3..971a14a 100644 --- a/libs/banglejs/jswrap_bangle.c +++ b/libs/banglejs/jswrap_bangle.c @@ -1103,6 +1103,24 @@ void jswrap_banglejs_kickPollWatchdog() { } #ifndef EMULATED + +void* pollInterrupt; + +/*JSON{ + "type" : "staticmethod", + "class" : "Bangle", + "name" : "setPollInterrupt", + "generate" : "jswrap_banglejs_setPollInterrupt", + "params" : [ + ["func", "JsVar", ""] + ], + "ifdef" : "BANGLEJS" +}*/ +bool jswrap_banglejs_setPollInterrupt(JsVar* func) { + pollInterrupt = jsvIsNativeFunction(func) ? jsvGetNativeFunctionPtr(func) : NULL; + return pollInterrupt; +} + /* Scan peripherals for any data that's needed * Also, holding down both buttons will reboot */ void peripheralPollHandler() { @@ -1182,8 +1200,8 @@ void peripheralPollHandler() { i2cBusy = true; unsigned char buf[7]; // check the magnetometer if we had it on + bool hasMagData = false; if (bangleFlags & JSBF_COMPASS_ON) { - bool newReading = false; #ifdef MAG_DEVICE_GMC303 buf[0]=0x10; jsi2cWrite(MAG_I2C, MAG_ADDR, 1, buf, false); @@ -1192,7 +1210,7 @@ void peripheralPollHandler() { mag.y = buf[1] | (buf[2]<<8); mag.x = buf[3] | (buf[4]<<8); mag.z = buf[5] | (buf[6]<<8); - newReading = true; + hasMagData = true; } #endif #ifdef MAG_DEVICE_UNKNOWN_0C @@ -1210,10 +1228,10 @@ void peripheralPollHandler() { buf[0]=0x3E; jsi2cWrite(MAG_I2C, MAG_ADDR, 1, buf, false); jsi2cRead(MAG_I2C, MAG_ADDR, 1, buf, true); - newReading = true; + hasMagData = true; } #endif - if (newReading) { + if (hasMagData) { if (mag.x

The approach is similar to setWatch(..., {irq: true}), without touching the ref count. And thats how I use it:

```js var c = E.compiledC(` // bool initialize(int, int, int) // void on_poll(bool, bool, int) // int get_acci() // int get_magi() short* accs = 1; short* mags = 1; unsigned char size = 1; unsigned char acci = 1; unsigned char magi = 1; bool initialize(short* acc_buffer, short* mag_buffer, unsigned char s) { if (!(acc_buffer && mag_buffer && (s % 3) == 0)) { return false; } accs = acc_buffer; mags = mag_buffer; size = s; acci = 0; magi = 0; return true; } void on_poll(bool acc, bool mag, short* data) { if (acc) { accs[acci] = data[0]; accs[acci + 1] = data[1]; accs[acci + 2] = data[2]; acci = (acci + 3) == size ? 0 : (acci + 3); } if (mag) { mags[magi] = data[3]; mags[magi + 1] = data[4]; mags[magi + 2] = data[5]; magi = (magi + 3) == size ? 0 : (magi + 3); } } unsigned char get_acci() { return acci; } unsigned char get_magi() { return magi; } `); var SIZE = 14*3; var accs = new Int16Array(SIZE); var mags = new Int16Array(SIZE); var acci = 0; var magi = 0; var poll_id; if (c.initialize(E.getAddressOf(accs, true), E.getAddressOf(mags, true), SIZE)) { Bangle.setPollInterrupt(c.on_poll); poll_id = setInterval(() => { var i = c.get_acci(); while (acci != i) { on_accel({x: accs[acci], y: accs[acci + 1], z: accs[acci + 2]}); acci += 3; if (acci == SIZE) { acci = 0; } } i = c.get_magi(); while (magi != i) { on_mag({x: mags[magi], y: mags[magi + 1], z: mags[magi + 2]}); magi += 3; if (magi == SIZE) { magi = 0; } } }, 80); } ```

Before the "fix" (initializing with 1), it would randomly alter magi and therefore lead to corrupted memory and occasionally a hard reset. Now it just stopped calling on_poll/changing acci. I run process.memory() every 5s to log memory consumption, if thats important.

gfwilliams commented 1 year ago

Would you consider adding a native callback to poll?

Afraid not - native functions called from IRQs have all kinds of issues - you can't access JS vars in a reliable way so it ends up being a nightmare for all but the case where you have only assembler/inline C and don't call any other functions.

Also, what happens normally is that the poll handler runs, posts an event and the second it exits the IRQ, Espruino starts in the main thread and processes that event. If that isn't happening, it'll be because some JS code is running on Espruino already and taking a while to run, so it's possible that you could fix this just by ensuring that all the JS functions executed quickly.

There's also the issue that you're wanting to compare with the magnetometer and Android but I think that's handled in exactly the same way in the poll handler, queueing events - so everything should still be processed in basically the right order.

I guess one option to handle this without making things unreliable might be to add a timestamp to the accelerometer events? I don't think the accelerometer used on the Bangle has one built in but I guess the poll handler could add one.

... but then you know the accelerometer events happen 12.5 times a second anyway, so I'm not sure you really need a timestamp?

notEvil commented 1 year ago

The limitations of native functions aren't a big deal in this case because its not about realtime JS but consistent readouts. To make JS functions quick so the events would be processed in time is virtually impossible outside a perfectly controlled environment. I just saw the health event causing a lag (the app "Health Tracking" does a lot to be fair).

Another angle would be to change accHistory to int16_t and provide access to it (accHistoryIdx is already there in Bangle.dbg). Don't know why I didn't consider this previously ...

notEvil commented 1 year ago
```diff diff --git a/libs/banglejs/jswrap_bangle.c b/libs/banglejs/jswrap_bangle.c index f8e3bf3..8907441 100644 --- a/libs/banglejs/jswrap_bangle.c +++ b/libs/banglejs/jswrap_bangle.c @@ -796,7 +796,8 @@ int accMagSquared; unsigned int accDiff; /// History of accelerometer readings -int8_t accHistory[ACCEL_HISTORY_LEN*3]; +#define ACC_HISTORY_ARRAY_LEN (ACCEL_HISTORY_LEN*3) +int16_t accHistory[ACC_HISTORY_ARRAY_LEN]; /// Index in accelerometer history of the last sample volatile uint8_t accHistoryIdx; /// How many samples have we been recording a gesture for? If 0, we're not recoding a gesture @@ -1323,10 +1324,10 @@ void peripheralPollHandler() { accMagSquared = acc.x*acc.x + acc.y*acc.y + acc.z*acc.z; accDiff = int_sqrt32(dx*dx + dy*dy + dz*dz); // save history - accHistoryIdx = (accHistoryIdx+3) % sizeof(accHistory); - accHistory[accHistoryIdx ] = clipi8(newx>>7); - accHistory[accHistoryIdx+1] = clipi8(newy>>7); - accHistory[accHistoryIdx+2] = clipi8(newz>>7); + accHistoryIdx = (accHistoryIdx+3) % ACC_HISTORY_ARRAY_LEN; + accHistory[accHistoryIdx ] = newx; + accHistory[accHistoryIdx+1] = newy; + accHistory[accHistoryIdx+2] = newz; // Power saving if (bangleFlags & JSBF_POWER_SAVE) { if (accDiff > POWER_SAVE_MIN_ACCEL) { @@ -1481,6 +1482,48 @@ void peripheralPollHandler() { i2cBusy = false; } +/*JSON{ + "type" : "staticmethod", + "class" : "Bangle", + "name" : "getAccHistory", + "generate" : "jswrap_banglejs_getAccHistory", + "params" : [ + ["index","int",""] + ], + "return" : ["JsVar",""], + "ifdef" : "BANGLEJS" +}*/ +JsVar* jswrap_banglejs_getAccHistory(int index) { + if (!(0 <= index && index < ACC_HISTORY_ARRAY_LEN && (index % 3) == 0)) { + return 0; + } + + JsVar* o = jsvNewObject(); + if (o) { + JsVar* array = jsvNewEmptyArray(); + if (array) { + int end_index = accHistoryIdx; + + while (index != end_index) { + JsVar* acc = jsvNewObject(); + if (acc) { + jsvObjectSetChildAndUnLock(acc, "x", jsvNewFromInteger(accHistory[index])); + jsvObjectSetChildAndUnLock(acc, "y", jsvNewFromInteger(accHistory[index + 1])); + jsvObjectSetChildAndUnLock(acc, "z", jsvNewFromInteger(accHistory[index + 2])); + jsvArrayPushAndUnLock(array, acc); + } + index = (index + 3) % ACC_HISTORY_ARRAY_LEN; + } + + jsvObjectSetChildAndUnLock(o, "hist", array); + jsvObjectSetChildAndUnLock(o, "idx", jsvNewFromInteger(end_index)); + } + } + + return o; +} + + #ifdef HEARTRATE static void hrmHandler(int ppgValue) { if (hrm_new(ppgValue)) { @@ -3946,13 +3989,13 @@ bool jswrap_banglejs_idle() { JsVar *arr = jsvNewTypedArray(ARRAYBUFFERVIEW_INT8, accGestureRecordedCount*3); if (arr) { int idx = accHistoryIdx - (accGestureRecordedCount*3); - while (idx<0) idx+=sizeof(accHistory); + while (idx<0) idx+=ACC_HISTORY_ARRAY_LEN; JsvArrayBufferIterator it; jsvArrayBufferIteratorNew(&it, arr, 0); for (int i=0;i>7)); jsvArrayBufferIteratorNext(&it); - if (idx>=(int)sizeof(accHistory)) idx-=sizeof(accHistory); + if (idx>=ACC_HISTORY_ARRAY_LEN) idx-=ACC_HISTORY_ARRAY_LEN; } jsvArrayBufferIteratorFree(&it); jsiQueueObjectCallbacks(bangle, JS_EVENT_PREFIX"gesture", &arr, 1); @@ -3989,11 +4032,11 @@ bool jswrap_banglejs_idle() { JsvArrayBufferIterator it; jsvArrayBufferIteratorNew(&it, v, 0); int idx = accHistoryIdx - (accGestureRecordedCount*3); - while (idx<0) idx+=sizeof(accHistory); + while (idx<0) idx+=ACC_HISTORY_ARRAY_LEN; for (int i=0;i>7)); jsvArrayBufferIteratorNext(&it); - if (idx>=(int)sizeof(accHistory)) idx-=sizeof(accHistory); + if (idx>=ACC_HISTORY_ARRAY_LEN) idx-=ACC_HISTORY_ARRAY_LEN; } jsvArrayBufferIteratorFree(&it); jsvUnLock(v); diff --git a/libs/banglejs/jswrap_bangle.h b/libs/banglejs/jswrap_bangle.h index 7402e87..ade9c4a 100644 --- a/libs/banglejs/jswrap_bangle.h +++ b/libs/banglejs/jswrap_bangle.h @@ -27,6 +27,7 @@ void jswrap_banglejs_setLocked(bool isLocked); int jswrap_banglejs_isLocked(); void jswrap_banglejs_setPollInterval(JsVarFloat interval); +JsVar* jswrap_banglejs_getAccHistory(int index); void jswrap_banglejs_setOptions(JsVar *options); JsVar *jswrap_banglejs_getOptions(); int jswrap_banglejs_isCharging(); ```

:) I will test this the next days and see if there are any issues. You can add it if you want, but I don't mind carrying this forward in a fork if you decide not to. Thanks for the support!

gfwilliams commented 1 year ago

Thanks - I think for now a fork might be best, and we see how it works for you.

Closing this issue for now as it appears it's just an issue with processing things at the right time