From c3d05901a05a274f30c15b8c0640b6ecdd973ac3 Mon Sep 17 00:00:00 2001 From: mark9064 <30447455+mark9064@users.noreply.github.com> Date: Thu, 22 Aug 2024 21:24:04 +0100 Subject: [PATCH] Refactor SystemTask state handling for resilience 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 --- src/components/ble/DfuService.cpp | 8 +- src/components/ble/NimbleController.cpp | 10 +- src/displayapp/DisplayApp.cpp | 23 +++- src/systemtask/SystemTask.cpp | 142 +++++++++++------------- src/systemtask/SystemTask.h | 5 +- 5 files changed, 100 insertions(+), 88 deletions(-) diff --git a/src/components/ble/DfuService.cpp b/src/components/ble/DfuService.cpp index b3f2ff10..2427513d 100644 --- 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 index f1411a3e..5059007a 100644 --- 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 index 79519621..076b4f8a 100644 --- 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 index 848fb54c..4c623883 100644 --- 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,14 +323,8 @@ 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: @@ -361,22 +332,14 @@ void SystemTask::Work() { 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(); + } - if (state == SystemTaskState::Sleeping && !(settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::RaiseWrist) || - settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::Shake) || - motionController.GetService()->IsMotionNotificationSubscribed())) { + // 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; +}; + +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 index 11dea52c..339587c1 100644 --- 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);