Rewrite time module in pure Rust #125

jbangelo · 2025-08-26T04:05:01Z

The next chunk of changes from #122.

This re-implements the time related functionality in native Rust. This one is a bit larger, mostly from the fact that the conversions in the C implementation are very inter-dependent and so several new bits of functionality were pulled in to make the Rust implementation mirror the C implementation. There's a few bits of functionality thrown out because they caused some significant unsoundness, but I'm sure there is still several bits of unsoundness in this translation (e.g. several panics/asserts are still present). I'd prefer to keep this PR as close to a straight translation as possible, and re-consider the soundness concerns in a future PR.

What's been changed

I don't think any implementations are still present but meaningfully modified

What's been removed

GloTime - GLONASS time is particularly complicated and rarely used
GpsTime::to_glo() and GpsTime::to_glo_hardcoded() - See above
impl Sub<GpsTime> for GpsTime - It turns out a std::time::Duration can't hold a negative value. This implementation would occasionally panic because of this. The chrono crate handles this better, maybe we can move towards their approach in a later PR?
UtcParams::decode() - Decoding low-level messages seem unlikely to be needed
impl Default for UtcParams - Defaulting this data can cause unexpected errors
impl Default for UtcTime - Defaulting this type can cause unexpected errors

What's been added

The time::consts module
GpsTime::from_date() and GpsTime::from_date_hardcoded()
GpsTime::to_mjd() and GpsTime::to_mjd_hardcoded()
GpsTime::to_date() and GpsTime::to_date_hardcoded()
UtcTime::to_date()
MJD::to_gps() and MJD::to_gps_hardcoded()
MJD::to_date()

JADC362

LGTM! As always, I just left minor comments regarding documentation. I do agree we should tackle the panics on a future PR, just to improve error handling.

swiftnav/src/time/gnss.rs

JADC362 · 2025-08-27T17:37:23Z

swiftnav/src/time/mjd.rs

+            - 7 * (year as i64 + (month as i64 + 9) / 12) / 4
+            - 3 * ((year as i64 + (month as i64 - 9) / 7) / 100 + 1) / 4


May I ask, why don't use some of the constants you defined in the consts module?

Only a few of the values are present in the constants module, mostly YEAR_MONTHS == 12 and WEEK_DAYS == 7. I think the 12 in this formula likely does represent months, but I'm not entirely sure that the 7 actually represents weeks. And 367 is likely distantly related to the number of days in a year. The original source (http://www.leapsecond.com/tools/gpsdate.c) doesn't really shed more light on the meaning.

I've come across these sorts of formula before, they are always just an overly clever formula someone with a lot of time worked out and the constants have no real meaning and can be treated as magic numbers, so I'd propose we keep them as magic numbers here and just point people to the original source in the doc comment.

JADC362 · 2025-08-27T17:41:41Z

swiftnav/src/time/utc.rs

