-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix issue when missing motion sensor was crashing instead of panicking. #360
Conversation
b20ffb1
to
c8a3d39
Compare
Build diffBase commit: 8f317c4337c6596c0e78abcb8f7a5786125b1c98
|
MicroBitAccelerometer & MicroBitCompass instances were never created, so the methods that checked for a null pointer to throw the panic weren't being executed.
c8a3d39
to
47cf1e1
Compare
Build diffBase commit: 8f317c4337c6596c0e78abcb8f7a5786125b1c98
|
* In essence, the LSM needs at least 6.4ms from power-up before we can use it. | ||
* https://github.com/microbit-foundation/codal-microbit/issues/33 | ||
*/ | ||
target_wait(10); |
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.
If the MicroBitAccelerometer::autoDetect()
already adds a wait, there is no need to wait here as well, which why it was removed.
/* | ||
* In essence, the LSM needs at least 6.4ms from power-up before we can use it. | ||
* https://github.com/microbit-foundation/codal-microbit/issues/33 | ||
*/ | ||
target_wait(10); | ||
|
||
// Add pullup resisitor to IRQ line (it's floating ACTIVE LO) | ||
irq1.getDigitalValue(); | ||
irq1.setPull(PullMode::Up); | ||
irq1.setActiveLo(); |
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.
Moved inside the if (!autoDetectCompleted)
to only wait and configure the GPIO once.
@microbit-carlos Maybe I'm not understanding this, but I didn't think CODAL uBit.accelerometer was ever intended to point to MicroBitAccelerometer. The fall back should be to the base class, codal::Accelerometer, with the panic in Accelerometer::requestUpdate. I thought MicroBitAccelerometer was retained in CODAL for compatabilty with code not using uBit. |
Yes, but we'd have to change the generic base class |
@JohnVidler @finneyj with a faulty motion sensor in the current CODAL we end up with a null pointer, so we when trying to execute methods of that instance the micro:bit crashes into an Arm exception handler. We could fix that by creating instances of one of these two options: This PR, for option b), instantiates On the other hand we can move the panic to the base class
|
Ah, I see. I was looking at how it's done in the DAL. If there might be other targets with Accelerometer subclasses that don't override requestUpdate, could a very simple subclass of Accelerometer, which just implements the panic in requestUpdate be a better alternative to MicroBitAccelerometer. I didn't understand why a panic was added to every MicroBitAccelerometer function |
This PR removes all the other methods from MicroBitAccelerometer and MicroBitCompass to only have the Yes, in that older version of the driver it was creating an instance of the base class Accelerometer: codal-microbit-v2/source/MicroBitAccelerometer.cpp Lines 92 to 93 in f974d92
But this older version was not throwing a panic when there was a faulty sensor, so things like uBit.accelerometer.getX() were simply returning zero.
|
Yes, sorry @microbit-carlos, I hadn’t understood the result of the changes. |
Thanks for raising these questions Martin! They are good for the conversation and ensuring we figure out the best approach 👍 |
In summary, when a motion sensor was not working, the
autoDetect()
function was returning a reference to a null pointer.In this case it looks like the compiler must have been automatically generating the
codal::Accelerometer
andcodal::Compass
copy constructors, and instances of those generic parent classes would end up asuBit.accelerometer
&uBit.compass
.With this PR we are returning an instance of
MicroBitAccelerometer
andMicroBitCompass
, capable to throw a panic on first usage. Instances of these classes were never included in the builds before, which explains the large size increase for this PR.The simplest update to these classes increased the build by almost 1KB, but the PR has been slimmed down to try to keep the build small. We'll see in the GitHub Actions PR comment the final size for this version.
I've also found a few places were we can reduce some memory consumption in codal-core, but they need independent review to ensure they don't have any unintended ramifications:
configure()
&requestUpdate()
pure virtual. codal-core#162AbstractButton::isPressed()
pure virtual. codal-core#163NVMController
pure virtual. codal-core#164Fixes #213.