More smart pointer usage adjustments

Author: hhvrc

src/CommandHandler.cpp

@@ -13,7 +13,6 @@ const char* const TAG = "CommandHandler";
 #include "events/Events.h"
 #include "Logging.h"
 #include "radio/RFTransmitter.h"
-#include "ReadWriteMutex.h"
 #include "SimpleMutex.h"
 #include "util/TaskUtils.h"
 
@@ -38,10 +37,32 @@ struct KnownShocker {
   int64_t lastActivityTimestamp;
 };
 
-static OpenShock::ReadWriteMutex s_rfTransmitterMutex            = {};
-static std::unique_ptr<OpenShock::RFTransmitter> s_rfTransmitter = nullptr;
+static OpenShock::SimpleMutex s_rfTransmitterMutex                = {};
+static std::shared_ptr<OpenShock::RFTransmitter> s_rfTransmitter = nullptr;
 
-static OpenShock::ReadWriteMutex s_keepAliveMutex = {};
+static std::shared_ptr<OpenShock::RFTransmitter> GetTransmitter()
+{
+  ScopedLock lock__(&s_rfTransmitterMutex);
+  return s_rfTransmitter;
+}
+static bool TryCreateTransmitter(gpio_num_t txPin)
+{
+  auto transmitter = std::make_shared<OpenShock::RFTransmitter>(txPin);
+  if (!transmitter->ok()) {
+    return false;
+  }
+
+  ScopedLock lock__(&s_rfTransmitterMutex);
+  s_rfTransmitter = std::move(transmitter);
+  return true;
+}
+static void DestroyTransmitter()
+{
+  ScopedLock lock__(&s_rfTransmitterMutex);
+  s_rfTransmitter = nullptr;
+}
+
+static OpenShock::SimpleMutex s_keepAliveMutex = {};
 static QueueHandle_t s_keepAliveQueue             = nullptr;
 static TaskHandle_t s_keepAliveTaskHandle         = nullptr;
 
@@ -86,13 +107,13 @@ static void commandhandler_keepalivetask(void* arg)
       if (cmdRef.lastActivityTimestamp + KEEP_ALIVE_INTERVAL < now) {
         OS_LOGV(TAG, "Sending keep-alive for shocker %u", cmdRef.shockerId);
 
-        ScopedReadLock rfLock__(&s_rfTransmitterMutex);
-        if (s_rfTransmitter == nullptr) {
+        auto transmitter = GetTransmitter();
+        if (transmitter == nullptr) {
           OS_LOGW(TAG, "RF Transmitter is not initialized, ignoring keep-alive");
           break;
         }
 
-        if (!s_rfTransmitter->SendCommand(cmdRef.model, cmdRef.shockerId, ShockerCommandType::Vibrate, 0, KEEP_ALIVE_DURATION, false)) {
+        if (!transmitter->SendCommand(cmdRef.model, cmdRef.shockerId, ShockerCommandType::Vibrate, 0, KEEP_ALIVE_DURATION, false)) {
           OS_LOGW(TAG, "Failed to send keep-alive for shocker %u", cmdRef.shockerId);
         }
 
@@ -112,7 +133,7 @@ bool _internalSetKeepAliveEnabled(bool enabled)
     return true;
   }
 
-  ScopedWriteLock lock__(&s_keepAliveMutex);
+  ScopedLock lock__(&s_keepAliveMutex);
 
   if (enabled) {
     OS_LOGV(TAG, "Enabling keep-alive task");
@@ -200,10 +221,8 @@ bool CommandHandler::Init()
     }
   }
 
-  s_rfTransmitter = std::make_unique<RFTransmitter>(txPin);
-  if (!s_rfTransmitter->ok()) {
+  if (!TryCreateTransmitter(txPin)) {
     OS_LOGE(TAG, "Failed to initialize RF Transmitter");
-    s_rfTransmitter = nullptr;
     return false;
   }
 
@@ -230,8 +249,7 @@ bool CommandHandler::Init()
 
 bool CommandHandler::Ok()
 {
-  ScopedReadLock lock__(&s_rfTransmitterMutex);
-  return s_rfTransmitter != nullptr;
+  return GetTransmitter() != nullptr;
 }
 
 SetGPIOResultCode CommandHandler::SetRfTxPin(gpio_num_t txPin)
@@ -240,16 +258,10 @@ SetGPIOResultCode CommandHandler::SetRfTxPin(gpio_num_t txPin)
     return SetGPIOResultCode::InvalidPin;
   }
 
-  ScopedWriteLock lock__(&s_rfTransmitterMutex);
-
-  if (s_rfTransmitter != nullptr) {
-    OS_LOGV(TAG, "Destroying existing RF transmitter");
-    s_rfTransmitter = nullptr;
-  }
+  DestroyTransmitter();
 
   OS_LOGV(TAG, "Creating new RF transmitter");
-  auto rfxmit = std::make_unique<RFTransmitter>(txPin);
-  if (!rfxmit->ok()) {
+  if (!TryCreateTransmitter(txPin)) {
     OS_LOGE(TAG, "Failed to initialize RF transmitter");
     return SetGPIOResultCode::InternalError;
   }
@@ -259,8 +271,6 @@ SetGPIOResultCode CommandHandler::SetRfTxPin(gpio_num_t txPin)
     return SetGPIOResultCode::InternalError;
   }
 
-  s_rfTransmitter = std::move(rfxmit);
-
   return SetGPIOResultCode::Success;
 }
 
