Skip to content

Commit 07bb78f

Browse files
aentingeraentinger
authored
Merge pull request #138 from arduino-libraries/ota-use-temp-file-for-download
Use temporary file for storing the downloaded OTA image.
2 parents 1ee31e5 + 248ce9c commit 07bb78f

File tree

6 files changed

+94
-23
lines changed

6 files changed

+94
-23
lines changed

‎extras/test/src/test_OTALogic.cpp

+54-4
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ TEST_CASE("OTAStorage initialisation fails", "[OTAStorage::init() -> returns fal
5959
Fake(Method(ota_storage, write));
6060
Fake(Method(ota_storage, close));
6161
Fake(Method(ota_storage, remove));
62+
Fake(Method(ota_storage, rename));
6263
Fake(Method(ota_storage, deinit));
6364

6465

@@ -92,6 +93,7 @@ TEST_CASE("OTAStorage opening of storage file fails", "[OTAStorage::open() -> re
9293
Fake(Method(ota_storage, write));
9394
Fake(Method(ota_storage, close));
9495
Fake(Method(ota_storage, remove));
96+
Fake(Method(ota_storage, rename));
9597
Fake(Method(ota_storage, deinit));
9698

9799

@@ -129,6 +131,7 @@ TEST_CASE("OTAStorage writing to storage file fails", "[OTAStorage::write() -> f
129131
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 */;});
130132
Fake(Method(ota_storage, close));
131133
Fake(Method(ota_storage, remove));
134+
Fake(Method(ota_storage, rename));
132135
Fake(Method(ota_storage, deinit));
133136

134137

@@ -165,6 +168,7 @@ TEST_CASE("Data overrun due to receiving too much data", "[OTALogic - Data Overr
165168
When(Method(ota_storage, write)).AlwaysDo([](uint8_t const * const /* buf */, size_t const num_bytes) -> size_t { return num_bytes; });
166169
Fake(Method(ota_storage, close));
167170
Fake(Method(ota_storage, remove));
171+
Fake(Method(ota_storage, rename));
168172
Fake(Method(ota_storage, deinit));
169173

170174

@@ -206,9 +210,10 @@ TEST_CASE("Valid OTA data is received ", "[OTALogic]")
206210
std::copy(buf, buf + num_bytes, std::back_inserter(ota_binary_data));
207211
return num_bytes;
208212
});
209-
Fake(Method(ota_storage, close));
210-
Fake(Method(ota_storage, remove));
211-
Fake(Method(ota_storage, deinit));
213+
Fake(Method(ota_storage, close));
214+
Fake(Method(ota_storage, remove));
215+
When(Method(ota_storage, rename)).Return(true);
216+
Fake(Method(ota_storage, deinit));
212217

213218

214219
/* Generate test data */
@@ -231,6 +236,11 @@ TEST_CASE("Valid OTA data is received ", "[OTALogic]")
231236
valid_ota_test_data.data.bin));
232237
}
233238

