From c92e8ab0864e0a54a5d0f6e5d8d8f5ec20271414 Mon Sep 17 00:00:00 2001 From: Sergey Polyakov Date: Tue, 17 Nov 2020 17:32:06 +0200 Subject: [PATCH] Address review comments --- communication/inc/protocol_defs.h | 35 ---------------- communication/src/firmware_update.cpp | 40 +++++++++---------- communication/src/firmware_update.h | 31 ++++++++++++++ communication/src/protocol_util.cpp | 2 +- hal/src/nRF52840/ota_flash_hal.cpp | 16 ++++---- services/inc/services_dynalib.h | 8 ++-- services/inc/system_error.h | 12 +++--- services/inc/underlying_type.h | 2 +- services/src/system_error.cpp | 12 +++--- system/src/firmware_update.cpp | 19 +++++---- .../communication/firmware_update.cpp | 4 +- user/makefile | 6 ++- wiring/inc/spark_wiring_error.h | 2 +- 13 files changed, 97 insertions(+), 92 deletions(-) diff --git a/communication/inc/protocol_defs.h b/communication/inc/protocol_defs.h index 2081135b0e..87082f0d0b 100644 --- a/communication/inc/protocol_defs.h +++ b/communication/inc/protocol_defs.h @@ -7,8 +7,6 @@ #include "system_error.h" #include "platforms.h" -#include "mbedtls_config.h" - typedef uint16_t product_id_t; typedef uint16_t product_firmware_version_t; @@ -24,39 +22,6 @@ namespace particle { namespace protocol { #define MAX_SUBSCRIPTIONS (6) // 2 system and 4 application -#if HAL_PLATFORM_OTA_PROTOCOL_V3 - -/** - * Minimum supported chunk size. - * - * This parameter affects the size of the chunk bitmap maintained by the protocol implementation. - */ -const size_t MIN_OTA_CHUNK_SIZE = 512; - -static_assert(MIN_OTA_CHUNK_SIZE % 4 == 0, "Invalid MIN_OTA_CHUNK_SIZE"); - -/** - * CoAP overhead per chunk message: - * - * Message header (4 bytes) - * Uri-Path option (2 bytes) - * Chunk-Index option (4-7 bytes) - * Payload marker (1 byte) - */ -const size_t OTA_CHUNK_COAP_OVERHEAD = 14; - -static_assert(MBEDTLS_SSL_MAX_CONTENT_LEN >= MIN_OTA_CHUNK_SIZE + OTA_CHUNK_COAP_OVERHEAD, - "MBEDTLS_SSL_MAX_CONTENT_LEN is too small"); - -/** - * Maximum chunk size. - */ -const size_t MAX_OTA_CHUNK_SIZE = (MBEDTLS_SSL_MAX_CONTENT_LEN - OTA_CHUNK_COAP_OVERHEAD) / 4 * 4; - -static_assert(MAX_OTA_CHUNK_SIZE >= MIN_OTA_CHUNK_SIZE, "Invalid MAX_OTA_CHUNK_SIZE"); - -#endif // HAL_PLATFORM_OTA_PROTOCOL_V3 - enum ProtocolError { /* 00 */ NO_ERROR, diff --git a/communication/src/firmware_update.cpp b/communication/src/firmware_update.cpp index db1b6d2ef9..a1cbcfb8d8 100644 --- a/communication/src/firmware_update.cpp +++ b/communication/src/firmware_update.cpp @@ -172,7 +172,7 @@ ProtocolError FirmwareUpdate::process() { } ProtocolError FirmwareUpdate::handleRequest(Message* msg, RequestHandlerFn handler) { - clear_error_message(); + clear_system_error_message(); CoapMessageDecoder d; int r = d.decode((const char*)msg->buf(), msg->length()); if (r < 0) { @@ -205,7 +205,7 @@ ProtocolError FirmwareUpdate::handleRequest(Message* msg, RequestHandlerFn handl } } else { // All confirmable messages defined by the protocol must have a token - ERROR_MESSAGE("Message token is missing"); + SYSTEM_ERROR_MESSAGE("Message token is missing"); r = sendErrorResponse(&ack, SYSTEM_ERROR_PROTOCOL, CoapType::ACK, d.id(), nullptr /* token */, 0 /* tokenSize */); if (r < 0) { return ProtocolError::IO_ERROR_GENERIC_SEND; @@ -353,7 +353,7 @@ int FirmwareUpdate::handleFinishRequest(const CoapMessageDecoder& d, CoapMessage flags |= FirmwareUpdateFlag::CANCEL; } else { if (fileOffset_ != fileSize_) { - ERROR_MESSAGE("Incomplete file transfer"); + SYSTEM_ERROR_MESSAGE("Incomplete file transfer"); return SYSTEM_ERROR_PROTOCOL; } LOG(INFO, "Validating firmware update"); @@ -402,12 +402,12 @@ int FirmwareUpdate::handleChunkRequest(const CoapMessageDecoder& d, CoapMessageE } ++stats_.receivedChunks; if (index == 0 || index > chunkCount_) { // Chunk indices are 1-based - ERROR_MESSAGE("Invalid chunk index: %u", index); + SYSTEM_ERROR_MESSAGE("Invalid chunk index: %u", index); return SYSTEM_ERROR_PROTOCOL; } if ((index < chunkCount_ && size != chunkSize_) || (index == chunkCount_ && (index - 1) * chunkSize_ + size != transferSize_)) { - ERROR_MESSAGE("Invalid chunk size: %u", (unsigned)size); + SYSTEM_ERROR_MESSAGE("Invalid chunk size: %u", (unsigned)size); return SYSTEM_ERROR_PROTOCOL; } bool isDupChunk = false; @@ -494,11 +494,11 @@ int FirmwareUpdate::handleChunkRequest(const CoapMessageDecoder& d, CoapMessageE int FirmwareUpdate::decodeStartRequest(const CoapMessageDecoder& d, size_t* fileSize, const char** fileHash, size_t* chunkSize, bool* discardData) { if (d.type() != CoapType::CON) { - ERROR_MESSAGE("Invalid message type"); + SYSTEM_ERROR_MESSAGE("Invalid message type"); return SYSTEM_ERROR_PROTOCOL; } if (!d.hasToken()) { - ERROR_MESSAGE("Invalid token size"); + SYSTEM_ERROR_MESSAGE("Invalid token size"); return SYSTEM_ERROR_PROTOCOL; } bool hasFileSize = false; @@ -511,7 +511,7 @@ int FirmwareUpdate::decodeStartRequest(const CoapMessageDecoder& d, size_t* file case OtaCoapOption::FILE_SIZE: { const size_t size = it.toUInt(); if (!size) { - ERROR_MESSAGE("Invalid file size: %u", (unsigned)size); + SYSTEM_ERROR_MESSAGE("Invalid file size: %u", (unsigned)size); return SYSTEM_ERROR_PROTOCOL; } *fileSize = size; @@ -520,7 +520,7 @@ int FirmwareUpdate::decodeStartRequest(const CoapMessageDecoder& d, size_t* file } case OtaCoapOption::FILE_SHA256: { if (it.size() != Sha256::HASH_SIZE) { - ERROR_MESSAGE("Invalid option size"); + SYSTEM_ERROR_MESSAGE("Invalid option size"); return SYSTEM_ERROR_PROTOCOL; } *fileHash = it.data(); @@ -530,7 +530,7 @@ int FirmwareUpdate::decodeStartRequest(const CoapMessageDecoder& d, size_t* file case OtaCoapOption::CHUNK_SIZE: { const size_t size = it.toUInt(); if (size < MIN_OTA_CHUNK_SIZE || size > MAX_OTA_CHUNK_SIZE || size % 4 != 0) { - ERROR_MESSAGE("Invalid chunk size: %u", (unsigned)size); + SYSTEM_ERROR_MESSAGE("Invalid chunk size: %u", (unsigned)size); return SYSTEM_ERROR_PROTOCOL; } *chunkSize = size; @@ -539,7 +539,7 @@ int FirmwareUpdate::decodeStartRequest(const CoapMessageDecoder& d, size_t* file } case OtaCoapOption::DISCARD_DATA: { if (it.size() != 0) { - ERROR_MESSAGE("Invalid option size"); + SYSTEM_ERROR_MESSAGE("Invalid option size"); return SYSTEM_ERROR_PROTOCOL; } *discardData = true; @@ -551,7 +551,7 @@ int FirmwareUpdate::decodeStartRequest(const CoapMessageDecoder& d, size_t* file } } if (!hasFileSize || !hasChunkSize) { - ERROR_MESSAGE("Invalid message options"); + SYSTEM_ERROR_MESSAGE("Invalid message options"); return SYSTEM_ERROR_PROTOCOL; } if (!hasFileHash) { @@ -565,11 +565,11 @@ int FirmwareUpdate::decodeStartRequest(const CoapMessageDecoder& d, size_t* file int FirmwareUpdate::decodeFinishRequest(const CoapMessageDecoder& d, bool* cancelUpdate, bool* discardData) { if (d.type() != CoapType::CON) { - ERROR_MESSAGE("Invalid message type"); + SYSTEM_ERROR_MESSAGE("Invalid message type"); return SYSTEM_ERROR_PROTOCOL; } if (!d.hasToken()) { - ERROR_MESSAGE("Invalid token size"); + SYSTEM_ERROR_MESSAGE("Invalid token size"); return SYSTEM_ERROR_PROTOCOL; } bool hasCancelUpdate = false; @@ -579,7 +579,7 @@ int FirmwareUpdate::decodeFinishRequest(const CoapMessageDecoder& d, bool* cance switch (it.option()) { case OtaCoapOption::CANCEL_UPDATE: { if (it.size() != 0) { - ERROR_MESSAGE("Invalid option size"); + SYSTEM_ERROR_MESSAGE("Invalid option size"); return SYSTEM_ERROR_PROTOCOL; } *cancelUpdate = true; @@ -588,7 +588,7 @@ int FirmwareUpdate::decodeFinishRequest(const CoapMessageDecoder& d, bool* cance } case OtaCoapOption::DISCARD_DATA: { if (it.size() != 0) { - ERROR_MESSAGE("Invalid option size"); + SYSTEM_ERROR_MESSAGE("Invalid option size"); return SYSTEM_ERROR_PROTOCOL; } *discardData = true; @@ -611,20 +611,20 @@ int FirmwareUpdate::decodeFinishRequest(const CoapMessageDecoder& d, bool* cance int FirmwareUpdate::decodeChunkRequest(const CoapMessageDecoder& d, const char** chunkData, size_t* chunkSize, unsigned* chunkIndex) { if (d.type() != CoapType::NON) { - ERROR_MESSAGE("Invalid message type"); + SYSTEM_ERROR_MESSAGE("Invalid message type"); return SYSTEM_ERROR_PROTOCOL; } if (d.hasToken()) { - ERROR_MESSAGE("Invalid token size"); + SYSTEM_ERROR_MESSAGE("Invalid token size"); return SYSTEM_ERROR_PROTOCOL; } if (d.payloadSize() == 0) { - ERROR_MESSAGE("Invalid payload size"); + SYSTEM_ERROR_MESSAGE("Invalid payload size"); return SYSTEM_ERROR_PROTOCOL; } const auto it = d.findOption(OtaCoapOption::CHUNK_INDEX); if (!it) { - ERROR_MESSAGE("Invalid message options"); + SYSTEM_ERROR_MESSAGE("Invalid message options"); return SYSTEM_ERROR_PROTOCOL; } *chunkIndex = it.toUInt(); diff --git a/communication/src/firmware_update.h b/communication/src/firmware_update.h index bf5ffc397a..2a44af314e 100644 --- a/communication/src/firmware_update.h +++ b/communication/src/firmware_update.h @@ -27,6 +27,8 @@ #include "system_defs.h" +#include "mbedtls_config.h" + #include #include @@ -34,6 +36,35 @@ namespace particle { namespace protocol { +/** + * Minimum supported chunk size. + * + * This parameter affects the size of the chunk bitmap maintained by the protocol implementation. + */ +const size_t MIN_OTA_CHUNK_SIZE = 512; + +static_assert(MIN_OTA_CHUNK_SIZE % 4 == 0, "Invalid MIN_OTA_CHUNK_SIZE"); + +/** + * CoAP overhead per chunk message: + * + * Message header (4 bytes) + * Uri-Path option (2 bytes) + * Chunk-Index option (4-7 bytes) + * Payload marker (1 byte) + */ +const size_t OTA_CHUNK_COAP_OVERHEAD = 14; + +static_assert(MBEDTLS_SSL_MAX_CONTENT_LEN >= MIN_OTA_CHUNK_SIZE + OTA_CHUNK_COAP_OVERHEAD, + "MBEDTLS_SSL_MAX_CONTENT_LEN is too small"); + +/** + * Maximum chunk size. + */ +const size_t MAX_OTA_CHUNK_SIZE = (MBEDTLS_SSL_MAX_CONTENT_LEN - OTA_CHUNK_COAP_OVERHEAD) / 4 * 4; + +static_assert(MAX_OTA_CHUNK_SIZE >= MIN_OTA_CHUNK_SIZE, "Invalid MAX_OTA_CHUNK_SIZE"); + /** * Size of the receiver window in bytes. * diff --git a/communication/src/protocol_util.cpp b/communication/src/protocol_util.cpp index b1de89d84b..fd795dec87 100644 --- a/communication/src/protocol_util.cpp +++ b/communication/src/protocol_util.cpp @@ -34,7 +34,7 @@ int formatDiagnosticPayload(char* buf, size_t size, int error) { w.beginObject(); w.name("code").value(error); const size_t codeEnd = w.dataSize(); - w.name("message").value(get_error_message(error)); + w.name("message").value(get_system_error_message(error)); w.endObject(); size_t payloadSize = w.dataSize(); if (size < payloadSize) { diff --git a/hal/src/nRF52840/ota_flash_hal.cpp b/hal/src/nRF52840/ota_flash_hal.cpp index 245e2be6b9..cd66bdd144 100644 --- a/hal/src/nRF52840/ota_flash_hal.cpp +++ b/hal/src/nRF52840/ota_flash_hal.cpp @@ -448,7 +448,7 @@ __attribute__((optimize("O0"))) int fetchModules(hal_module_t* modules, size_t m bool hasNext = true; do { if (!fetch_module(&module, &bounds, userDepsOptional, flags)) { - ERROR_MESSAGE("Unable to fetch module"); + SYSTEM_ERROR_MESSAGE("Unable to fetch module"); return SYSTEM_ERROR_OTA_MODULE_NOT_FOUND; } if (count < maxModuleCount) { @@ -461,7 +461,7 @@ __attribute__((optimize("O0"))) int fetchModules(hal_module_t* modules, size_t m // Forge a module bounds structure for the next module in the OTA region const size_t moduleSize = module_length(info) + 4 /* CRC-32 */; if (bounds.maximum_size < moduleSize) { - ERROR_MESSAGE("Invalid module size"); + SYSTEM_ERROR_MESSAGE("Invalid module size"); return SYSTEM_ERROR_OTA_INVALID_SIZE; } bounds.start_address += moduleSize; @@ -481,7 +481,7 @@ __attribute__((optimize("O0"))) int validateModules(const hal_module_t* modules, LOG(INFO, "Validating module; type: %u; index: %u; version: %u", (unsigned)module_function(info), (unsigned)module_index(info), (unsigned)module_version(info)); if (module->validity_result != module->validity_checked) { - ERROR_MESSAGE("Validation failed; result: 0x%02x; checked: 0x%02x", (unsigned)module->validity_result, + SYSTEM_ERROR_MESSAGE("Validation failed; result: 0x%02x; checked: 0x%02x", (unsigned)module->validity_result, (unsigned)module->validity_checked); return validityResultToSystemError(module->validity_result, module->validity_checked); } @@ -489,12 +489,12 @@ __attribute__((optimize("O0"))) int validateModules(const hal_module_t* modules, const bool compressed = (info->flags & MODULE_INFO_FLAG_COMPRESSED); if (module->module_info_offset > 0 && (dropModuleInfo || compressed)) { // Module with the DROP_MODULE_INFO or COMPRESSED flag set can't have a vector table - ERROR_MESSAGE("Invalid module format"); + SYSTEM_ERROR_MESSAGE("Invalid module format"); return SYSTEM_ERROR_OTA_INVALID_FORMAT; } const auto moduleFunc = module_function(info); if (compressed && moduleFunc != MODULE_FUNCTION_USER_PART && moduleFunc != MODULE_FUNCTION_SYSTEM_PART) { - ERROR_MESSAGE("Unsupported compressed module"); // TODO + SYSTEM_ERROR_MESSAGE("Unsupported compressed module"); // TODO return SYSTEM_ERROR_OTA_UNSUPPORTED_MODULE; } if (moduleFunc == MODULE_FUNCTION_NCP_FIRMWARE) { @@ -502,12 +502,12 @@ __attribute__((optimize("O0"))) int validateModules(const hal_module_t* modules, const auto moduleNcp = module_mcu_target(info); const auto currentNcp = platform_current_ncp_identifier(); if (moduleNcp != currentNcp) { - ERROR_MESSAGE("NCP module is not for this platform; module NCP: 0x%02x; current NCP: 0x%02x", + SYSTEM_ERROR_MESSAGE("NCP module is not for this platform; module NCP: 0x%02x; current NCP: 0x%02x", (unsigned)moduleNcp, (unsigned)currentNcp); return SYSTEM_ERROR_OTA_INVALID_PLATFORM; } #else - ERROR_MESSAGE("NCP module is not updatable on this platform"); + SYSTEM_ERROR_MESSAGE("NCP module is not updatable on this platform"); return SYSTEM_ERROR_OTA_UNSUPPORTED_MODULE; #endif // !HAL_PLATFORM_NCP_UPDATABLE } @@ -597,7 +597,7 @@ __attribute__((optimize("O0"))) int HAL_FLASH_End(void* reserved) const bool ok = FLASH_AddToNextAvailableModulesSlot(FLASH_SERIAL, otaAddr, FLASH_INTERNAL, (uint32_t)info.module_start_address, moduleSize + 4 /* CRC-32 */, moduleFunc, slotFlags); if (!ok) { - ERROR_MESSAGE("No module slot available"); + SYSTEM_ERROR_MESSAGE("No module slot available"); result = SYSTEM_ERROR_NO_MEMORY; } else { result = HAL_UPDATE_APPLIED_PENDING_RESTART; diff --git a/services/inc/services_dynalib.h b/services/inc/services_dynalib.h index 6a93c650bf..fc3eab6ba1 100644 --- a/services/inc/services_dynalib.h +++ b/services/inc/services_dynalib.h @@ -72,7 +72,7 @@ DYNALIB_FN(26, services, log_enabled, int(int, const char*, void*)) DYNALIB_FN(27, services, log_level_name, const char*(int, void*)) DYNALIB_FN(28, services, log_set_callbacks, void(log_message_callback_type, log_write_callback_type, log_enabled_callback_type, void*)) DYNALIB_FN(29, services, set_thread_current_function_pointers, void(void*, void*, void*, void*, void*)) -DYNALIB_FN(30, services, get_default_error_message, const char*(int, void*)) +DYNALIB_FN(30, services, get_default_system_error_message, const char*(int, void*)) DYNALIB_FN(31, services, LED_SetCallbacks, void(LedCallbacks, void*)) DYNALIB_FN(32, services, led_set_status_active, void(LEDStatusData*, int, void*)) DYNALIB_FN(33, services, led_set_update_enabled, void(int, void*)) @@ -110,9 +110,9 @@ DYNALIB_FN(55, services, pb_encode_varint, bool(pb_ostream_t*, pb_uint64_t)) # define BASE_IDX 40 #endif -DYNALIB_FN(BASE_IDX + 0, services, set_error_message, void(const char*, ...)) -DYNALIB_FN(BASE_IDX + 1, services, clear_error_message, void()) -DYNALIB_FN(BASE_IDX + 2, services, get_error_message, const char*(int)) +DYNALIB_FN(BASE_IDX + 0, services, set_system_error_message, void(const char*, ...)) +DYNALIB_FN(BASE_IDX + 1, services, clear_system_error_message, void()) +DYNALIB_FN(BASE_IDX + 2, services, get_system_error_message, const char*(int)) DYNALIB_END(services) diff --git a/services/inc/system_error.h b/services/inc/system_error.h index 28dc3425f2..e8f198c71d 100644 --- a/services/inc/system_error.h +++ b/services/inc/system_error.h @@ -87,10 +87,10 @@ * * This macro also logs the error message under the current logging category. */ -#define ERROR_MESSAGE(_fmt, ...) \ +#define SYSTEM_ERROR_MESSAGE(_fmt, ...) \ do { \ LOG(ERROR, _fmt, ##__VA_ARGS__); \ - set_error_message(_fmt, ##__VA_ARGS__); \ + set_system_error_message(_fmt, ##__VA_ARGS__); \ } while (false) /** @@ -115,12 +115,12 @@ extern "C" { */ // TODO: Perhaps it would be better to not allow overriding the currently set error message until // it's explicitly cleared -void set_error_message(const char* fmt, ...); +void set_system_error_message(const char* fmt, ...); /** * Clear the last error message. */ -void clear_error_message(); +void clear_system_error_message(); /** * Get the last error message. @@ -131,7 +131,7 @@ void clear_error_message(); * If `error` is negative and the last error message is not set, this function will return the * default message for the error code. */ -const char* get_error_message(int error); +const char* get_system_error_message(int error); /** * Get the default error message for the error code. @@ -139,7 +139,7 @@ const char* get_error_message(int error); * @param error Error code. * @return Error message. */ -const char* get_default_error_message(int error, void* reserved); +const char* get_default_system_error_message(int error, void* reserved); #ifdef __cplusplus } // extern "C" diff --git a/services/inc/underlying_type.h b/services/inc/underlying_type.h index 2eb2949c78..bbb3f7941b 100644 --- a/services/inc/underlying_type.h +++ b/services/inc/underlying_type.h @@ -21,7 +21,7 @@ // This macro defines operators for comparing values of an enum with values of an integral type. // This can be useful when handling the result of a function that can either return a negative -// error code or some non-error value defined by a enum class +// error code or some non-error value defined by an enum class #define PARTICLE_DEFINE_ENUM_COMPARISON_OPERATORS(_type) \ inline bool operator==(_type a, std::underlying_type<_type>::type b) { \ return a == (_type)b; \ diff --git a/services/src/system_error.cpp b/services/src/system_error.cpp index 6dcfdcecdc..5fd3e6d2b8 100644 --- a/services/src/system_error.cpp +++ b/services/src/system_error.cpp @@ -57,7 +57,7 @@ char g_errorMsg[ERROR_MESSAGE_BUFFER_SIZE] = {}; } // namespace -void set_error_message(const char* fmt, ...) { +void set_system_error_message(const char* fmt, ...) { #if HAL_PLATFORM_ERROR_MESSAGES if (SYSTEM_THREAD_CURRENT()) { va_list args; @@ -71,7 +71,7 @@ void set_error_message(const char* fmt, ...) { #endif } -void clear_error_message() { +void clear_system_error_message() { #if HAL_PLATFORM_ERROR_MESSAGES if (SYSTEM_THREAD_CURRENT()) { g_errorMsg[0] = '\0'; @@ -79,18 +79,18 @@ void clear_error_message() { #endif } -const char* get_error_message(int error) { +const char* get_system_error_message(int error) { #if HAL_PLATFORM_ERROR_MESSAGES if (!SYSTEM_THREAD_CURRENT() || !g_errorMsg[0]) { - return get_default_error_message(error, nullptr /* reserved */); + return get_default_system_error_message(error, nullptr /* reserved */); } return g_errorMsg; #else - return get_default_error_message(error, nullptr); + return get_default_system_error_message(error, nullptr); #endif } -const char* get_default_error_message(int error, void* reserved) { +const char* get_default_system_error_message(int error, void* reserved) { if (error < 0) { #if HAL_PLATFORM_ERROR_MESSAGES switch (error) { diff --git a/system/src/firmware_update.cpp b/system/src/firmware_update.cpp index d5f6d3f4d8..452b946b39 100644 --- a/system/src/firmware_update.cpp +++ b/system/src/firmware_update.cpp @@ -119,7 +119,7 @@ int FirmwareUpdate::startUpdate(size_t fileSize, const char* fileHash, size_t* p } #endif if (updating_) { - ERROR_MESSAGE("Firmware update is already in progress"); + SYSTEM_ERROR_MESSAGE("Firmware update is already in progress"); return SYSTEM_ERROR_INVALID_STATE; } if (!System.updatesEnabled() && !System.updatesForced()) { @@ -238,7 +238,7 @@ int FirmwareUpdate::saveChunk(const char* chunkData, size_t chunkSize, size_t ch const uintptr_t addr = HAL_OTA_FlashAddress() + chunkOffset; int r = HAL_FLASH_Update((const uint8_t*)chunkData, addr, chunkSize, nullptr); if (r != 0) { - ERROR_MESSAGE("Failed to save firmware data: %d", r); + SYSTEM_ERROR_MESSAGE("Failed to save firmware data: %d", r); endUpdate(false /* ok */); return SYSTEM_ERROR_FLASH_IO; } @@ -344,15 +344,20 @@ int FirmwareUpdate::updateTransferState(const char* chunkData, size_t chunkSize, } const auto persist = &state->persist; const size_t partialSizeBefore = persist->partialSize; - // Check if the chunk is adjacent to or overlaps with the contiguous fragment of the data for - // which we have already calculated the checksum + // Check if the newly stored chunk forms a contiguous block with the data for which we have already + // calculated the checksum if (persist->partialSize >= chunkOffset && persist->partialSize < chunkOffset + chunkSize) { const auto n = chunkOffset + chunkSize - persist->partialSize; CHECK(state->partialHash.update(chunkData + persist->partialSize - chunkOffset, n)); persist->partialSize += n; } - // Chunks are not necessarily transferred sequentially. We may need to read them back from the - // OTA section to calculate the checksum of the data transferred so far + // It is not required to store chunks sequentially. If the newly stored chunk fills a gap between + // the previously stored chunks so that they now form a contiguous block, the checksum has to be + // updated accordingly. + // + // Note that it is the responsibility of the calling code to keep track of the transferred chunks + // and their positions against each other if the underlying protocol allows transferring them out + // of order if (partialSize > persist->partialSize) { char buf[OTA_FLASH_READ_BLOCK_SIZE] = {}; uintptr_t addr = HAL_OTA_FlashAddress() + persist->partialSize; @@ -393,7 +398,7 @@ int FirmwareUpdate::finalizeTransferState() { return SYSTEM_ERROR_OTA_INVALID_SIZE; } if (memcmp(persist->partialHash, persist->fileHash, Sha256::HASH_SIZE) != 0) { - ERROR_MESSAGE("Integrity check of a resumed update has failed"); + SYSTEM_ERROR_MESSAGE("Integrity check of a resumed update has failed"); return SYSTEM_ERROR_OTA_RESUMED_UPDATE_FAILED; } state->file.close(); diff --git a/test/unit_tests/communication/firmware_update.cpp b/test/unit_tests/communication/firmware_update.cpp index 1e363f17aa..bd55542ccc 100644 --- a/test/unit_tests/communication/firmware_update.cpp +++ b/test/unit_tests/communication/firmware_update.cpp @@ -322,7 +322,7 @@ TEST_CASE("FirmwareUpdate") { auto cb = w.callbacksMock(); When(Method(cb, startFirmwareUpdate)).Do([](size_t fileSize, const char* fileHash, size_t* partialSize, unsigned flags) { - ERROR_MESSAGE("YOU SHALL NOT PASS!"); + SYSTEM_ERROR_MESSAGE("YOU SHALL NOT PASS!"); return SYSTEM_ERROR_NOT_ALLOWED; }); w.sendStart(1000 /* fileSize */, std::string() /* fileHash */, 512 /* chunkSize */, false /* discardData */); @@ -798,7 +798,7 @@ TEST_CASE("FirmwareUpdate") { } SECTION("failed validation") { When(Method(cb, finishFirmwareUpdate)).AlwaysDo([=](unsigned flags) { - ERROR_MESSAGE("YOU SHALL NOT PASS!"); + SYSTEM_ERROR_MESSAGE("YOU SHALL NOT PASS!"); return SYSTEM_ERROR_NOT_ALLOWED; }); w.sendChunk(1 /* index */, genString(512) /* data */); diff --git a/user/makefile b/user/makefile index 5b2944f7d4..630299b913 100644 --- a/user/makefile +++ b/user/makefile @@ -3,7 +3,11 @@ USER_MODULE_PATH=. BUILD_PATH_EXT = $(USER_BUILD_PATH_EXT) -DEPENDENCIES = wiring system services communication hal crypto +DEPENDENCIES = wiring system services communication hal + +ifdef TEST +DEPENDENCIES += crypto +endif include ../build/platform-id.mk diff --git a/wiring/inc/spark_wiring_error.h b/wiring/inc/spark_wiring_error.h index bd84b01f70..c67d3b819c 100644 --- a/wiring/inc/spark_wiring_error.h +++ b/wiring/inc/spark_wiring_error.h @@ -98,7 +98,7 @@ inline particle::Error::Type particle::Error::type() const { } inline const char* particle::Error::message() const { - return msg_ ? msg_ : get_default_error_message((system_error_t)type_, nullptr); + return msg_ ? msg_ : get_default_system_error_message((system_error_t)type_, nullptr); } inline bool particle::Error::operator==(const Error& error) const {