Stop failed DFU sessions wedging the radio or failing silently

src/components/ble/DfuService.cpp

@@ -138,11 +138,21 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf* om) {
                    bootloaderSize,
                    applicationSize);
 
-      // Wait until SystemTask has disabled sleeping
+      // Wait until SystemTask has disabled sleeping.
       // This isn't quite correct, as we don't actually know
-      // if BleFirmwareUpdateStarted has been received yet
-      while (!systemTask.IsSleepDisabled()) {
+      // if BleFirmwareUpdateStarted has been received yet.
+      // Bounded on purpose: this runs on the BLE host task, so waiting here forever stalls
+      // every GATT operation and leaves disconnect events unprocessed, and the watch cannot
+      // advertise again until it is rebooted. If the wake lock never arrives, continue
+      // without it - a DFU that races the sleep timer beats a radio wedged until reboot.
+      constexpr uint8_t sleepLockWaitTicks = 200; // 200 * 5ms = 1s
+      uint8_t waited = 0;
+      while (!systemTask.IsSleepDisabled() && waited < sleepLockWaitTicks) {
         vTaskDelay(pdMS_TO_TICKS(5));
+        waited++;
+      }
+      if (!systemTask.IsSleepDisabled()) {
+        NRF_LOG_INFO("[DFU] -> Wake lock never arrived, continuing anyway");
       }
 
       dfuImage.Erase();
@@ -201,12 +211,26 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf* om) {
     }
       return 0;
     default:
-      // Invalid state
+      // Data arriving in a state that cannot consume it. Report it once - reporting every
+      // stray packet would flood the notification path - so the host learns it is talking
+      // to a watch parked mid-transfer instead of waiting for a reply that never comes.
+      if (!strayPacketReported) {
+        strayPacketReported = true;
+        SendInvalidState(connectionHandle, Opcodes::ReceiveFirmwareImage);
+      }
       return 0;
   }
   return 0;
 }
 
+void DfuService::SendInvalidState(uint16_t connectionHandle, Opcodes opcode) {
+  uint8_t data[4] {static_cast<uint8_t>(Opcodes::Response),
+                   static_cast<uint8_t>(opcode),
+                   static_cast<uint8_t>(ErrorCodes::InvalidState),
+                   static_cast<uint8_t>(state)};
+  notificationManager.Send(connectionHandle, controlPointCharacteristicHandle, data, 4);
+}
+
 int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf* om) {
   auto opcode = static_cast<Opcodes>(om->om_data[0]);
   NRF_LOG_INFO("[DFU] -> ControlPointHandler");
@@ -215,6 +239,7 @@ int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf* om) {
     case Opcodes::StartDFU: {
       if (state != States::Idle && state != States::Start) {
         NRF_LOG_INFO("[DFU] -> Start DFU requested, but we are not in Idle state");
+        SendInvalidState(connectionHandle, Opcodes::StartDFU);
         return 0;
       }
       if (state == States::Start) {
@@ -239,6 +264,7 @@ int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf* om) {
     case Opcodes::InitDFUParameters: {
       if (state != States::Init) {
         NRF_LOG_INFO("[DFU] -> Init DFU requested, but we are not in Init state");
+        SendInvalidState(connectionHandle, Opcodes::InitDFUParameters);
         return 0;
       }
       bool isInitComplete = (om->om_data[1] != 0);
@@ -260,6 +286,7 @@ int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf* om) {
     case Opcodes::ReceiveFirmwareImage:
       if (state != States::Init) {
         NRF_LOG_INFO("[DFU] -> Receive firmware image requested, but we are not in Start Init");
+        SendInvalidState(connectionHandle, Opcodes::ReceiveFirmwareImage);
         return 0;
       }
       // TODO the chunk size is dependent of the implementation of the host application...
@@ -270,6 +297,7 @@ int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf* om) {
     case Opcodes::ValidateFirmware: {
       if (state != States::Validate) {
         NRF_LOG_INFO("[DFU] -> Validate firmware image requested, but we are not in Data state %d", state);
+        SendInvalidState(connectionHandle, Opcodes::ValidateFirmware);
         return 0;
       }
 
@@ -300,6 +328,7 @@ int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf* om) {
     case Opcodes::ActivateImageAndReset:
       if (state != States::Validated) {
         NRF_LOG_INFO("[DFU] -> Activate image and reset requested, but we are not in Validated state");
+        SendInvalidState(connectionHandle, Opcodes::ActivateImageAndReset);
         return 0;
       }
       NRF_LOG_INFO("[DFU] -> Activate image and reset!");
@@ -318,6 +347,7 @@ void DfuService::OnTimeout() {
 
 void DfuService::Reset() {
   state = States::Idle;
+  strayPacketReported = false;
   nbPacketsToNotify = 0;
   nbPacketReceived = 0;
   bytesReceived = 0;

src/components/ble/DfuService.h

@@ -119,6 +119,7 @@ namespace Pinetime {
 
       enum class States : uint8_t { Idle, Init, Start, Data, Validate, Validated };
       States state = States::Idle;
+      bool strayPacketReported = false;
 
       enum class ImageTypes : uint8_t {
         NoImage = 0x00,
@@ -160,6 +161,7 @@ namespace Pinetime {
       int SendDfuRevision(os_mbuf* om) const;
       int WritePacketHandler(uint16_t connectionHandle, os_mbuf* om);
       int ControlPointHandler(uint16_t connectionHandle, os_mbuf* om);
+      void SendInvalidState(uint16_t connectionHandle, Opcodes opcode);
 
       TimerHandle_t timeoutTimer;
     };