239+
THEN("The temporary file UPDATE.BIN.TMP should have been renamed to UPDATE.BIN")
240+
{
241+
Verify(Method(ota_storage, rename)).Once();
242+
}
243+
234244
THEN("The OTA logic should be in the 'Reset' state")
235245
{
236246
REQUIRE(ota_logic.state() == OTAState::Reset);
@@ -244,6 +254,45 @@ TEST_CASE("Valid OTA data is received ", "[OTALogic]")
244254

245255
/**************************************************************************************/
246256

257+
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]")
258+
{
259+
Mock<OTAStorage> ota_storage;
260+
261+
/* Configure mock object */
262+
When(Method(ota_storage, init)).Return(true);
263+
When(Method(ota_storage, open)).Return(true);
264+
When(Method(ota_storage, write)).AlwaysDo([](uint8_t const * const /* buf */, size_t const num_bytes) -> size_t { return num_bytes; });
265+
Fake(Method(ota_storage, close));
266+
Fake(Method(ota_storage, remove));
267+
When(Method(ota_storage, rename)).Return(false);
268+
Fake(Method(ota_storage, deinit));
269+
270+
271+
/* Generate test data */
272+
ota::OTAData valid_ota_test_data;
273+
ota::generate_valid_ota_data(valid_ota_test_data);
274+
275+
276+
/* Perform test */
277+
OTALogic ota_logic;
278+
ota_logic.setOTAStorage(ota_storage.get());
279+
simulateOTABinaryReception(ota_logic, valid_ota_test_data);
280+
281+
282+
/* Perform checks */
283+
THEN("The OTA logic should be in the 'Error' state")
284+
{
285+
REQUIRE(ota_logic.state() == OTAState::Error);
286+
}
287+
288+
THEN("The OTA error should be set to OTAError::RenameOfTempFileFailed")
289+
{
290+
REQUIRE(ota_logic.error() == OTAError::RenameOfTempFileFailed);
291+
}
292+
}
293+
294+
/**************************************************************************************/
295+
247296
TEST_CASE("Invalid OTA data is received ", "[OTALogic - CRC wrong]")
248297
{
249298
Mock<OTAStorage> ota_storage;
@@ -255,6 +304,7 @@ TEST_CASE("Invalid OTA data is received ", "[OTALogic - CRC wrong]")
255304
When(Method(ota_storage, write)).AlwaysDo([](uint8_t const * const /* buf */, size_t const num_bytes) -> size_t { return num_bytes; });
256305
Fake(Method(ota_storage, close));
257306
Fake(Method(ota_storage, remove));
307+
Fake(Method(ota_storage, rename));
258308
Fake(Method(ota_storage, deinit));
259309

260310

@@ -268,7 +318,7 @@ TEST_CASE("Invalid OTA data is received ", "[OTALogic - CRC wrong]")
268318
ota_logic.setOTAStorage(ota_storage.get());
269319
simulateOTABinaryReception(ota_logic, invalid_valid_ota_test_data_crc_wrong);
270320

271-
321+
272322
/* Perform checks */
273323
THEN("there should be no binary file be stored on the OTA storage")
274324
{

‎src/utility/ota/OTALogic.cpp

+16-4
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ OTAError OTALogic::update()
7676
case OTAState::WaitForBinary: _ota_state = handle_WaitForBinary (); break;
7777
case OTAState::BinaryReceived: _ota_state = handle_BinaryReceived(); break;
7878
case OTAState::Verify: _ota_state = handle_Verify (); break;
79+
case OTAState::Rename: _ota_state = handle_Rename (); break;
7980
case OTAState::Reset: _ota_state = handle_Reset (); break;
8081
case OTAState::Error: break;
8182
}
@@ -123,7 +124,7 @@ OTAState OTALogic::handle_Idle()
123124

124125
OTAState OTALogic::handle_StartDownload()
125126
{
126-
if(_ota_storage->open()) {
127+
if(_ota_storage->open("UPDATE.BIN.TMP")) {
127128
return OTAState::WaitForHeader;
128129
} else {
129130
_ota_error = OTAError::StorageOpenFailed;
@@ -212,15 +213,26 @@ OTAState OTALogic::handle_BinaryReceived()
212213
OTAState OTALogic::handle_Verify()
213214
{
214215
if(_ota_bin_data.crc32 == _ota_bin_data.hdr_crc32) {
215-
_ota_storage->deinit();
216-
return OTAState::Reset;
216+
return OTAState::Rename;
217217
} else {
218-
_ota_storage->remove();
218+
_ota_storage->remove("UPDATE.BIN.TMP");
219219
_ota_error = OTAError::ChecksumMismatch;
220220
return OTAState::Error;
221221
}
222222
}
223223

224+
OTAState OTALogic::handle_Rename()
225+
{
226+
if(_ota_storage->rename("UPDATE.BIN.TMP", "UPDATE.BIN")) {
227+
_ota_storage->deinit();
228+
return OTAState::Reset;
229+
}
230+
else {
231+
_ota_error = OTAError::RenameOfTempFileFailed;
232+
return OTAState::Error;
233+
}
234+
}
235+
224236
OTAState OTALogic::handle_Reset()
225237
{
226238
#if !defined(HOST) && !defined(ESP8266)

‎src/utility/ota/OTALogic.h

+9-7
Original file line numberDiff line numberDiff line change
@@ -45,17 +45,18 @@ static size_t const MQTT_OTA_BUF_SIZE = 256;
4545

4646
enum class OTAState
4747
{
48-
Init, Idle, StartDownload, WaitForHeader, HeaderReceived, WaitForBinary, BinaryReceived, Verify, Reset, Error
48+
Init, Idle, StartDownload, WaitForHeader, HeaderReceived, WaitForBinary, BinaryReceived, Verify, Rename, Reset, Error
4949
};
5050

5151
enum class OTAError : int
5252
{
53-
None = 0,
54-
StorageInitFailed = 1,
55-
StorageOpenFailed = 2,
56-
StorageWriteFailed = 3,
57-
ChecksumMismatch = 4,
58-
ReceivedDataOverrun = 5
53+
None = 0,
54+
StorageInitFailed = 1,
55+
StorageOpenFailed = 2,
56+
StorageWriteFailed = 3,
57+
ChecksumMismatch = 4,
58+
ReceivedDataOverrun = 5,
59+
RenameOfTempFileFailed = 6
5960
};
6061

6162
/******************************************************************************
@@ -114,6 +115,7 @@ class OTALogic
114115
OTAState handle_WaitForBinary();
115116
OTAState handle_BinaryReceived();
116117
OTAState handle_Verify();
118+
OTAState handle_Rename();
117119
OTAState handle_Reset();
118120

119121
void init_mqtt_ota_buffer();

‎src/utility/ota/OTAStorage.h

+3-2
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,11 @@ class OTAStorage
4545

4646
virtual Type type () = 0;
4747
virtual bool init () = 0;
48-
virtual bool open () = 0;
48+
virtual bool open (char const * file_name) = 0;
4949
virtual size_t write (uint8_t const * const buf, size_t const num_bytes) = 0;
5050
virtual void close () = 0;
51-
virtual void remove() = 0;
51+
virtual void remove(char const * file_name) = 0;
52+
virtual bool rename(char const * old_file_name, char const * new_file_name) = 0;
5253
virtual void deinit() = 0;
5354

5455
};

‎src/utility/ota/OTAStorage_MKRMEM.cpp

+9-4
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,10 @@ bool OTAStorage_MKRMEM::init()
5454
return true;
5555
}
5656

57-
bool OTAStorage_MKRMEM::open()
57+
bool OTAStorage_MKRMEM::open(char const * file_name)
5858
{
5959
filesystem.clearerr();
60-
_file = new File(filesystem.open("UPDATE.BIN", CREATE | WRITE_ONLY| TRUNCATE));
60+
_file = new File(filesystem.open(file_name, CREATE | WRITE_ONLY| TRUNCATE));
6161
if(SPIFFS_OK != filesystem.err()) {
6262
Debug.print(DBG_ERROR, "OTAStorage_MKRMEM::open - open() failed with error code %d", filesystem.err());
6363
delete _file;
@@ -77,9 +77,14 @@ void OTAStorage_MKRMEM::close()
7777
delete _file;
7878
}
7979

80-
void OTAStorage_MKRMEM::remove()
80+
void OTAStorage_MKRMEM::remove(char const * file_name)
8181
{
82-
filesystem.remove("UPDATE.BIN");
82+
filesystem.remove(file_name);
83+
}
84+
85+
bool OTAStorage_MKRMEM::rename(char const * old_file_name, char const * new_file_name)
86+
{
87+
return (SPIFFS_OK == filesystem.rename(old_file_name, new_file_name));
8388
}
8489

8590
void OTAStorage_MKRMEM::deinit()

‎src/utility/ota/OTAStorage_MKRMEM.h

+3-2
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,11 @@ class OTAStorage_MKRMEM : public OTAStorage
4343

4444
virtual Type type () override { return Type::MKRMEM; }
4545
virtual bool init () override;
46-
virtual bool open () override;
46+
virtual bool open (char const * file_name) override;
4747
virtual size_t write (uint8_t const * const buf, size_t const num_bytes) override;
4848
virtual void close () override;
49-
virtual void remove() override;
49+
virtual void remove(char const * file_name) override;
50+
virtual bool rename(char const * old_file_name, char const * new_file_name) override;
5051
virtual void deinit() override;
5152

5253

0 commit comments

Comments
 (0)