Skip to content
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

timeofweek: introduce time of week construction and conversion #180

Merged
merged 22 commits into from
Dec 6, 2022

Conversation

gwbres
Copy link
Collaborator

@gwbres gwbres commented Dec 2, 2022

  • (wk, tow) is usually how an epoch is described by a GNSS receiver, usually in a GNSS timescale.
    wk is a rolling counter, counting the elapsed weeks since the first epoch of that time scale.
    tow is the amount of seconds since Sunday midnight of that week.

tow is usually expressed in [s] or [ms] by receivers, we use [ns] in our case, to maintain crate consistency and never be limited.

Signed-off-by: Guillaume W. Bres guillaume.bressaix@gmail.com

@gwbres
Copy link
Collaborator Author

gwbres commented Dec 2, 2022

Hello @ChristopherRabotin,

I'd like to introduce these two methods to deal with GNSS receivers easily.
from_timeofweek would help parse a timestamp from a GNSS receiver very easily.
to_timeofweek would help perform computations from GNSS receiver data. Our timescale conversion and Leap second management would then become of invaluable help when perfoming such calculations.

The wk week number counter in a given GNSS time scale works fine, it's fairly easy with what we already have.
But the tow would be the number of nanoseconds, between wk considered weekday, and closest Sunday, considering Midnight as the starting point. I have not figured how to determine this duration as of right now.
I'm thinking about introducing a reference "Monday", like first monday encountered, in the TAI timescale

If you look at the test method, we expect 345_618_000_000_000 which is exactly 4 * 3600 * 24 because we're describing a Thursday midnight, and that is the difference with Sunday midnight, and 18_000_000_000 are the leap seconds ever accounted for, on that week :)

@gwbres gwbres force-pushed the timeofweek branch 2 times, most recently from ee8c197 to 0e79e1a Compare December 2, 2022 11:18
* (wk, tow) is usually how an epoch is expressed
by a GNSS receiver, usually in a GNSS timescale.
Wk is a rolling counter, and tow is the amount of seconds since Sunday midnight
of that week.

`tow` is usually expressed in [s] or [ms] by receivers,
we use [ns] here, to maintain crate consistency and never be limited.

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
@ChristopherRabotin
Copy link
Member

Sure, that looks like it could be helpful, so thanks for starting this work.
I'm not sure I understand: is time_of_week the duration since the week started? If so, when does the week start? On midnight between Saturday and Sunday, or on midnight between between Sunday and Monday? I contribute to this PR over the weekend.

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
@gwbres gwbres marked this pull request as ready for review December 2, 2022 15:04
@gwbres
Copy link
Collaborator Author

gwbres commented Dec 2, 2022

Hello Chris, I improved the PR description and tried to improve the autodocs.

I'm not sure I understand: is time_of_week the duration since the week started?

to_timeofweek is probably poorly named.

For example, GNSS vehicles describe an epoch in their timescale, with a rolling week counter ("wk" - maybe we should also improve some variables name), and "tow" is the offset between the current instant and the closest sunday midnight.
See this UTC/GPST calculator

I managed to create the constructor, currently named from_timeofweek (wk, tow) which seems to work fine, and the existing objects fill the requirements.

For the reverse operation (wk , tow_offset) = epoch.to_timeofweek() it is more tricky because Hifitime does not have weekday and week determination methods. But it was surprisingly easier than I thought.

See this new method

pub fn Epoch::weekday_tai(&self) -> u8 {}

which gives an unsigned integer number wrapping at 6, which will help describe a weekday, starting on 0: Monday

Then I introduced

pub fn closest_utc_start_of_week(&self) -> Self 

which for Self, returns the closest Sunday Midnight UTC epoch.
In this one, I struggle a bit to strip the H:M:S offset, and bring it back to midnight.

This is the main information I need to evaluate what I notated tow so far: the offset in the current week.

in

pub fn Epoch::to_timeofweek_utc(&self) -> (u32, u64)

the rolling week counter is easily determined.
Then I determine the closest sunday midnight
I struggle a bit to determine the elapsed duration since Self and that day.
I don't understand why, in our implementation we have 37 leap seconds, while the calculator above says it should be 18

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
@ChristopherRabotin
Copy link
Member

Understood, thanks Guillaume! I'll start the review soon and hope to be able to help on the bug this weekend. This looks very helpful even to other issues like #183 .

@gwbres
Copy link
Collaborator Author

gwbres commented Dec 2, 2022

Understood, thanks Guillaume! I'll start the review soon and hope to be able to help on the bug this weekend. This looks very helpful even to other issues like #183 .

