From 664ca5e009fc966c1fbba5a651aed2fbf77c8ce2 Mon Sep 17 00:00:00 2001 From: Alexander Entinger Date: Tue, 9 Jun 2020 11:00:08 +0200 Subject: [PATCH 1/3] Redefine open/remove to allow the handing over of the file parameter as well as adding function for renaming a file on the OTA storage medium. --- src/utility/ota/OTAStorage.h | 5 +++-- src/utility/ota/OTAStorage_MKRMEM.cpp | 13 +++++++++---- src/utility/ota/OTAStorage_MKRMEM.h | 5 +++-- 3 files changed, 15 insertions(+), 8 deletions(-) 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; From 44acce9a3a112ae018b3bc5bb7415f3eb9a74f18 Mon Sep 17 00:00:00 2001 From: Alexander Entinger Date: Tue, 9 Jun 2020 11:02:32 +0200 Subject: [PATCH 2/3] Handle a dedicated 'rename' step in which the file used for downloading (UPDATE.BIN.TMP) is renamed to the one expected by the SFU (UPDATE.BIN). This step is only performed when the complete file with valid checksum has been received --- src/utility/ota/OTALogic.cpp | 20 ++++++++++++++++---- src/utility/ota/OTALogic.h | 16 +++++++++------- 2 files changed, 25 insertions(+), 11 deletions(-) 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(); From 248ce9c09395001df638485f87032fdcf187de9b Mon Sep 17 00:00:00 2001 From: Alexander Entinger Date: Tue, 9 Jun 2020 11:03:00 +0200 Subject: [PATCH 3/3] Extending unit test code to incorporate the extra step --- extras/test/src/test_OTALogic.cpp | 58 ++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 4 deletions(-) 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") {