Use temporary file for storing the downloaded OTA image.

extras/test/src/test_OTALogic.cpp

@@ -59,6 +59,7 @@ TEST_CASE("OTAStorage initialisation fails", "[OTAStorage::init() -> returns fal
   Fake(Method(ota_storage, write));
   Fake(Method(ota_storage, close));
   Fake(Method(ota_storage, remove));
+  Fake(Method(ota_storage, rename));
   Fake(Method(ota_storage, deinit));
 
 
@@ -92,6 +93,7 @@ TEST_CASE("OTAStorage opening of storage file fails", "[OTAStorage::open() -> re
   Fake(Method(ota_storage, write));
   Fake(Method(ota_storage, close));
   Fake(Method(ota_storage, remove));
+  Fake(Method(ota_storage, rename));
   Fake(Method(ota_storage, deinit));
 
 
@@ -129,6 +131,7 @@ TEST_CASE("OTAStorage writing to storage file fails", "[OTAStorage::write() -> f
   When(Method(ota_storage, write)).AlwaysDo([](uint8_t const * const /* buf */, size_t const /* num_bytes */) -> size_t { return 0 /* should return num_bytes in case of success */;});
   Fake(Method(ota_storage, close));
   Fake(Method(ota_storage, remove));
+  Fake(Method(ota_storage, rename));
   Fake(Method(ota_storage, deinit));
 
 
@@ -165,6 +168,7 @@ TEST_CASE("Data overrun due to receiving too much data", "[OTALogic - Data Overr
   When(Method(ota_storage, write)).AlwaysDo([](uint8_t const * const /* buf */, size_t const num_bytes) -> size_t { return num_bytes; });
   Fake(Method(ota_storage, close));
   Fake(Method(ota_storage, remove));
+  Fake(Method(ota_storage, rename));
   Fake(Method(ota_storage, deinit));
 
 
@@ -206,9 +210,10 @@ TEST_CASE("Valid OTA data is received ", "[OTALogic]")
       std::copy(buf, buf + num_bytes, std::back_inserter(ota_binary_data));
       return num_bytes;
     });
-  Fake(Method(ota_storage, close)); 
-  Fake(Method(ota_storage, remove)); 
-  Fake(Method(ota_storage, deinit)); 
+  Fake(Method(ota_storage, close));
+  Fake(Method(ota_storage, remove));
+  When(Method(ota_storage, rename)).Return(true);
+  Fake(Method(ota_storage, deinit));
 
 
   /* Generate test data */
@@ -231,6 +236,11 @@ TEST_CASE("Valid OTA data is received ", "[OTALogic]")
                        valid_ota_test_data.data.bin));
   }
 
