GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Continuous time update - Alternative implementation to #2041 by JF002 · Pull Request #2054 · InfiniTimeOrg/InfiniTime

Conversation

JF002 commented May 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Continuous time update - Alternative implementation to #2041

d7e4f79

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 Bot commented May 1, 2024

Copy link

Copy Markdown

Build size and comparison to main:

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JF002 mentioned this pull request

May 1, 2024

Continuous time updates #2041

Merged

mark9064 reviewed

May 5, 2024

View reviewed changes

mark9064 left a comment

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

mark9064 May 2, 2024

Copy link

Copy Markdown

Member

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.

Should probably extend the critical section to include updating localTime

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

mark9064 May 4, 2024

Copy link

Copy Markdown

Member

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.

Formatting typo?

Suggested change

;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/components/datetime/DateTimeController.cpp

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

mark9064 May 4, 2024

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JF002 commented May 11, 2024

Copy link

Copy Markdown

Collaborator Author

@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:

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JF002 commented Jun 9, 2024

Copy link

Copy Markdown

Collaborator Author

Closing in favor of #2041.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JF002 closed this

Jun 9, 2024

Reviewers

mark9064

mark9064 left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

JF002 commented May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 1, 2024

Uh oh!

mark9064 left a comment

Choose a reason for hiding this comment

Uh oh!

mark9064 May 2, 2024

Choose a reason for hiding this comment

Uh oh!

mark9064 May 4, 2024

Choose a reason for hiding this comment

Uh oh!

mark9064 May 4, 2024

Choose a reason for hiding this comment

Uh oh!

JF002 commented May 11, 2024

Refactoring needed

Context

What needs to be done?

Uh oh!

JF002 commented Jun 9, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JF002 commented May 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.