Clean-up time conversions on the way to fix #30990 (#31021)

* Clean-up time conversions locations - Test conversions in Time Sync cluster were not in a unit-testable location. - TestTimeUtils was not using nl-unit-test - Time sync cluster server build rules were missing dependencies - Documentation for many time conversion methods was not accurate - `secondsToMilliseconds` did not match coding style - Cleaned-up cut and paste errors in some adjacent tests. Issue #30990 Testing done: - Unit tests and integration tests still pass. - Started tests for edge conditions (which found #30990) * Restyled by clang-format * Add backwards compatibility --------- Co-authored-by: [email protected] <[email protected]> Co-authored-by: Restyled.io <[email protected]>
project-chip · Jan 6, 2024 · 1139319 · 1139319
1 parent 61aec89
commit 1139319
Show file tree

Hide file tree

Showing 10 changed files with 154 additions and 83 deletions.
diff --git a/src/app/chip_data_model.gni b/src/app/chip_data_model.gni
@@ -268,6 +268,7 @@ template("chip_data_model") {
       } else if (cluster == "time-synchronization-server") {
         sources += [
           "${_app_root}/clusters/${cluster}/${cluster}.cpp",
+          "${_app_root}/clusters/${cluster}/${cluster}.h",
           "${_app_root}/clusters/${cluster}/DefaultTimeSyncDelegate.cpp",
           "${_app_root}/clusters/${cluster}/TimeSyncDataProvider.cpp",
         ]

diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp
@@ -122,33 +122,11 @@ Delegate * GetDefaultDelegate()
 } // namespace app
 } // namespace chip
 
