diff --git a/extras/test/src/test_OTALogic.cpp b/extras/test/src/test_OTALogic.cpp index ba3833623..505bb6fb5 100644 --- a/extras/test/src/test_OTALogic.cpp +++ b/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 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 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") { diff --git a/src/utility/ota/OTALogic.cpp b/src/utility/ota/OTALogic.cpp index ea8216511..698fd0e4f 100644 --- a/src/utility/ota/OTALogic.cpp +++ b/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) diff --git a/src/utility/ota/OTALogic.h b/src/utility/ota/OTALogic.h index 87bd4d039..f6d5bd083 100644 --- a/src/utility/ota/OTALogic.h +++ b/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(); diff --git a/src/utility/ota/OTAStorage.h b/src/utility/ota/OTAStorage.h index 2f690856b..29a09c752 100644 --- a/src/utility/ota/OTAStorage.h +++ b/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; }; diff --git a/src/utility/ota/OTAStorage_MKRMEM.cpp b/src/utility/ota/OTAStorage_MKRMEM.cpp index 5ab44f3ee..f7b3aabd9 100644 --- a/src/utility/ota/OTAStorage_MKRMEM.cpp +++ b/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() diff --git a/src/utility/ota/OTAStorage_MKRMEM.h b/src/utility/ota/OTAStorage_MKRMEM.h index 2bf75ed0c..d4cf3e4fd 100644 --- a/src/utility/ota/OTAStorage_MKRMEM.h +++ b/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;