Bangle.getPressure() fails intermittently

[d3nd3]

EDIT: This code was initially shared without the (function())() wrapping, so the promise callbacks were acting weird due to line by line upload of the webIDE. I think the test has to be re-ran to validate how much errors we got. (Potentially None.) Latest undefined returns from 2v20 = 3/100 attempts.

My firmware = 2v20

let sleepProm = function(delay) {
  return new Promise((resolve) => {
    setTimeout(()=> {
      resolve();
    },delay);
  });
};

(function() {

let pressurePromise = Bangle.getPressure();
for (let i = 0; i < 60; i++) {
  pressurePromise = pressurePromise.then((data)=>{
    if ( typeof(data) === "undefined" )
      print("undefined");
    else console.log(data);
    return sleepProm(500).then(()=>{
      return Bangle.getPressure();
    });
  });
}
})();

It doesn't error nearly as much, if correctly ran from file or wrapped in (function(){ })() for RAM.

[nxdefiant]

Can reproduce.

[thyttan]

Related I think:

• https://github.com/espruino/Espruino/pull/2438
• https://github.com/espruino/BangleApps/pull/3104#issuecomment-1868152281

[bobrippling]

Potential duplicate of https://github.com/espruino/BangleApps/issues/3086 too

I wonder whether the change which makes pressure errors more explicit has made this more visible, or introduced a regression.

How does that code behave on a pre-v20 firmware?

[d3nd3]

I have the "SPL07" model barometer, from Bangle.barometerRd(0x0D) == 16

[nxdefiant]

Same barometer model.

I see the errors also with the 2v19 firmware:

{ "temperature": 21.47023264567, "pressure": 1008.37144546888, "altitude": 40.69670611543 } undefined Uncaught Error: Unhandled promise rejection: Error: Unhandled promise rejection: undefined in function called from system { "temperature": 21.46512959798, "pressure": 1008.38661297299, "altitude": 40.56993258807 }

[d3nd3]

By commenting out:

if (bangleFlags & JSBF_BAROMETER_ON) {
    if (jswrap_banglejs_barometerPoll()) {
      bangleTasks |= JSBT_PRESSURE_DATA;
      jshHadEvent();
    }
  }

within peripheralPollHandler(), there is no longer the error. So my feeling is that it sometimes is called on top of each other, creating 2 requests or something, once in peripheralPollHandler() because BAROMETER is turned ON, and once in jswrap_banglejs_getPressure_callback promise, by jswrap_banglejs_getPressure. The fact it uses a promise could be partly related, we have to disable the peripheralPollHandler() call to jswrap_banglejs_barometerPoll() whilst the getPressure() request is occuring it seems. The function jswrap_banglejs_barometerPoll() performs a request(write), then a read.

    buf[0] = SPL06_PRSB2; jsi2cWrite(PRESSURE_I2C, PRESSURE_ADDR, 1, buf, false);
    jsi2cRead(PRESSURE_I2C, PRESSURE_ADDR, 6, buf, true);

Ye, this is a clue. I don't have enough understanding to describe fully, hopefully someone can fill in the blanks and come up with a good solution? If we take the accel as an example, it gets its data in one place within the pollHandler() and shares the data to getAccel(). I wonder if we could simply do the same here, instead of re-talking to the hardware again?

[d3nd3]

The perfect variable that could be used for this is :

https://github.com/espruino/Espruino/blob/c1824f087b2618fb53711482c49323da4b6e4d67/libs/banglejs/jswrap_bangle.c#L5054

if (bangleFlags & JSBF_BAROMETER_ON && !promisePressure) {
    if (jswrap_banglejs_barometerPoll()) {
      bangleTasks |= JSBT_PRESSURE_DATA;
      jshHadEvent();
    }
  }

This could work. I will give it a try.

[d3nd3]

It works, but here is another way to think about the current code.

When the barometer is ON, we are fetching the value inside the peripheralPollHandler(). If we turn the barometer ON inside of Bangle.getPressure() Which we do AND we are using timeout too ( so its not that fast ). I beg the question, why don't we just do what the streaming code does, and use the value that was fetched during the periPollHandler?

Is it because it returns false not ready, so you are worried the data isn't fresh enough? I think with the polling rate its fresh enough.. and would save a lot of this problems.

So I have 2 solutions:

1. The above code with the && !promisePressure .... OR
2. Don't call jswrap_banglejs_barometerPoll when calling Bangle.getPressure(), resolve the promise within the jswrap_banglejs_idle() at the same location streaming Barometer data is set.

https://github.com/espruino/Espruino/blob/c1824f087b2618fb53711482c49323da4b6e4d67/libs/banglejs/jswrap_bangle.c#L4241