@@ -280,9 +290,9 @@ bool CommandHandler::SetKeepAliveEnabled(bool enabled)
 
 gpio_num_t CommandHandler::GetRfTxPin()
 {
-  ScopedReadLock lock__(&s_rfTransmitterMutex);
-  if (s_rfTransmitter != nullptr) {
-    return s_rfTransmitter->GetTxPin();
+  auto transmitter = GetTransmitter();
+  if (transmitter != nullptr) {
+    return transmitter->GetTxPin();
   }
 
   gpio_num_t txPin;
@@ -296,17 +306,15 @@ gpio_num_t CommandHandler::GetRfTxPin()
 
 bool CommandHandler::HandleCommand(ShockerModelType model, uint16_t shockerId, ShockerCommandType type, uint8_t intensity, uint16_t durationMs)
 {
-  ScopedReadLock lock__rf(&s_rfTransmitterMutex);
-
-  if (s_rfTransmitter == nullptr) {
+  auto transmitter = GetTransmitter();
+  if (transmitter == nullptr) {
     OS_LOGW(TAG, "RF Transmitter is not initialized, ignoring command");
     return false;
   }
 
-  bool ok = s_rfTransmitter->SendCommand(model, shockerId, type, intensity, durationMs);
+  bool ok = transmitter->SendCommand(model, shockerId, type, intensity, durationMs);
 
-  lock__rf.unlock();
-  ScopedReadLock lock__ka(&s_keepAliveMutex);
+  ScopedLock lock__ka(&s_keepAliveMutex);
 
   if (ok && s_keepAliveQueue != nullptr) {
     KnownShocker cmd {.killTask = false, .model = model, .shockerId = shockerId, .lastActivityTimestamp = OpenShock::millis() + durationMs};

src/VisualStateManager.cpp

@@ -34,8 +34,8 @@ const uint64_t kWiFiScanningFlag                 = 1 << 7;
 const uint64_t kStatusOKMask = kWebSocketConnectedFlag | kHasIpAddressFlag | kWiFiConnectedFlag;
 
 static std::atomic<uint64_t> s_stateFlags = 0;
-static std::shared_ptr<OpenShock::PinPatternManager> s_builtInLedManager;
-static std::shared_ptr<OpenShock::RGBPatternManager> s_RGBLedManager;
+static std::unique_ptr<OpenShock::PinPatternManager> s_builtInLedManager;
+static std::unique_ptr<OpenShock::RGBPatternManager> s_RGBLedManager;
 
 inline void _setStateFlag(uint64_t flag, bool state)
 {
@@ -359,7 +359,7 @@ bool VisualStateManager::Init()
   bool ledActive = false;
 
   if (OPENSHOCK_LED_GPIO != GPIO_NUM_NC) {
-    s_builtInLedManager = std::make_shared<PinPatternManager>(static_cast<gpio_num_t>(OPENSHOCK_LED_GPIO));
+    s_builtInLedManager = std::make_unique<PinPatternManager>(static_cast<gpio_num_t>(OPENSHOCK_LED_GPIO));
     if (!s_builtInLedManager->IsValid()) {
       OS_LOGE(TAG, "Failed to initialize built-in LED manager");
       return false;
@@ -368,7 +368,7 @@ bool VisualStateManager::Init()
   }
 
   if (OPENSHOCK_LED_WS2812B != GPIO_NUM_NC) {
-    s_RGBLedManager = std::make_shared<RGBPatternManager>(static_cast<gpio_num_t>(OPENSHOCK_LED_WS2812B));
+    s_RGBLedManager = std::make_unique<RGBPatternManager>(static_cast<gpio_num_t>(OPENSHOCK_LED_WS2812B));
     if (!s_RGBLedManager->IsValid()) {
       OS_LOGE(TAG, "Failed to initialize RGB LED manager");
       return false;

src/captiveportal/Manager.cpp

@@ -28,11 +28,27 @@ static std::atomic<bool> s_alwaysEnabled                 = false;
 static std::atomic<bool> s_forceClosed                   = false;
 static esp_timer_handle_t s_captivePortalUpdateLoopTimer = nullptr;
 static SimpleMutex s_instanceMutex;
-static std::unique_ptr<CaptivePortal::CaptivePortalInstance> s_instance = nullptr;
+static std::shared_ptr<CaptivePortal::CaptivePortalInstance> s_instance = nullptr;
+
+static std::shared_ptr<CaptivePortal::CaptivePortalInstance> GetInstance()
+{
+  ScopedLock lock__(&s_instanceMutex);
+  return s_instance;
+}
+static void CreateInstance()
+{
+  ScopedLock lock__(&s_instanceMutex);
+  s_instance = std::make_shared<CaptivePortal::CaptivePortalInstance>();
+}
+static void DestroyInstance()
+{
+  ScopedLock lock__(&s_instanceMutex);
+  s_instance = nullptr;
+}
 
 static bool captiveportal_start()
 {
-  if (s_instance != nullptr) {
+  if (GetInstance() != nullptr) {
     OS_LOGD(TAG, "Already started");
     return true;
   }
@@ -63,20 +79,20 @@ static bool captiveportal_start()
     hostname = OPENSHOCK_FW_HOSTNAME;
   }
 
-  s_instance = std::make_unique<CaptivePortal::CaptivePortalInstance>();
+  CreateInstance();
 
   return true;
 }
 static void captiveportal_stop()
 {
-  if (s_instance == nullptr) {
+  if (GetInstance() == nullptr) {
     OS_LOGD(TAG, "Already stopped");
     return;
   }
 
   OS_LOGI(TAG, "Stopping captive portal");
 
-  s_instance = nullptr;
+  DestroyInstance();
 
   WiFi.softAPdisconnect(true);
 }
@@ -87,9 +103,7 @@ static void captiveportal_updateloop(void*)
   bool commandHandlerOk = CommandHandler::Ok();
   bool shouldBeRunning  = (s_alwaysEnabled || !gatewayConnected || !commandHandlerOk) && !s_forceClosed;
 
-  ScopedLock lock__(&s_instanceMutex);
-
-  if (s_instance == nullptr) {
+  if (GetInstance() == nullptr) {
     if (shouldBeRunning) {
       OS_LOGD(TAG, "Starting captive portal");
       OS_LOGD(TAG, "  alwaysEnabled: %s", s_alwaysEnabled ? "true" : "false");
@@ -155,10 +169,7 @@ bool CaptivePortal::ForceClose(uint32_t timeoutMs)
 {
   s_forceClosed = true;
 
-  {
-    ScopedLock lock__(&s_instanceMutex);
-    if (s_instance == nullptr) return true;
-  }
+  if (GetInstance() == nullptr) return true;
 
   while (timeoutMs > 0) {
     uint32_t delay = std::min(timeoutMs, static_cast<uint32_t>(10U));
@@ -167,55 +178,51 @@ bool CaptivePortal::ForceClose(uint32_t timeoutMs)
 
     timeoutMs -= delay;
 
-    {
-      ScopedLock lock__(&s_instanceMutex);
-      if (s_instance == nullptr) return true;
-    }
+    if (GetInstance() == nullptr) return true;
   }
 
   return false;
 }
 
 bool CaptivePortal::IsRunning()
 {
-  ScopedLock lock__(&s_instanceMutex);
-  return s_instance != nullptr;
+  return GetInstance() != nullptr;
 }
 
 bool CaptivePortal::SendMessageTXT(uint8_t socketId, std::string_view data)
 {
-  ScopedLock lock__(&s_instanceMutex);
-  if (s_instance == nullptr) return false;
+  auto instance = GetInstance();
+  if (instance == nullptr) return false;
 
-  s_instance->sendMessageTXT(socketId, data);
+  instance->sendMessageTXT(socketId, data);
 
   return true;
 }
 bool CaptivePortal::SendMessageBIN(uint8_t socketId, tcb::span<const uint8_t> data)
 {
-  ScopedLock lock__(&s_instanceMutex);
-  if (s_instance == nullptr) return false;
+  auto instance = GetInstance();
+  if (instance == nullptr) return false;
 
-  s_instance->sendMessageBIN(socketId, data);
+  instance->sendMessageBIN(socketId, data);
 
   return true;
 }
 
 bool CaptivePortal::BroadcastMessageTXT(std::string_view data)
 {
-  ScopedLock lock__(&s_instanceMutex);
-  if (s_instance == nullptr) return false;
+  auto instance = GetInstance();
+  if (instance == nullptr) return false;
 
-  s_instance->broadcastMessageTXT(data);
+  instance->broadcastMessageTXT(data);
 
   return true;
 }
 bool CaptivePortal::BroadcastMessageBIN(tcb::span<const uint8_t> data)
 {
-  ScopedLock lock__(&s_instanceMutex);
-  if (s_instance == nullptr) return false;
+  auto instance = GetInstance();
+  if (instance == nullptr) return false;
 
-  s_instance->broadcastMessageBIN(data);
+  instance->broadcastMessageBIN(data);
 
   return true;
 }

src/http/HTTPRequestManager.cpp

@@ -89,19 +89,14 @@ static std::shared_ptr<OpenShock::RateLimiter> createRateLimiterForURL(std::stri
     return nullptr;
   }
 
-  s_rateLimitsMutex.lock(portMAX_DELAY);
+  OpenShock::ScopedLock lock__(&s_rateLimitsMutex);
 
   auto it = s_rateLimits.find(domain);
   if (it == s_rateLimits.end()) {
-    s_rateLimits.emplace(domain, createRateLimiterForDomain(domain));
-    it = s_rateLimits.find(domain);
+    it = s_rateLimits.emplace(domain, createRateLimiterForDomain(domain)).first;
   }
 
-  auto result = it->second;
-
-  s_rateLimitsMutex.unlock();
-
-  return result;
+  return it->second;
 }
 
 struct StreamReaderResult {