|
| 1 | +# Refactoring needed |
| 2 | + |
| 3 | +## Context |
| 4 | + |
| 5 | +The [PR #2041 - Continuous time updates](https://github.com/InfiniTimeOrg/InfiniTime/pull/2041) highlighted some |
| 6 | +limitations in the design of DateTimeController: the granularity of the time returned by `DateTime::CurrentDateTime()` |
| 7 | +is limited by the frequency at which SystemTask calls `DateTime::UpdateTime()`, which is currently set to 100ms. |
| 8 | + |
| 9 | +@mark9064 provided more details |
| 10 | +in [this comment](https://github.com/InfiniTimeOrg/InfiniTime/pull/2041#issuecomment-2048528967). |
| 11 | + |
| 12 | +The [PR #2041 - Continuous time updates](https://github.com/InfiniTimeOrg/InfiniTime/pull/2041) provided some changes |
| 13 | +to `DateTime` controller that improves the granularity of the time returned by `DateTime::CurrentDateTime()`. |
| 14 | + |
| 15 | +However, the review showed that `DateTime` cannot be `const` anymore, even when it's only used to get the current time, |
| 16 | +since `DateTime::CurrentDateTime()` changes the internal state of the instance. |
| 17 | + |
| 18 | +We tried to identify alternative implementation that would have maintained the "const correctness" but we eventually |
| 19 | +figured that this would lead to a re-design of `DateTime` which was out of scope of the initial PR (Continuous time |
| 20 | +updates and always on display). |
| 21 | + |
| 22 | +So we decided to merge this PR #2041 and agree to fix/improve `DateTime` later on. |
| 23 | + |
| 24 | +## What needs to be done? |
| 25 | + |
| 26 | +Improve/redesign `DateTime` so that it |
| 27 | + |
| 28 | +* provides a very granular (ideally down to the millisecond) date and time via `CurrentDateTime()`. |
| 29 | +* can be declared/passed as `const` when it's only used to **get** the time. |
| 30 | +* limits the use of mutex as much as possible (an ideal implementation would not use any mutex, but this might not be |
| 31 | + possible). |
| 32 | +* improves the design of `DateTime::Seconds()`, `DateTime::Minutes()`, `DateTime::Hours()`, etc as |
| 33 | + explained [in this comment](https://github.com/InfiniTimeOrg/InfiniTime/pull/2054#pullrequestreview-2037033105). |
| 34 | + |
| 35 | +Once this redesign is implemented, all instances/references to `DateTime` should be reviewed and updated to use `const` |
| 36 | +where appropriate. |
| 37 | + |
| 38 | +Please check the following PR to get more context about this redesign: |
| 39 | + |
| 40 | +* [#2041 - Continuous time updates by @mark9064](https://github.com/InfiniTimeOrg/InfiniTime/pull/2041) |
| 41 | +* [#2054 - Continuous time update - Alternative implementation to #2041 by @JF002](https://github.com/InfiniTimeOrg/InfiniTime/pull/2054) |
0 commit comments