Fix issue when missing motion sensor was crashing instead of panicking.

[microbit-carlos]

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 and codal::Compass copy constructors, and instances of those generic parent classes would end up as uBit.accelerometer & uBit.compass.

With this PR we are returning an instance of MicroBitAccelerometer and MicroBitCompass, 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:

• 152 bytes: https://github.com/lancaster-university/codal-core/pull/162
• 40 bytes: https://github.com/lancaster-university/codal-core/pull/163
• 184 bytes: https://github.com/lancaster-university/codal-core/pull/164

Fixes https://github.com/lancaster-university/codal-microbit-v2/issues/213.

[github-actions[bot]]

Build diff

Base commit: 8f317c4337c6596c0e78abcb8f7a5786125b1c98 Action run: https://github.com/lancaster-university/codal-microbit-v2/actions/runs/5724678688

     VM SIZE    
 -------------- 
   +67%    +185    /home/runner/work/codal-microbit-v2/codal-microbit-v2/libraries/codal-microbit-v2/source/MicroBitAccelerometer.cpp
  +0.6%    +128    [section .text]
  +350%    +112    /home/runner/work/codal-microbit-v2/codal-microbit-v2/libraries/codal-microbit-v2/source/MicroBitCompass.cpp
 -34.0%     -33    [section .bss]
  +0.1%    +392    TOTAL

[github-actions[bot]]

Build diff

Base commit: 8f317c4337c6596c0e78abcb8f7a5786125b1c98 Action run: https://github.com/lancaster-university/codal-microbit-v2/actions/runs/5725328513

     VM SIZE    
 -------------- 
   +72%    +201    /home/runner/work/codal-microbit-v2/codal-microbit-v2/libraries/codal-microbit-v2/source/MicroBitAccelerometer.cpp
  +400%    +128    /home/runner/work/codal-microbit-v2/codal-microbit-v2/libraries/codal-microbit-v2/source/MicroBitCompass.cpp
  +0.6%    +128    [section .text]
 -34.0%     -33    [section .bss]
  +0.1%    +424    TOTAL

[martinwork]

@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.

[microbit-carlos]

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 codal::Accelerometer & codal::Compass to panic in the requestUpdate and configure() methods, which might have an effect in non-micro:bit targets, and it's not a pattern used in any of the other CODAL base classes. I was also under the impression that the CODAL generic error codes and the micro:bit specific 50 & 51 were clashing or could clash in different targets, which I thought it's why they were added to the MicroBitAccelerometer & MicroBitCompass classes in https://github.com/lancaster-university/codal-microbit-v2/commit/93318d2676a0659c180c05a2fcb11fccc54d6bb6.

[microbit-carlos]

@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: a) Base class Accelerometer or Compass updated instances that throw a panic on first usage b) Derived classes MicroBitAccelerometer or MicroBitCompass that throw a panic on first usage

This PR, for option b), instantiates MicroBitCompass & MicroBitAccelerometer, which I assumed was the desired effect of https://github.com/lancaster-university/codal-microbit-v2/commit/93318d2676a0659c180c05a2fcb11fccc54d6bb6.

On the other hand we can move the panic to the base class Accelerometer & Compass (right now they return DEVICE_NOT_SUPPORTED). On a quick test locally, this option would be about 240 bytes smaller (assuming this PR would be merged together with https://github.com/lancaster-university/codal-core/pull/162, which saves 152 bytes, but it's incompatible with option a)). However, there are a couple of considerations about this approach:

• Is this a pattern (throw a panic on base classes) something you'd be happy to introduce to CODAL?
• Is this going to have an effect to other targets?
• Are we happy to enforce error codes 50 & 51 to be reserved for accelerometer and compass errors in all targets?
  • Right now error code ranges are stipulated by CODAL (like peripheral error), but specific errors (like accelerometer or compass) are defined in each individual target, so there is a chance these values are already used by a different target to indicate something else?

[martinwork]

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 https://github.com/lancaster-university/codal-microbit-v2/blob/f974d92661e650b8fb998089564e9575fe8190e5/source/MicroBitAccelerometer.cpp https://github.com/lancaster-university/codal-microbit-v2/blob/master/source/MicroBitAccelerometer.cpp

[microbit-carlos]

could a very simple subclass of Accelerometer, which just implements the panic in requestUpdate be a better alternative to MicroBitAccelerometer.

This PR removes all the other methods from MicroBitAccelerometer and MicroBitCompass to only have the configure() and requestUpdate() methods, which either throw a panic if it didn't detect a motion sensor, or call the respective method from the detected driver. Which is more or less what you are describing here unless I've missunderstood? The PR diff might be a bit noisy, maybe it's more clean when looking at the file directly: this PR -> codal-microbit-v2/source/MicroBitAccelerometer.cpp#L89-L115

https://github.com/lancaster-university/codal-microbit-v2/blob/f974d92661e650b8fb998089564e9575fe8190e5/source/MicroBitAccelerometer.cpp

Yes, in that older version of the driver it was creating an instance of the base class Accelerometer: https://github.com/lancaster-university/codal-microbit-v2/blob/f974d92661e650b8fb998089564e9575fe8190e5/source/MicroBitAccelerometer.cpp#L92-L93 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.

[martinwork]

Yes, sorry @microbit-carlos, I hadn’t understood the result of the changes.

[microbit-carlos]

Thanks for raising these questions Martin! They are good for the conversation and ensuring we figure out the best approach 👍