-
-
Notifications
You must be signed in to change notification settings - Fork 754
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
getPressure: handle error when we don't have an exception #2438
getPressure: handle error when we don't have an exception #2438
Conversation
JsVar *msg = jsvNewFromString("I2C barometer error"); | ||
JsVar *exception = jswrap_internalerror_constructor(msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I have the locking correct here - jswrap_internalerror_constructor
unlocks msg
, but would appreciate a second pair of eyes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think jswrap_internalerror_constructor
does unlock msg - if it's a function called from JS (which I'm pretty sure it is as it's the constructor) then the caller is responsible for unlocking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might've misread the code - I think here it unlocks in jsvObjectSetChildAndUnLock
Lines 51 to 62 in deedeaf
JsVar *_jswrap_error_constructor(JsVar *msg, char *type) { | |
JsVar *d = jspNewObject(0,type); | |
if (!d) return 0; | |
if (msg) { | |
msg = jsvAsString(msg); | |
jsvObjectSetChildAndUnLock(d, "message", msg); | |
} | |
jsvObjectSetChildAndUnLock(d, "type", jsvNewFromString(type)); | |
return d; | |
} |
(⬆️ msg
here)
Lines 3195 to 3198 in deedeaf
void jsvObjectSetChildAndUnLock(JsVar *obj, const char *name, JsVar *child) { | |
jsvObjectSetChild(obj, name, child); | |
jsvUnLock(child); | |
} |
(⬆️ child
here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh - jsvObjectSetChildAndUnLock(d, "message", msg);
does unlock msg
but on the line above, msg = jsvAsString(msg);
replaces msg
with a newly locked version of msg
that's guaranteed to be a string. I guess for readability it should really be written to a new variable instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense - I could make that change as part of this PR if you like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's ok - i'll do it separately now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but you still need to unlock msg
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes - I was assuming jsvAsString
unlocked the value it was given, with us replacing it. Thanks for the pointer!
Looks like there's a build error here as well? https://github.com/espruino/Espruino/actions/runs/7112131428/job/19361472245?pr=2438 edit: ahh - you made a comment about this above |
deedeaf
to
a1bc1f9
Compare
This is ready for another look btw :) |
Thanks - just looked again and this looks good! Merging now! |
This handles an I2C error, originally ocurring in #2137, found again by chance in an unrelated PR.
When we create a promise for
getPressure
, we now look at bothjspGetException
and thenjspHasError
(separately).If the former is null but we have an error, we raise an exception about the I2C communication with the barometer.
Previous behaviour would raise an exception with no payload, leading to the
Uncaught Error: Unhandled promise rejection: undefined
seen in the BangleApps pull request above.