+  THEN("The temporary file UPDATE.BIN.TMP should have been renamed to UPDATE.BIN")
+  {
+    Verify(Method(ota_storage, rename)).Once();
+  }
+
   THEN("The OTA logic should be in the 'Reset' state")
   {
     REQUIRE(ota_logic.state() == OTAState::Reset);
@@ -244,6 +254,45 @@ TEST_CASE("Valid OTA data is received ", "[OTALogic]")
 
 /**************************************************************************************/
 
+TEST_CASE("Valid OTA data is received but the rename step failed (identical too device being turned off during writing of file)", "[OTALogic - Rename fail]")
+{
+  Mock<OTAStorage> ota_storage;
+
+  /* Configure mock object */
+  When(Method(ota_storage, init)).Return(true);
+  When(Method(ota_storage, open)).Return(true);
+  When(Method(ota_storage, write)).AlwaysDo([](uint8_t const * const /* buf */, size_t const num_bytes) -> size_t { return num_bytes; });
+  Fake(Method(ota_storage, close));
+  Fake(Method(ota_storage, remove));
+  When(Method(ota_storage, rename)).Return(false);
+  Fake(Method(ota_storage, deinit));
+
+
+  /* Generate test data */
+  ota::OTAData valid_ota_test_data;
+  ota::generate_valid_ota_data(valid_ota_test_data);
+
+
+  /* Perform test */
+  OTALogic ota_logic;
+  ota_logic.setOTAStorage(ota_storage.get());
+  simulateOTABinaryReception(ota_logic, valid_ota_test_data);
+
+
+  /* Perform checks */
+  THEN("The OTA logic should be in the 'Error' state")
+  {
+    REQUIRE(ota_logic.state() == OTAState::Error);
+  }
+
+  THEN("The OTA error should be set to OTAError::RenameOfTempFileFailed")
+  {
+    REQUIRE(ota_logic.error() == OTAError::RenameOfTempFileFailed);
+  }
+}
+
+/**************************************************************************************/
+
 TEST_CASE("Invalid OTA data is received ", "[OTALogic - CRC wrong]")
 {
   Mock<OTAStorage> ota_storage;
@@ -255,6 +304,7 @@ TEST_CASE("Invalid OTA data is received ", "[OTALogic - CRC wrong]")
   When(Method(ota_storage, write)).AlwaysDo([](uint8_t const * const /* buf */, size_t const num_bytes) -> size_t { return num_bytes; });
   Fake(Method(ota_storage, close)); 
   Fake(Method(ota_storage, remove)); 
+  Fake(Method(ota_storage, rename));
   Fake(Method(ota_storage, deinit)); 
 
 
@@ -268,7 +318,7 @@ TEST_CASE("Invalid OTA data is received ", "[OTALogic - CRC wrong]")
   ota_logic.setOTAStorage(ota_storage.get());
   simulateOTABinaryReception(ota_logic, invalid_valid_ota_test_data_crc_wrong);
 
-  
+
   /* Perform checks */
   THEN("there should be no binary file be stored on the OTA storage")
   {

src/utility/ota/OTALogic.cpp

@@ -76,6 +76,7 @@ OTAError OTALogic::update()
     case OTAState::WaitForBinary:  _ota_state = handle_WaitForBinary (); break;
     case OTAState::BinaryReceived: _ota_state = handle_BinaryReceived(); break;
     case OTAState::Verify:         _ota_state = handle_Verify        (); break;
+    case OTAState::Rename:         _ota_state = handle_Rename        (); break;
     case OTAState::Reset:          _ota_state = handle_Reset         (); break;
     case OTAState::Error:                                                break;
     }
@@ -123,7 +124,7 @@ OTAState OTALogic::handle_Idle()
 
 OTAState OTALogic::handle_StartDownload()
 {
-  if(_ota_storage->open()) {
+  if(_ota_storage->open("UPDATE.BIN.TMP")) {
     return OTAState::WaitForHeader;
   } else {
     _ota_error = OTAError::StorageOpenFailed;
@@ -212,15 +213,26 @@ OTAState OTALogic::handle_BinaryReceived()
 OTAState OTALogic::handle_Verify()
 {
   if(_ota_bin_data.crc32 == _ota_bin_data.hdr_crc32) {
-    _ota_storage->deinit();
-    return OTAState::Reset;
+    return OTAState::Rename;
   } else {
-    _ota_storage->remove();
+    _ota_storage->remove("UPDATE.BIN.TMP");
     _ota_error = OTAError::ChecksumMismatch;
     return OTAState::Error;
   }
 }
 
+OTAState OTALogic::handle_Rename()
+{
+  if(_ota_storage->rename("UPDATE.BIN.TMP", "UPDATE.BIN")) {
+    _ota_storage->deinit();
+    return OTAState::Reset;
+  }
+  else {
+    _ota_error = OTAError::RenameOfTempFileFailed;
+    return OTAState::Error;
+  }
+}
+
 OTAState OTALogic::handle_Reset()
 {
 #if !defined(HOST) && !defined(ESP8266)

src/utility/ota/OTALogic.h

@@ -45,17 +45,18 @@ static size_t const MQTT_OTA_BUF_SIZE = 256;
 
 enum class OTAState
 {
-  Init, Idle, StartDownload, WaitForHeader, HeaderReceived, WaitForBinary, BinaryReceived, Verify, Reset, Error
+  Init, Idle, StartDownload, WaitForHeader, HeaderReceived, WaitForBinary, BinaryReceived, Verify, Rename, Reset, Error
 };
 
 enum class OTAError : int
 {
-  None                = 0,
-  StorageInitFailed   = 1,
-  StorageOpenFailed   = 2,
-  StorageWriteFailed  = 3,
-  ChecksumMismatch    = 4,
-  ReceivedDataOverrun = 5
+  None                   = 0,
+  StorageInitFailed      = 1,
+  StorageOpenFailed      = 2,
+  StorageWriteFailed     = 3,
+  ChecksumMismatch       = 4,
+  ReceivedDataOverrun    = 5,
+  RenameOfTempFileFailed = 6
 };
 
 /******************************************************************************
@@ -114,6 +115,7 @@ class OTALogic
   OTAState handle_WaitForBinary();
   OTAState handle_BinaryReceived();
   OTAState handle_Verify();
+  OTAState handle_Rename();
   OTAState handle_Reset();
 
   void init_mqtt_ota_buffer();

src/utility/ota/OTAStorage.h

@@ -45,10 +45,11 @@ class OTAStorage
 
   virtual Type   type  () = 0;
   virtual bool   init  () = 0;
-  virtual bool   open  () = 0;
+  virtual bool   open  (char const * file_name) = 0;
   virtual size_t write (uint8_t const * const buf, size_t const num_bytes) = 0;
   virtual void   close () = 0;
-  virtual void   remove() = 0;
+  virtual void   remove(char const * file_name) = 0;
+  virtual bool   rename(char const * old_file_name, char const * new_file_name) = 0;
   virtual void   deinit() = 0;
 
 };

src/utility/ota/OTAStorage_MKRMEM.cpp

@@ -54,10 +54,10 @@ bool OTAStorage_MKRMEM::init()
   return true;
 }
 
-bool OTAStorage_MKRMEM::open()
+bool OTAStorage_MKRMEM::open(char const * file_name)
 {
   filesystem.clearerr();
-  _file = new File(filesystem.open("UPDATE.BIN", CREATE | WRITE_ONLY| TRUNCATE));
+  _file = new File(filesystem.open(file_name, CREATE | WRITE_ONLY| TRUNCATE));
   if(SPIFFS_OK != filesystem.err()) {
     Debug.print(DBG_ERROR, "OTAStorage_MKRMEM::open - open() failed with error code %d", filesystem.err());
     delete _file;
@@ -77,9 +77,14 @@ void OTAStorage_MKRMEM::close()
   delete _file;
 }
 
-void OTAStorage_MKRMEM::remove()
+void OTAStorage_MKRMEM::remove(char const * file_name)
 {
-  filesystem.remove("UPDATE.BIN");
+  filesystem.remove(file_name);
+}
+
+bool OTAStorage_MKRMEM::rename(char const * old_file_name, char const * new_file_name)
+{
+  return (SPIFFS_OK == filesystem.rename(old_file_name, new_file_name));
 }
 
 void OTAStorage_MKRMEM::deinit()

src/utility/ota/OTAStorage_MKRMEM.h

@@ -43,10 +43,11 @@ class OTAStorage_MKRMEM : public OTAStorage
 
   virtual Type   type  () override { return Type::MKRMEM; }
   virtual bool   init  () override;
-  virtual bool   open  () override;
+  virtual bool   open  (char const * file_name) override;
   virtual size_t write (uint8_t const * const buf, size_t const num_bytes) override;
   virtual void   close () override;
-  virtual void   remove() override;
+  virtual void   remove(char const * file_name) override;
+  virtual bool   rename(char const * old_file_name, char const * new_file_name) override;
   virtual void   deinit() override;