From 1b7821a474da216bd89052902347f97f2a1d796c Mon Sep 17 00:00:00 2001 From: jacqueline Date: Tue, 6 Feb 2024 21:47:28 +1100 Subject: [PATCH] improve the locking strategy of the bluetooth fsm --- src/drivers/bluetooth.cpp | 53 +++++++++++++++++-------------- src/drivers/include/bluetooth.hpp | 4 ++- 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/drivers/bluetooth.cpp b/src/drivers/bluetooth.cpp index c5912d4d..0b30c7c1 100644 --- a/src/drivers/bluetooth.cpp +++ b/src/drivers/bluetooth.cpp @@ -40,17 +40,20 @@ DRAM_ATTR static StreamBufferHandle_t sStream = nullptr; static tasks::WorkerPool* sBgWorker; auto gap_cb(esp_bt_gap_cb_event_t event, esp_bt_gap_cb_param_t* param) -> void { + auto lock = bluetooth::BluetoothState::lock(); tinyfsm::FsmList::dispatch( bluetooth::events::internal::Gap{.type = event, .param = param}); } auto avrcp_cb(esp_avrc_ct_cb_event_t event, esp_avrc_ct_cb_param_t* param) -> void { + auto lock = bluetooth::BluetoothState::lock(); tinyfsm::FsmList::dispatch( bluetooth::events::internal::Avrc{.type = event, .param = param}); } auto a2dp_cb(esp_a2d_cb_event_t event, esp_a2d_cb_param_t* param) -> void { + auto lock = bluetooth::BluetoothState::lock(); tinyfsm::FsmList::dispatch( bluetooth::events::internal::A2dp{.type = event, .param = param}); } @@ -72,6 +75,7 @@ Bluetooth::Bluetooth(NvsStorage& storage, tasks::WorkerPool& bg_worker) { } auto Bluetooth::Enable() -> bool { + auto lock = bluetooth::BluetoothState::lock(); tinyfsm::FsmList::dispatch( bluetooth::events::Enable{}); @@ -79,20 +83,24 @@ auto Bluetooth::Enable() -> bool { } auto Bluetooth::Disable() -> void { + auto lock = bluetooth::BluetoothState::lock(); tinyfsm::FsmList::dispatch( bluetooth::events::Disable{}); } auto Bluetooth::IsEnabled() -> bool { + auto lock = bluetooth::BluetoothState::lock(); return !bluetooth::BluetoothState::is_in_state(); } auto Bluetooth::IsConnected() -> bool { + auto lock = bluetooth::BluetoothState::lock(); return bluetooth::BluetoothState::is_in_state(); } auto Bluetooth::ConnectedDevice() -> std::optional { - if (!IsConnected()) { + auto lock = bluetooth::BluetoothState::lock(); + if (!bluetooth::BluetoothState::is_in_state()) { return {}; } auto looking_for = bluetooth::BluetoothState::preferred_device(); @@ -108,6 +116,7 @@ auto Bluetooth::ConnectedDevice() -> std::optional { } auto Bluetooth::KnownDevices() -> std::vector { + auto lock = bluetooth::BluetoothState::lock(); std::vector out = bluetooth::BluetoothState::devices(); std::sort(out.begin(), out.end(), [](const auto& a, const auto& b) -> bool { return a.signal_strength < b.signal_strength; @@ -117,6 +126,7 @@ auto Bluetooth::KnownDevices() -> std::vector { auto Bluetooth::SetPreferredDevice(std::optional dev) -> void { + auto lock = bluetooth::BluetoothState::lock(); auto cur = bluetooth::BluetoothState::preferred_device(); if (dev && cur && dev->mac == cur->mac) { return; @@ -130,10 +140,12 @@ auto Bluetooth::SetPreferredDevice(std::optional dev) } auto Bluetooth::PreferredDevice() -> std::optional { + auto lock = bluetooth::BluetoothState::lock(); return bluetooth::BluetoothState::preferred_device(); } auto Bluetooth::SetSource(StreamBufferHandle_t src) -> void { + auto lock = bluetooth::BluetoothState::lock(); if (src == bluetooth::BluetoothState::source()) { return; } @@ -143,12 +155,14 @@ auto Bluetooth::SetSource(StreamBufferHandle_t src) -> void { } auto Bluetooth::SetVolume(uint8_t vol) -> void { + auto lock = bluetooth::BluetoothState::lock(); tinyfsm::FsmList::dispatch( bluetooth::events::ChangeVolume{.volume = vol}); } auto Bluetooth::SetEventHandler(std::function cb) -> void { + auto lock = bluetooth::BluetoothState::lock(); bluetooth::BluetoothState::event_handler(cb); } @@ -296,7 +310,7 @@ auto Scanner::HandleDeviceDiscovery(const esp_bt_gap_cb_param_t& param) NvsStorage* BluetoothState::sStorage_; Scanner* BluetoothState::sScanner_; -std::mutex BluetoothState::sDevicesMutex_{}; +std::mutex BluetoothState::sFsmMutex{}; std::map BluetoothState::sDevices_{}; std::optional BluetoothState::sPreferredDevice_{}; std::optional BluetoothState::sConnectingDevice_{}; @@ -311,8 +325,11 @@ auto BluetoothState::Init(NvsStorage& storage) -> void { tinyfsm::FsmList::start(); } +auto BluetoothState::lock() -> std::lock_guard { + return std::lock_guard{sFsmMutex}; +} + auto BluetoothState::devices() -> std::vector { - std::lock_guard lock{sDevicesMutex_}; std::vector out; for (const auto& device : sDevices_) { out.push_back(device.second); @@ -321,44 +338,36 @@ auto BluetoothState::devices() -> std::vector { } auto BluetoothState::preferred_device() -> std::optional { - std::lock_guard lock{sDevicesMutex_}; return sPreferredDevice_; } auto BluetoothState::preferred_device(std::optional addr) -> void { - std::lock_guard lock{sDevicesMutex_}; sPreferredDevice_ = addr; } auto BluetoothState::source() -> StreamBufferHandle_t { - std::lock_guard lock{sDevicesMutex_}; return sSource_.load(); } auto BluetoothState::source(StreamBufferHandle_t src) -> void { - std::lock_guard lock{sDevicesMutex_}; sSource_.store(src); } auto BluetoothState::event_handler(std::function cb) -> void { - std::lock_guard lock{sDevicesMutex_}; sEventHandler_ = cb; } auto BluetoothState::react(const events::DeviceDiscovered& ev) -> void { bool is_preferred = false; - { - std::lock_guard lock{sDevicesMutex_}; - bool already_known = sDevices_.contains(ev.device.address); - sDevices_[ev.device.address] = ev.device; + bool already_known = sDevices_.contains(ev.device.address); + sDevices_[ev.device.address] = ev.device; - if (sPreferredDevice_ && ev.device.address == sPreferredDevice_->mac) { - is_preferred = true; - } + if (sPreferredDevice_ && ev.device.address == sPreferredDevice_->mac) { + is_preferred = true; + } - if (sEventHandler_ && !already_known) { - std::invoke(sEventHandler_, Event::kKnownDevicesChanged); - } + if (sEventHandler_ && !already_known) { + std::invoke(sEventHandler_, Event::kKnownDevicesChanged); } if (is_preferred && sPreferredDevice_) { @@ -487,11 +496,8 @@ void Idle::react(const events::Disable& ev) { void Idle::react(const events::PreferredDeviceChanged& ev) { bool is_discovered = false; - { - std::lock_guard lock{sDevicesMutex_}; - if (sPreferredDevice_ && sDevices_.contains(sPreferredDevice_->mac)) { - is_discovered = true; - } + if (sPreferredDevice_ && sDevices_.contains(sPreferredDevice_->mac)) { + is_discovered = true; } if (is_discovered) { connect(*sPreferredDevice_); @@ -506,6 +512,7 @@ TimerHandle_t sTimeoutTimer; static void timeoutCallback(TimerHandle_t) { sBgWorker->Dispatch([]() { + auto lock = bluetooth::BluetoothState::lock(); tinyfsm::FsmList::dispatch( events::ConnectTimedOut{}); }); diff --git a/src/drivers/include/bluetooth.hpp b/src/drivers/include/bluetooth.hpp index c2962e64..2936fb8e 100644 --- a/src/drivers/include/bluetooth.hpp +++ b/src/drivers/include/bluetooth.hpp @@ -105,6 +105,8 @@ class BluetoothState : public tinyfsm::Fsm { public: static auto Init(NvsStorage& storage) -> void; + static auto lock() -> std::lock_guard; + static auto devices() -> std::vector; static auto preferred_device() -> std::optional; @@ -141,7 +143,7 @@ class BluetoothState : public tinyfsm::Fsm { static NvsStorage* sStorage_; static Scanner* sScanner_; - static std::mutex sDevicesMutex_; + static std::mutex sFsmMutex; static std::map sDevices_; static std::optional sPreferredDevice_;