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
Bangle.getPressure() fails intermittently #2445
Comments
Can reproduce. |
Potential duplicate of espruino/BangleApps#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? |
I have the "SPL07" model barometer, from |
Same barometer model. I see the errors also with the 2v19 firmware:
|
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 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? |
The perfect variable that could be used for this is : Espruino/libs/banglejs/jswrap_bangle.c Line 5054 in c1824f0
if (bangleFlags & JSBF_BAROMETER_ON && !promisePressure) {
if (jswrap_banglejs_barometerPoll()) {
bangleTasks |= JSBT_PRESSURE_DATA;
jshHadEvent();
}
} This could work. I will give it a try. |
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(). 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:
Espruino/libs/banglejs/jswrap_bangle.c Line 4241 in c1824f0
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;
}
}
} |
See compiled artifacts here. Feel free to test the changes of that PR here. |
Ok, I just realized something. Could a few of you re-test the initial "pretense/claim" with normal v20 firmware?. That would explain why : 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 |
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?
Thank you for your work on this, I will try to check that later. |
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. |
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.
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:
But I'll look into doing this now... |
I sort of repeated what you wrote, but ye... If you are still with me. |
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 |
Ye, I probably am wrong with that one. Nevermind then. I forgot about the getPressureReady boolean check I had in place. |
…alPollHandler won't try and access barometer while we're initialising Full info: #2445 (comment)
Thanks for your work on this - just merged! |
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
It doesn't error nearly as much, if correctly ran from file or wrapped in (function(){ })() for RAM.
The text was updated successfully, but these errors were encountered: