Improve DFU procedure :

- correctly write all bytes to flash
 - check CRC
 - Fix bug in notification : they cannot be sent from the control point handler (because it seems you cannot send a notification and a write acknowledge at the same time) using a timer (quick'n'dirty implementation to be improved)
 - Improve dfu screen
 - Reset if dfu image is correctly copied into flash and crc is ok.

src/CMakeLists.txt

@@ -355,6 +355,7 @@ list(APPEND SOURCE_FILES
   Components/Ble/DfuService.cpp
   Components/Ble/CurrentTimeService.cpp
   Components/Ble/AlertNotificationService.cpp
+  Components/Ble/DfuImage.cpp
   drivers/Cst816s.cpp
   FreeRTOS/port.c
   FreeRTOS/port_cmsis_systick.c
@@ -408,6 +409,7 @@ set(INCLUDE_FILES
   Components/Ble/CurrentTimeClient.h
   Components/Ble/AlertNotificationClient.h
   Components/Ble/DfuService.h
+  Components/Ble/DfuImage.h
   drivers/Cst816s.h
   FreeRTOS/portmacro.h
   FreeRTOS/portmacro_cmsis.h

src/Components/Ble/BleController.h

@@ -7,6 +7,7 @@ namespace Pinetime {
   namespace Controllers {
     class Ble {
       public:
+        enum class FirmwareUpdateStates {Idle, Running, Validated, Error};
 
         Ble() = default;
         bool IsConnected() const {return isConnected;}
@@ -17,15 +18,18 @@ namespace Pinetime {
         void StopFirmwareUpdate();
         void FirmwareUpdateTotalBytes(uint32_t totalBytes);
         void FirmwareUpdateCurrentBytes(uint32_t currentBytes);
+        void State(FirmwareUpdateStates state) { firmwareUpdateState = state; }
 
         bool IsFirmwareUpdating() const { return isFirmwareUpdating; }
         uint32_t FirmwareUpdateTotalBytes() const { return firmwareUpdateTotalBytes; }
         uint32_t FirmwareUpdateCurrentBytes() const { return firmwareUpdateCurrentBytes; }
+        FirmwareUpdateStates State() const { return firmwareUpdateState; }
       private:
         bool isConnected = false;
         bool isFirmwareUpdating = false;
         uint32_t firmwareUpdateTotalBytes = 0;
         uint32_t firmwareUpdateCurrentBytes = 0;
+        FirmwareUpdateStates firmwareUpdateState = FirmwareUpdateStates::Idle;
 
     };
   }

src/Components/Ble/DfuService.cpp

@@ -16,7 +16,13 @@ int DfuServiceCallback(uint16_t conn_handle, uint16_t attr_handle,
   return dfuService->OnServiceData(conn_handle, attr_handle, ctxt);
 }
 
-DfuService::DfuService(Pinetime::System::SystemTask& systemTask, Pinetime::Controllers::Ble& bleController, Pinetime::Drivers::SpiNorFlash& spiNorFlash) :
+void NotificationTimerCallback( TimerHandle_t xTimer ) {
+  auto dfuService = static_cast<DfuService *>(pvTimerGetTimerID( xTimer ));
+  dfuService->OnNotificationTimer();
+}
+
+DfuService::DfuService(Pinetime::System::SystemTask &systemTask, Pinetime::Controllers::Ble &bleController,
+                       Pinetime::Drivers::SpiNorFlash &spiNorFlash) :
         systemTask{systemTask},
         bleController{bleController},
         spiNorFlash{spiNorFlash},
@@ -58,10 +64,8 @@ DfuService::DfuService(Pinetime::System::SystemTask& systemTask, Pinetime::Contr
                 {
                         0
                 },
-        }
+        } {
-
+  notificationTimer = xTimerCreate ("notificationTimer", 1000, pdFALSE, this, NotificationTimerCallback);
-        {
-
 }
 
 void DfuService::Init() {
@@ -71,9 +75,12 @@ void DfuService::Init() {
 
 int DfuService::OnServiceData(uint16_t connectionHandle, uint16_t attributeHandle, ble_gatt_access_ctxt *context) {
 
-  ble_gatts_find_chr((ble_uuid_t*)&serviceUuid, (ble_uuid_t*)&packetCharacteristicUuid, nullptr, &packetCharacteristicHandle);
+  ble_gatts_find_chr((ble_uuid_t *) &serviceUuid, (ble_uuid_t *) &packetCharacteristicUuid, nullptr,
-  ble_gatts_find_chr((ble_uuid_t*)&serviceUuid, (ble_uuid_t*)&controlPointCharacteristicUuid, nullptr, &controlPointCharacteristicHandle);
+                     &packetCharacteristicHandle);
-  ble_gatts_find_chr((ble_uuid_t*)&serviceUuid, (ble_uuid_t*)&revisionCharacteristicUuid, nullptr, &revisionCharacteristicHandle);
+  ble_gatts_find_chr((ble_uuid_t *) &serviceUuid, (ble_uuid_t *) &controlPointCharacteristicUuid, nullptr,
+                     &controlPointCharacteristicHandle);
+  ble_gatts_find_chr((ble_uuid_t *) &serviceUuid, (ble_uuid_t *) &revisionCharacteristicUuid, nullptr,
+                     &revisionCharacteristicHandle);
 
   if (attributeHandle == packetCharacteristicHandle) {
     if (context->op == BLE_GATT_ACCESS_OP_WRITE_CHR)
@@ -105,15 +112,12 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf *om) {
       bootloaderSize = om->om_data[4] + (om->om_data[5] << 8) + (om->om_data[6] << 16) + (om->om_data[7] << 24);
       applicationSize = om->om_data[8] + (om->om_data[9] << 8) + (om->om_data[10] << 16) + (om->om_data[11] << 24);
       bleController.FirmwareUpdateTotalBytes(applicationSize);
-      NRF_LOG_INFO("[DFU] -> Start data received : SD size : %d, BT size : %d, app size : %d", softdeviceSize, bootloaderSize, applicationSize);
+      NRF_LOG_INFO("[DFU] -> Start data received : SD size : %d, BT size : %d, app size : %d", softdeviceSize,
+                   bootloaderSize, applicationSize);
 
       for (int erased = 0; erased < maxImageSize; erased += 0x1000) {
 #if 1
         spiNorFlash.SectorErase(writeOffset + erased);
-
-        auto p =  spiNorFlash.ProgramFailed();
-        auto e = spiNorFlash.EraseFailed();
-        NRF_LOG_INFO("[DFU] Erasing sector %d - %d-%d", erased, p, e);
 #endif
       }
 
@@ -125,16 +129,19 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf *om) {
     case States::Init: {
       uint16_t deviceType = om->om_data[0] + (om->om_data[1] << 8);
       uint16_t deviceRevision = om->om_data[2] + (om->om_data[3] << 8);
-      uint32_t applicationVersion = om->om_data[4] + (om->om_data[5] << 8) + (om->om_data[6] << 16) + (om->om_data[7] << 24);
+      uint32_t applicationVersion =
+              om->om_data[4] + (om->om_data[5] << 8) + (om->om_data[6] << 16) + (om->om_data[7] << 24);
       uint16_t softdeviceArrayLength = om->om_data[8] + (om->om_data[9] << 8);
       uint16_t sd[softdeviceArrayLength];
       for (int i = 0; i < softdeviceArrayLength; i++) {
-        sd[i] = om->om_data[10 + (i*2)] + (om->om_data[(i*2)+1] << 8);
+        sd[i] = om->om_data[10 + (i * 2)] + (om->om_data[10 + (i * 2) + 1] << 8);
       }
-      uint16_t crc = om->om_data[10 + (softdeviceArrayLength*2)] + (om->om_data[10 + (softdeviceArrayLength*2)] << 8);
+      expectedCrc =
+              om->om_data[10 + (softdeviceArrayLength * 2)] + (om->om_data[10 + (softdeviceArrayLength * 2) + 1] << 8);
 
-      NRF_LOG_INFO("[DFU] -> Init data received : deviceType = %d, deviceRevision = %d, applicationVersion = %d, nb SD = %d, First SD = %d, CRC = %u",
+      NRF_LOG_INFO(
-                   deviceType, deviceRevision, applicationVersion, softdeviceArrayLength, sd[0], crc);
+              "[DFU] -> Init data received : deviceType = %d, deviceRevision = %d, applicationVersion = %d, nb SD = %d, First SD = %d, CRC = %u",
+              deviceType, deviceRevision, applicationVersion, softdeviceArrayLength, sd[0], expectedCrc);
 
       return 0;
     }
@@ -146,8 +153,7 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf *om) {
       if (firstCrc) {
         tempCrc = ComputeCrc(om->om_data, om->om_len, NULL);
         firstCrc = false;
-        }
+      } else
-        else
         tempCrc = ComputeCrc(om->om_data, om->om_len, &tempCrc);
 
       if (nbPacketReceived > 0 && (nbPacketReceived % nbPacketsToNotify) == 0) {
@@ -163,9 +169,10 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf *om) {
 
 
 
-      if((nbPacketReceived % nbPacketsToNotify) == 0) {
+      if ((nbPacketReceived % nbPacketsToNotify) == 0 && bytesReceived != applicationSize) {
         uint8_t data[5]{static_cast<uint8_t>(Opcodes::PacketReceiptNotification),
-                        (uint8_t)(bytesReceived&0x000000FFu),(uint8_t)(bytesReceived>>8u), (uint8_t)(bytesReceived>>16u),(uint8_t)(bytesReceived>>24u) };
+                        (uint8_t) (bytesReceived & 0x000000FFu), (uint8_t) (bytesReceived >> 8u),
+                        (uint8_t) (bytesReceived >> 16u), (uint8_t) (bytesReceived >> 24u)};
         NRF_LOG_INFO("[DFU] -> Send packet notification: %d bytes received", bytesReceived);
         SendNotification(connectionHandle, data, 5);
       }
@@ -176,18 +183,16 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf *om) {
 
           spiNorFlash.Write(writeOffset + ((nbPacketReceived - remaningPacket) * 20), tempBuffer, remaningPacket * 20);
         }
-          if(applicationSize < maxImageSize) {
+
+        if (applicationSize < maxImageSize)
           WriteMagicNumber();
-          }
 
         uint8_t data[3]{static_cast<uint8_t>(Opcodes::Response),
                         static_cast<uint8_t>(Opcodes::ReceiveFirmwareImage),
                         static_cast<uint8_t>(ErrorCodes::NoError)};
-        NRF_LOG_INFO("[DFU] -> Send packet notification : all bytes received! CRC = %u", tempCrc);
+        NRF_LOG_INFO("[DFU] -> Send packet notification : all bytes received! CRC = %u -- %d", tempCrc, connectionHandle);
         SendNotification(connectionHandle, data, 3);
         state = States::Validate;
-
-        Validate();
       }
     }
       return 0;
@@ -217,6 +222,7 @@ int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf *om) {
         NRF_LOG_INFO("[DFU] -> Start DFU, mode = Application");
         state = States::Start;
         bleController.StartFirmwareUpdate();
+        bleController.State(Pinetime::Controllers::Ble::FirmwareUpdateStates::Running);
         bleController.FirmwareUpdateTotalBytes(0xffffffffu);
         bleController.FirmwareUpdateCurrentBytes(0);
         systemTask.PushMessage(Pinetime::System::SystemTask::Messages::BleFirmwareUpdateStarted);
@@ -236,11 +242,12 @@ int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf *om) {
       NRF_LOG_INFO("[DFU] -> Init DFU parameters %s", isInitComplete ? " complete" : " not complete");
 
       if (isInitComplete) {
-        uint8_t data[3]{static_cast<uint8_t>(Opcodes::Response),
+        notificationBuffer[0] = static_cast<uint8_t>(Opcodes::Response);
-                        static_cast<uint8_t>(Opcodes::InitDFUParameters),
+        notificationBuffer[1] = static_cast<uint8_t>(Opcodes::InitDFUParameters);
-                        (isInitComplete ? uint8_t{1} : uint8_t{0})};
+        notificationBuffer[2] =  (isInitComplete ? uint8_t{1} : uint8_t{0});
-        NRF_LOG_INFO("SEND NOTIF : %d %d %d", data[0], data[1], data[2]);
+        notificationSize = 3;
-        SendNotification(connectionHandle, data, 3);
+        notificatonConnectionHandle = connectionHandle;
+        xTimerStart(notificationTimer, 0);
         return 0;
       }
     }
@@ -262,12 +269,28 @@ int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf *om) {
         NRF_LOG_INFO("[DFU] -> Validate firmware image requested, but we are not in Data state");
         return 0;
       }
-      NRF_LOG_INFO("[DFU] -> Validate firmware");
+
+      NRF_LOG_INFO("[DFU] -> Validate firmware image requested -- %d", connectionHandle);
+
+      if(Validate()){
         state = States::Validated;
-      uint8_t data[3]{static_cast<uint8_t>(Opcodes::Response),
+
-                      static_cast<uint8_t>(Opcodes::ValidateFirmware),
+        notificationBuffer[0] = static_cast<uint8_t>(Opcodes::Response);
-                      static_cast<uint8_t>(ErrorCodes::NoError)};
+        notificationBuffer[1] = static_cast<uint8_t>(Opcodes::ValidateFirmware);
-      SendNotification(connectionHandle, data, 3);
+        notificationBuffer[2] =  static_cast<uint8_t>(ErrorCodes::NoError);
+        notificationSize = 3;
+        notificatonConnectionHandle = connectionHandle;
+        xTimerStart(notificationTimer, 0);
+
+      } else {
+        notificationBuffer[0] = static_cast<uint8_t>(Opcodes::Response);
+        notificationBuffer[1] = static_cast<uint8_t>(Opcodes::ValidateFirmware);
+        notificationBuffer[2] =  static_cast<uint8_t>(ErrorCodes::CrcError);
+        notificationSize = 3;
+        notificatonConnectionHandle = connectionHandle;
+        xTimerStart(notificationTimer, 0);
+      }
+
       return 0;
     }
     case Opcodes::ActivateImageAndReset:
@@ -279,7 +302,8 @@ int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf *om) {
       bleController.StopFirmwareUpdate();
       systemTask.PushMessage(Pinetime::System::SystemTask::Messages::BleFirmwareUpdateFinished);
       return 0;
-    default: return 0;
+    default:
+      return 0;
   }
 }
 
@@ -289,12 +313,10 @@ void DfuService::SendNotification(uint16_t connectionHandle, const uint8_t *data
   ASSERT(ret == 0);
 }
 
-uint16_t DfuService::ComputeCrc(uint8_t const * p_data, uint32_t size, uint16_t const * p_crc)
+uint16_t DfuService::ComputeCrc(uint8_t const *p_data, uint32_t size, uint16_t const *p_crc) {
-{
   uint16_t crc = (p_crc == NULL) ? 0xFFFF : *p_crc;
 
-    for (uint32_t i = 0; i < size; i++)
+  for (uint32_t i = 0; i < size; i++) {
-    {
     crc = (uint8_t) (crc >> 8) | (crc << 8);
     crc ^= p_data[i];
     crc ^= (uint8_t) (crc & 0xFF) >> 4;
@@ -305,7 +327,7 @@ uint16_t DfuService::ComputeCrc(uint8_t const * p_data, uint32_t size, uint16_t
   return crc;
 }
 
-void DfuService::Validate() {
+bool DfuService::Validate() {
   uint32_t chunkSize = 200;
   int currentOffset = 0;
   uint16_t crc = 0;
@@ -318,13 +340,21 @@ void DfuService::Validate() {
     if (first) {
       crc = ComputeCrc(tempBuffer, readSize, NULL);
       first = false;
-        }
+    } else
-        else
       crc = ComputeCrc(tempBuffer, readSize, &crc);
     currentOffset += readSize;
   }
 
-    NRF_LOG_INFO("CRC : %u", crc);
+  NRF_LOG_INFO("Expected CRC : %u - Processed CRC : %u", expectedCrc, crc);
+  bool crcOk = (crc == expectedCrc);
+  if (crcOk) {
+    bleController.State(Pinetime::Controllers::Ble::FirmwareUpdateStates::Validated);
+    NRF_LOG_INFO("Image OK");
+  } else {
+    bleController.State(Pinetime::Controllers::Ble::FirmwareUpdateStates::Error);
+    NRF_LOG_INFO("Image Error : bad CRC");
+  }
+  return crcOk;
 }
 
 void DfuService::WriteMagicNumber() {
@@ -338,3 +368,10 @@ void DfuService::WriteMagicNumber() {
   uint32_t offset = writeOffset + (maxImageSize - (4 * sizeof(uint32_t)));
   spiNorFlash.Write(offset, reinterpret_cast<uint8_t *>(magic), 4 * sizeof(uint32_t));
 }
+
+void DfuService::OnNotificationTimer() {
+  if(notificationSize > 0) {
+    SendNotification(notificatonConnectionHandle, notificationBuffer, notificationSize);
+    notificationSize = 0;
+  }
+}

src/Components/Ble/DfuService.h

@@ -1,4 +1,5 @@
 #pragma once
+
 #include <cstdint>
 #include <array>
 
@@ -13,14 +14,19 @@ namespace Pinetime {
   }
   namespace Controllers {
     class Ble;
+
     class DfuService {
       public:
         DfuService(Pinetime::System::SystemTask &systemTask, Pinetime::Controllers::Ble &bleController,
                    Pinetime::Drivers::SpiNorFlash &spiNorFlash);
+
         void Init();
-        void Validate();
+
+        bool Validate();
 
         int OnServiceData(uint16_t connectionHandle, uint16_t attributeHandle, ble_gatt_access_ctxt *context);
+        void OnNotificationTimer();
+
       private:
         Pinetime::System::SystemTask &systemTask;
         Pinetime::Controllers::Ble &bleController;
@@ -63,7 +69,9 @@ namespace Pinetime {
         uint16_t controlPointCharacteristicHandle;
         uint16_t revisionCharacteristicHandle;
 
-        enum class States : uint8_t {Idle, Init, Start, Data, Validate, Validated};
+        enum class States : uint8_t {
+            Idle, Init, Start, Data, Validate, Validated
+        };
         States state = States::Idle;
 
         enum class ImageTypes : uint8_t {
@@ -85,7 +93,14 @@ namespace Pinetime {
             PacketReceiptNotification = 0x11
         };
 
-        enum class ErrorCodes { NoError = 0x01};
+        enum class ErrorCodes {
+            NoError = 0x01,
+            InvalidState = 0x02,
+            NotSupported = 0x03,
+            DataSizeExceedsLimits = 0x04,
+            CrcError = 0x05,
+            OperationFailed = 0x06
+        };
 
         uint8_t nbPacketsToNotify = 0;
         uint32_t nbPacketReceived = 0;
@@ -96,19 +111,29 @@ namespace Pinetime {
         uint32_t bootloaderSize = 0;
         uint32_t applicationSize = 0;
         static constexpr uint32_t maxImageSize = 475136;
+        uint16_t expectedCrc = 0;
 
         int SendDfuRevision(os_mbuf *om) const;
+
         void SendNotification(uint16_t connectionHandle, const uint8_t *data, const size_t size);
+
         int WritePacketHandler(uint16_t connectionHandle, os_mbuf *om);
+
         int ControlPointHandler(uint16_t connectionHandle, os_mbuf *om);
 
         uint8_t tempBuffer[200];
+
         uint16_t ComputeCrc(uint8_t const *p_data, uint32_t size, uint16_t const *p_crc);
 
         bool firstCrc = true;
         uint16_t tempCrc = 0;
 
         void WriteMagicNumber();
+        TimerHandle_t notificationTimer;
+
+        uint16_t notificatonConnectionHandle = 0;
+        size_t notificationSize = 0;
+        uint8_t notificationBuffer[10];
     };
   }
 }

src/DisplayApp/Screens/FirmwareUpdate.cpp

@@ -26,6 +26,15 @@ FirmwareUpdate::FirmwareUpdate(Pinetime::Applications::DisplayApp *app, Pinetime
   lv_label_set_text(percentLabel, "");
   lv_obj_set_auto_realign(percentLabel, true);
   lv_obj_align(percentLabel, bar1, LV_ALIGN_OUT_TOP_MID, 0, 60);
+
+  button = lv_btn_create(lv_scr_act(), NULL);
+  //lv_obj_set_event_cb(button, event_handler);
+  lv_obj_align(button, NULL, LV_ALIGN_IN_BOTTOM_MID, 0, 0);
+  lv_obj_set_hidden(button, true);
+
+  labelBtn = lv_label_create(button, NULL);
+  lv_label_set_text(labelBtn, "Back");
+  lv_obj_set_hidden(labelBtn, true);
 }
 
 FirmwareUpdate::~FirmwareUpdate() {
@@ -33,6 +42,29 @@ FirmwareUpdate::~FirmwareUpdate() {
 }
 
 bool FirmwareUpdate::Refresh() {
+  switch(bleController.State()) {
+    default:
+    case Pinetime::Controllers::Ble::FirmwareUpdateStates::Idle:
+    case Pinetime::Controllers::Ble::FirmwareUpdateStates::Running:
+      if(state != States::Running)
+        state = States::Running;
+      return DisplayProgression();
+    case Pinetime::Controllers::Ble::FirmwareUpdateStates::Validated:
+      if(state != States::Validated) {
+        UpdateValidated();
+        state = States::Validated;
+      }
+      return running;
+    case Pinetime::Controllers::Ble::FirmwareUpdateStates::Error:
+      if(state != States::Error) {
+        UpdateError();
+        state = States::Error;
+      }
+      return running;
+  }
+}
+
+bool FirmwareUpdate::DisplayProgression() const {
   float current = bleController.FirmwareUpdateCurrentBytes() / 1024.0f;
   float total = bleController.FirmwareUpdateTotalBytes() / 1024.0f;
   int16_t pc = (current / total) * 100.0f;
@@ -47,3 +79,16 @@ bool FirmwareUpdate::OnButtonPushed() {
   running = false;
   return true;
 }
+
+void FirmwareUpdate::UpdateValidated() {
+  lv_label_set_recolor(percentLabel, true);
+  lv_label_set_text(percentLabel, "#00ff00 Image Ok!#");
+}
+
+void FirmwareUpdate::UpdateError() {
+  lv_label_set_recolor(percentLabel, true);
+  lv_label_set_text(percentLabel, "#ff0000 Error!#");
+
+  lv_obj_set_hidden(labelBtn, false);
+  lv_obj_set_hidden(button, false);
+}

src/DisplayApp/Screens/FirmwareUpdate.h

@@ -26,13 +26,22 @@ namespace Pinetime {
           bool OnButtonPushed() override;
 
         private:
+          enum class States { Idle, Running, Validated, Error };
           Pinetime::Controllers::Ble& bleController;
           lv_obj_t* bar1;
           lv_obj_t* percentLabel;
           lv_obj_t* titleLabel;
-          char percentStr[10];
+          lv_obj_t* labelBtn;
+          lv_obj_t* button;
+          mutable char percentStr[10];
           bool running = true;
+          States state;
 
+          bool DisplayProgression() const;
+
+          void UpdateValidated();
+
+          void UpdateError();
       };
     }
   }

src/SystemTask/SystemTask.cpp

@@ -118,6 +118,7 @@ void SystemTask::Work() {
           break;
         case Messages::BleFirmwareUpdateFinished:
           displayApp->PushMessage(Pinetime::Applications::DisplayApp::Messages::BleFirmwareUpdateFinished);
+          NVIC_SystemReset();
           break;
         default: break;
       }