Continuous time update - Alternative implementation to #2041 #2054

JF002 · 2024-05-01T18:17:45Z

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).

To test it, I slowed down SystemTask so that it runs every 5s and DisplayApp every 1s. In both tasks, I logged the return of GetCurrentDateTime().

On the original implementation, the result was:

<info> app: SYSTEMTASK : 2000
<info> app: SYSTEMTASK : 2000
<info> app: SYSTEMTASK : 3000
<info> app: DISPLAYTASK : 3000
<info> app: SYSTEMTASK : 4000
<info> app: DISPLAYTASK : 4000
<info> app: DISPLAYTASK : 4000
<info> app: DISPLAYTASK : 4000
<info> app: SYSTEMTASK : 6000
<info> app: DISPLAYTASK : 6000
<info> app: DISPLAYTASK : 6000
<info> app: DISPLAYTASK : 6000

DisplayTAsk didn't get any new value until SystemTask updated the current time.

With this implementation:

<info> app: SYSTEMTASK : 39000
<info> app: SYSTEMTASK : 39000
<info> app: DISPLAYTASK : 40000
<info> app: DISPLAYTASK : 41000
<info> app: SYSTEMTASK : 41000
<info> app: DISPLAYTASK : 41000
<info> app: SYSTEMTASK : 42000
<info> app: DISPLAYTASK : 42000
<info> app: SYSTEMTASK : 43000
<info> app: DISPLAYTASK : 43000
<info> app: SYSTEMTASK : 43000
<info> app: DISPLAYTASK : 43000
<info> app: SYSTEMTASK : 43000
<info> app: DISPLAYTASK : 43000
<info> app: DISPLAYTASK : 44000
<info> app: SYSTEMTASK : 45000

As you can see, DisplayTask gets intermediate results between 2 updates from SystemTask.

@mark9064 What do you think of this?

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: [email protected]

github-actions · 2024-05-01T18:21:15Z

Build size and comparison to main:

Section	Size	Difference
text	377304B	304B
data	940B	0B
bss	63540B	0B

mark9064

This is certainly an improvement over the current implementation (regarding current time reporting).

However, it does cause Seconds() Minutes() to desynchronise from the current reported time, as these methods use localTime which is only updated whenever the system task / full update runs.

Therefore a pattern we have across the codebase

currentDateTime = std::chrono::time_point_cast<std::chrono::seconds>(dateTimeController.CurrentDateTime());
if (currentDateTime.IsUpdated()) {
  uint8_t hour = dateTimeController.Hours();
  uint8_t minute = dateTimeController.Minutes();
  uint8_t second = dateTimeController.Seconds();

is broken by this changeset. Due to DirtyValue usage, the time will then not be updated for another second.

I see two fixes:

Do a full state update each time (closer to what my PR does)
Refactor the rest of the codebase to remove these Seconds() etc functions etc or to change these to calculate from the current time

In my opinion the API of Seconds() etc is poorly designed. In most operating systems, you first fetch the current time and then you convert the time stamp you received to minutes, seconds, hours, whatever you need. InfiniTime should probably promote this conversion strategy. I believe modern C++ time handling should allow this fairly easily? (if it does not, we should probably create a new type that implements an interface similar to the one below)

So the above example becomes (pseudocode)

currentDateTime = std::chrono::time_point_cast<std::chrono::seconds>(dateTimeController.CurrentDateTime());
if (currentDateTime.IsUpdated()) {
  uint8_t hour = currentDateTime.ToHours();
  uint8_t minute = currentDateTime.ToMinutes();
  uint8_t second = currentDateTime.ToSeconds();

and then the problem is resolved. This also resolves the problem of another task updating localTime between for example a Minutes() and Seconds() call which removes a possible inconsistency.

mark9064 · 2024-05-02T22:08:37Z

src/components/datetime/DateTimeController.cpp

+ xSemaphoreTake(mutex, portMAX_DELAY);
+ uint32_t systick_counter = nrf_rtc_counter_get(portNRF_RTC_REG);
+ UpdateTime(systick_counter);
+ xSemaphoreGive(mutex);


Should probably extend the critical section to include updating localTime

mark9064 · 2024-05-04T23:35:11Z

src/components/datetime/DateTimeController.cpp

+ uint32_t systick_counter = nrf_rtc_counter_get(portNRF_RTC_REG);
+ auto correctedDelta = GetTickFromPreviousSystickCounter(systick_counter) / configTICK_RATE_HZ;
+ auto result = currentDateTime + std::chrono::seconds(correctedDelta);
+ ;


Formatting typo?

Suggested change

;

mark9064 · 2024-05-04T23:51:13Z

src/components/datetime/DateTimeController.cpp

+ if (systickCounter >= rest) {
+ previousSystickCounter = systickCounter - rest;
+ } else {
+ previousSystickCounter = static_cast<uint32_t>(portNRF_RTC_MAXTICKS) - (rest - systickCounter);


Suggested change

previousSystickCounter = static_cast<uint32_t>(portNRF_RTC_MAXTICKS) - (rest - systickCounter);

previousSystickCounter = static_cast<uint32_t>(portNRF_RTC_MAXTICKS) - (rest - systickCounter - 1);

previousSystickCounter should semantically be systickCounter - rest
Suppose rest = 1, sysTickCounter = 0
previousSystickCounter should be portNRF_RTC_MAXTICKS but instead is portNRF_RTC_MAXTICKS - 1

JF002 · 2024-05-11T20:27:17Z

@mark9064 Thank yo so much for this review!

When reading your comment, it occurred to me that redesigning the whole DateTime class should be out of scope of this PR (and the one related to the Always On display functionality). I don't want to keep your changes and work as hostage because of some limitations in the current design.

So here's my suggestion : Add a TODO file to your PR that details what need to be done to improve the design (context, analysis, objectives of the refactoring). This approach allows us to continue working on the always on feature while maintaining the technical debt under control.

Here is what this TODO file might look like:

Refactoring needed

Context

The PR #2041 - Continuous time update 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 update 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 implement 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:

JF002 mentioned this pull request May 1, 2024

Continuous time updates #2041

Open

mark9064 reviewed May 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continuous time update - Alternative implementation to #2041 #2054

Continuous time update - Alternative implementation to #2041 #2054

JF002 commented May 1, 2024 •

edited

github-actions bot commented May 1, 2024

mark9064 left a comment

mark9064 May 2, 2024

mark9064 May 4, 2024

mark9064 May 4, 2024

JF002 commented May 11, 2024

	previousSystickCounter = static_cast<uint32_t>(portNRF_RTC_MAXTICKS) - (rest - systickCounter);
	previousSystickCounter = static_cast<uint32_t>(portNRF_RTC_MAXTICKS) - (rest - systickCounter - 1);

Continuous time update - Alternative implementation to #2041 #2054

Are you sure you want to change the base?

Continuous time update - Alternative implementation to #2041 #2054

Conversation

JF002 commented May 1, 2024 • edited

github-actions bot commented May 1, 2024

mark9064 left a comment

Choose a reason for hiding this comment

mark9064 May 2, 2024

Choose a reason for hiding this comment

mark9064 May 4, 2024

Choose a reason for hiding this comment

mark9064 May 4, 2024

Choose a reason for hiding this comment

JF002 commented May 11, 2024

Refactoring needed

Context

What needs to be done?

JF002 commented May 1, 2024 •

edited