search

lancaster-university / microbit-dal

http://lancaster-university.github.io/microbit-docs
Other
254 stars 130 forks source link

Tight loop checking gestures does not recognise shake #372

Closed microbit-carlos closed 5 years ago

microbit-carlos commented 6 years ago

We had reports of this code in MicroPython v1.0.0-rc.2 not registering the Shake gesture (while the old v0.9 doesn't have this issue):

from microbit import *
while True:
    print(accelerometer.current_gesture())
    sleep(200)

The MicroPython method accelerometer.current_gesture() basically reads the accelerometer.getGesture() uBit method: @bbcmicrobit/micropython/source/microbit/microbitaccelerometer.cpp#L134-L139

mp_obj_t microbit_accelerometer_current_gesture(mp_obj_t self_in) {
    microbit_accelerometer_obj_t *self = (microbit_accelerometer_obj_t*)self_in;
    update(self);
    return MP_OBJ_NEW_QSTR(gesture_name_map[ubit_accelerometer->getGesture()]);
}
MP_DEFINE_CONST_FUN_OBJ_1(microbit_accelerometer_current_gesture_obj, microbit_accelerometer_current_gesture);

So I've tested the accelerometer.current_gesture() in a similar C++ program, and I can confirm that it has the same issue of not detecting the shake gesture, while it detects the UP/DOWN/LEFT/RIGHT correctly:

#include "MicroBit.h"

MicroBit uBit;

const char* getAccGestureName(int eventValue) {
    switch (eventValue) {
        case  MICROBIT_ACCELEROMETER_EVT_NONE:       return ".";
        case  MICROBIT_ACCELEROMETER_EVT_TILT_UP:    return "\nUP";
        case  MICROBIT_ACCELEROMETER_EVT_TILT_DOWN:  return "\nDOWN";
        case  MICROBIT_ACCELEROMETER_EVT_TILT_LEFT:  return "\nLEFT";
        case  MICROBIT_ACCELEROMETER_EVT_TILT_RIGHT: return "\nRIGHT";
        case  MICROBIT_ACCELEROMETER_EVT_FACE_UP:    return "\nUP";
        case  MICROBIT_ACCELEROMETER_EVT_FACE_DOWN:  return "\nDOWN";
        case  MICROBIT_ACCELEROMETER_EVT_FREEFALL:   return "\nFALL";
        case  MICROBIT_ACCELEROMETER_EVT_3G:         return "\n3G";
        case  MICROBIT_ACCELEROMETER_EVT_6G:         return "\n6G";
        case  MICROBIT_ACCELEROMETER_EVT_8G:         return "\n8G";
        case  MICROBIT_ACCELEROMETER_EVT_SHAKE:      return "\nSHAKE";
        default:                                     return "\n?";
    }
}

int main() {
    uBit.init();
    uBit.display.print('!');
    uBit.serial.send("Start:\n");

    for (;;) {
        uBit.accelerometer.updateSample();
        uBit.serial.send(getAccGestureName(uBit.accelerometer.getGesture()));
        uBit.sleep(200);
    }
}

I've tested this with the microbit#v2.0.0-rc11 tag (pre 1.5 work) and the latest tag v2.1.0-rc2, both showing the same result.

Hex file example from microbit v2.1.0-rc2: shake-test-cpp-microbit-v2.1.0-rc2.hex.zip

MicroPython v0.9 uses DAL v1, so I've tested a similar program with microbit-dal#v1.4.19b and in this case it detects the shake gestures as expected:

{
  "name": "microbit-dalv1-shake-test",
  "version": "0.0.3",
  "description": "Test program",
  "license": "MIT",
  "dependencies": {
    "microbit-dal": "lancaster-university/microbit-dal#v1.4.19b"
  },
  "targetDependencies": {},
  "bin": "./src"
}
#include "MicroBit.h"

const char* getAccGestureName(int gestureValue) {
    switch (gestureValue) {
        case  GESTURE_NONE:       return ".";
        case  GESTURE_UP:         return "\nUP";
        case  GESTURE_DOWN:       return "\nDOWN";
        case  GESTURE_LEFT:       return "\nLEFT";
        case  GESTURE_RIGHT:      return "\nRIGHT";
        case  GESTURE_FACE_UP:    return "\nUP";
        case  GESTURE_FACE_DOWN:  return "\nDOWN";
        case  GESTURE_FREEFALL:   return "\nFALL";
        case  GESTURE_3G:         return "\n3G";
        case  GESTURE_6G:         return "\n6G";
        case  GESTURE_8G:         return "\n8G";
        case  GESTURE_SHAKE:      return "\nSHAKE";
        default:                  return "\n?";
    }
}

void app_main() {
    uBit.display.print('!');
    uBit.serial.sendString("Start:\n");
    for (;;) {
        uBit.accelerometer.update();
        uBit.serial.sendString(getAccGestureName(uBit.accelerometer.getGesture()));
        uBit.sleep(200);
    }
}

Hex file for testing: microbit-dalv1-shake-test-combined.hex.zip

dpgeorge commented 6 years ago

Just a quick thought: you may need to reduce the delay time between calls to current_gesture() so it can update the internal state that computes the gestures. I know that we had some reports in the past about gestures not working and it was fixed by calling the function faster.

DavidWhaleMEF commented 6 years ago

@dpgeorge Yes it was me that reported this (from bitio). It does indeed work if you call current_gesture() faster. Although that is not always possible in every application.

microbit-carlos commented 6 years ago

In this case the worry is that this is an relatively common "first steps" example code, and we have a regression where code/examples/tutorials that used to work (and still do if using the online editor) do not work anymore in MicroPython v1.0.0.

dpgeorge commented 6 years ago

Do you think it's a DAL issue or uPy issue? I guess because it's posted here it's a DAL issue, and the evidence is that the C++ program above used to work with the DAL but no longer does.

So we need to understand what changed in the DAL that make the C++ example above stop working. git bisect can help do that.

carlosperate commented 6 years ago

This looks like a DAL issue to me, since the same C++ example exhibits the same issue (working in DAL v1.4, not in v2.0/v2.1). I'll see if I can biset tomorrow and catch the change that introduced this.

microbit-carlos commented 6 years ago

Ironically the change that introduced this issue is this one: https://github.com/lancaster-university/microbit-dal/commit/e53abf30915e2ecfc9fa943fdebac2506a3b7d0e (comparison with the two commits I tested: https://github.com/lancaster-university/microbit-dal/compare/9dbe43f...e53abf3 shake-test-v2.0.0-rc3-9dbe43f.hex.zip shake-test-v2.0.0-rc3-e53abf3.hex.zip)

This change was introduced as part of PR https://github.com/lancaster-university/microbit-dal/pull/159 for issue #145, to improve the sensitivity to detect the shake gesture.

dpgeorge commented 6 years ago

This change was introduced as part of PR #159 for issue #145, to improve the sensitivity to detect the shake gesture.

It seems #159 increased the damping of the shake detection, but decreased threshold. I'm not sure of the full logic of the shake detection, but maybe the damping needs to be decreased to get back to similar behaviour as before?

microbit-carlos commented 6 years ago

Okay, so long story short, it looks like the fix will be to simply register the Shake as the last detected gesture (lastGesture = MICROBIT_ACCELEROMETER_EVT_SHAKE;) before this return statement: https://github.com/lancaster-university/microbit-dal/blob/master/source/drivers/MicroBitAccelerometer.cpp#L459

The MicroBitAccelerometer::getGesture() method basically returns MicroBitAccelerometer::lastGesture: https://github.com/lancaster-university/microbit-dal/blob/2cff906f019944ae15d532157c1301c940f67862/source/drivers/MicroBitAccelerometer.cpp#L718-L721

And lastGesture is only set at the end of MicroBitAccelerometer::updateGesture(): https://github.com/lancaster-university/microbit-dal/blob/2cff906f019944ae15d532157c1301c940f67862/source/drivers/MicroBitAccelerometer.cpp#L474-L480

The gesture detection algorithm is implemented in MicroBitAccelerometer::instantaneousPosture(). For detecting all other gestures it only takes in consideration the last sample, but for shake there is some historical data preserved to determine if the current sample is considered a shake. So, this is where the shake tolerance, damping and retransmission delay are used to calculate the shake gesture: https://github.com/lancaster-university/microbit-dal/blob/5d40fcf4794ba519516b8b61e837caf497a6b2dc/source/drivers/MicroBitAccelerometer.cpp#L178-L268

The method MicroBitAccelerometer::updateGesture() checks what is the currently detected gesture by calling instantaneousPosture() (this is the only place where instantaneousPosture is called). Before the gesture is saved to the lastGesture variable and gesture event is triggered, it applies some additional damping to reduce jitter and ensure the same gesture is not continuously triggered.

https://github.com/lancaster-university/microbit-dal/blob/5d40fcf4794ba519516b8b61e837caf497a6b2dc/source/drivers/MicroBitAccelerometer.cpp#L309-L336

However, if instantaneousPosture() returns a shake gesture, it immediately triggers the event and exits early, without going through the logic of saving it into the lastGesture variable: https://github.com/lancaster-university/microbit-dal/blob/5d40fcf4794ba519516b8b61e837caf497a6b2dc/source/drivers/MicroBitAccelerometer.cpp#L309-L316

Because there is already damping and a retransmission delay inside instantaneousPosture() we can bypass the second filtering performed after that point (at the end of updateGesture()). So we can keep the early return, but we have to also register the last gesture by adding the line I mentioned at the beginning of this post:

    // Determine what it looks like we're doing based on the latest sample...
    uint16_t g = instantaneousPosture();

    if (g == MICROBIT_ACCELEROMETER_EVT_SHAKE)
    {
        MicroBitEvent e(MICROBIT_ID_GESTURE, MICROBIT_ACCELEROMETER_EVT_SHAKE);
+       lastGesture = MICROBIT_ACCELEROMETER_EVT_SHAKE;
        return;
    }

To be able to visualise how the gestures were detected and filtered I edited to DAL to buffer the last 256 detected gestures (so save the output of MicroBitAccelerometer::instantaneousPosture()) and then printed all that data to serial while I was moving the micro:bit.

Source code here: https://gist.github.com/microbit-carlos/15ebccfb880f6b10c276d8e473fa9f54

The test code in essence did this in an loop:

Update accelerometer sample
Print the last **recorded** gesture
Print "S:\n"
Print the last 256 **detected** gestures
Print "E.\n"
If A is pressed:
    Clear the detected gestures history

And so the serial console had the following format, where the detection history was printed with all non-shake gestures as commas, and the "no gesture" output as periods (the last recorded gesture is always fully spelled):

LEFT
S:
,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,.,,,,,.,,,,,,,,,,,,,,,,,,,,,,,,,.,,,,,,SHAKE,,,,,,,,,.,,.,,,,,,,,,,.,,,.,,,,,,,,,,,.,,,,,,,,,.,.,,.,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
E.

There is also a video here that quickly shows how, even if I shake the micro:bit continuously, the instantaneousPosture() return value is SHAKE only once until the damping/retransmission allows the next one. The commas and periods in between the SHAKEs also show how MicroBitAccelerometer::instantaneousPosture() still registers other gestures (like face up/down/left/right), or the "none" gesture: https://youtu.be/mTU62Qr--jo

microbit-carlos commented 6 years ago

MicroPython with this patch for testing: MicroPython-v1.0.0-rc.2-gesture-patch.hex.zip

Example MicroPython programs that didn't work with v1.0.0-rc.2 that work with the patch:

from microbit import *
while True:
    if accelerometer.current_gesture() == "shake":
        display.show(Image.HAPPY)
        sleep(1000)
    else:
        display.show(Image.ANGRY)
from microbit import *
while True:
    if accelerometer.is_gesture("shake"):
        display.show(Image.HAPPY)
        sleep(1000)
    else:
        display.show(Image.SAD)

(Edit: accelerometer.was_gesture("shake") was working already in v1.0.0-rc.2, so only the two examples above apply)

To compare the old a new behaviour:

from microbit import *
while True:
    print("Current  : {}".format(accelerometer.current_gesture()))
    print("Is Shake : {}".format(accelerometer.is_gesture("shake")))
    print("Was shake: {}\n".format(accelerometer.was_gesture("shake")))
    sleep(30)

There are some differences on the output, I'd say for the better, but they are still changes that would need to be documented.

New shake output:

Current  : right
Is Shake : False
Was shake: False

Current  : shake
Is Shake : True
Was shake: True

Current  : shake
Is Shake : True
Was shake: False

Current  : shake
Is Shake : True
Was shake: False

Current  : shake
Is Shake : True
Was shake: False

Current  : shake
Is Shake : False
Was shake: False

Current  : right
Is Shake : False
Was shake: False

Shake output from v0.9 is harder to trigger and lasts longer:

Current  : right
Is Shake : False
Was shake: False

Current  : shake
Is Shake : True
Was shake: True

Current  : shake
Is Shake : True
Was shake: False

Current  : shake
Is Shake : True
Was shake: False

Current  : shake
Is Shake : True
Was shake: False

Current  : shake
Is Shake : True
Was shake: False

... repeats another 25 times ...

Current  : right
Is Shake : False
Was shake: False
dpgeorge commented 6 years ago

Thanks @carlosperate for the detailed analysis and fix. Just a note: in MicroPython only current_gesture() and is_gesture() use the DAL's ::getGesture(). was_gesture() doesn't use that DAL function so its behaviour is unchanged.

microbit-carlos commented 6 years ago

Thanks for the clarification @dpgeorge, looking back at the hexes I used for testing I was indeed testing is_gesture twice instead of was_gesture. I've edited my previous message to note this only affects is_gesture and current_gesture.

finneyj commented 5 years ago

Thanks @microbit-carlos. Closing this off as your patch is known to correct this issue.