Address review comments

particle-iot · Nov 17, 2020 · c92e8ab · c92e8ab
1 parent 2e31a6d
commit c92e8ab
Show file tree

Hide file tree

Showing 13 changed files with 97 additions and 92 deletions.
diff --git 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
@@ -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
@@ -27,13 +27,44 @@
 
 #include "system_defs.h"
 
+#include "mbedtls_config.h"
+
 #include <cstdint>
 #include <cstddef>
 
 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
@@ -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
@@ -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,33 +481,33 @@ __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);
         }
         const bool dropModuleInfo = (info->flags & MODULE_INFO_FLAG_DROP_MODULE_INFO);
         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) {
 #if HAL_PLATFORM_NCP_UPDATABLE
             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
@@ -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
@@ -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,15 +131,15 @@ 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.
  *
  * @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"