Continuous time updates

[mark9064]

This changes CurrentDateTime() so every call it fetches the time fresh from the RTC. Mutual exclusion must now be used to protect access to the timekeeping data as many tasks may modify it.

This change has no power / performance impact (verified by testing in #1869)

One rationale of this is simply correctness - this removes the 0.1s jitter on the current time created by the system task. This jitter was causing problems for frame pacing with always on display. But it's also nice to have correctness when it costs no performance. This also paves the way to lengthening the system task period if we wanted to explore that further. For now, that still breaks motion wake (and probably more) so it's a no go.

There's also a few other misc fixes like correcting the tick overflow calculation and using the appropriate constants for readability.

PR split from #1869

[github-actions[bot]]

Build size and comparison to main:	Section	Size	Difference
text	377272B	256B
data	940B	0B
bss	63548B	0B

[mark9064]

Hmm, not sure about simulator breakage. Anyone know what's causing the issue is there? Is FreeRTOS not available?

[FintasticMan]

Yeah, the sim doesn't use FreeRTOS. All the files using it are duplicated with the RTOS function calls removed in InfiniSim.

[mark9064]

(InfiniSim PR now ready)

[JF002]

I'm a little worried of this new mutex. I applied the original design on purpose : SystemTask is the only class with "write accesss" to the clock. DateTimeController is const everywhere else. All modules (except the date/time setting) do not need to change anything to the clock, they just need to know what's the current time. This new implementation make it impossible (well, maybe mutable can workaround this) to const DateTimeController.

It might not be a big deal but it adds complexity and synchronization between tasks.

This jitter was causing problems for frame pacing with always on display

Could you give more details about this? I'm not sure what frame pacing is in the context of the always on feature.

[mark9064]

Yeah I don't love the mutex either. The reason why I went with this is because when anything on the watch calls CurrentDateTime(), it should get the current time. At the moment, it gets the time as of whenever the system task last executed its loop. This reduces the granularity of the main system clock to 100ms. Also, if something blocks the system task then the time can be more incorrect.

Apps like the stopwatch which measure wall clock time should use the current time, but they can't due to this imprecision. So it uses the scheduler tick instead which is currently tied to the RTC (on NRF the RTC drives the scheduler tick), but semantically this is wrong (another platform may drive the scheduler another way).

TLDR: I think CurrentDateTime() should provide the current time accurately

Regarding frame pacing: Suppose LVGL runs at t=0ms, t=500ms, t=1000ms, t=1500ms etc as in always on, and that display scanout is perfectly synchronised with LVGL. Suppose system task runs at t=0ms,t=101,202,303,404,505,606,707,808,909,1010,1111,1212,... (it always takes a little longer than 100 ticks as that's the timeout, I'm assuming 1 tick=1ms to make the example clear)

At t=0ms, LVGL will get the current time and display 00:00:00 as expected. At t=500ms, the current time is still 00:00:00 At t=1000ms, the current time is still 00:00:00 (but it should be 00:00:01) At t=1500ms, the current time is 00:00:01 (it got updated when the systemtask ran at t=1010ms).

In normal operation, LVGL runs every 20ms, so the time changing 20ms late isn't noticeable. But in always on, LVGL runs every 500ms and the time changing 500ms late is very noticeable for watchfaces that use seconds - in other words a second has appeared to last 1.5 seconds. This is what I referred to when I said there are problems with frame pacing.

Another solution would be to have CurrentDateTime() fetch the current RTC time but not update any state within the controller, splitting out state updates to a new method. But this still has synchronisation problems as the system task could be halfway through updating state inside the time controller when another task calls CurrentDateTime()

[JF002]

Thank you very much for this detailed explanation. Fast, accurate and precise time is indeed a core functionality of a watch so this PR makes sense.

However, if you don't mind, I would like to explore other options before we settle on this implementation. The 2 points to I would like to improve are

• The constness of DateTimeController : Most of the time, the DateTimeController is used to get the time, so it make sense for watch faces for example, to get a const reference to it. The fact that, internally, the time is updated also during the call to CurrentDateTime() is an implementation detail and should not be visible from the outside of the class.
• The mutex : even if it looks good, a new mutex is a new source of bugs (mostly deadlocks) and I would like to use them only if they are really needed.

Here are a few options that I would like to explore:

• use mutable on the variables that are updated when the time is updated so we can still declare the method CurrentDateTime() as const.
• Reduce the scope of the mutex : It's currently applied on the whole DateTime::UpdateTime() method. This method does quite a lot of things (notify new hours, half hours and days, process the utime) in addition to just keeping track of the current time.
• Keep the original implementation (SystemTask updates DateTimeController::currentDateTime, other tasks only read it) but extend the function CurrentDateTime() so that it takes the value of currentDateTime and applies the offset since the last time it was updated before returning the value, but doesn't store this new processed value. This way, there's a single writer of the current time, and the value returned by CurrentDateTime is always corrected according to the most recent value from the RTC timer.

Let me know what you think of those options. I'm willing to make a proof-of-concept if you want me to ;-)

[mark9064]

I'm totally happy with any implementation that results in up to date time, not attached to this one! It's worth knowing that the mutex is priority inverting though. I imagine you're already familiar but if not: if another task is holding it when a higher priority task needs it, the lower priority task will be raised up to match the priority of the higher task.

Currently, this implementation will deadlock if the system task message queue is full, and the system task is currently waiting for the mutex. Priority inversion assures that a low priority task cannot block the system task (or any higher priority task) in any other way.

• I didn't even know mutable existed, so I'll need to research that one :)
• Reducing the scope of the mutex is largely unimportant in my opinion as the critical section is already fast. The function checks whether the second count changed, and exits immediately if it does not. This means that the second half of the function (which has the blocking characteristics discussed above) can only execute at most once per second. We have also already seen that adding this synchronisation did not change power usage, so we know the CPU does not spend more time awake with the synchronisation in place. Though I cannot measure it, I think it follows that mutex contention is very low.
• The offset option was actually what I considered first. I went with this approach as I'm not sure the current structure of the controller allows the system task to update the controller state atomically (need to change previousSystickCounter and currentDateTime at the same time). We actually already have this correctness issue as DisplayApp and the current time service can update the time at the same time as the system task and cause weirdness.

One thing I've just noticed. We could move just the system task messages out of the mutex. This would make it impossible for the function to block any other task (and therefore deadlock) in any scenario. Though of course the system task could still deadlock itself, but it can do this already.

[JF002]

I've just implemented the "offset option" in #2054. @mark9064 What do you think of this one?

[JF002]

@mark9064 If you're OK with this suggestion, I'll add the mentionned TODO file and merge this branch :)

[mark9064]

In hindsight, you're totally right that a full refactor would be out of scope for this, I really appreciate your feedback and explanation. Resolving to go ahead with this and rework later makes sense. The TODO file looks great, I've fixed a couple typos but other than that I think it's good to go. Adjusted version below + in code block

Where exactly should this file go?

PS: I'll rebase the branch after the TODO file is in

---

Refactoring needed

Context

The PR #2041 - Continuous time updates 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.

The PR #2041 - Continuous time updates 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.

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
• #2054 - Continuous time update - Alternative implementation to #2041 by @JF002

  
  # Refactoring needed
  ## Context

The PR #2041 - Continuous time updates 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.

The PR #2041 - Continuous time updates 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.

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
• #2054 - Continuous time update - Alternative implementation to #2041 by @JF002

[JF002]

@mark9064 I've just added the TODO file in src/components/datetime. I'll merge this branch as soon as the Actions are successful :)