+        dt_ls: i8,
+        dt_lsf: i8,
+    ) -> UtcParams {
+        assert!(tot.is_valid() && t_lse.is_valid());


We could tackle this on a later PR as you mentioned, but would prefer here to create an UtcError enum similar to the GPS time one, and return that instead of panicking.

JADC362 · 2025-08-27T17:43:06Z

swiftnav/src/time/utc.rs

+///
+/// Note: This implementation does not aim to be able to represent arbitrary dates and times.
+/// It is only meant to represent dates and times over the period that GNSS systems have been
+/// around. Specifically it shouldn't be relied on for dates significantly before January 6th 1980,


Didn't saw if we are actually panicking or returning an error if the user tries to use this struct for dates before 1980

I don't think we panic, but I'm not sure if it makes sense to do that or double check it. This is meant to indicate that we don't really test that everything continues to work for dates before the start of GPS time. The UTC specific functionality likely would, but the conversion to/from GPS time likely will break. That's where we should really should put sanity checks and error types. I'll update this to make sure that we largely only test on dates back this far, and to use the chrono crate if you need correct handling of dates further back in time.

swiftnav/src/time/utc.rs

Co-authored-by: Alejandro Duarte <[email protected]>

swiftnav/src/time/gnss.rs

tzilist · 2025-08-29T18:54:08Z

swiftnav/src/time/gnss.rs

+        }
+    }
+    /// Makes a new GPS time object without checking the validity of the given values.
+    pub(crate) const fn new_unchecked(wn: i16, tow: f64) -> GpsTime {


We should make this as unsafe. It's a pretty common convention in rust to do this to ensure that the user is super careful (even though it has nothing to do with borrow checking).

An example:
https://doc.rust-lang.org/src/core/pin.rs.html#1349

I know this is something that people are moving towards, but honestly I am not a fan. For the record that pin new_unchecked you linked to is correctly marked as unsafe as it can lead to UB if the pin api contract is not upheld. I am kind of skeptical setting a negative week for i16, or NaN for tow can lead to undefined behavior and memory safety by itself... rather it leads to logic errors, but i leave it to @jbangelo judgement there.

That said, there is precedent in the rust community for using unsafe for things that you know can lead to logic errors: e.g. https://docs.rs/sguaba/latest/sguaba/#use-of-unsafe

up to you

As I mentioned in the PR description I'd like to limit the API and behavior changes made in this PR, but I do want to go through this module in particular with a fine tooth comb and sort out potential numerical issues with the arithmetic and make a more idiomatic API. I'm open to marking this as unsafe in a follow up PR if we want to adopt that idiom. I do agree with you @pcrumley that it's an abuse of the keyword, which also makes me skeptical but I've not done much research on the subject to see the arguments for it.

swiftnav/src/time/gnss.rs

tzilist · 2025-08-29T19:15:07Z

swiftnav/src/time/gnss.rs

+        assert!(self.is_valid());
+        assert!(self >= GAL_TIME_START);


Same question here for most of this, I would expect returning an error will be better. We can let the caller decide if they would like to panic or not rather than forcing them to do so here.

The only time i'd encourage panics is if there is truly an unrecoverable error that occurs.

tzilist · 2025-08-29T19:24:09Z

swiftnav/src/time/mod.rs

+            let diff = test_case.diff(&round_trip).abs();
+            assert!(
+                diff < TOW_TOL,
+                "gps2mjd2gps failure. original: {:?}, round trip: {:?}, diff: {}, TOW_TOL: {}",


Can we use rust's face new inline feature for these asserts?

Suggested change

"gps2mjd2gps failure. original: {:?}, round trip: {:?}, diff: {}, TOW_TOL: {}",

assert!(

diff < TOW_TOL,

"gps2mjd2gps failure. original: {test_case:?}, round trip: {round_trip:?}, diff: {diff}, TOW_TOL: {TOW_TOL}"

);

That's a feature new in 1.89, right?

1.58, except for old editions in the panic! macro. https://blog.rust-lang.org/2022/01/13/Rust-1.58.0/

Oh yeah, just realized that this crate is on edition 2018. I'm going to leave this as is for the moment, and when we bump the edition (which we should) I think these should get caught by a clippy lint (especially if we enable pedantic)

swiftnav/src/time/mjd.rs

swiftnav/src/ionosphere.rs

jbangelo force-pushed the jbangelo/riir-time branch from d1b6701 to e1acb6f Compare August 26, 2025 04:06

Rewrite time module in pure Rust

3caed50

jbangelo force-pushed the jbangelo/riir-time branch from e1acb6f to 3caed50 Compare August 26, 2025 04:44

jbangelo requested review from tzilist, JADC362 and john-michaelburke August 26, 2025 04:49

jbangelo marked this pull request as ready for review August 26, 2025 04:49

jbangelo requested a review from a team as a code owner August 26, 2025 04:49

JADC362 approved these changes Aug 27, 2025

View reviewed changes

jbangelo and others added 4 commits August 27, 2025 11:59

Update swiftnav/src/time/gnss.rs

8a69907

Co-authored-by: Alejandro Duarte <[email protected]>

Update swiftnav/src/time/gnss.rs

bbdd121

Co-authored-by: Alejandro Duarte <[email protected]>

Apply suggestions from code review

cbd0db2

Co-authored-by: Alejandro Duarte <[email protected]>

PR feedback

2fb5936

tzilist requested changes Aug 29, 2025

View reviewed changes

pcrumley approved these changes Aug 29, 2025

View reviewed changes

swiftnav/src/time/mjd.rs Outdated Show resolved Hide resolved

swiftnav/src/ionosphere.rs Outdated Show resolved Hide resolved

jbangelo added 2 commits August 29, 2025 14:49

PR feedback

f322824

Make use of 0.0 consistent

e254ac1

jbangelo requested a review from tzilist August 29, 2025 21:54

jbangelo merged commit c02e8c6 into master Aug 30, 2025
6 checks passed

jbangelo deleted the jbangelo/riir-time branch August 30, 2025 00:20

		- 7 * (year as i64 + (month as i64 + 9) / 12) / 4
		- 3 * ((year as i64 + (month as i64 - 9) / 7) / 100 + 1) / 4

Rewrite time module in pure Rust #125

Rewrite time module in pure Rust #125

Uh oh!

Conversation

jbangelo commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's been changed

What's been removed

What's been added

Uh oh!

JADC362 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbangelo commented Aug 26, 2025 •

edited

Loading