diff options
Diffstat (limited to 'src/components/datetime')
| -rw-r--r-- | src/components/datetime/DateTimeController.cpp | 77 | ||||
| -rw-r--r-- | src/components/datetime/DateTimeController.h | 20 | ||||
| -rw-r--r-- | src/components/datetime/TODO.md | 41 |
3 files changed, 107 insertions, 31 deletions
diff --git a/src/components/datetime/DateTimeController.cpp b/src/components/datetime/DateTimeController.cpp index 9e9fb6e4..d439821b 100644 --- a/src/components/datetime/DateTimeController.cpp +++ b/src/components/datetime/DateTimeController.cpp @@ -1,22 +1,42 @@ #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; namespace { - char const* DaysStringShort[] = {"--", "MON", "TUE", "WED", "THU", "FRI", "SAT", "SUN"}; - char const* DaysStringShortLow[] = {"--", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"}; - char const* MonthsString[] = {"--", "JAN", "FEB", "MAR", "APR", "MAY", "JUN", "JUL", "AUG", "SEP", "OCT", "NOV", "DEC"}; - char const* MonthsStringLow[] = {"--", "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"}; + constexpr const char* const DaysStringShort[] = {"--", "MON", "TUE", "WED", "THU", "FRI", "SAT", "SUN"}; + constexpr const char* const DaysStringShortLow[] = {"--", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"}; + constexpr const char* const MonthsString[] = {"--", "JAN", "FEB", "MAR", "APR", "MAY", "JUN", "JUL", "AUG", "SEP", "OCT", "NOV", "DEC"}; + constexpr const char* const MonthsStringLow[] = + {"--", "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"}; + + constexpr int compileTimeAtoi(const char* str) { + int result = 0; + while (*str >= '0' && *str <= '9') { + result = result * 10 + *str - '0'; + str++; + } + return result; + } } DateTime::DateTime(Controllers::Settings& settingsController) : settingsController {settingsController} { + mutex = xSemaphoreCreateMutex(); + ASSERT(mutex != nullptr); + xSemaphoreGive(mutex); + + // __DATE__ is a string of the format "MMM DD YYYY", so an offset of 7 gives the start of the year + SetTime(compileTimeAtoi(&__DATE__[7]), 1, 1, 0, 0, 0); } 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,15 +49,19 @@ 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); + if (systemTask != nullptr) { + systemTask->PushMessage(System::Messages::OnNewTime); + } } void DateTime::SetTimeZone(int8_t timezone, int8_t dst) { @@ -45,25 +69,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); @@ -115,8 +148,8 @@ const char* DateTime::MonthShortToStringLow(Months month) { return MonthsStringLow[static_cast<uint8_t>(month)]; } -const char* DateTime::DayOfWeekShortToStringLow() const { - return DaysStringShortLow[static_cast<uint8_t>(DayOfWeek())]; +const char* DateTime::DayOfWeekShortToStringLow(Days day) { + return DaysStringShortLow[static_cast<uint8_t>(day)]; } void DateTime::Register(Pinetime::System::SystemTask* systemTask) { @@ -140,9 +173,9 @@ std::string DateTime::FormattedTime() { hour12 = (hour == 12) ? 12 : hour - 12; amPmStr = "PM"; } - sprintf(buff, "%i:%02i %s", hour12, minute, amPmStr); + snprintf(buff, sizeof(buff), "%i:%02i %s", hour12, minute, amPmStr); } else { - sprintf(buff, "%02i:%02i", hour, minute); + snprintf(buff, sizeof(buff), "%02i:%02i", hour, minute); } return std::string(buff); } diff --git a/src/components/datetime/DateTimeController.h b/src/components/datetime/DateTimeController.h index 0bf6ac2a..a005f9ac 100644 --- a/src/components/datetime/DateTimeController.h +++ b/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 { @@ -39,14 +41,12 @@ namespace Pinetime { * * used to update difference between utc and local time (see UtcOffset()) * - * parameters are in quarters of an our. Following the BLE CTS specification, + * parameters are in quarters of an hour. Following the BLE CTS specification, * timezone is expected to be constant over DST which will be reported in * dst field. */ void SetTimeZone(int8_t timezone, int8_t dst); - void UpdateTime(uint32_t systickCounter); - uint16_t Year() const { return 1900 + localTime.tm_year; } @@ -122,14 +122,12 @@ namespace Pinetime { const char* MonthShortToString() const; const char* DayOfWeekShortToString() const; static const char* MonthShortToStringLow(Months month); - const char* DayOfWeekShortToStringLow() const; + 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}; diff --git a/src/components/datetime/TODO.md b/src/components/datetime/TODO.md new file mode 100644 index 00000000..e9590898 --- /dev/null +++ b/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)
\ No newline at end of file |
