Refactor SystemTask state handling for resilience

mark9064 · NeroBurner · commit c3d05901a05a · 2024-09-21T21:08:07.000+02:00
State transitions now happen immediately where possible
This simplifies state management in general,
and prevents bugs such as the chime issue from occurring in the first place
diff --git a/src/components/ble/DfuService.cpp b/src/components/ble/DfuService.cpp
@@ -124,9 +124,11 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf* om) {
                    bootloaderSize,
                    applicationSize);
 
-      // wait until SystemTask has finished waking up all devices
-      while (systemTask.IsSleeping()) {
-        vTaskDelay(50); // 50ms
+      // 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()) {
+        vTaskDelay(pdMS_TO_TICKS(5));
       }
 
       dfuImage.Erase();
diff --git a/src/components/ble/NimbleController.cpp b/src/components/ble/NimbleController.cpp
@@ -454,9 +454,15 @@ void NimbleController::PersistBond(struct ble_gap_conn_desc& desc) {
     /* Wakeup Spi and SpiNorFlash before accessing the file system
      * This should be fixed in the FS driver
      */
-    systemTask.PushMessage(Pinetime::System::Messages::GoToRunning);
     systemTask.PushMessage(Pinetime::System::Messages::DisableSleeping);
-    vTaskDelay(10);
+
+    // This isn't quite correct
+    // SystemTask could receive EnableSleeping right after passing this check
+    // We need some guarantee that the SystemTask has processed the above message
+    // before we can continue
+    while (!systemTask.IsSleepDisabled()) {
+      vTaskDelay(pdMS_TO_TICKS(5));
+    }
 
     lfs_file_t file_p;
 
diff --git a/src/displayapp/DisplayApp.cpp b/src/displayapp/DisplayApp.cpp
@@ -255,9 +255,20 @@ void DisplayApp::Refresh() {
           isDimmed = true;
           brightnessController.Set(Controllers::BrightnessController::Levels::Low);
         }
-        if (IsPastSleepTime()) {
-          systemTask->PushMessage(System::Messages::GoToSleep);
-          state = States::Idle;
+        if (IsPastSleepTime() && uxQueueMessagesWaiting(msgQueue) == 0) {
+          PushMessageToSystemTask(System::Messages::GoToSleep);
+          // Can't set state to Idle here, something may send
+          // DisableSleeping before this GoToSleep arrives
+          // Instead we check we have no messages queued before sending GoToSleep
+          // This works as the SystemTask is higher priority than DisplayApp
+          // As soon as we send GoToSleep, SystemTask pre-empts DisplayApp
+          // Whenever DisplayApp is running again, it is guaranteed that
+          // SystemTask has handled the message
+          // If it responded, we will have a GoToSleep waiting in the queue
+          // By checking that there are no messages in the queue, we avoid
+          // resending GoToSleep when we already have a response
+          // SystemTask is resilient to duplicate messages, this is an
+          // optimisation to reduce pressure on the message queues
         }
       } else if (isDimmed) {
         isDimmed = false;
@@ -273,6 +284,9 @@ void DisplayApp::Refresh() {
   if (xQueueReceive(msgQueue, &msg, queueTimeout) == pdTRUE) {
     switch (msg) {
       case Messages::GoToSleep:
+        if (state != States::Running) {
+          break;
+        }
         while (brightnessController.Level() != Controllers::BrightnessController::Levels::Low) {
           brightnessController.Lower();
           vTaskDelay(100);
@@ -307,6 +321,9 @@ void DisplayApp::Refresh() {
         lv_disp_trig_activity(nullptr);
         break;
       case Messages::GoToRunning:
+        if (state == States::Running) {
+          break;
+        }
         if (settingsController.GetAlwaysOnDisplay()) {
           lcd.LowPowerOff();
         } else {
diff --git a/src/systemtask/SystemTask.cpp b/src/systemtask/SystemTask.cpp
@@ -196,29 +196,11 @@ void SystemTask::Work() {
           }
           break;
         case Messages::DisableSleeping:
+          GoToRunning();
           doNotGoToSleep = true;
           break;
         case Messages::GoToRunning:
-          // SPI doesn't go to sleep for always on mode
-          if (!settingsController.GetAlwaysOnDisplay()) {
-            spi.Wakeup();
-          }
-
-          // Double Tap needs the touch screen to be in normal mode
-          if (!settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::DoubleTap)) {
-            touchPanel.Wakeup();
-          }
-
-          spiNorFlash.Wakeup();
-
-          displayApp.PushMessage(Pinetime::Applications::Display::Messages::GoToRunning);
-          heartRateApp.PushMessage(Pinetime::Applications::HeartRateTask::Messages::WakeUp);
-
-          if (bleController.IsRadioEnabled() && !bleController.IsConnected()) {
-            nimbleController.RestartFastAdv();
-          }
-
-          state = SystemTaskState::Running;
+          GoToRunning();
           break;
         case Messages::TouchWakeUp: {
           if (touchHandler.ProcessTouchInfo(touchPanel.GetTouchInfo())) {
@@ -235,13 +217,7 @@ void SystemTask::Work() {
           break;
         }
         case Messages::GoToSleep:
-          if (doNotGoToSleep) {
-            break;
-          }
-          state = SystemTaskState::GoingToSleep; // Already set in PushMessage()
-          NRF_LOG_INFO("[systemtask] Going to sleep");
-          displayApp.PushMessage(Pinetime::Applications::Display::Messages::GoToSleep);
-          heartRateApp.PushMessage(Pinetime::Applications::HeartRateTask::Messages::GoToSleep);
+          GoToSleep();
           break;
         case Messages::OnNewTime:
           if (alarmController.State() == Controllers::AlarmController::AlarmState::Set) {
@@ -250,16 +226,14 @@ void SystemTask::Work() {
           break;
         case Messages::OnNewNotification:
           if (settingsController.GetNotificationStatus() == Pinetime::Controllers::Settings::Notification::On) {
-            if (state == SystemTaskState::Sleeping) {
+            if (IsSleeping()) {
               GoToRunning();
             }
             displayApp.PushMessage(Pinetime::Applications::Display::Messages::NewNotification);
           }
           break;
         case Messages::SetOffAlarm:
-          if (state == SystemTaskState::Sleeping) {
-            GoToRunning();
-          }
+          GoToRunning();
           displayApp.PushMessage(Pinetime::Applications::Display::Messages::AlarmTriggered);
           break;
         case Messages::BleConnected:
@@ -268,10 +242,8 @@ void SystemTask::Work() {
           bleDiscoveryTimer = 5;
           break;
         case Messages::BleFirmwareUpdateStarted:
+          GoToRunning();
           doNotGoToSleep = true;
-          if (state == SystemTaskState::Sleeping) {
-            GoToRunning();
-          }
           displayApp.PushMessage(Pinetime::Applications::Display::Messages::BleFirmwareUpdateStarted);
           break;
         case Messages::BleFirmwareUpdateFinished:
@@ -282,10 +254,8 @@ void SystemTask::Work() {
           break;
         case Messages::StartFileTransfer:
           NRF_LOG_INFO("[systemtask] FS Started");
+          GoToRunning();
           doNotGoToSleep = true;
-          if (state == SystemTaskState::Sleeping) {
-            GoToRunning();
-          }
           // TODO add intent of fs access icon or something
           break;
         case Messages::StopFileTransfer:
@@ -318,6 +288,13 @@ void SystemTask::Work() {
           HandleButtonAction(action);
         } break;
         case Messages::OnDisplayTaskSleeping:
+          // The state was set to GoingToSleep when GoToSleep() was called
+          // If the state is no longer GoingToSleep, we have since transitioned back to Running
+          // In this case absorb the OnDisplayTaskSleeping
+          // as DisplayApp is about to receive GoToRunning
+          if (state != SystemTaskState::GoingToSleep) {
+            break;
+          }
           if (BootloaderVersion::IsValid()) {
             // First versions of the bootloader do not expose their version and cannot initialize the SPI NOR FLASH
             // if it's in sleep mode. Avoid bricked device by disabling sleep mode on these versions.
@@ -346,37 +323,23 @@ void SystemTask::Work() {
           if (settingsController.GetNotificationStatus() != Controllers::Settings::Notification::Sleep &&
               settingsController.GetChimeOption() == Controllers::Settings::ChimesOption::Hours &&
               alarmController.State() != AlarmController::AlarmState::Alerting) {
-            // if sleeping, we can't send a chime to displayApp yet (SPI flash switched off)
-            // request running first and repush the chime message
-            if (state == SystemTaskState::Sleeping) {
-              GoToRunning();
-              PushMessage(msg);
-            } else {
-              displayApp.PushMessage(Pinetime::Applications::Display::Messages::Chime);
-            }
+            GoToRunning();
+            displayApp.PushMessage(Pinetime::Applications::Display::Messages::Chime);
           }
           break;
         case Messages::OnNewHalfHour:
           using Pinetime::Controllers::AlarmController;
           if (settingsController.GetNotificationStatus() != Controllers::Settings::Notification::Sleep &&
               settingsController.GetChimeOption() == Controllers::Settings::ChimesOption::HalfHours &&
               alarmController.State() != AlarmController::AlarmState::Alerting) {
-            // if sleeping, we can't send a chime to displayApp yet (SPI flash switched off)
-            // request running first and repush the chime message
-            if (state == SystemTaskState::Sleeping) {
-              GoToRunning();
-              PushMessage(msg);
-            } else {
-              displayApp.PushMessage(Pinetime::Applications::Display::Messages::Chime);
-            }
+            GoToRunning();
+            displayApp.PushMessage(Pinetime::Applications::Display::Messages::Chime);
           }
           break;
         case Messages::OnChargingEvent:
           batteryController.ReadPowerState();
+          GoToRunning();
           displayApp.PushMessage(Applications::Display::Messages::OnChargingEvent);
-          if (state == SystemTaskState::Sleeping) {
-            GoToRunning();
-          }
           break;
         case Messages::MeasureBatteryTimerExpired:
           batteryController.MeasureVoltage();
@@ -385,9 +348,7 @@ void SystemTask::Work() {
           nimbleController.NotifyBatteryLevel(batteryController.PercentRemaining());
           break;
         case Messages::OnPairing:
-          if (state == SystemTaskState::Sleeping) {
-            GoToRunning();
-          }
+          GoToRunning();
           displayApp.PushMessage(Pinetime::Applications::Display::Messages::ShowPairingKey);
           break;
         case Messages::BleRadioEnableToggle:
@@ -422,14 +383,50 @@ void SystemTask::Work() {
 #pragma clang diagnostic pop
 }
 
-void SystemTask::UpdateMotion() {
-  if (state == SystemTaskState::GoingToSleep || state == SystemTaskState::WakingUp) {
+void SystemTask::GoToRunning() {
+  if (state == SystemTaskState::Running) {
     return;
   }
+  // SPI doesn't go to sleep for always on mode
+  if (!settingsController.GetAlwaysOnDisplay()) {
+    spi.Wakeup();
+  }
+
+  // Double Tap needs the touch screen to be in normal mode
+  if (!settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::DoubleTap)) {
+    touchPanel.Wakeup();
+  }
 
-  if (state == SystemTaskState::Sleeping && !(settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::RaiseWrist) ||
-                                              settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::Shake) ||
-                                              motionController.GetService()->IsMotionNotificationSubscribed())) {
+  spiNorFlash.Wakeup();
+
+  displayApp.PushMessage(Pinetime::Applications::Display::Messages::GoToRunning);
+  heartRateApp.PushMessage(Pinetime::Applications::HeartRateTask::Messages::WakeUp);
+
+  if (bleController.IsRadioEnabled() && !bleController.IsConnected()) {
+    nimbleController.RestartFastAdv();
+  }
+
+  state = SystemTaskState::Running;
+};
+
+void SystemTask::GoToSleep() {
+  if (IsSleeping()) {
+    return;
+  }
+  if (IsSleepDisabled()) {
+    return;
+  }
+  NRF_LOG_INFO("[systemtask] Going to sleep");
+  displayApp.PushMessage(Pinetime::Applications::Display::Messages::GoToSleep);
+  heartRateApp.PushMessage(Pinetime::Applications::HeartRateTask::Messages::GoToSleep);
+
+  state = SystemTaskState::GoingToSleep;
+};
+
+void SystemTask::UpdateMotion() {
+  if (IsSleeping() && !(settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::RaiseWrist) ||
+                        settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::Shake) ||
+                        motionController.GetService()->IsMotionNotificationSubscribed())) {
     return;
   }
 
@@ -452,7 +449,7 @@ void SystemTask::UpdateMotion() {
   }
   if (settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::LowerWrist) && state == SystemTaskState::Running &&
       motionController.ShouldLowerSleep()) {
-    PushMessage(Messages::GoToSleep);
+    GoToSleep();
   }
 }
 
@@ -468,7 +465,7 @@ void SystemTask::HandleButtonAction(Controllers::ButtonActions action) {
   switch (action) {
     case Actions::Click:
       // If the first action after fast wakeup is a click, it should be ignored.
-      if (!fastWakeUpDone && state != SystemTaskState::GoingToSleep) {
+      if (!fastWakeUpDone) {
         displayApp.PushMessage(Applications::Display::Messages::ButtonPushed);
       }
       break;
@@ -488,17 +485,10 @@ void SystemTask::HandleButtonAction(Controllers::ButtonActions action) {
   fastWakeUpDone = false;
 }
 
-void SystemTask::GoToRunning() {
-  if (state == SystemTaskState::Sleeping) {
-    state = SystemTaskState::WakingUp;
-    PushMessage(Messages::GoToRunning);
-  }
-}
-
 void SystemTask::OnTouchEvent() {
   if (state == SystemTaskState::Running) {
     PushMessage(Messages::OnTouchEvent);
-  } else if (state == SystemTaskState::Sleeping) {
+  } else {
     if (settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::SingleTap) or
         settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::DoubleTap)) {
       PushMessage(Messages::TouchWakeUp);
@@ -507,10 +497,6 @@ void SystemTask::OnTouchEvent() {
 }
 
 void SystemTask::PushMessage(System::Messages msg) {
-  if (msg == Messages::GoToSleep && !doNotGoToSleep) {
-    state = SystemTaskState::GoingToSleep;
-  }
-
   if (in_isr()) {
     BaseType_t xHigherPriorityTaskWoken = pdFALSE;
     xQueueSendFromISR(systemTasksMsgQueue, &msg, &xHigherPriorityTaskWoken);
diff --git a/src/systemtask/SystemTask.h b/src/systemtask/SystemTask.h
@@ -52,7 +52,7 @@ namespace Pinetime {
   namespace System {
     class SystemTask {
     public:
-      enum class SystemTaskState { Sleeping, Running, GoingToSleep, WakingUp };
+      enum class SystemTaskState { Sleeping, Running, GoingToSleep };
       SystemTask(Drivers::SpiMaster& spi,
                  Pinetime::Drivers::SpiNorFlash& spiNorFlash,
                  Drivers::TwiMaster& twiMaster,
@@ -88,7 +88,7 @@ namespace Pinetime {
       };
 
       bool IsSleeping() const {
-        return state == SystemTaskState::Sleeping || state == SystemTaskState::WakingUp;
+        return state != SystemTaskState::Running;
       }
 
     private:
@@ -131,6 +131,7 @@ namespace Pinetime {
       bool fastWakeUpDone = false;
 
       void GoToRunning();
+      void GoToSleep();
       void UpdateMotion();
       bool stepCounterMustBeReset = false;
       static constexpr TickType_t batteryMeasurementPeriod = pdMS_TO_TICKS(10 * 60 * 1000);
