Refactor OTA error handling and reporting

communication/src/chunked_transfer.cpp

@@ -293,6 +293,7 @@ ProtocolError ChunkedTransfer::handle_update_done(token_t token, Message& messag
         auto duration = (callbacks->millis() - update_begin_) / 1000;
         LOG(INFO, "Update done in %us; avg rate %u B/s", duration, file.file_length / duration);
         reset_updating();
+        // TODO: Add a flag to skip module validation
         callbacks->finish_firmware_update(file, UpdateFlag::SUCCESS, NULL);
     }
     else

dynalib/inc/module_info.h

@@ -41,6 +41,7 @@ typedef struct module_dependency_t {
 typedef enum module_info_flags_t {
     MODULE_INFO_FLAG_NONE               = 0x00,
     MODULE_INFO_FLAG_DROP_MODULE_INFO   = 0x01, // Indicates that the module_info_t header preceding the actual binary
+                                                // and potentially module_info_suffix_t + CRC in the end of the binary (depending on platform/module)
                                                 // need to be skipped when copying/writing this module into its target location.
     MODULE_INFO_FLAG_COMPRESSED         = 0x02, // Indicates that the module data is compressed.
     MODULE_INFO_FLAG_COMBINED           = 0x04  // Indicates that this module is combined with another module.
@@ -149,6 +150,7 @@ module_function_t  module_function(const module_info_t* mi);
 module_store_t module_store(const module_info_t* mi);
 uint32_t module_length(const module_info_t* mi);
 uint8_t module_index(const module_info_t* mi);
+uint16_t module_version(const module_info_t* mi);
 
 uint16_t module_platform_id(const module_info_t* mi);
 

dynalib/inc/module_info.inc

@@ -49,6 +49,10 @@ uint8_t module_index(const module_info_t* mi) {
     return mi ? module_index_(mi->module_index) : 0xFF;
 }
 
+uint16_t module_version(const module_info_t* mi) {
+    return mi ? mi->module_version : 0xFFFF;
+}
+
 uint16_t module_platform_id(const module_info_t* mi) {
     return mi ? mi->platform_id : 0xFFFF;
 }

hal/inc/hal_dynalib_ota.h

@@ -48,10 +48,10 @@ DYNALIB_FN(4, hal_ota, HAL_OTA_Flashed_ResetStatus, void(void))
 
 DYNALIB_FN(5, hal_ota, HAL_FLASH_Begin, bool(uint32_t, uint32_t, void*))
 DYNALIB_FN(6, hal_ota, HAL_FLASH_Update, int(const uint8_t*, uint32_t, uint32_t, void*))
-DYNALIB_FN(7, hal_ota, HAL_FLASH_End, hal_update_complete_t(hal_module_t*))
-DYNALIB_FN(8, hal_ota, HAL_FLASH_OTA_Validate, int(hal_module_t*, bool, module_validation_flags_t, void*))
+DYNALIB_FN(7, hal_ota, HAL_FLASH_End, int(void*))
+DYNALIB_FN(8, hal_ota, HAL_FLASH_OTA_Validate, int(bool, module_validation_flags_t, void*))
 DYNALIB_FN(9, hal_ota, HAL_OTA_Add_System_Info, void(hal_system_info_t* info, bool create, void* reserved))
-DYNALIB_FN(10, hal_ota, HAL_FLASH_ApplyPendingUpdate,  hal_update_complete_t(hal_module_t*, bool, void*))
+DYNALIB_FN(10, hal_ota, HAL_FLASH_ApplyPendingUpdate, int(bool, void*))
 DYNALIB_END(hal_ota)
 
 #endif	/* HAL_DYNALIB_OTA_H */

hal/inc/ota_flash_hal.h

@@ -121,7 +121,7 @@ uint16_t HAL_OTA_ChunkSize();
 
 flash_device_t HAL_OTA_FlashDevice();
 
-int HAL_FLASH_OTA_Validate(hal_module_t* mod, bool userDepsOptional, module_validation_flags_t flags, void* reserved);
+int HAL_FLASH_OTA_Validate(bool userDepsOptional, module_validation_flags_t flags, void* reserved);
 
 /**
  * Erase a region of flash in preparation for flashing content.
@@ -137,20 +137,24 @@ bool HAL_FLASH_Begin(uint32_t address, uint32_t length, void* reserved);
 int HAL_FLASH_Update(const uint8_t *pBuffer, uint32_t address, uint32_t length, void* reserved);
 
 typedef enum {
-    // Add new error codes here
-    HAL_UPDATE_ERROR,
-    HAL_UPDATE_APPLIED_PENDING_RESTART,
-    HAL_UPDATE_APPLIED
+    HAL_UPDATE_APPLIED_PENDING_RESTART = 0,
+    HAL_UPDATE_APPLIED = 1
 } hal_update_complete_t;
 
-hal_update_complete_t HAL_FLASH_End(hal_module_t* module);
+/**
+ * Apply the OTA update.
+ *
+ * @return A value defined by `hal_update_complete_t` or a negative result code in case of an error.
+ */
+int HAL_FLASH_End(void* reserved);
 
 /**
  * @param module Optional pointer to a module that receives the module definition of the firmware that was flashed.
  * @param dryRun when true, only test that the system has a pending update in memory. When false, the test is performed and the module
  *   applied if valid (or the bootloader instructed to apply it.)
+ * @return A value defined by `hal_update_complete_t` or a negative result code in case of an error.
  */
-hal_update_complete_t HAL_FLASH_ApplyPendingUpdate(hal_module_t* module, bool dryRun, void* reserved);
+int HAL_FLASH_ApplyPendingUpdate(bool dryRun, void* reserved);
 
 uint32_t HAL_FLASH_ModuleAddress(uint32_t address);
 uint32_t HAL_FLASH_ModuleLength(uint32_t address);

hal/shared/platform_ncp.h

@@ -64,7 +64,7 @@ PlatformNCPIdentifier platform_current_ncp_identifier();
 /**
  * Update the NCP firmware from the given module. The module has been validated for integrity and matching platform and dependencies checked.
  */
-hal_update_complete_t platform_ncp_update_module(const hal_module_t* module);
+int platform_ncp_update_module(const hal_module_t* module);
 
 /**
  * Augments the module info with data retrieved from the NCP.

hal/src/argon/platform_ncp_argon.cpp

@@ -70,10 +70,10 @@ class OtaUpdateSourceStream : public particle::InputStream {
 // FIXME: This function accesses the module info via XIP and may fail to parse it correctly under
 // some not entirely clear circumstances. Disabling compiler optimizations helps to work around
 // the problem
-__attribute__((optimize("O0"))) hal_update_complete_t platform_ncp_update_module(const hal_module_t* module) {
+__attribute__((optimize("O0"))) int platform_ncp_update_module(const hal_module_t* module) {
     const auto ncpClient = particle::wifiNetworkManager()->ncpClient();
     SPARK_ASSERT(ncpClient);
-    CHECK_RETURN(ncpClient->on(), HAL_UPDATE_ERROR);
+    CHECK(ncpClient->on());
     // we pass only the actual binary after the module info and up to the suffix
     const uint8_t* start = (const uint8_t*)module->info;
     static_assert(sizeof(module_info_t)==24, "expected module info size to be 24");
@@ -88,7 +88,7 @@ __attribute__((optimize("O0"))) hal_update_complete_t platform_ncp_update_module
     }
     r = ncpClient->updateFirmware(&moduleStream, length);
     LED_On(LED_RGB);
-    CHECK_RETURN(r, HAL_UPDATE_ERROR);
+    CHECK(r);
     r = ncpClient->getFirmwareModuleVersion(&version);
     if (r == 0) {
         LOG(INFO, "ESP32 firmware version updated to version %d", version);

hal/src/gcc/ota_flash_hal.cpp

@@ -51,16 +51,16 @@ int HAL_FLASH_OTA_Validate(hal_module_t* mod, bool userDepsOptional, module_vali
   return 0;
 }
 
- hal_update_complete_t HAL_FLASH_End(hal_module_t* mod)
+int HAL_FLASH_End(void* reserved)
 {
 	 fclose(output_file);
 	 output_file = NULL;
      return HAL_UPDATE_APPLIED;
 }
 
-hal_update_complete_t HAL_FLASH_ApplyPendingUpdate(hal_module_t* module, bool dryRun, void* reserved)
+int HAL_FLASH_ApplyPendingUpdate(bool dryRun, void* reserved)
 {
-    return HAL_UPDATE_ERROR;
+    return SYSTEM_ERROR_UNKNOWN;
 }
 
 /**

hal/src/nRF52840/bootloader.cpp

@@ -54,7 +54,7 @@ class OTAUpdateStream : public OutputStream {
 	}
 
 	int flush() override {
-		return HAL_FLASH_End(nullptr)!=HAL_UPDATE_ERROR;
+		return HAL_FLASH_End(nullptr);
 	}
 };