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 <tobias.baumgartl@ksat-stuttgart.de>
Reviewed-on: #74
This commit was merged in pull request #74.
This commit is contained in:
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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; }
|
||||
|
||||
@@ -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_ */
|
||||
|
||||
@@ -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_ */
|
||||
|
||||
Reference in New Issue
Block a user