Something like:

  if (bangleTasks & JSBT_PRESSURE_DATA) {
    JsVar *o = jswrap_banglejs_getBarometerObject();
    if (o) {
      jsiQueueObjectCallbacks(bangle, JS_EVENT_PREFIX"pressure", &o, 1);
      jsvUnLock(o);

      if ( promisePressure ) {
        // disable sensor now we have a result
        JsVar *id = jsvNewFromString("getPressure");
        jswrap_banglejs_setBarometerPower(0, id);
        jsvUnLock(id);
        // resolve the promise
        jspromise_resolve(promisePressure, o);
        jsvUnLock(promisePressure);
        promisePressure = 0;
      }
    }
  }

[d3nd3]

See compiled artifacts here. https://github.com/d3nd3/Espruino/actions/workflows/build.yml

Feel free to test the changes of that PR here. It still uses the timeout, to preserve the startup delay that is apparently required.

[d3nd3]

Ok, I just realized something. The initial test/demo I had in first post did not contain any function wrap around it, so it suffered from the timing issue from uploading via RAM. Its possible our "testing" was biased.

Could a few of you re-test the initial "pretense/claim" with normal v20 firmware?.

That would explain why : https://forum.espruino.com/comments/17251740/

claims it is fixed for them.

It seems the errors were not as apparent as we thought. However my code does fix an edge case where an expired value is returned from getPressure() due to using Bangle.setBarometerPower(1) just before calling Bangle.getPressure(), where it doesn't give it time to 'startup'.

[nxdefiant]

Thats my post. I believe your issue is slightly different from the one in the forum post, at least from the symptoms. With the original issue the only way to recover is to write reset to the SPL06 and restart the Bangle afterwards.

For your issue with firmware .19 and .20 a single restart is enough.

I see no change when running from storage.

Btw: Have you also seen this?

{ "temperature": 31.22230529785, "pressure": 1000.36244646234, "altitude": 107.85447947598 } { "temperature": 99.00003306070, "pressure": 1005.86008057821, "altitude": 61.70860231534 } Uncaught undefined

Thank you for your work on this, I will try to check that later.

[d3nd3]

So I ran a test using a counter each time its undefined, I got 3 times out of 100 calls with 2v20.

In my pull request, it produced 0 undefined, which is expected because the promise is not resolved until jswrap_banglejs_barometerPoll() returns true, and it retries every idle loop, just like the streaming method does.

So, even though the error rate is drastically lower than first pointed out, its probably better to use my PR.

[gfwilliams]

Thanks for this! Sorry it's taken a while to get round to this but I wanted to take some time to look into it fully.

Great spot about the interference with peripheralPollHandler! Swapping to use just that to read sounds much better.

Bangle.on("pressure") triggers during the temporary activation during Bangle.getPressure().

Yes, having this fire sounds good - if you're registered to get pressure readings you might as well get them all.

Looking at the code, I think some small changes would be really nice to take the opportunity to clean the existing code up:

• Make pressureOnSince into pressureOnTime - storing just the time that the pressure sensor has been on (this gets around issues if the system time gets changed while we're reading, and matches what we do for other stuff like the wake counters)
• Try and get rid of the timeout for getPressure completely (since it only sets getPressureReady). I think we can just see if (promisePressure && pressureOnTime>powerOnTimeout)
• Make getPressure always wait for peripheralPollHandler to resolve the promise - so you always get the 'latest' data, not the last. It also removes some duplicate code.

But I'll look into doing this now...

[d3nd3]

I sort of repeated what you wrote, but ye... In which case, the 'delay' from turn on, for the SPL06_007_EN would not be needed anymore, not sure about the other devices. I mean, jswrap_banglejs_barometerPoll is returning false, which should cover the case when its not ready? Because on setBarometerPower() ... if (isOn) bangleFlags |= JSBF_BAROMETER_ON; is set, meaning BarometerPoll is fired in peripheralPollHandler().

If you are still with me. TLDR: Yes, with the polling strategy, we should not need the delay at all, because its no longer a one-shot trick.

[gfwilliams]

In which case, the 'delay' from turn on, for the SPL06_007_EN would not be needed anymore

I'm not sure? I think the concern is that right after you turn it on, it might well say it's ready, but the data in it is wrong/stale.

So you can imagine that you turn the sensor on but pretty much immediately peripheralPollHandler runs and pulls out a reading?

Adding the delay in peripheralPollHandler might also stop potentially dodgy readings being put in the 'on'... handler too

[d3nd3]

Ye, I probably am wrong with that one. Nevermind then. I forgot about the getPressureReady boolean check I had in place.

[gfwilliams]

Thanks for your work on this - just merged!