lancaster-university / codal-microbit-v2

CODAL target for the micro:bit v2.x series of devices
MIT License
43 stars 52 forks source link

LevelDetectorSPL::getValue() and LevelDetector::getValue() mic on demand activation broken #314

Closed martinwork closed 1 year ago

martinwork commented 1 year ago

With current master (v0.2.51), LevelDetectorSPL::getValue() doesn't activate the mic and so returns zero always. The MicroBitAudio event handler is not called, and the mic light is off. activateForEvents(true) is the same.

Since https://github.com/lancaster-university/codal-core/commit/7b0cccbdef4e470a23f4263f61ce8d7a6545ac73, LevelDetectorSPL calls upstream.connect(*this) only in it's constructor.

I got the mic working by calling uBit.audio.activateMic(). This triggers a call to the MicroBitAudio event handler, which calls activateMic() again because mic->output.isFlowing().

I think wasAwake in LevelDetectorSPL::getValue() will always be true except for during the first 1ms after this->timeout has been set. https://github.com/lancaster-university/codal-core/blob/4b2b35e3b0db84d62a4cf3d3b582da66974793fb/source/streams/LevelDetectorSPL.cpp#L213

Compare previous versions. https://github.com/lancaster-university/codal-core/blob/c918ffb562722c5892e600a1c750c78e59a322a3/source/streams/LevelDetectorSPL.cpp#L156 https://github.com/lancaster-university/codal-core/blob/2d58a1816b478e228ed956a074d597fc2f3a11b5/source/streams/LevelDetectorSPL.cpp#L127

LevelDetectorSPL::activateForEvents is not used in the same way as LevelDetector::activateForEvents. https://github.com/lancaster-university/codal-microbit-v2/blob/1df00c870dd6b11591fccee46a597a9604017820/model/MicroBit.cpp#L350

LevelDetector::getValue() still calls upstream.connect(), but it calls upstream.connect and target_wait(100) every time. https://github.com/lancaster-university/codal-core/blob/4b2b35e3b0db84d62a4cf3d3b582da66974793fb/source/streams/LevelDetector.cpp#L117

Compare previous version. https://github.com/lancaster-university/codal-core/blob/60e06fed9b56dca2f90cde5ea91a128e8b85de45/source/streams/LevelDetector.cpp#L107

This program sees non-zero levels with v0.2.50-mailbox.0, but only zero with v0.2.51.

#include "MicroBit.h"

MicroBit uBit;

void forever()
{
    while (true)
    {
        int level = uBit.audio.levelSPL->getValue();
        //uBit.serial.printf( "%d\n", (int) level);
        if ( level > 0)
        {
            uBit.display.image.setPixelValue( 0, 0, uBit.display.image.getPixelValue(0, 0) ? 0 : 255);
            uBit.sleep(50);
            uBit.display.image.setPixelValue( 0, 0, uBit.display.image.getPixelValue(0, 0) ? 0 : 255);
        }
        uBit.sleep(20);
    }
}

int main()
{
    uBit.init();
    create_fiber( forever);
    release_fiber();
}
JohnVidler commented 1 year ago

Confirmed - it got lost on the way to the updated stream IO design.

This will be patched in the next tag.

microbit-carlos commented 1 year ago

Has this been fixed in 0.2.52?

If so, @martinwork could you retest again?

microbit-carlos commented 1 year ago

John has confirmed in zoom that this is implemented in v0.2.52. @martinwork when you get a chance could you retest and if we are good we can close this ticket.

martinwork commented 1 year ago

@microbit-carlos I get non-zero values now. Thanks @JohnVidler!

I'm testing current master, assuming that is 0.2.52.

LevelDetectorSPL::getValue() typically takes 27us instead of 11us.

I guess system_timer_current_time() takes some time!

wasAwake is unused in both getValue()s, so we could remove that system_timer_current_time() https://github.com/lancaster-university/codal-core/blob/master/source/streams/LevelDetectorSPL.cpp#L222

LevelDetector::getValue() still calls target_wait(100) every time. https://github.com/lancaster-university/codal-core/blob/a1e1db692909a260ab341c89b292e085daecd259/source/streams/LevelDetector.cpp#L120

If I change my test program to call uBit.audio.level->getValue(), the microphone LED flashes on and off, apparently with every call. Time for the call measures around 97000us.

../libraries/codal-core/source/streams/LevelDetector.cpp: In member function 'int codal::LevelDetector::getValue()':
../libraries/codal-core/source/streams/LevelDetector.cpp:118:10: warning: unused variable 'wasAwake' [-Wunused-variable]
  118 |     bool wasAwake = this->activated || system_timer_current_time() - this->timeout;
      |          ^~~~~~~~
[10/26] Building CXX object libraries/codal-core/CMakeFiles/codal-core.dir/source/streams/LevelDetectorSPL.cpp.obj
../libraries/codal-core/source/streams/LevelDetectorSPL.cpp: In member function 'float codal::LevelDetectorSPL::getValue()':
../libraries/codal-core/source/streams/LevelDetectorSPL.cpp:222:10: warning: unused variable 'wasAwake' [-Wunused-variable]
  222 |     bool wasAwake = this->activated || system_timer_current_time() - this->timeout;
      |          ^~~~~~~~

Times in us

        CODAL_TIMESTAMP t0 = system_timer_current_time_us();
        int level = uBit.audio.levelSPL->getValue();
        CODAL_TIMESTAMP t1 = system_timer_current_time_us();
        uBit.serial.printf( "%d %d\n", (int) level, (int)( t1 - t0));

current master (v0.2.52)

0 206
0 27
58 28
51 27
58 27
51 27

v0.2.50-mailbox.0

0 213
59 11
50 11
50 11
51 11
51 17
51 11