Merge 21c3b1ebb0 into f8f8993fac

As the Wiki was replaced by a new documentation website and is read-only, we should update the links, to point to the most up-to-date documentation.

README.md

@@ -9,8 +9,8 @@ Fast open-source firmware for the [PineTime smartwatch](https://pine64.org/devic
 - [Getting started with InfiniTime](doc/gettingStarted/gettingStarted-1.0.md)
 - [Updating the software](doc/gettingStarted/updating-software.md)
 - [About the firmware and bootloader](doc/gettingStarted/about-software.md)
-- [PineTimeStyle Watch face](https://wiki.pine64.org/wiki/PineTimeStyle)
-  - [Weather integration](https://wiki.pine64.org/wiki/Infinitime-Weather)
+- [PineTimeStyle Watch face](https://pine64.org/documentation/PineTime/Watchfaces/PineTimeStyle)
+  - [Weather integration](https://pine64.org/documentation/PineTime/Software/InfiniTime_weather/)
 
 ### Companion apps
 

src/components/ble/SimpleWeatherService.cpp

@@ -80,7 +80,7 @@ int WeatherCallback(uint16_t /*connHandle*/, uint16_t /*attrHandle*/, struct ble
   return static_cast<Pinetime::Controllers::SimpleWeatherService*>(arg)->OnCommand(ctxt);
 }
 
-SimpleWeatherService::SimpleWeatherService(const DateTime& dateTimeController) : dateTimeController(dateTimeController) {
+SimpleWeatherService::SimpleWeatherService(DateTime& dateTimeController) : dateTimeController(dateTimeController) {
 }
 
 void SimpleWeatherService::Init() {

src/components/ble/SimpleWeatherService.h

@@ -40,7 +40,7 @@ namespace Pinetime {
 
     class SimpleWeatherService {
     public:
-      explicit SimpleWeatherService(const DateTime& dateTimeController);
+      explicit SimpleWeatherService(DateTime& dateTimeController);
 
       void Init();
 
@@ -140,7 +140,7 @@ namespace Pinetime {
 
       uint16_t eventHandle {};
 
-      const Pinetime::Controllers::DateTime& dateTimeController;
+      Pinetime::Controllers::DateTime& dateTimeController;
 
       std::optional<CurrentWeather> currentWeather;
       std::optional<Forecast> forecast;

src/components/datetime/DateTimeController.cpp

@@ -1,6 +1,8 @@
 #include "components/datetime/DateTimeController.h"
 #include <libraries/log/nrf_log.h>
 #include <systemtask/SystemTask.h>
+#include <hal/nrf_rtc.h>
+#include "nrf_assert.h"
 
 using namespace Pinetime::Controllers;
 
@@ -12,11 +14,16 @@ namespace {
 }
 
 DateTime::DateTime(Controllers::Settings& settingsController) : settingsController {settingsController} {
+  mutex = xSemaphoreCreateMutex();
+  ASSERT(mutex != nullptr);
+  xSemaphoreGive(mutex);
 }
 
 void DateTime::SetCurrentTime(std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds> t) {
+  xSemaphoreTake(mutex, portMAX_DELAY);
   this->currentDateTime = t;
-  UpdateTime(previousSystickCounter); // Update internal state without updating the time
+  UpdateTime(previousSystickCounter, true); // Update internal state without updating the time
+  xSemaphoreGive(mutex);
 }
 
 void DateTime::SetTime(uint16_t year, uint8_t month, uint8_t day, uint8_t hour, uint8_t minute, uint8_t second) {
@@ -29,13 +36,15 @@ void DateTime::SetTime(uint16_t year, uint8_t month, uint8_t day, uint8_t hour,
     /* .tm_year = */ year - 1900,
   };
 
-  tm.tm_isdst = -1; // Use DST value from local time zone
-  currentDateTime = std::chrono::system_clock::from_time_t(std::mktime(&tm));
-
   NRF_LOG_INFO("%d %d %d ", day, month, year);
   NRF_LOG_INFO("%d %d %d ", hour, minute, second);
 
-  UpdateTime(previousSystickCounter);
+  tm.tm_isdst = -1; // Use DST value from local time zone
+
+  xSemaphoreTake(mutex, portMAX_DELAY);
+  currentDateTime = std::chrono::system_clock::from_time_t(std::mktime(&tm));
+  UpdateTime(previousSystickCounter, true);
+  xSemaphoreGive(mutex);
 
   systemTask->PushMessage(System::Messages::OnNewTime);
 }
@@ -45,25 +54,34 @@ void DateTime::SetTimeZone(int8_t timezone, int8_t dst) {
   dstOffset = dst;
 }
 
-void DateTime::UpdateTime(uint32_t systickCounter) {
+std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds> DateTime::CurrentDateTime() {
+  xSemaphoreTake(mutex, portMAX_DELAY);
+  UpdateTime(nrf_rtc_counter_get(portNRF_RTC_REG), false);
+  xSemaphoreGive(mutex);
+  return currentDateTime;
+}
+
+void DateTime::UpdateTime(uint32_t systickCounter, bool forceUpdate) {
   // Handle systick counter overflow
   uint32_t systickDelta = 0;
   if (systickCounter < previousSystickCounter) {
-    systickDelta = 0xffffff - previousSystickCounter;
+    systickDelta = static_cast<uint32_t>(portNRF_RTC_MAXTICKS) - previousSystickCounter;
     systickDelta += systickCounter + 1;
   } else {
     systickDelta = systickCounter - previousSystickCounter;
   }
 
-  /*
-   * 1000 ms = 1024 ticks
-   */
-  auto correctedDelta = systickDelta / 1024;
-  auto rest = systickDelta % 1024;
+  auto correctedDelta = systickDelta / configTICK_RATE_HZ;
+  // If a second hasn't passed, there is nothing to do
+  // If the time has been changed, set forceUpdate to trigger internal state updates
+  if (correctedDelta == 0 && !forceUpdate) {
+    return;
+  }
+  auto rest = systickDelta % configTICK_RATE_HZ;
   if (systickCounter >= rest) {
     previousSystickCounter = systickCounter - rest;
   } else {
-    previousSystickCounter = 0xffffff - (rest - systickCounter);
+    previousSystickCounter = static_cast<uint32_t>(portNRF_RTC_MAXTICKS) - (rest - systickCounter - 1);
   }
 
   currentDateTime += std::chrono::seconds(correctedDelta);

src/components/datetime/DateTimeController.h

@@ -5,6 +5,8 @@
 #include <ctime>
 #include <string>
 #include "components/settings/Settings.h"
+#include <FreeRTOS.h>
+#include <semphr.h>
 
 namespace Pinetime {
   namespace System {
@@ -45,8 +47,6 @@ namespace Pinetime {
        */
       void SetTimeZone(int8_t timezone, int8_t dst);
 
-      void UpdateTime(uint32_t systickCounter);
-
       uint16_t Year() const {
         return 1900 + localTime.tm_year;
       }
@@ -124,12 +124,10 @@ namespace Pinetime {
       static const char* MonthShortToStringLow(Months month);
       static const char* DayOfWeekShortToStringLow(Days day);
 
-      std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds> CurrentDateTime() const {
-        return currentDateTime;
-      }
+      std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds> CurrentDateTime();
 
-      std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds> UTCDateTime() const {
-        return currentDateTime - std::chrono::seconds((tzOffset + dstOffset) * 15 * 60);
+      std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds> UTCDateTime() {
+        return CurrentDateTime() - std::chrono::seconds((tzOffset + dstOffset) * 15 * 60);
       }
 
       std::chrono::seconds Uptime() const {
@@ -141,10 +139,14 @@ namespace Pinetime {
       std::string FormattedTime();
 
     private:
+      void UpdateTime(uint32_t systickCounter, bool forceUpdate);
+
       std::tm localTime;
       int8_t tzOffset = 0;
       int8_t dstOffset = 0;
 
+      SemaphoreHandle_t mutex = nullptr;
+
       uint32_t previousSystickCounter = 0;
       std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds> currentDateTime;
       std::chrono::seconds uptime {0};

src/components/datetime/TODO.md

@@ -0,0 +1,41 @@
+# Refactoring needed
+
+## Context
+
+The [PR #2041 - Continuous time updates](https://github.com/InfiniTimeOrg/InfiniTime/pull/2041) highlighted some
+limitations in the design of DateTimeController: the granularity of the time returned by `DateTime::CurrentDateTime()`
+is limited by the frequency at which SystemTask calls `DateTime::UpdateTime()`, which is currently set to 100ms.
+
+@mark9064 provided more details
+in [this comment](https://github.com/InfiniTimeOrg/InfiniTime/pull/2041#issuecomment-2048528967).
+
+The [PR #2041 - Continuous time updates](https://github.com/InfiniTimeOrg/InfiniTime/pull/2041) provided some changes
+to `DateTime` controller that improves the granularity of the time returned by `DateTime::CurrentDateTime()`.
+
+However, the review showed that `DateTime` cannot be `const` anymore, even when it's only used to get the current time,
+since `DateTime::CurrentDateTime()` changes the internal state of the instance.
+
+We tried to identify alternative implementation that would have maintained the "const correctness" but we eventually
+figured that this would lead to a re-design of `DateTime` which was out of scope of the initial PR (Continuous time
+updates and always on display).
+
+So we decided to merge this PR #2041 and agree to fix/improve `DateTime` later on.
+
+## What needs to be done?
+
+Improve/redesign `DateTime` so that it
+
+* provides a very granular (ideally down to the millisecond) date and time via `CurrentDateTime()`.
+* can be declared/passed as `const` when it's only used to **get** the time.
+* limits the use of mutex as much as possible (an ideal implementation would not use any mutex, but this might not be
+  possible).
+* improves the design of `DateTime::Seconds()`, `DateTime::Minutes()`, `DateTime::Hours()`, etc as
+  explained [in this comment](https://github.com/InfiniTimeOrg/InfiniTime/pull/2054#pullrequestreview-2037033105).
+
+Once this redesign is implemented, all instances/references to `DateTime` should be reviewed and updated to use `const`
+where appropriate.
+
+Please check the following PR to get more context about this redesign:
+
+* [#2041 - Continuous time updates by @mark9064](https://github.com/InfiniTimeOrg/InfiniTime/pull/2041)
+* [#2054 - Continuous time update - Alternative implementation to #2041 by @JF002](https://github.com/InfiniTimeOrg/InfiniTime/pull/2054)

src/displayapp/screens/WatchFaceAnalog.h

@@ -75,7 +75,7 @@ namespace Pinetime {
 
         BatteryIcon batteryIcon;
 
-        const Controllers::DateTime& dateTimeController;
+        Controllers::DateTime& dateTimeController;
         const Controllers::Battery& batteryController;
         const Controllers::Ble& bleController;
         Controllers::NotificationManager& notificationManager;

src/drivers/Bma421.cpp

@@ -126,13 +126,6 @@ Bma421::Values Bma421::Process() {
   uint32_t steps = 0;
   bma423_step_counter_output(&steps, &bma);
 
-  int32_t temperature;
-  bma4_get_temperature(&temperature, BMA4_DEG, &bma);
-  temperature = temperature / 1000;
-
-  uint8_t activity = 0;
-  bma423_activity_output(&activity, &bma);
-
   // X and Y axis are swapped because of the way the sensor is mounted in the PineTime
   return {steps, data.y, data.x, data.z};
 }

src/drivers/St7789.cpp

@@ -1,3 +1,4 @@
+#include <cstring>
 #include "drivers/St7789.h"
 #include <hal/nrf_gpio.h>
 #include <nrfx_log.h>
@@ -16,10 +17,9 @@ void St7789::Init() {
   HardwareReset();
   SoftwareReset();
   SleepOut();
-  ColMod();
+  PixelFormat();
   MemoryDataAccessControl();
-  ColumnAddressSet();
-  RowAddressSet();
+  SetAddrWindow(0, 0, Width, Height);
 // P8B Mirrored version does not need display inversion.
 #ifndef DRIVER_DISPLAY_MIRROR
   DisplayInversionOn();
@@ -97,8 +97,9 @@ void St7789::SleepIn() {
   sleepIn = true;
 }
 
-void St7789::ColMod() {
-  WriteCommand(static_cast<uint8_t>(Commands::ColMod));
+void St7789::PixelFormat() {
+  WriteCommand(static_cast<uint8_t>(Commands::PixelFormat));
+  // 65K colours, 16-bit per pixel
   WriteData(0x55);
 }
 
@@ -118,22 +119,6 @@ void St7789::MemoryDataAccessControl() {
 #endif
 }
 
-void St7789::ColumnAddressSet() {
-  WriteCommand(static_cast<uint8_t>(Commands::ColumnAddressSet));
-  WriteData(0x00);
-  WriteData(0x00);
-  WriteData(Width >> 8u);
-  WriteData(Width & 0xffu);
-}
-
-void St7789::RowAddressSet() {
-  WriteCommand(static_cast<uint8_t>(Commands::RowAddressSet));
-  WriteData(0x00);
-  WriteData(0x00);
-  WriteData(320u >> 8u);
-  WriteData(320u & 0xffu);
-}
-
 void St7789::DisplayInversionOn() {
   WriteCommand(static_cast<uint8_t>(Commands::DisplayInversionOn));
 }
@@ -148,16 +133,23 @@ void St7789::DisplayOn() {
 
 void St7789::SetAddrWindow(uint16_t x0, uint16_t y0, uint16_t x1, uint16_t y1) {
   WriteCommand(static_cast<uint8_t>(Commands::ColumnAddressSet));
-  WriteData(x0 >> 8);
-  WriteData(x0 & 0xff);
-  WriteData(x1 >> 8);
-  WriteData(x1 & 0xff);
+  uint8_t colArgs[] = {
+    static_cast<uint8_t>(x0 >> 8), // x start MSB
+    static_cast<uint8_t>(x0),      // x start LSB
+    static_cast<uint8_t>(x1 >> 8), // x end MSB
+    static_cast<uint8_t>(x1)       // x end LSB
+  };
+  WriteData(colArgs, sizeof(colArgs));
 
   WriteCommand(static_cast<uint8_t>(Commands::RowAddressSet));
-  WriteData(y0 >> 8);
-  WriteData(y0 & 0xff);
-  WriteData(y1 >> 8);
-  WriteData(y1 & 0xff);
+  uint8_t rowArgs[] = {
+    static_cast<uint8_t>(y0 >> 8), // y start MSB
+    static_cast<uint8_t>(y0),      // y start LSB
+    static_cast<uint8_t>(y1 >> 8), // y end MSB
+    static_cast<uint8_t>(y1)       // y end LSB
+  };
+  memcpy(addrWindowArgs, rowArgs, sizeof(rowArgs));
+  WriteData(addrWindowArgs, sizeof(addrWindowArgs));
 }
 
 void St7789::WriteToRam(const uint8_t* data, size_t size) {
@@ -179,8 +171,12 @@ void St7789::DisplayOff() {
 void St7789::VerticalScrollStartAddress(uint16_t line) {
   verticalScrollingStartAddress = line;
   WriteCommand(static_cast<uint8_t>(Commands::VerticalScrollStartAddress));
-  WriteData(line >> 8u);
-  WriteData(line & 0x00ffu);
+  uint8_t args[] = {
+    static_cast<uint8_t>(line >> 8), // Frame memory line pointer MSB
+    static_cast<uint8_t>(line)       // Frame memory line pointer LSB
+  };
+  memcpy(verticalScrollArgs, args, sizeof(args));
+  WriteData(verticalScrollArgs, sizeof(verticalScrollArgs));
 }
 
 void St7789::Uninit() {

src/drivers/St7789.h

@@ -40,7 +40,7 @@ namespace Pinetime {
       void SleepOut();
       void EnsureSleepOutPostDelay();
       void SleepIn();
-      void ColMod();
+      void PixelFormat();
       void MemoryDataAccessControl();
       void DisplayInversionOn();
       void NormalModeOn();
@@ -68,16 +68,17 @@ namespace Pinetime {
         MemoryDataAccessControl = 0x36,
         VerticalScrollDefinition = 0x33,
         VerticalScrollStartAddress = 0x37,
-        ColMod = 0x3a,
+        PixelFormat = 0x3a,
         VdvSet = 0xc4,
       };
       void WriteData(uint8_t data);
       void WriteData(const uint8_t* data, size_t size);
-      void ColumnAddressSet();
 
       static constexpr uint16_t Width = 240;
       static constexpr uint16_t Height = 320;
-      void RowAddressSet();
+
+      uint8_t addrWindowArgs[4];
+      uint8_t verticalScrollArgs[2];
     };
   }
 }

src/systemtask/SystemTask.cpp

@@ -410,8 +410,6 @@ void SystemTask::Work() {
     }
 
     monitor.Process();
-    uint32_t systick_counter = nrf_rtc_counter_get(portNRF_RTC_REG);
-    dateTimeController.UpdateTime(systick_counter);
     NoInit_BackUpTime = dateTimeController.CurrentDateTime();
     if (nrf_gpio_pin_read(PinMap::Button) == 0) {
       watchdog.Reload();