-constexpr uint64_t kChipEpochUsSinceUnixEpoch =
-    static_cast<uint64_t>(kChipEpochSecondsSinceUnixEpoch) * chip::kMicrosecondsPerSecond;
-
-static bool ChipEpochToUnixEpochMicro(uint64_t chipEpochTime, uint64_t & unixEpochTime)
-{
-    // in case chipEpochTime is too big and result overflows return false
-    if (chipEpochTime + kChipEpochUsSinceUnixEpoch < kChipEpochUsSinceUnixEpoch)
-    {
-        return false;
-    }
-    unixEpochTime = chipEpochTime + kChipEpochUsSinceUnixEpoch;
-    return true;
-}
-
-static bool UnixEpochToChipEpochMicro(uint64_t unixEpochTime, uint64_t & chipEpochTime)
-{
-    VerifyOrReturnValue(unixEpochTime >= kChipEpochUsSinceUnixEpoch, false);
-    chipEpochTime = unixEpochTime - kChipEpochUsSinceUnixEpoch;
-
-    return true;
-}
-
 static CHIP_ERROR UpdateUTCTime(uint64_t UTCTimeInChipEpochUs)
 {
     uint64_t UTCTimeInUnixEpochUs;
 
-    VerifyOrReturnError(ChipEpochToUnixEpochMicro(UTCTimeInChipEpochUs, UTCTimeInUnixEpochUs), CHIP_ERROR_INVALID_TIME);
+    VerifyOrReturnError(ChipEpochToUnixEpochMicros(UTCTimeInChipEpochUs, UTCTimeInUnixEpochUs), CHIP_ERROR_INVALID_TIME);
     uint64_t secs = UTCTimeInChipEpochUs / chip::kMicrosecondsPerSecond;
     // https://github.com/project-chip/connectedhomeip/issues/27501
     VerifyOrReturnError(secs <= UINT32_MAX, CHIP_IM_GLOBAL_STATUS(ResourceExhausted));
@@ -820,7 +798,7 @@ CHIP_ERROR TimeSynchronizationServer::GetLocalTime(EndpointId ep, DataModel::Nul
     TimeState newState = UpdateDSTOffsetState();
     VerifyOrReturnError(TimeState::kInvalid != newState, CHIP_ERROR_INVALID_TIME);
     ReturnErrorOnFailure(System::SystemClock().GetClock_RealTime(utcTime));
-    VerifyOrReturnError(UnixEpochToChipEpochMicro(utcTime.count(), chipEpochTime), CHIP_ERROR_INVALID_TIME);
+    VerifyOrReturnError(UnixEpochToChipEpochMicros(utcTime.count(), chipEpochTime), CHIP_ERROR_INVALID_TIME);
     if (TimeState::kChanged == UpdateTimeZoneState())
     {
         emitTimeZoneStatusEvent(ep);
@@ -863,7 +841,7 @@ TimeState TimeSynchronizationServer::UpdateTimeZoneState()
 
     VerifyOrReturnValue(System::SystemClock().GetClock_RealTime(utcTime) == CHIP_NO_ERROR, TimeState::kInvalid);
     VerifyOrReturnValue(tzList.size() != 0, TimeState::kInvalid);
-    VerifyOrReturnValue(UnixEpochToChipEpochMicro(utcTime.count(), chipEpochTime), TimeState::kInvalid);
+    VerifyOrReturnValue(UnixEpochToChipEpochMicros(utcTime.count(), chipEpochTime), TimeState::kInvalid);
 
     for (size_t i = 0; i < tzList.size(); i++)
     {
@@ -902,7 +880,7 @@ TimeState TimeSynchronizationServer::UpdateDSTOffsetState()
 
     VerifyOrReturnValue(System::SystemClock().GetClock_RealTime(utcTime) == CHIP_NO_ERROR, TimeState::kInvalid);
     VerifyOrReturnValue(dstList.size() != 0, TimeState::kInvalid);
-    VerifyOrReturnValue(UnixEpochToChipEpochMicro(utcTime.count(), chipEpochTime), TimeState::kInvalid);
+    VerifyOrReturnValue(UnixEpochToChipEpochMicros(utcTime.count(), chipEpochTime), TimeState::kInvalid);
 
     for (size_t i = 0; i < dstList.size(); i++)
     {
@@ -1066,7 +1044,7 @@ CHIP_ERROR TimeSynchronizationAttrAccess::Read(const ConcreteReadAttributePath &
             return aEncoder.EncodeNull();
         }
         VerifyOrReturnError(System::SystemClock().GetClock_RealTime(utcTimeUnix) == CHIP_NO_ERROR, aEncoder.EncodeNull());
-        VerifyOrReturnError(UnixEpochToChipEpochMicro(utcTimeUnix.count(), chipEpochTime), aEncoder.EncodeNull());
+        VerifyOrReturnError(UnixEpochToChipEpochMicros(utcTimeUnix.count(), chipEpochTime), aEncoder.EncodeNull());
         return aEncoder.Encode(chipEpochTime);
     }
     case Granularity::Id: {

diff --git a/src/app/tests/TestICDManager.cpp b/src/app/tests/TestICDManager.cpp
@@ -129,7 +129,7 @@ class TestICDManager
      *
      * @param time_ms: Value in milliseconds.
      */
-    static void AdvanceClockAndRunEventLoop(TestContext * ctx, uint32_t time_ms)
+    static void AdvanceClockAndRunEventLoop(TestContext * ctx, uint64_t time_ms)
     {
         ctx->mMockClock.AdvanceMonotonic(System::Clock::Timeout(time_ms));
         ctx->GetIOContext().DriveIO();
@@ -141,13 +141,13 @@ class TestICDManager
 
         // After the init we should be in Idle mode
         NL_TEST_ASSERT(aSuite, ctx->mICDManager.mOperationalState == ICDManager::OperationalState::IdleMode);
-        AdvanceClockAndRunEventLoop(ctx, secondsToMilliseconds(ICDConfigurationData::GetInstance().GetIdleModeDurationSec()) + 1);
+        AdvanceClockAndRunEventLoop(ctx, SecondsToMilliseconds(ICDConfigurationData::GetInstance().GetIdleModeDurationSec()) + 1);
         // Idle mode interval expired, ICDManager transitioned to the ActiveMode.
         NL_TEST_ASSERT(aSuite, ctx->mICDManager.mOperationalState == ICDManager::OperationalState::ActiveMode);
         AdvanceClockAndRunEventLoop(ctx, ICDConfigurationData::GetInstance().GetActiveModeDurationMs() + 1);
         // Active mode interval expired, ICDManager transitioned to the IdleMode.
         NL_TEST_ASSERT(aSuite, ctx->mICDManager.mOperationalState == ICDManager::OperationalState::IdleMode);
-        AdvanceClockAndRunEventLoop(ctx, secondsToMilliseconds(ICDConfigurationData::GetInstance().GetIdleModeDurationSec()) + 1);
+        AdvanceClockAndRunEventLoop(ctx, SecondsToMilliseconds(ICDConfigurationData::GetInstance().GetIdleModeDurationSec()) + 1);
         // Idle mode interval expired, ICDManager transitioned to the ActiveMode.
         NL_TEST_ASSERT(aSuite, ctx->mICDManager.mOperationalState == ICDManager::OperationalState::ActiveMode);
 

diff --git a/src/ble/tests/TestBleErrorStr.cpp b/src/ble/tests/TestBleErrorStr.cpp
@@ -118,7 +118,7 @@ int TestBleErrorStr()
     // clang-format off
     nlTestSuite theSuite =
 	{
-        "Ble-Error-Strings",
+        "Test BLE range error strings conversions",
         &sTests[0],
         nullptr,
         nullptr

diff --git a/src/controller/tests/TestReadChunking.cpp b/src/controller/tests/TestReadChunking.cpp
@@ -780,7 +780,7 @@ void TestReadChunking::TestDynamicEndpoint(nlTestSuite * apSuite, void * apConte
         NL_TEST_ASSERT(apSuite, readCallback.mOnReportEnd);
     }
 
-    chip::test_utils::SleepMillis(secondsToMilliseconds(2));
+    chip::test_utils::SleepMillis(SecondsToMilliseconds(2));
 
     // Destroying the read client will terminate the subscription transaction.
     ctx.DrainAndServiceIO();
@@ -1045,7 +1045,7 @@ void TestReadChunking::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void * a
         }
     }
 
-    chip::test_utils::SleepMillis(secondsToMilliseconds(3));
+    chip::test_utils::SleepMillis(SecondsToMilliseconds(3));
 
     // Destroying the read client will terminate the subscription transaction.
     ctx.DrainAndServiceIO();

diff --git a/src/lib/core/tests/TestCHIPErrorStr.cpp b/src/lib/core/tests/TestCHIPErrorStr.cpp
@@ -225,7 +225,7 @@ int TestCHIPErrorStr()
     // clang-format off
     nlTestSuite theSuite =
 	{
-        "Core-Error-Strings",
+        "Test CHIP_ERROR string conversions",
         &sTests[0],
         nullptr,
         nullptr

diff --git a/src/lib/support/TimeUtils.cpp b/src/lib/support/TimeUtils.cpp
@@ -563,11 +563,30 @@ void ChipEpochToCalendarTime(uint32_t chipEpochTime, uint16_t & year, uint8_t &
                                         dayOfMonth, hour, minute, second);
 }
 
-bool UnixEpochToChipEpochTime(uint32_t unixEpochTime, uint32_t & chipEpochTime)
+bool ChipEpochToUnixEpochMicros(uint64_t chipEpochTimeMicros, uint64_t & outUnixEpochTimeMicros)
 {
-    VerifyOrReturnError(unixEpochTime >= kChipEpochSecondsSinceUnixEpoch, false);
+    if ((chipEpochTimeMicros + kChipEpochUsSinceUnixEpoch) < kChipEpochUsSinceUnixEpoch)
+    {
+        return false;
+    }
+
+    outUnixEpochTimeMicros = chipEpochTimeMicros + kChipEpochUsSinceUnixEpoch;
+    return true;
+}
+
+bool UnixEpochToChipEpochMicros(uint64_t unixEpochTimeMicros, uint64_t & outChipEpochTimeMicros)
+{
+    VerifyOrReturnValue(unixEpochTimeMicros >= kChipEpochUsSinceUnixEpoch, false);
+    outChipEpochTimeMicros = unixEpochTimeMicros - kChipEpochUsSinceUnixEpoch;
+
+    return true;
+}
+
+bool UnixEpochToChipEpochTime(uint32_t unixEpochTimeSeconds, uint32_t & outChipEpochTimeSeconds)
+{
+    VerifyOrReturnError(unixEpochTimeSeconds >= kChipEpochSecondsSinceUnixEpoch, false);
 
-    chipEpochTime = unixEpochTime - kChipEpochSecondsSinceUnixEpoch;
+    outChipEpochTimeSeconds = unixEpochTimeSeconds - kChipEpochSecondsSinceUnixEpoch;
 
     return true;
 }

diff --git a/src/lib/support/TimeUtils.h b/src/lib/support/TimeUtils.h
@@ -111,18 +111,22 @@ enum
     kChipEpochSecondsSinceUnixEpoch = kChipEpochDaysSinceUnixEpoch * kSecondsPerDay,
 };
 
-extern bool IsLeapYear(uint16_t year);
-extern uint8_t DaysInMonth(uint16_t year, uint8_t month);
-extern uint8_t FirstWeekdayOfYear(uint16_t year);
-extern void OrdinalDateToCalendarDate(uint16_t year, uint16_t dayOfYear, uint8_t & month, uint8_t & dayOfMonth);
-extern void CalendarDateToOrdinalDate(uint16_t year, uint8_t month, uint8_t dayOfMonth, uint16_t & dayOfYear);
-extern bool CalendarDateToDaysSinceUnixEpoch(uint16_t year, uint8_t month, uint8_t dayOfMonth, uint32_t & daysSinceEpoch);
-extern bool DaysSinceUnixEpochToCalendarDate(uint32_t daysSinceEpoch, uint16_t & year, uint8_t & month, uint8_t & dayOfMonth);
-extern bool AdjustCalendarDate(uint16_t & year, uint8_t & month, uint8_t & dayOfMonth, int32_t relativeDays);
-extern bool CalendarTimeToSecondsSinceUnixEpoch(uint16_t year, uint8_t month, uint8_t dayOfMonth, uint8_t hour, uint8_t minute,
-                                                uint8_t second, uint32_t & secondsSinceEpoch);
-extern void SecondsSinceUnixEpochToCalendarTime(uint32_t secondsSinceEpoch, uint16_t & year, uint8_t & month, uint8_t & dayOfMonth,
-                                                uint8_t & hour, uint8_t & minute, uint8_t & second);
+// Difference in microseconds between Unix epoch (Jan 1 1970 00:00:00) and CHIP Epoch (Jan 1 2000 00:00:00).
+constexpr uint64_t kChipEpochUsSinceUnixEpoch =
+    static_cast<uint64_t>(kChipEpochSecondsSinceUnixEpoch) * chip::kMicrosecondsPerSecond;
+
+bool IsLeapYear(uint16_t year);
+uint8_t DaysInMonth(uint16_t year, uint8_t month);
+uint8_t FirstWeekdayOfYear(uint16_t year);
+void OrdinalDateToCalendarDate(uint16_t year, uint16_t dayOfYear, uint8_t & month, uint8_t & dayOfMonth);
+void CalendarDateToOrdinalDate(uint16_t year, uint8_t month, uint8_t dayOfMonth, uint16_t & dayOfYear);
+bool CalendarDateToDaysSinceUnixEpoch(uint16_t year, uint8_t month, uint8_t dayOfMonth, uint32_t & daysSinceEpoch);
+bool DaysSinceUnixEpochToCalendarDate(uint32_t daysSinceEpoch, uint16_t & year, uint8_t & month, uint8_t & dayOfMonth);
+bool AdjustCalendarDate(uint16_t & year, uint8_t & month, uint8_t & dayOfMonth, int32_t relativeDays);
+bool CalendarTimeToSecondsSinceUnixEpoch(uint16_t year, uint8_t month, uint8_t dayOfMonth, uint8_t hour, uint8_t minute,
+                                         uint8_t second, uint32_t & secondsSinceEpoch);
+void SecondsSinceUnixEpochToCalendarTime(uint32_t secondsSinceEpoch, uint16_t & year, uint8_t & month, uint8_t & dayOfMonth,
+                                         uint8_t & hour, uint8_t & minute, uint8_t & second);
 
 /**
  *  @brief Convert a calendar date and time to the number of seconds since CHIP Epoch (2000-01-01 00:00:00 UTC).
@@ -141,8 +145,8 @@ extern void SecondsSinceUnixEpochToCalendarTime(uint32_t secondsSinceEpoch, uint
  *  @return   True if the date/time was converted successfully. False if the given year falls outside the
  *            representable range.
  */
-extern bool CalendarToChipEpochTime(uint16_t year, uint8_t month, uint8_t dayOfMonth, uint8_t hour, uint8_t minute, uint8_t second,
-                                    uint32_t & chipEpochTime);
+bool CalendarToChipEpochTime(uint16_t year, uint8_t month, uint8_t dayOfMonth, uint8_t hour, uint8_t minute, uint8_t second,
+                             uint32_t & chipEpochTime);
 
 /**
  *  @brief Convert the number of seconds since CHIP Epoch (2000-01-01 00:00:00 UTC) to a calendar date and time.
@@ -158,34 +162,64 @@ extern bool CalendarToChipEpochTime(uint16_t year, uint8_t month, uint8_t dayOfM
  *  @param minute         Minute (0-59).
  *  @param second         Second (0-59).
  */
-extern void ChipEpochToCalendarTime(uint32_t chipEpochTime, uint16_t & year, uint8_t & month, uint8_t & dayOfMonth, uint8_t & hour,
-                                    uint8_t & minute, uint8_t & second);
+void ChipEpochToCalendarTime(uint32_t chipEpochTime, uint16_t & year, uint8_t & month, uint8_t & dayOfMonth, uint8_t & hour,
+                             uint8_t & minute, uint8_t & second);
 
 /**
- *  @brief Convert the number of seconds since Unix Epoch (1970-01-01 00:00:00 UTC) to
+ *  @brief Convert the number of seconds since Unix Epoch (1970-01-01 00:00:00 GMT TAI) to
  *         CHIP Epoch (2000-01-01 00:00:00 UTC).
  *
  *  @details  Input time values are limited to positive values up to 2^32-1. This limits the
  *            representable date range to the year 2135.
  *
- *  @param unixEpochTime  Number of seconds since 1970-01-01 00:00:00 UTC.
- *  @param chipEpochTime  Number of seconds since 2000-01-01 00:00:00 UTC.
+ *  @param[in] unixEpochTimeSeconds  Number of seconds since 1970-01-01 00:00:00 GMT TAI.
+ *  @param[out] outChipEpochTimeSeconds  Number of seconds since 2000-01-01 00:00:00 UTC.
+ *
+ *  @return   True if the time was converted successfully. False if the given Unix epoch time
+ *            falls outside the representable range.
+ */
+bool UnixEpochToChipEpochTime(uint32_t unixEpochTimeSeconds, uint32_t & outChipEpochTimeSeconds);
+
+/**
+ *  @brief Convert the number of microseconds since CHIP Epoch (2000-01-01 00:00:00 UTC) to
+ *         Unix Epoch (1970-01-01 00:00:00 GMT TAI).
+ *
+ *  @param[in] chipEpochTimeMicros  Number of microseconds since 2000-01-01 00:00:00 UTC.
+ *  @param[out] outUnixEpochTimeMicros  Number of microseconds since 1970-01-01 00:00:00 GMT TAI.
+ *
+ *  @return   True if the time was converted successfully. False if the given CHIP epoch time
+ *            falls outside the representable range.
+ */
+bool ChipEpochToUnixEpochMicros(uint64_t chipEpochTimeMicros, uint64_t & outUnixEpochTimeMicros);
+
+/**
+ *  @brief Convert the number of microseconds since Unix Epoch (1970-01-01 00:00:00 GMT TAI) to
+ *         CHIP Epoch (2000-01-01 00:00:00 UTC).
+ *
+ *  @param[in] unixEpochTimeMicros  Number of microseconds since 1970-01-01 00:00:00 GMT TAI.
+ *  @param[out] outChipEpochTimeMicros  Number of microseconds since 2000-01-01 00:00:00 UTC.
  *
  *  @return   True if the time was converted successfully. False if the given Unix epoch time
  *            falls outside the representable range.
  */
-extern bool UnixEpochToChipEpochTime(uint32_t unixEpochTime, uint32_t & chipEpochTime);
+bool UnixEpochToChipEpochMicros(uint64_t unixEpochTimeMicros, uint64_t & outChipEpochTimeMicros);
 
 /**
- *  @def secondsToMilliseconds
+ *  @def SecondsToMilliseconds
  *
  *  @brief
  *    Convert integer seconds to milliseconds.
  *
  */
-inline uint32_t secondsToMilliseconds(uint32_t seconds)
+inline uint64_t SecondsToMilliseconds(uint32_t seconds)
 {
     return (seconds * kMillisecondsPerSecond);
 }
 
+// For backwards-compatibility of public API.
+[[deprecated("Use SecondsToMilliseconds")]] inline uint64_t secondsToMilliseconds(uint32_t seconds)
+{
+    return SecondsToMilliseconds(seconds);
+}
+
 } // namespace chip
diff --git a/src/lib/support/tests/TestErrorStr.cpp b/src/lib/support/tests/TestErrorStr.cpp
@@ -183,7 +183,7 @@ int TestErrorStr()
     // clang-format off
     nlTestSuite theSuite =
 	{
-        "-Error-Strings",
+        "Test the ErrorStr primitives",
         &sTests[0],
         nullptr,
         nullptr