Merge pull request #2346 from particle-iot/clear_dct_slots/ch84957

Clear module slots in DCT when preparing for an OTA update

communication/inc/protocol.h

@@ -385,9 +385,13 @@ class Protocol
 		protocol_flags |= ProtocolFlag::DEVICE_INITIATED_DESCRIBE;
 	}
 
-	void enable_compressed_ota()
+	void set_compressed_ota_enabled(bool enabled)
 	{
-		protocol_flags |= ProtocolFlag::COMPRESSED_OTA;
+		if (enabled) {
+			protocol_flags |= ProtocolFlag::COMPRESSED_OTA;
+		} else {
+			protocol_flags &= ~ProtocolFlag::COMPRESSED_OTA;
+		}
 	}
 
 	void set_system_version(uint16_t version)

communication/inc/protocol_defs.h

@@ -149,8 +149,8 @@ enum Enum
 {
     PING = 0, ///< Keepalive interval (set).
     FAST_OTA = 1, ///< Enable/disable fast OTA (set).
-    DEVICE_INITIATED_DESCRIBE = 2, ///< Enable device-initiated describe messages (set).
-    COMPRESSED_OTA = 3, ///< Enable support for compressed/combined OTA updates (set).
+    ENABLE_DEVICE_INITIATED_DESCRIBE = 2, ///< Enable device-initiated describe messages (set).
+    COMPRESSED_OTA = 3, ///< Enable/disable support for compressed/combined OTA updates (set).
     SYSTEM_MODULE_VERSION = 4, ///< Module version of the system firmware (set).
     MAX_BINARY_SIZE = 5, ///< Maximum size of a firmware binary (set).
     OTA_CHUNK_SIZE = 6, ///< Size of an OTA update chunk (set).

communication/src/spark_protocol_functions.cpp

@@ -205,12 +205,12 @@ int spark_protocol_set_connection_property(ProtocolFacade* protocol, unsigned pr
         protocol->set_fast_ota(value);
         return 0;
     }
-    case Connection::DEVICE_INITIATED_DESCRIBE: {
+    case Connection::ENABLE_DEVICE_INITIATED_DESCRIBE: {
         protocol->enable_device_initiated_describe();
         return 0;
     }
     case Connection::COMPRESSED_OTA: {
-        protocol->enable_compressed_ota();
+        protocol->set_compressed_ota_enabled(value);
         return 0;
     }
     case Connection::SYSTEM_MODULE_VERSION: {

hal/src/nRF52840/ota_flash_hal.cpp

@@ -354,7 +354,10 @@ uint16_t HAL_OTA_ChunkSize()
 
 bool HAL_FLASH_Begin(uint32_t address, uint32_t length, void* reserved)
 {
-    FLASH_Begin(address, length);
+    const int r = FLASH_Begin(address, length);
+    if (r != FLASH_ACCESS_RESULT_OK) {
+        return false;
+    }
     return true;
 }
 

hal/src/stm32f2xx/ota_flash_hal_stm32f2xx.cpp

@@ -250,7 +250,10 @@ uint16_t HAL_OTA_ChunkSize()
 
 bool HAL_FLASH_Begin(uint32_t address, uint32_t length, void* reserved)
 {
-    FLASH_Begin(address, length);
+    const int r = FLASH_Begin(address, length);
+    if (r != FLASH_ACCESS_RESULT_OK) {
+        return false;
+    }
     return true;
 }
 

platform/MCU/STM32F2xx/SPARK_Firmware_Driver/src/flash_mal.c

@@ -873,16 +873,43 @@ uint32_t FLASH_PagesMask(uint32_t imageSize, uint32_t pageSize)
     return numPages;
 }
 
-void FLASH_Begin(uint32_t FLASH_Address, uint32_t imageSize)
+int FLASH_Begin(uint32_t FLASH_Address, uint32_t imageSize)
 {
     system_flags.OTA_FLASHED_Status_SysFlag = 0x0000;
     Save_SystemFlags();
 
+    // Clear all non-factory module slots in the DCT
+    const size_t slot_count = MAX_MODULES_SLOT - GEN_START_SLOT;
+    size_t slot_offs = DCT_FLASH_MODULES_OFFSET + sizeof(platform_flash_modules_t) * GEN_START_SLOT;
+    for (size_t i = 0; i < slot_count; ++i) {
+        uint16_t magic_num = 0;
+        int r = dct_read_app_data_copy(slot_offs + offsetof(platform_flash_modules_t, magicNumber), &magic_num,
+                sizeof(magic_num));
+        if (r != 0) {
+            return FLASH_ACCESS_RESULT_ERROR;
+        }
+        if (magic_num == 0xabcd) {
+            // Mark the slot as unused
+            magic_num = 0xffff;
+            r = dct_write_app_data(&magic_num, slot_offs + offsetof(platform_flash_modules_t, magicNumber),
+                    sizeof(magic_num));
+            if (r != 0) {
+                return FLASH_ACCESS_RESULT_ERROR;
+            }
+        }
+        slot_offs += sizeof(platform_flash_modules_t);
+    }
+
 #ifdef USE_SERIAL_FLASH
-    FLASH_EraseMemory(FLASH_SERIAL, FLASH_Address, imageSize);
+    const bool ok = FLASH_EraseMemory(FLASH_SERIAL, FLASH_Address, imageSize);
 #else
-    FLASH_EraseMemory(FLASH_INTERNAL, FLASH_Address, imageSize);
+    const bool ok = FLASH_EraseMemory(FLASH_INTERNAL, FLASH_Address, imageSize);
 #endif
+    if (!ok) {
+        return FLASH_ACCESS_RESULT_ERROR;
+    }
+
+    return FLASH_ACCESS_RESULT_OK;
 }
 
 int FLASH_Update(const uint8_t *pBuffer, uint32_t address, uint32_t bufferSize)

platform/MCU/nRF52840/inc/flash_access.h

@@ -69,7 +69,7 @@ void FLASH_ClearFlags(void);
 void FLASH_Erase(void);
 void FLASH_Backup(uint32_t FLASH_Address);
 void FLASH_Restore(uint32_t FLASH_Address);
-void FLASH_Begin(uint32_t FLASH_Address, uint32_t imageSize);
+int FLASH_Begin(uint32_t FLASH_Address, uint32_t imageSize);
 int FLASH_Update(const uint8_t *pBuffer, uint32_t address, uint32_t bufferSize);
 void FLASH_End(void);
 

platform/MCU/nRF52840/src/flash_mal.c

@@ -952,16 +952,43 @@ void FLASH_Restore(uint32_t FLASH_Address)
 #endif
 }
 
-void FLASH_Begin(uint32_t FLASH_Address, uint32_t imageSize)
+int FLASH_Begin(uint32_t FLASH_Address, uint32_t imageSize)
 {
     system_flags.OTA_FLASHED_Status_SysFlag = 0x0000;
     Save_SystemFlags();
 
+    // Clear all non-factory module slots in the DCT
+    const size_t slot_count = MAX_MODULES_SLOT - GEN_START_SLOT;
+    size_t slot_offs = DCT_FLASH_MODULES_OFFSET + sizeof(platform_flash_modules_t) * GEN_START_SLOT;
+    for (size_t i = 0; i < slot_count; ++i) {
+        uint16_t magic_num = 0;
+        int r = dct_read_app_data_copy(slot_offs + offsetof(platform_flash_modules_t, magicNumber), &magic_num,
+                sizeof(magic_num));
+        if (r != 0) {
+            return FLASH_ACCESS_RESULT_ERROR;
+        }
+        if (magic_num == 0xabcd) {
+            // Mark the slot as unused
+            magic_num = 0xffff;
+            r = dct_write_app_data(&magic_num, slot_offs + offsetof(platform_flash_modules_t, magicNumber),
+                    sizeof(magic_num));
+            if (r != 0) {
+                return FLASH_ACCESS_RESULT_ERROR;
+            }
+        }
+        slot_offs += sizeof(platform_flash_modules_t);
+    }
+
 #ifdef USE_SERIAL_FLASH
-    FLASH_EraseMemory(FLASH_SERIAL, FLASH_Address, imageSize);
+    const bool ok = FLASH_EraseMemory(FLASH_SERIAL, FLASH_Address, imageSize);
 #else
-    FLASH_EraseMemory(FLASH_INTERNAL, FLASH_Address, imageSize);
+    const bool ok = FLASH_EraseMemory(FLASH_INTERNAL, FLASH_Address, imageSize);
 #endif
+    if (!ok) {
+        return FLASH_ACCESS_RESULT_ERROR;
+    }
+
+    return FLASH_ACCESS_RESULT_OK;
 }
 
 int FLASH_Update(const uint8_t *pBuffer, uint32_t address, uint32_t bufferSize)

platform/MCU/shared/STM32/inc/flash_access.h

@@ -69,7 +69,7 @@ void FLASH_ClearFlags(void);
 void FLASH_Erase(void);
 void FLASH_Backup(uint32_t FLASH_Address);
 void FLASH_Restore(uint32_t FLASH_Address);
-void FLASH_Begin(uint32_t FLASH_Address, uint32_t imageSize);
+int FLASH_Begin(uint32_t FLASH_Address, uint32_t imageSize);
 int FLASH_Update(const uint8_t *pBuffer, uint32_t address, uint32_t bufferSize);
 void FLASH_End(void);
 int FLASH_ReadOTP(uint32_t offset, uint8_t* pBuffer, uint32_t bufferSize);

services/inc/str_util.h

@@ -18,6 +18,7 @@
 #pragma once
 
 #include <cstring>
+#include <cstdint>
 #include <cctype>
 
 namespace particle {
@@ -102,4 +103,32 @@ inline bool endsWith(const char* str, const char* suffix) {
  */
 size_t escape(const char* src, const char* spec, char esc, char* dest, size_t destSize);
 
+/**
+ * Converts binary data to a hex-encoded string. The output is always null-terminated, unless the
+ * size of the destination buffer is 0.
+ *
+ * @param src Source data.
+ * @param srcSize Size of the source data.
+ * @param dest Destination buffer.
+ * @param destSize Size of the destination buffer.
+ *
+ * @return Number of characters written to the destination buffer, not including the trailing `\0`.
+ */
+inline size_t toHex(const void* src, size_t srcSize, char* dest, size_t destSize) {
+    static const char alpha[16] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };
+    size_t n = 0;
+    auto srcBytes = (const uint8_t*)src;
+    for (size_t i = 0; i < srcSize && n + 1 < destSize; ++i) {
+        const auto b = srcBytes[i];
+        dest[n++] = alpha[b >> 4];
+        if (n + 1 < destSize) {
+            dest[n++] = alpha[b & 0x0f];
+        }
+    }
+    if (n < destSize) {
+        dest[n] = '\0';
+    }
+    return n;
+}
+
 } // particle

system/src/system_cloud_internal.cpp

@@ -1079,12 +1079,12 @@ void Spark_Protocol_Init(void)
         spark_protocol_init(sp, (const char*) id, keys, callbacks, descriptor);
 
         // Enable device-initiated describe messages
-        spark_protocol_set_connection_property(sp, protocol::Connection::DEVICE_INITIATED_DESCRIBE, 0, nullptr, nullptr);
+        spark_protocol_set_connection_property(sp, protocol::Connection::ENABLE_DEVICE_INITIATED_DESCRIBE, 0, nullptr, nullptr);
 
 #if HAL_PLATFORM_COMPRESSED_OTA
         // Enable compressed/combined OTA updates
         if (bootloader_get_version() >= COMPRESSED_OTA_MIN_BOOTLOADER_VERSION) {
-            spark_protocol_set_connection_property(sp, protocol::Connection::COMPRESSED_OTA, 0, nullptr, nullptr);
+            spark_protocol_set_connection_property(sp, protocol::Connection::COMPRESSED_OTA, 1, nullptr, nullptr);
         }
 #endif // HAL_PLATFORM_COMPRESSED_OTA
 

test/unit_tests/services/str_util.cpp

@@ -22,7 +22,7 @@
 using namespace particle;
 
 TEST_CASE("escape()") {
-    char buf[10] = {};
+    char buf[100] = {};
 
     SECTION("source string can be empty") {
         auto n = escape("", "", '@', buf, sizeof(buf));
@@ -50,7 +50,7 @@ TEST_CASE("escape()") {
         CHECK(strcmp(buf, "abcd") == 0);
     }
     SECTION("destination buffer can be smaller than the source string") {
-        auto n = escape("abcdefghijklmnop", "ap", '@', buf, sizeof(buf));
+        auto n = escape("abcdefghijklmnop", "ap", '@', buf, 10);
         CHECK(n == 18); // Size of the entire escaped string
         CHECK(strcmp(buf, "@abcdefgh") == 0);
     }
@@ -59,3 +59,44 @@ TEST_CASE("escape()") {
         CHECK(n == 6);
     }
 }
+
+TEST_CASE("toHex()") {
+    char buf[100] = {};
+
+    SECTION("source data can be empty") {
+        auto n = toHex(nullptr, 0, buf, sizeof(buf));
+        CHECK(n == 0);
+    }
+    SECTION("null-terminates the destination buffer") {
+        memset(buf, 0xff, sizeof(buf));
+        auto n = toHex(nullptr, 0, buf, sizeof(buf));
+        CHECK(n == 0);
+        CHECK(buf[0] == '\0');
+    }
+    SECTION("converts data to hex correctly") {
+        auto n = toHex("\x01\x23\x45\x67\x89\xab\xcd\xef", 8, buf, sizeof(buf));
+        CHECK(n == 16);
+        CHECK(strcmp(buf, "0123456789abcdef") == 0);
+    }
+    SECTION("destination buffer can be smaller than necessary to store the entire string") {
+        memset(buf, 0xff, sizeof(buf));
+        auto n = toHex("\x01\x23\x45\x67\x89\xab\xcd\xef", 8, buf, 1);
+        CHECK(n == 0);
+        CHECK(strcmp(buf, "") == 0);
+        n = toHex("\x01\x23\x45\x67\x89\xab\xcd\xef", 8, buf, 2);
+        CHECK(n == 1);
+        CHECK(strcmp(buf, "0") == 0);
+        n = toHex("\x01\x23\x45\x67\x89\xab\xcd\xef", 8, buf, 3);
+        CHECK(n == 2);
+        CHECK(strcmp(buf, "01") == 0);
+        n = toHex("\x01\x23\x45\x67\x89\xab\xcd\xef", 8, buf, 4);
+        CHECK(n == 3);
+        CHECK(strcmp(buf, "012") == 0);
+    }
+    SECTION("destination buffer can be empty") {
+        memset(buf, 0xff, sizeof(buf));
+        auto n = toHex("\x01\x23\x45\x67\x89\xab\xcd\xef", 8, buf, 0);
+        CHECK(n == 0);
+        CHECK((uint8_t)buf[0] == 0xff);
+    }
+}

user/tests/integration/.gitignore

@@ -0,0 +1 @@
+/node_modules/

user/tests/integration/ota/multiple_ota_no_reset/multiple_ota_no_reset.cpp

@@ -0,0 +1,75 @@
+#include "application.h"
+#include "test.h"
+
+#include "storage_hal.h"
+#include "scope_guard.h"
+#include "str_util.h"
+
+namespace {
+
+#if PLATFORM_GEN == 3
+const auto OTA_SECTION_STORAGE = HAL_STORAGE_ID_EXTERNAL_FLASH;
+const auto OTA_SECTION_ADDRESS = EXTERNAL_FLASH_OTA_ADDRESS;
+#elif PLATFORM_ID == PLATFORM_ELECTRON || PLATFORM_ID == PLATFORM_PHOTON || PLATFORM_ID == PLATFORM_P1
+const auto OTA_SECTION_STORAGE = HAL_STORAGE_ID_INTERNAL_FLASH;
+const auto OTA_SECTION_ADDRESS = 0x080c0000; // See the platform's ota_module_bounds.c
+#else
+#error "Unsupported platform"
+#endif
+
+retained char origAppHash[65] = {}; // Hex-encoded
+
+bool getAppHash(char* buf, size_t size) {
+    hal_system_info_t info = {};
+    info.size = sizeof(info);
+    const int r = system_info_get_unstable(&info, 0 /* flags */, nullptr /* reserved */);
+    if (r != 0) {
+        return false;
+    }
+    SCOPE_GUARD({
+        system_info_free_unstable(&info, nullptr /* reserved */);
+    });
+    for (size_t i = 0; i < info.module_count; ++i) {
+        const auto& module = info.modules[i];
+        if (module.info.module_function == MODULE_FUNCTION_USER_PART) {
+            toHex(module.suffix.sha, sizeof(module.suffix.sha), buf, size);
+            return true;
+        }
+    }
+    return false;
+}
+
+} // namespace
+
+test(01_disable_resets_and_connect) {
+#if HAL_PLATFORM_COMPRESSED_OTA
+    // Disable compressed OTA updates so that it's easier to mess with module checksums
+    spark_protocol_set_connection_property(spark_protocol_instance(), protocol::Connection::COMPRESSED_OTA,
+            0 /* value */, nullptr /* data */, nullptr /* reserved */);
+#endif
+    System.disableReset();
+    Particle.connect();
+    waitUntil(Particle.connected);
+}
+
+test(02_flash_binaries) {
+    // Get the original app hash prior to sending any updates to the device
+    assertTrue(getAppHash(origAppHash, sizeof(origAppHash)));
+}
+
+test(03_fix_ota_binary_and_reset) {
+    // The original issue (https://github.com/particle-iot/device-os/pull/2346) can't be reproduced
+    // easily by flashing application binaries. As a workaround, we're restoring the correct platform
+    // ID in the binary that is currently stored in the OTA section so that the bootloader can apply
+    // it if there still happens to be a valid module slot in the DCT
+    const uint16_t id = PLATFORM_ID;
+    assertEqual(hal_storage_write(OTA_SECTION_STORAGE, OTA_SECTION_ADDRESS + offsetof(module_info_t, platform_id),
+            (const uint8_t*)&id, sizeof(id)), sizeof(id));
+}
+
+test(04_validate_module_info) {
+    char appHash[65] = {};
+    assertTrue(getAppHash(appHash, sizeof(appHash)));
+    // The app hash should not have changed
+    assertEqual(strcmp(appHash, origAppHash), 0);
+}