New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Continuous time updates #2041
base: main
Are you sure you want to change the base?
Continuous time updates #2041
Conversation
Build size and comparison to main:
|
Hmm, not sure about simulator breakage. Anyone know what's causing the issue is there? Is FreeRTOS not available? |
Yeah, the sim doesn't use FreeRTOS. All the files using it are duplicated with the RTOS function calls removed in InfiniSim. |
*/ | ||
auto correctedDelta = systickDelta / 1024; | ||
auto rest = systickDelta % 1024; | ||
auto correctedDelta = systickDelta / configTICK_RATE_HZ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I think about it, the RTOS tick rate isn't necessarily the RTC tick rate. But we drive the RTOS tick from the RTC. Part of me wonders whether we should separate the RTOS tick and RTC tick at all. The simplicity of a unified tick would be nice
(InfiniSim PR now ready) |
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. It might not be a big deal but it adds complexity and synchronization between tasks.
Could you give more details about this? I'm not sure what frame pacing is in the context of the always on feature. |
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. 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() |
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
Here are a few options that I would like to explore:
Let me know what you think of those options. I'm willing to make a proof-of-concept if you want me to ;-) |
I'm totally happy with any implementation that results in up to date time, not attached to this one! 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.
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. |
This is an alternative implementation to #2041 we talked about in this comment (#2041 (comment)). This implementation does not change the state of the DateTime controller (previousSystickCounter and currentDateTime fields) in GetCurrentDateTime(). This allows to keep the method GetCurrentDateTime() const. I also applied a small refactoring of the methods UpdateTime() to avoid trying to lock the same mutex multiple times (FreeRTOS mutexes are not reentrant). Co-authored-by: 30447455+mark9064@users.noreply.github.com
@mark9064 If you're OK with this suggestion, I'll add the mentionned TODO file and merge this branch :) |
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