From 03d3b6cd681ed87e23c47a9dc7ea2cd1b2f1705e Mon Sep 17 00:00:00 2001 From: Tobias Baumgartl Date: Fri, 25 Sep 2026 14:24:14 +0200 Subject: [PATCH] fix: do not mix an epoch offset into the FreeRTOS monotonic clock (#74) `Clock::getClockMonotonic` on FreeRTOS returned an epoch offset added on top of the uptime: ```cpp *time = Timekeeper::instance()->getMonotonicClockOffset() + getUptime(); ``` `monotonicClockOffset` is a `timeval` member with no initializer, and `Timekeeper`'s constructor only initialises `offset`. The singleton is heap allocated, so the member holds indeterminate bytes until `Timekeeper::setOffset` latches it, which happens on the first `Clock::setClock`. Until that point `getClockMonotonic` returns garbage plus uptime, and at the latch it jumps by whatever the difference happens to be. Any `Countdown` or `Stopwatch` armed before the latch expires wrongly, in both directions: - forward jump: `getCurrentTime() - startTime >= timeout` - backward jump: the `getCurrentTime() < startTime` guard in `Countdown::hasTimedOut` On our OBC this is not a race but the normal case. The CoreController sets the clock from its own task, so everything constructed during object creation and early boot is on the wrong side of the latch. ## Why the offset can go The monotonicity came from the uptime alone. The offset only made the value look epoch based, and nothing depends on that: - `Countdown` uses `getCurrentTime() - startTime` - `Stopwatch` uses `endTime - startTime` - `PeriodicHelper::performPeriodicHkGeneration` uses `now - setSpec.lastGenerated` Those are the only three callers in the framework. All take differences. The Linux and host implementations already return `CLOCK_MONOTONIC_RAW` with no offset, so FreeRTOS was the only OSAL where `getClockMonotonic` meant something different. The interface doc in `Clock.h` also already describes the intended contract: "less suited when the absolute time is required", with `CLOCK_MONOTONIC_RAW` named as the reference implementation. ## Changes - `osal/freertos/Clock.cpp`: `getClockMonotonic` returns `getUptime()` - `osal/freertos/Timekeeper.{h,cpp}`: drop `monotonicClockOffset`, `monotonicClockInitialized`, `getMonotonicClockOffset` and the never defined `setMonotonicClockOffset`. `setOffset` is now a one liner - `timemanager/Clock.h`: drop the `monotonicClockInitialized` and `monotonicClockOffset` statics, which were declared but never defined or used `Clock::getClock` is unchanged and still returns offset plus uptime, so the wall clock is unaffected. ## Compatibility `getClockMonotonic` on FreeRTOS now counts from scheduler start instead of the epoch. Code that compares a monotonic timestamp against a wall clock value would break, but that would already be broken on Linux, and no such code exists. --------- Co-authored-by: Tobias Baumgartl Reviewed-on: https://egit.irs.uni-stuttgart.de/KSat/fsfw/pulls/74 --- src/fsfw/osal/freertos/Clock.cpp | 3 ++- src/fsfw/osal/freertos/Timekeeper.cpp | 10 +--------- src/fsfw/osal/freertos/Timekeeper.h | 6 ------ src/fsfw/timemanager/Clock.h | 2 -- 4 files changed, 3 insertions(+), 18 deletions(-) diff --git a/src/fsfw/osal/freertos/Clock.cpp b/src/fsfw/osal/freertos/Clock.cpp index 8c7f36c2..16d2ad8f 100644 --- a/src/fsfw/osal/freertos/Clock.cpp +++ b/src/fsfw/osal/freertos/Clock.cpp @@ -48,7 +48,8 @@ ReturnValue_t Clock::getClock(timeval* time) { } ReturnValue_t Clock::getClockMonotonic(timeval* time) { - *time = Timekeeper::instance()->getMonotonicClockOffset() + getUptime(); + // Retrieves the FreeRTOS tick count, which is hopefully monotonic + *time = getUptime(); return returnvalue::OK; } diff --git a/src/fsfw/osal/freertos/Timekeeper.cpp b/src/fsfw/osal/freertos/Timekeeper.cpp index 078d88cb..6649e099 100644 --- a/src/fsfw/osal/freertos/Timekeeper.cpp +++ b/src/fsfw/osal/freertos/Timekeeper.cpp @@ -17,13 +17,7 @@ Timekeeper* Timekeeper::instance() { return myinstance; } -void Timekeeper::setOffset(const timeval& offset) { - if (not monotonicClockInitialized) { - this->monotonicClockOffset = offset; - monotonicClockInitialized = true; - } - this->offset = offset; -} +void Timekeeper::setOffset(const timeval& offset) { this->offset = offset; } timeval Timekeeper::ticksToTimeval(TickType_t ticks) { timeval uptime; @@ -39,5 +33,3 @@ timeval Timekeeper::ticksToTimeval(TickType_t ticks) { } TickType_t Timekeeper::getTicks() { return xTaskGetTickCount(); } - -const timeval Timekeeper::getMonotonicClockOffset() const { return monotonicClockOffset; } diff --git a/src/fsfw/osal/freertos/Timekeeper.h b/src/fsfw/osal/freertos/Timekeeper.h index f7970bc8..9068b902 100644 --- a/src/fsfw/osal/freertos/Timekeeper.h +++ b/src/fsfw/osal/freertos/Timekeeper.h @@ -18,14 +18,9 @@ class Timekeeper { Timekeeper(); timeval offset; - // Set when offset is initialized the first time - timeval monotonicClockOffset; - bool monotonicClockInitialized = false; static Timekeeper* myinstance; - void setMonotonicClockOffset(const timeval& monotonicClockOffset); - public: static Timekeeper* instance(); virtual ~Timekeeper(); @@ -39,7 +34,6 @@ class Timekeeper { const timeval& getOffset() const; void setOffset(const timeval& offset); - const timeval getMonotonicClockOffset() const; }; #endif /* FRAMEWORK_OSAL_FREERTOS_TIMEKEEPER_H_ */ diff --git a/src/fsfw/timemanager/Clock.h b/src/fsfw/timemanager/Clock.h index e9c1dd05..6fe6486c 100644 --- a/src/fsfw/timemanager/Clock.h +++ b/src/fsfw/timemanager/Clock.h @@ -192,8 +192,6 @@ class Clock { static MutexIF *timeMutex; static uint16_t leapSeconds; static bool leapSecondsSet; - static bool monotonicClockInitialized; - static timeval monotonicClockOffset; }; #endif /* FSFW_TIMEMANAGER_CLOCK_H_ */