Fix crash on audio end from IRQ, refactor A2DP (#2189)

Fixes #2188

We get a call to stop the audio channel from a timer/IRQ context, so can't
safely remove the IRQ handler for the AudioBufferManager.  The SDK will panic.

Because the IRQ handler will be a noop if it's not uninstalled, we will
instead just leave our shared handler in place and let it do nothing.

Use a common BluetoothLock RAII in BluetoothAudio to clen up the code and
automatically lock BT for the AVRCP button methods.

libraries/AudioBufferManager/src/AudioBufferManager.cpp

@@ -24,8 +24,9 @@
 #include <hardware/irq.h>
 #include "AudioBufferManager.h"
 
-static int                 __channelCount = 0;    // # of channels left.  When we hit 0, then remove our handler
-static AudioBufferManager* __channelMap[12];      // Lets the IRQ handler figure out where to dispatch to
+static int                 __channelCount = 0;     // # of channels left.  When we hit 0, then remove our handler
+static AudioBufferManager* __channelMap[12];       // Lets the IRQ handler figure out where to dispatch to
+static bool                __irqInstalled = false; // Have we put in our IRQ handler yet?
 
 AudioBufferManager::AudioBufferManager(size_t bufferCount, size_t bufferWords, int32_t silenceSample, PinMode direction, enum dma_channel_transfer_size dmaSize) {
     _running = false;
@@ -78,11 +79,6 @@ AudioBufferManager::~AudioBufferManager() {
             dma_channel_unclaim(_channelDMA[i]);
             __channelCount--;
         }
-        if (!__channelCount) {
-            irq_set_enabled(DMA_IRQ_0, false);
-            // TODO - how can we know if there are no other parts of the core using DMA0 IRQ??
-            irq_remove_handler(DMA_IRQ_0, _irq);
-        }
     }
     interrupts();
     for (int i = 0; i < 2; i++) {
@@ -120,7 +116,7 @@ bool AudioBufferManager::begin(int dreq, volatile void *pioFIFOAddr) {
             return false;
         }
     }
-    bool needSetIRQ = __channelCount == 0;
+
     // Need to know both channels to set up ping-pong, so do in 2 stages
     for (auto i = 0; i < 2; i++) {
         dma_channel_config c = dma_channel_get_default_config(_channelDMA[i]);
@@ -146,9 +142,10 @@ bool AudioBufferManager::begin(int dreq, volatile void *pioFIFOAddr) {
         __channelMap[_channelDMA[i]] = this;
         __channelCount++;
     }
-    if (needSetIRQ) {
+    if (!__irqInstalled) {
         irq_add_shared_handler(DMA_IRQ_0, _irq, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
         irq_set_enabled(DMA_IRQ_0, true);
+        __irqInstalled = true;
     }
 
     dma_channel_start(_channelDMA[0]);

libraries/BluetoothAudio/examples/A2DPSink/A2DPSink.ino

@@ -45,7 +45,6 @@ void setup() {
 
 void loop() {
   if (BOOTSEL) {
-    __lockBluetooth();
     if (status == A2DPSink::PAUSED) {
       a2dp.play();
       Serial.printf("Resuming\n");
@@ -53,7 +52,6 @@ void loop() {
       a2dp.pause();
       Serial.printf("Pausing\n");
     }
-    __unlockBluetooth();
     while (BOOTSEL);
   }
 }

libraries/BluetoothAudio/src/A2DPSink.cpp

@@ -191,9 +191,10 @@ bool A2DPSink::begin() {
 }
 
 bool A2DPSink::disconnect() {
-    __lockBluetooth();
-    a2dp_sink_disconnect(a2dp_sink_a2dp_connection.a2dp_cid);
-    __unlockBluetooth();
+    BluetoothLock b;
+    if (_connected) {
+        a2dp_sink_disconnect(a2dp_sink_a2dp_connection.a2dp_cid);
+    }
     if (!_running || !_connected) {
         return false;
     }
@@ -202,10 +203,11 @@ bool A2DPSink::disconnect() {
 }
 
 void A2DPSink::clearPairing() {
-    disconnect();
-    __lockBluetooth();
+    BluetoothLock b;
+    if (_connected) {
+        a2dp_sink_disconnect(a2dp_sink_a2dp_connection.a2dp_cid);
+    }
     gap_delete_all_link_keys();
-    __unlockBluetooth();
 }
 
 

libraries/BluetoothAudio/src/A2DPSink.h

@@ -22,6 +22,7 @@
 
 #include <Arduino.h>
 #include "BluetoothHCI.h"
+#include "BluetoothLock.h"
 #include "BluetoothAudioConsumer.h"
 #include "BluetoothMediaConfigurationSBC.h"
 
@@ -120,60 +121,70 @@ public:
     void playback_handler(int16_t * buffer, uint16_t num_audio_frames);
 
     void play() {
+        BluetoothLock b;
         if (_connected) {
             avrcp_controller_play(a2dp_sink_avrcp_connection.avrcp_cid);
         }
     }
 
     void stop() {
+        BluetoothLock b;
         if (_connected) {
             avrcp_controller_stop(a2dp_sink_avrcp_connection.avrcp_cid);
         }
     }
 
     void pause() {
+        BluetoothLock b;
         if (_connected) {
             avrcp_controller_pause(a2dp_sink_avrcp_connection.avrcp_cid);
         }
     }
 
     void fastForward() {
+        BluetoothLock b;
         if (_connected) {
             avrcp_controller_fast_forward(a2dp_sink_avrcp_connection.avrcp_cid);
         }
     }
 
     void rewind() {
+        BluetoothLock b;
         if (_connected) {
             avrcp_controller_rewind(a2dp_sink_avrcp_connection.avrcp_cid);
         }
     }
 
     void forward() {
+        BluetoothLock b;
         if (_connected) {
             avrcp_controller_forward(a2dp_sink_avrcp_connection.avrcp_cid);
         }
     }
 
     void backward() {
+        BluetoothLock b;
         if (_connected) {
             avrcp_controller_backward(a2dp_sink_avrcp_connection.avrcp_cid);
         }
     }
 
     void volumeUp() {
+        BluetoothLock b;
         if (_connected) {
             avrcp_controller_volume_up(a2dp_sink_avrcp_connection.avrcp_cid);
         }
     }
 
     void volumeDown() {
+        BluetoothLock b;
         if (_connected) {
             avrcp_controller_volume_down(a2dp_sink_avrcp_connection.avrcp_cid);
         }
     }
 
     void mute() {
+        BluetoothLock b;
         if (_connected) {
             avrcp_controller_mute(a2dp_sink_avrcp_connection.avrcp_cid);
         }

libraries/BluetoothAudio/src/A2DPSource.cpp

@@ -196,11 +196,11 @@ bool A2DPSource::connect(const uint8_t *addr) {
         return false;
     }
 }
-
 bool A2DPSource::disconnect() {
-    __lockBluetooth();
-    a2dp_source_disconnect(media_tracker.a2dp_cid);
-    __unlockBluetooth();
+    BluetoothLock b;
+    if (_connected) {
+        a2dp_source_disconnect(media_tracker.a2dp_cid);
+    }
     if (!_running || !_connected) {
         return false;
     }
@@ -209,17 +209,20 @@ bool A2DPSource::disconnect() {
 }
 
 void A2DPSource::clearPairing() {
-    disconnect();
-    __lockBluetooth();
+    BluetoothLock b;
+    if (_connected) {
+        a2dp_source_disconnect(media_tracker.a2dp_cid);
+    }
     gap_delete_all_link_keys();
-    __unlockBluetooth();
 }
 
 // from Print (see notes on write() methods below)
 size_t A2DPSource::write(const uint8_t *buffer, size_t size) {
+    BluetoothLock b;
+
     size_t count = 0;
     size /= 2;
-    __lockBluetooth();
+
     // First copy from writer to either end of
     uint32_t start = _pcmWriter;
     uint32_t end = _pcmReader > _pcmWriter ? _pcmReader : _pcmBufferSize;
@@ -244,13 +247,12 @@ size_t A2DPSource::write(const uint8_t *buffer, size_t size) {
         _pcmWriter += end - start;
         _pcmWriter %= _pcmBufferSize;
     }
-    __unlockBluetooth();
     return count;
 }
 
 int A2DPSource::availableForWrite() {
+    BluetoothLock b;
     int avail = 0;
-    __lockBluetooth();
     if (_pcmWriter == _pcmReader) {
         avail =  _pcmBufferSize - 1;
     } else if (_pcmReader > _pcmWriter) {
@@ -258,7 +260,6 @@ int A2DPSource::availableForWrite() {
     } else {
         avail = _pcmBufferSize - _pcmWriter + _pcmReader - 1;
     }
-    __unlockBluetooth();
     return avail;
 }
 

libraries/BluetoothAudio/src/A2DPSource.h

@@ -22,6 +22,7 @@
 
 #include <Arduino.h>
 #include "BluetoothHCI.h"
+#include "BluetoothLock.h"
 #include "BluetoothMediaConfigurationSBC.h"
 #include <functional>
 #include <list>
@@ -84,13 +85,12 @@ public:
     }
 
     bool getUnderflow() {
+        BluetoothLock b;
         if (!_running) {
             return false;
         }
-        __lockBluetooth();
         auto ret = _underflow;
         _underflow = false;
-        __unlockBluetooth();
         return ret;
     }
 

libraries/BluetoothAudio/src/BluetoothAudio.h

+#include "BluetoothLock.h"
 #include "BluetoothDevice.h"
 #include "BluetoothHCI.h"
 #include "BluetoothMediaConfigurationSBC.h"

libraries/BluetoothAudio/src/BluetoothHCI.cpp

@@ -24,6 +24,7 @@
 #include <memory>
 
 #include "BluetoothHCI.h"
+#include "BluetoothLock.h"
 
 #define CCALLBACKNAME _BTHCICB
 #include <ctocppcallback.h>
@@ -46,10 +47,9 @@ void BluetoothHCI_::begin() {
 }
 
 void BluetoothHCI_::uninstall() {
-    __lockBluetooth();
+    BluetoothLock b;
     hci_remove_event_handler(&hci_event_callback_registration);
     _running = false;
-    __unlockBluetooth();
 }
 
 bool BluetoothHCI_::running() {
@@ -72,9 +72,11 @@ std::list<BTDeviceInfo> BluetoothHCI_::scan(uint32_t mask, int scanTimeSec, bool
         inquiryTime = 1;
     }
     DEBUGV("HCI::scan(): inquiry start\n");
-    __lockBluetooth();
-    gap_inquiry_start(inquiryTime);
-    __unlockBluetooth();
+    // Only need to lock around the inquiry start command, not the wait
+    {
+        BluetoothLock b;
+        gap_inquiry_start(inquiryTime);
+    }
     if (async) {
         return _btdList;
     }

libraries/BluetoothAudio/src/BluetoothLock.h

+/*
+    Bluetooth lock helper class
+
+    Copyright (c) 2024 Earle F. Philhower, III <earlephilhower@yahoo.com>
+
+    This library is free software; you can redistribute it and/or
+    modify it under the terms of the GNU Lesser General Public
+    License as published by the Free Software Foundation; either
+    version 2.1 of the License, or (at your option) any later version.
+
+    This library is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+    Lesser General Public License for more details.
+
+    You should have received a copy of the GNU Lesser General Public
+    License along with this library; if not, write to the Free Software
+    Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+*/
+
+#pragma once
+
+#include <Arduino.h>
+
+class BluetoothLock {
+public:
+    BluetoothLock() {
+        __lockBluetooth();
+    }
+    ~BluetoothLock() {
+        __unlockBluetooth();
+    }
+};