Merge pull request #2269 from particle-iot/feature/tracker-cache-esp3…

…2-firmware-version

[Gen 3] Fix warm boot; [Tracker] Cache ESP32 firmware version

hal/network/lwip/cellular/pppncpnetif.cpp

@@ -119,29 +119,32 @@ void PppNcpNetif::loop(void* arg) {
         self->celMan_->ncpClient()->enable(); // Make sure the client is enabled
         os_semaphore_take(self->netifSemaphore_, timeout, false);
 
-        if (self->expectedNcpState_ == NcpState::ON && self->celMan_->ncpClient()->ncpState() != NcpState::ON) {
-            auto r = self->celMan_->ncpClient()->on();
-            if (r != SYSTEM_ERROR_NONE && r != SYSTEM_ERROR_ALREADY_EXISTS) {
-                LOG(ERROR, "Failed to initialize cellular NCP client: %d", r);
+        // We shouldn't be enforcing state on boot!
+        if (self->lastNetifEvent_ != NetifEvent::None) {
+            if (self->expectedNcpState_ == NcpState::ON && self->celMan_->ncpClient()->ncpState() != NcpState::ON) {
+                auto r = self->celMan_->ncpClient()->on();
+                if (r != SYSTEM_ERROR_NONE && r != SYSTEM_ERROR_ALREADY_EXISTS) {
+                    LOG(ERROR, "Failed to initialize cellular NCP client: %d", r);
+                }
+            }
+            if (self->expectedConnectionState_ == NcpConnectionState::CONNECTED &&
+                self->celMan_->ncpClient()->connectionState() == NcpConnectionState::DISCONNECTED &&
+                self->celMan_->ncpClient()->ncpState() == NcpState::ON) {
+                self->upImpl();
             }
-        }
-        if (self->expectedConnectionState_ == NcpConnectionState::CONNECTED && 
-              self->celMan_->ncpClient()->connectionState() == NcpConnectionState::DISCONNECTED &&
-              self->celMan_->ncpClient()->ncpState() == NcpState::ON) {
-            self->upImpl();
-        }
 
-        if (self->expectedConnectionState_ == NcpConnectionState::DISCONNECTED && 
-              self->celMan_->ncpClient()->connectionState() != NcpConnectionState::DISCONNECTED) {
-            self->downImpl();
-        }
-        if (self->expectedNcpState_ == NcpState::OFF && self->celMan_->ncpClient()->connectionState() == NcpConnectionState::DISCONNECTED) {
-            if_power_state_t pwrState = IF_POWER_STATE_NONE;
-            self->getPowerState(&pwrState);
-            if (pwrState == IF_POWER_STATE_UP) {
-                auto r = self->celMan_->ncpClient()->off();
-                if (r != SYSTEM_ERROR_NONE) {
-                    LOG(ERROR, "Failed to turn off NCP client: %d", r);
+            if (self->expectedConnectionState_ == NcpConnectionState::DISCONNECTED &&
+                self->celMan_->ncpClient()->connectionState() != NcpConnectionState::DISCONNECTED) {
+                self->downImpl();
+            }
+            if (self->expectedNcpState_ == NcpState::OFF && self->celMan_->ncpClient()->connectionState() == NcpConnectionState::DISCONNECTED) {
+                if_power_state_t pwrState = IF_POWER_STATE_NONE;
+                self->getPowerState(&pwrState);
+                if (pwrState == IF_POWER_STATE_UP) {
+                    auto r = self->celMan_->ncpClient()->off();
+                    if (r != SYSTEM_ERROR_NONE) {
+                        LOG(ERROR, "Failed to turn off NCP client: %d", r);
+                    }
                 }
             }
         }

hal/network/lwip/esp32/esp32ncpnetif.cpp

@@ -172,29 +172,32 @@ void Esp32NcpNetif::loop(void* arg) {
         self->wifiMan_->ncpClient()->enable(); // Make sure the client is enabled
         os_semaphore_take(self->netifSemaphore_, timeout, false);
 
-        if (self->expectedNcpState_ == NcpState::ON && self->wifiMan_->ncpClient()->ncpState() != NcpState::ON) {
-            auto r = self->wifiMan_->ncpClient()->on();
-            if (r != SYSTEM_ERROR_NONE && r != SYSTEM_ERROR_ALREADY_EXISTS) {
-                LOG(ERROR, "Failed to initialize cellular NCP client: %d", r);
+        // We shouldn't be enforcing state on boot!
+        if (self->lastNetifEvent_ != NetifEvent::None) {
+            if (self->expectedNcpState_ == NcpState::ON && self->wifiMan_->ncpClient()->ncpState() != NcpState::ON) {
+                auto r = self->wifiMan_->ncpClient()->on();
+                if (r != SYSTEM_ERROR_NONE && r != SYSTEM_ERROR_ALREADY_EXISTS) {
+                    LOG(ERROR, "Failed to initialize cellular NCP client: %d", r);
+                }
+            }
+            if (self->expectedConnectionState_ == NcpConnectionState::CONNECTED &&
+                self->wifiMan_->ncpClient()->connectionState() == NcpConnectionState::DISCONNECTED &&
+                self->wifiMan_->ncpClient()->ncpState() == NcpState::ON) {
+                self->upImpl();
             }
-        }
-        if (self->expectedConnectionState_ == NcpConnectionState::CONNECTED && 
-              self->wifiMan_->ncpClient()->connectionState() == NcpConnectionState::DISCONNECTED &&
-              self->wifiMan_->ncpClient()->ncpState() == NcpState::ON) {
-            self->upImpl();
-        }
 
-        if (self->expectedConnectionState_ == NcpConnectionState::DISCONNECTED && 
-              self->wifiMan_->ncpClient()->connectionState() != NcpConnectionState::DISCONNECTED) {
-            self->downImpl();
-        }
-        if (self->expectedNcpState_ == NcpState::OFF && self->wifiMan_->ncpClient()->connectionState() == NcpConnectionState::DISCONNECTED) {
-            if_power_state_t pwrState = IF_POWER_STATE_NONE;
-            self->getPowerState(&pwrState);
-            if (pwrState == IF_POWER_STATE_UP) {
-                auto r = self->wifiMan_->ncpClient()->off();
-                if (r != SYSTEM_ERROR_NONE) {
-                    LOG(ERROR, "Failed to turn off NCP client: %d", r);
+            if (self->expectedConnectionState_ == NcpConnectionState::DISCONNECTED &&
+                self->wifiMan_->ncpClient()->connectionState() != NcpConnectionState::DISCONNECTED) {
+                self->downImpl();
+            }
+            if (self->expectedNcpState_ == NcpState::OFF && self->wifiMan_->ncpClient()->connectionState() == NcpConnectionState::DISCONNECTED) {
+                if_power_state_t pwrState = IF_POWER_STATE_NONE;
+                self->getPowerState(&pwrState);
+                if (pwrState == IF_POWER_STATE_UP) {
+                    auto r = self->wifiMan_->ncpClient()->off();
+                    if (r != SYSTEM_ERROR_NONE) {
+                        LOG(ERROR, "Failed to turn off NCP client: %d", r);
+                    }
                 }
             }
         }

hal/network/ncp/wifi/platform_ncp_update.cpp

@@ -15,20 +15,26 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#undef LOG_COMPILE_TIME_LEVEL
+
 #include "platform_ncp.h"
 #include "network/ncp/wifi/ncp.h"
 #include "network/ncp/wifi/wifi_network_manager.h"
 #include "network/ncp/wifi/wifi_ncp_client.h"
+#include "network/ncp/ncp_client.h"
 #include "led_service.h"
 #include "check.h"
 #include "scope_guard.h"
 #include "system_led_signal.h"
 #include "stream.h"
 #include "debug.h"
 #include "ota_flash_hal_impl.h"
+#include "system_cache.h"
 
 #include <algorithm>
 
+namespace {
+
 class OtaUpdateSourceStream : public particle::InputStream {
 public:
     OtaUpdateSourceStream(const uint8_t* buffer, size_t length) : buffer(buffer), remaining(length) {}
@@ -80,12 +86,54 @@ class OtaUpdateSourceStream : public particle::InputStream {
     size_t remaining;
 };
 
+int invalidateWifiNcpVersionCache() {
+#if HAL_PLATFORM_NCP_COUNT > 1
+    using namespace particle::services;
+    // Cache will be updated by the NCP client itself
+    LOG(TRACE, "Invalidating cached ESP32 NCP firmware version");
+    return SystemCache::instance().del(SystemCacheKey::WIFI_NCP_FIRMWARE_VERSION);
+#else
+    return 0;
+#endif // HAL_PLATFORM_NCP_COUNT > 1
+}
+
+int getWifiNcpFirmwareVersion(uint16_t* ncpVersion) {
+    uint16_t version = 0;
+#if HAL_PLATFORM_NCP_COUNT > 1
+    using namespace particle::services;
+    int res = SystemCache::instance().get(SystemCacheKey::WIFI_NCP_FIRMWARE_VERSION, &version, sizeof(version));
+    if (res == sizeof(version)) {
+        LOG(TRACE, "Cached ESP32 NCP firmware version: %d", (int)version);
+        *ncpVersion = version;
+        return 0;
+    }
+
+    if (res >= 0) {
+        invalidateWifiNcpVersionCache();
+    }
+#endif // HAL_PLATFORM_NCP_COUNT > 1
+
+    // Not present in cache or caching not supported, call into NCP client
+    const auto ncpClient = particle::wifiNetworkManager()->ncpClient();
+    SPARK_ASSERT(ncpClient);
+    const particle::NcpClientLock lock(ncpClient);
+    CHECK(ncpClient->on());
+    CHECK(ncpClient->getFirmwareModuleVersion(&version));
+    *ncpVersion = version;
+
+    return 0;
+}
+
+} // anonymous
+
 // FIXME: This function accesses the module info via XIP and may fail to parse it correctly under
 // some not entirely clear circumstances. Disabling compiler optimizations helps to work around
 // the problem
 __attribute__((optimize("O0"))) int platform_ncp_update_module(const hal_module_t* module) {
     const auto ncpClient = particle::wifiNetworkManager()->ncpClient();
     SPARK_ASSERT(ncpClient);
+    // Holding a lock for the whole duration of the operation, otherwise the netif may potentially poweroff the NCP
+    const particle::NcpClientLock lock(ncpClient);
     CHECK(ncpClient->on());
     // we pass only the actual binary after the module info and up to the suffix
     const uint8_t* start = (const uint8_t*)module->info;
@@ -99,6 +147,7 @@ __attribute__((optimize("O0"))) int platform_ncp_update_module(const hal_module_
     if (r == 0) {
         LOG(INFO, "Updating ESP32 firmware from version %d to version %d", version, module->info->module_version);
     }
+    invalidateWifiNcpVersionCache();
     r = ncpClient->updateFirmware(&moduleStream, length);
     LED_On(PARTICLE_LED_RGB);
     CHECK(r);
@@ -114,14 +163,10 @@ int platform_ncp_fetch_module_info(hal_system_info_t* sys_info, bool create) {
         hal_module_t* module = sys_info->modules + i;
         if (!memcmp(&module->bounds, &module_ncp_mono, sizeof(module_ncp_mono))) {
             if (create) {
-                const auto ncpClient = particle::wifiNetworkManager()->ncpClient();
-                SPARK_ASSERT(ncpClient);
-                CHECK(ncpClient->on());
-                uint16_t version;
-                int error = ncpClient->getFirmwareModuleVersion(&version);
-                if (error) {
-                    version = 0;
-                }
+                uint16_t version = 0;
+                // Defaults to zero in case of failure
+                getWifiNcpFirmwareVersion(&version);
+
                 // todo - we could augment the getFirmwareModuleVersion command to retrieve more details
                 auto info = new module_info_t();
                 CHECK_TRUE(info, SYSTEM_ERROR_NO_MEMORY);

hal/network/ncp_client/esp32/esp32_ncp_client.cpp

@@ -35,6 +35,7 @@ LOG_SOURCE_CATEGORY("ncp.esp32.client");
 #include "check.h"
 
 #include <cstdlib>
+#include "system_cache.h"
 
 #define CHECK_PARSER(_expr) \
         ({ \
@@ -75,6 +76,21 @@ size_t espEscape(const char* src, char* dest, size_t destSize) {
     return escape(src, ",\"\\", '\\', dest, destSize);
 }
 
+int updateCachedNcpFirmwareVersion(uint16_t version) {
+#if HAL_PLATFORM_NCP_COUNT > 1
+    using namespace particle::services;
+
+    uint16_t cached = 0;
+    int r = SystemCache::instance().get(SystemCacheKey::WIFI_NCP_FIRMWARE_VERSION, &cached, sizeof(cached));
+    if (r == sizeof(cached) && cached == version) {
+        return 0;
+    }
+    return SystemCache::instance().set(SystemCacheKey::WIFI_NCP_FIRMWARE_VERSION, &version, sizeof(version));
+#else
+    return 0;
+#endif // HAL_PLATFORM_NCP_COUNT > 0
+}
+
 const auto ESP32_NCP_MAX_MUXER_FRAME_SIZE = 1536;
 const auto ESP32_NCP_KEEPALIVE_PERIOD = 5000; // milliseconds
 const auto ESP32_NCP_KEEPALIVE_MAX_MISSED = 5;
@@ -274,7 +290,11 @@ int Esp32NcpClient::getFirmwareVersionString(char* buf, size_t size) {
 int Esp32NcpClient::getFirmwareModuleVersion(uint16_t* ver) {
     const NcpClientLock lock(this);
     CHECK(checkParser());
-    return getFirmwareModuleVersionImpl(ver);
+    const int r = getFirmwareModuleVersionImpl(ver);
+    if (!r) {
+        updateCachedNcpFirmwareVersion(*ver);
+    }
+    return r;
 }
 
 int Esp32NcpClient::getFirmwareModuleVersionImpl(uint16_t* ver) {

services/inc/system_cache.h

@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2020 Particle Industries, Inc.  All rights reserved.
+ *
+ * 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 3 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, see <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include "tlv_file.h"
+
+namespace particle { namespace services {
+
+enum class SystemCacheKey : uint16_t {
+    WIFI_NCP_FIRMWARE_VERSION = 0x0000
+};
+
+class SystemCache {
+public:
+    static SystemCache& instance();
+
+    // Just straight up proxying to TlvFile
+    // TODO: add support for multiple values for a key?
+    int get(SystemCacheKey key, void* value, size_t length);
+    int set(SystemCacheKey key, const void* value, size_t length);
+    int del(SystemCacheKey key);
+
+    SystemCache(SystemCache const&) = delete;
+    SystemCache(SystemCache&&) = delete;
+    SystemCache& operator=(SystemCache const&) = delete;
+    SystemCache& operator=(SystemCache &&) = delete;
+
+protected:
+    SystemCache();
+
+private:
+    settings::TlvFile tlv_;
+};
+
+} } // particle::service

services/src/system_cache.cpp

@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) 2020 Particle Industries, Inc.  All rights reserved.
+ *
+ * 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 3 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, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hal_platform.h"
+
+#if HAL_PLATFORM_FILESYSTEM
+
+#include "system_cache.h"
+#include "enumclass.h"
+#include "check.h"
+
+namespace particle { namespace services {
+
+SystemCache::SystemCache()
+        : tlv_("/sys/cache.dat") {
+}
+
+SystemCache& SystemCache::instance() {
+    static SystemCache cache;
+    cache.tlv_.init();
+    return cache;
+}
+
+int SystemCache::get(SystemCacheKey key, void* value, size_t length) {
+    return tlv_.get(to_underlying(key), (uint8_t*)value, length);
+}
+
+int SystemCache::set(SystemCacheKey key, const void* value, size_t length) {
+    CHECK_TRUE(length <= sizeof(uint16_t), SYSTEM_ERROR_TOO_LARGE);
+    return tlv_.set(to_underlying(key), (const uint8_t*)value, length);
+}
+
+int SystemCache::del(SystemCacheKey key) {
+    return tlv_.del(to_underlying(key));
+}
+
+} } // particle::services
+
+#endif // HAL_PLATFORM_FILESYSTEM

system/src/control/cellular.cpp

@@ -116,6 +116,7 @@ int getIccid(ctrl_request* req) {
     CHECK_TRUE(cellMgr, SYSTEM_ERROR_UNKNOWN);
     const auto ncpClient = cellMgr->ncpClient();
     CHECK_TRUE(ncpClient, SYSTEM_ERROR_UNKNOWN);
+    const NcpClientLock lock(ncpClient);
     CHECK(ncpClient->on());
     char buf[32] = {};
     CHECK(ncpClient->getIccid(buf, sizeof(buf)));

system/src/control/config.cpp

@@ -108,6 +108,7 @@ int getNcpFirmwareVersion(ctrl_request* req) {
     CHECK_TRUE(wifiMgr, SYSTEM_ERROR_UNKNOWN);
     const auto ncpClient = wifiMgr->ncpClient();
     CHECK_TRUE(ncpClient, SYSTEM_ERROR_UNKNOWN);
+    const NcpClientLock lock(ncpClient);
     CHECK(ncpClient->on());
     char verStr[32] = {};
     CHECK(ncpClient->getFirmwareVersionString(verStr, sizeof(verStr)));