Yep! indeed. I decided not to introduce a new Unit or timeserie, to keep things simple. But a Week granularity would help.
Anyway, we can even split this PR, get rid of the GNSS stuff and keep the WeekDay counter method, which works great and will be useful in itself

Copy link
Member

@ChristopherRabotin ChristopherRabotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments for when I can contribute back to this branch. This is great works, thanks Guillaume!

src/epoch.rs Outdated Show resolved Hide resolved
src/epoch.rs Outdated Show resolved Hide resolved
src/epoch.rs Outdated Show resolved Hide resolved
src/epoch.rs Outdated Show resolved Hide resolved
src/epoch.rs Outdated Show resolved Hide resolved
src/epoch.rs Outdated Show resolved Hide resolved
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
@gwbres
Copy link
Collaborator Author

gwbres commented Dec 2, 2022

I don't understand why, [in one of the new tests where I compare some new methods to the previously mentioned GPST/UTC calculator] we end up on our side with 37 leap seconds, while the calculator says it should be 18

probably because leap seconds are accounted not only once but twice, due to a timescale mix up somewhere

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
This is good!

Signed-off-by: Christopher Rabotin <christopher.rabotin@gmail.com>
@ChristopherRabotin
Copy link
Member

Okay, I've started the review, and I'll push to your branch in a bit. This is quite good work, thanks!

Also add `with_hms` and `with_hms_strict` to set the time of a given Epoch

Closes nyx-space#183

Signed-off-by: Christopher Rabotin <christopher.rabotin@gmail.com>
Needs more testing for sure

Signed-off-by: Christopher Rabotin <christopher.rabotin@gmail.com>
@ChristopherRabotin
Copy link
Member

Okay, I fixed the time of week calculation. I think it needs more testing, most notably reciprocity testing. Otherwise, this looks good!

@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2022

Codecov Report

Attention: Patch coverage is 91.26214% with 18 lines in your changes missing coverage. Please review.

Project coverage is 80.63%. Comparing base (2e3b8ad) to head (704fbff).
Report is 377 commits behind head on master.

Files with missing lines Patch % Lines
src/weekday.rs 83.54% 13 Missing ⚠️
src/epoch.rs 97.45% 3 Missing ⚠️
src/timescale.rs 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   79.32%   80.63%   +1.30%     
==========================================
  Files           9       10       +1     
  Lines        2588     2794     +206     
==========================================
+ Hits         2053     2253     +200     
- Misses        535      541       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Christopher Rabotin <christopher.rabotin@gmail.com>
@ChristopherRabotin
Copy link
Member

ChristopherRabotin commented Dec 3, 2022

I don't think from_time_of_week_utc can work because to_time_of_week lost the information on the year, month, and the day. I'll see if I can think of a signature change and let you choose whether or not that is acceptable for your use-case.

Edit: Actually, I guess you made it work when it's in the same time scale. I've added a test that does not work, but maybe it's because the test I added is incorrect.

Found bug in time_of_week initialization?

Signed-off-by: Christopher Rabotin <christopher.rabotin@gmail.com>
@ChristopherRabotin ChristopherRabotin linked an issue Dec 3, 2022 that may be closed by this pull request
This is the first step in nyx-space#182

Signed-off-by: Christopher Rabotin <christopher.rabotin@gmail.com>
tests/weekday.rs Show resolved Hide resolved
src/epoch.rs Show resolved Hide resolved
src/weekday.rs Show resolved Hide resolved
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
src/epoch.rs Show resolved Hide resolved
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
src/timescale.rs Outdated Show resolved Hide resolved
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
tests/epoch.rs Outdated Show resolved Hide resolved
@gwbres
Copy link
Collaborator Author

gwbres commented Dec 5, 2022

Hello Chris,
in case you're interested in, this is my use case.

To determine the position of a satellite vehicle in the sky, you need to solve Kepler's equations.
Time at epoch k (t(k)) is one out of several parameters.
It is expressed in the timescale (so referenced to GPST_REF_EPOCH) and the vehicles stream the number of weeks since that day, and the amount of nanosecond within that week

@ChristopherRabotin
Copy link
Member

I cannot seem to figure out where this time of week bug with UTC time scales comes from. I'm pretty sure that something is wrong in the calculation of time_of_week due to some rounding somewhere, but I cannot understand where exactly. I'll flag this as a bug for now.

Signed-off-by: Christopher Rabotin <christopher.rabotin@gmail.com>
@ChristopherRabotin ChristopherRabotin merged commit aa1ea21 into nyx-space:master Dec 6, 2022
@ChristopherRabotin
Copy link
Member

I ended up implementing a hot fix. Sorry for the delay in getting this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weekday based calculations
3 participants