From 8d127d5734b172339fcf70253f1cf25bba8d242d Mon Sep 17 00:00:00 2001 From: hhvrc Date: Sun, 29 Oct 2023 18:10:19 +0100 Subject: [PATCH 1/2] Static analysis and code cleanup --- platformio.ini | 3 + src/PinPatternManager.cpp | 2 +- src/RFTransmitter.cpp | 4 +- src/WiFiManager.cpp | 154 +++++++++++++++----------------------- src/WiFiScanManager.cpp | 6 +- 5 files changed, 70 insertions(+), 99 deletions(-) diff --git a/platformio.ini b/platformio.ini index b0ae9d61..0c51be1b 100644 --- a/platformio.ini +++ b/platformio.ini @@ -35,6 +35,9 @@ upload_speed = 921600 monitor_speed = 115200 monitor_filters = esp32_exception_decoder +; Static code analysis +check_tool = cppcheck + [env:fs] ; This exists so we don't build a filesystem per board. diff --git a/src/PinPatternManager.cpp b/src/PinPatternManager.cpp index 96e1f028..66324710 100644 --- a/src/PinPatternManager.cpp +++ b/src/PinPatternManager.cpp @@ -21,7 +21,7 @@ PinPatternManager::PinPatternManager(unsigned int gpioPin) , m_patternLength(0) , m_taskHandle(nullptr) , m_taskSemaphore(xSemaphoreCreateBinary()) { - snprintf(m_name, sizeof(m_name), "PinPatternManager-%d", m_gpioPin); + snprintf(m_name, sizeof(m_name), "PinPatternManager-%u", m_gpioPin); pinMode(gpioPin, OUTPUT); xSemaphoreGive(m_taskSemaphore); } diff --git a/src/RFTransmitter.cpp b/src/RFTransmitter.cpp index d1b896c6..018dee95 100644 --- a/src/RFTransmitter.cpp +++ b/src/RFTransmitter.cpp @@ -56,8 +56,8 @@ RFTransmitter::~RFTransmitter() { } bool RFTransmitter::SendCommand(ShockerModelType model, std::uint16_t shockerId, ShockerCommandType type, std::uint8_t intensity, std::uint16_t durationMs) { - if (!ok()) { - ESP_LOGW(m_name, "RFTransmitter is not ok"); + if (m_queueHandle == nullptr) { + ESP_LOGE(m_name, "Queue is null"); return false; } diff --git a/src/WiFiManager.cpp b/src/WiFiManager.cpp index 65fc54d1..64e9becd 100644 --- a/src/WiFiManager.cpp +++ b/src/WiFiManager.cpp @@ -3,12 +3,12 @@ #include "CaptivePortal.h" #include "Config.h" #include "FormatHelpers.h" +#include "Logging.h" #include "Mappers/EspWiFiTypesMapper.h" #include "Time.h" #include "Utils/HexUtils.h" #include "VisualStateManager.h" #include "WiFiScanManager.h" -#include "Logging.h" #include "_fbs/DeviceToLocalMessage_generated.h" @@ -200,19 +200,32 @@ bool GetNextWiFiNetwork(OpenShock::Config::WiFiCredentials& creds) { return found; } +std::vector::iterator _findNetwork(std::function predicate) { + return std::find_if(s_wifiNetworks.begin(), s_wifiNetworks.end(), predicate); +} +std::vector::iterator _findNetwork(const char* ssid) { + return _findNetwork([ssid](const WiFiNetwork& net) { return strcmp(net.ssid, ssid) == 0; }); +} +std::vector::iterator _findNetwork(const std::uint8_t (&bssid)[6]) { + return _findNetwork([bssid](const WiFiNetwork& net) { return memcmp(net.bssid, bssid, sizeof(bssid)) == 0; }); +} + void _evWiFiConnected(arduino_event_t* event) { s_wifiConnected = true; s_wifiConnecting = false; auto& info = event->event_info.wifi_sta_connected; - for (auto& net : s_wifiNetworks) { - if (memcmp(net.bssid, info.bssid, sizeof(net.bssid)) == 0) { - s_connectedCredentialsID = net.credentialsID; - _broadcastWifiConnected(net); - break; - } + auto it = _findNetwork(info.bssid); + if (it == s_wifiNetworks.end()) { + ESP_LOGW(TAG, "Connected to unknown network " BSSID_FMT, BSSID_ARG(info.bssid)); + return; } + + ESP_LOGI(TAG, "Connected to network %s (" BSSID_FMT ")", it->ssid, BSSID_ARG(it->bssid)); + + s_connectedCredentialsID = it->credentialsID; + _broadcastWifiConnected(*it); } void _evWiFiDisconnected(arduino_event_t* event) { s_wifiConnected = false; @@ -260,19 +273,19 @@ void _evWiFiScanStatusChanged(OpenShock::WifiScanStatus status) { void _evWiFiNetworkDiscovery(const wifi_ap_record_t* record) { OpenShock::WifiAuthMode authMode = Mappers::GetWiFiAuthModeEnum(record->authmode); - for (auto& net : s_wifiNetworks) { - if (memcmp(net.bssid, record->bssid, sizeof(net.bssid)) == 0) { - memcpy(net.ssid, record->ssid, sizeof(net.ssid)); - net.channel = record->primary; - net.rssi = record->rssi; - net.authMode = authMode; - net.scansMissed = 0; + auto it = _findNetwork(record->bssid); + if (it != s_wifiNetworks.end()) { + // Update the network + memcpy(it->ssid, record->ssid, sizeof(it->ssid)); + it->channel = record->primary; + it->rssi = record->rssi; + it->authMode = authMode; + it->scansMissed = 0; - _broadcastWifiNetworkUpdated(net); - ESP_LOGV(TAG, "Updated network %s (" BSSID_FMT ") with new scan info", net.ssid, BSSID_ARG(net.bssid)); + _broadcastWifiNetworkUpdated(*it); + ESP_LOGV(TAG, "Updated network %s (" BSSID_FMT ") with new scan info", it->ssid, BSSID_ARG(it->bssid)); - return; - } + return; } WiFiNetwork network(record->ssid, record->bssid, record->primary, record->rssi, authMode, 0); @@ -330,58 +343,44 @@ bool _authenticate(const WiFiNetwork& net, const std::string& password) { bool WiFiManager::Save(const char* ssid, const std::string& password) { ESP_LOGV(TAG, "Authenticating to network %s", ssid); - for (auto& net : s_wifiNetworks) { - if (strcmp(net.ssid, ssid) == 0) { - ESP_LOGV(TAG, "Network with SSID %s was resolved", ssid); - - return _authenticate(net, password); - } - } + auto it = _findNetwork(ssid); + if (it == s_wifiNetworks.end()) { + ESP_LOGE(TAG, "Failed to find network with SSID %s", ssid); - ESP_LOGE(TAG, "Failed to find network with SSID %s", ssid); + _broadcastErrorMessage("network_not_found"); - _broadcastErrorMessage("network_not_found"); + return false; + } - return false; + return _authenticate(*it, password); } bool WiFiManager::Save(const std::uint8_t (&bssid)[6], const std::string& password) { ESP_LOGV(TAG, "Authenticating to network " BSSID_FMT, BSSID_ARG(bssid)); - for (auto& net : s_wifiNetworks) { - static_assert(sizeof(net.bssid) == sizeof(bssid), "BSSID size mismatch"); - if (memcmp(net.bssid, bssid, sizeof(bssid)) == 0) { - ESP_LOGV(TAG, "Network with BSSID " BSSID_FMT " was resolved to SSID %s", BSSID_ARG(bssid), net.ssid); - - return _authenticate(net, password); - } - } + auto it = _findNetwork(bssid); + if (it == s_wifiNetworks.end()) { + ESP_LOGE(TAG, "Failed to find network with BSSID " BSSID_FMT, BSSID_ARG(bssid)); - ESP_LOGE(TAG, "Failed to find network with BSSID " BSSID_FMT, BSSID_ARG(bssid)); + _broadcastErrorMessage("network_not_found"); - _broadcastErrorMessage("network_not_found"); + return false; + } - return false; + return _authenticate(*it, password); } bool WiFiManager::Forget(const char* ssid) { ESP_LOGV(TAG, "Forgetting network %s", ssid); - std::uint8_t credsId = 0; - for (auto& net : s_wifiNetworks) { - if (strcmp(net.ssid, ssid) == 0) { - credsId = net.credentialsID; - - net.credentialsID = 0; - break; - } - } - - if (credsId == 0) { + auto it = _findNetwork(ssid); + if (it == s_wifiNetworks.end()) { ESP_LOGE(TAG, "Failed to find network with SSID %s", ssid); return false; } + std::uint8_t credsId = it->credentialsID; + // Check if the network is currently connected if (s_connectedCredentialsID == credsId) { // Disconnect from the network @@ -397,21 +396,14 @@ bool WiFiManager::Forget(const char* ssid) { bool WiFiManager::Forget(const std::uint8_t (&bssid)[6]) { ESP_LOGV(TAG, "Forgetting network " BSSID_FMT, BSSID_ARG(bssid)); - std::uint8_t credsId = 0; - for (auto& net : s_wifiNetworks) { - if (memcmp(net.bssid, bssid, sizeof(bssid)) == 0) { - credsId = net.credentialsID; - - net.credentialsID = 0; - break; - } - } - - if (credsId == 0) { + auto it = std::find_if(s_wifiNetworks.begin(), s_wifiNetworks.end(), [bssid](const WiFiNetwork& net) { return memcmp(net.bssid, bssid, sizeof(bssid)) == 0; }); + if (it == s_wifiNetworks.end()) { ESP_LOGE(TAG, "Failed to find network with BSSID " BSSID_FMT, BSSID_ARG(bssid)); return false; } + std::uint8_t credsId = it->credentialsID; + // Check if the network is currently connected if (s_connectedCredentialsID == credsId) { // Disconnect from the network @@ -424,46 +416,22 @@ bool WiFiManager::Forget(const std::uint8_t (&bssid)[6]) { return true; } -bool WiFiManager::IsSaved(const char* ssid) { - ESP_LOGV(TAG, "Checking if network with SSID %s is saved", ssid); +bool _isSaved(std::function predicate) { + const auto& credentials = Config::GetWiFiCredentials(); - for (auto& creds : Config::GetWiFiCredentials()) { - if (creds.ssid == ssid) { - ESP_LOGV(TAG, "Network with SSID %s is saved", ssid); - return true; - } - } + return std::any_of(credentials.begin(), credentials.end(), predicate); +} - ESP_LOGV(TAG, "Network with SSID %s is not saved", ssid); - return false; +bool WiFiManager::IsSaved(const char* ssid) { + return _isSaved([ssid](const Config::WiFiCredentials& creds) { return creds.ssid == ssid; }); } bool WiFiManager::IsSaved(const std::uint8_t (&bssid)[6]) { - ESP_LOGV(TAG, "Checking if network with BSSID " BSSID_FMT " is saved", BSSID_ARG(bssid)); - - for (auto& creds : Config::GetWiFiCredentials()) { - if (memcmp(creds.bssid, bssid, sizeof(bssid)) == 0) { - ESP_LOGV(TAG, "Network with BSSID " BSSID_FMT " is saved", BSSID_ARG(bssid)); - return true; - } - } - - ESP_LOGV(TAG, "Network with BSSID " BSSID_FMT " is not saved", BSSID_ARG(bssid)); - return false; + return _isSaved([bssid](const Config::WiFiCredentials& creds) { return memcmp(creds.bssid, bssid, sizeof(bssid)) == 0; }); } bool WiFiManager::IsSaved(const char* ssid, const std::uint8_t (&bssid)[6]) { - ESP_LOGV(TAG, "Checking if network with SSID %s or BSSID " BSSID_FMT " is saved", ssid, BSSID_ARG(bssid)); - - for (auto& creds : Config::GetWiFiCredentials()) { - if (creds.ssid == ssid || memcmp(creds.bssid, bssid, sizeof(bssid)) == 0) { - ESP_LOGV(TAG, "Network with SSID %s or BSSID " BSSID_FMT " is saved", ssid, BSSID_ARG(bssid)); - return true; - } - } - - ESP_LOGV(TAG, "Network with SSID %s or BSSID " BSSID_FMT " is not saved", ssid, BSSID_ARG(bssid)); - return false; + return _isSaved([ssid, bssid](const Config::WiFiCredentials& creds) { return creds.ssid == ssid || memcmp(creds.bssid, bssid, sizeof(bssid)) == 0; }); } bool _connect(const Config::WiFiCredentials& creds) { diff --git a/src/WiFiScanManager.cpp b/src/WiFiScanManager.cpp index af337da1..eff3776b 100644 --- a/src/WiFiScanManager.cpp +++ b/src/WiFiScanManager.cpp @@ -84,16 +84,16 @@ void _iterateChannel() { } void _evScanCompleted(arduino_event_id_t event, arduino_event_info_t info) { - std::uint16_t numNetworks = WiFi.scanComplete(); + std::int16_t numNetworks = WiFi.scanComplete(); if (numNetworks < 0) { _handleScanError(numNetworks); return; } - for (std::uint16_t i = 0; i < numNetworks; i++) { + for (std::int16_t i = 0; i < numNetworks; i++) { wifi_ap_record_t* record = reinterpret_cast(WiFi.getScanInfoByIndex(i)); if (record == nullptr) { - ESP_LOGE(TAG, "Failed to get scan info for network #%u", i); + ESP_LOGE(TAG, "Failed to get scan info for network #%d", i); return; } From 1c56ab0828fb87d01b6f620be3c8c2d4c2687c76 Mon Sep 17 00:00:00 2001 From: hhvrc Date: Sun, 29 Oct 2023 23:24:41 +0100 Subject: [PATCH 2/2] Ignore dumb files --- .gitignore | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.gitignore b/.gitignore index da53f979..e67a2fd2 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,8 @@ data/www # ignore local config files *.local + +# ignore generated files +.vs +packages +*.sln