Closed robamu closed 5 months ago
I guess this could make sense, especially if MonotonicTime
becomes its own crate.
Thinking loud, here are some things to consider:
chrono
is still pre-1.0 (hence might see breaking changes) and that this can already done (albeit clumsily) with a one-liner without needing to switch an extra feature, i.e. DateTime::from_timestamp(time.as_unix_secs(37), time.subsec_nanos())
,chrono
crate over the time
crates? In any case, for forward compatibility it would be best to name the methods according to the crate (e.g. as_chrono_date_time()
-- but for the time
crate it's harder to find a name that isn't confusing...),MonotonicTime::as_unix_secs
ignores this issue since there is anyway no consensus on how to define UNIX time stamps prior to 1972, but for UTC this would need to be accounted for,from_chrono_date_time()
, also with a leap seconds argument.Please let me know what you think.
Not strictly related, but looking at the
Edit: OK, it looks more complex than that, apparently some fields use TAI and others use UTC, and to makes things more confusing, the TAI stamps use a 1958 epoch...
Edit2: also, the spacepackets
crate it looks like the CCSDS time code should be TAI-based and not UTC based (based on a quick look at the spec so I could be wrong, but I'd be surprised if they didn't use a monotonic time stamp).spacepackets
crate seems to support conversion between both, but if I'm not mistaken leap seconds are ignored.
chrono
was one of the first major time libraries I found which is why I selected it. To be honest, from having a look at the download counts for both libraries, both seem to be used commonly. Maybe I'll even add time
support to the spacepackets
library. I like the idea with distinct names. Maybe as_timelib_date_time()
would work alternatively?About spacepackets
: All timecodes except CUC are UTC based. I'll have a look at this again..
You indeed found a bug. I ignored the leap second aspect for CUC. I tried a first fix here: https://egit.irs.uni-stuttgart.de/rust/spacepackets/pulls/69 .. time is complicated.
I will be busy today but if you want I can have a look at the spacepacket
's fix tomorrow. Just one think that caught my eyes in the tests' comments: there were actually leaps seconds before 1972, but these were not integer numbers. For the particular case of the 1958 epoch, however, AFAIK the number of leap seconds was indeed 0 (by design). Yes, it's all but simple...
Further rabbit hole digging material: https://github.com/skyfielders/python-skyfield/issues/679
To be honest, I am now also working on refactoring other components of the time library. The UnixTimestamp
is also planned to have nanosecond precision, so other structures like the CUC timestamp can also be more easily created from a UnixTimestamp without losing subsecond precision. I'd also like to bringt the whole API more in line with all the other time APIs available, when it comes to things like naming.
Incidentally, the UnixTime
structure becomes almost identical to the MonotonicTime
except the leap second offset with that change. In any case, I might create conversion methods to/from MonotonicTime for the UnixTime
, especially if MonotonicTime
goes into its own crate. Another idea I had would be to simply move the UnixTime
into that new crate as the MonotonicTime
struct as well, especially because those two are so similar and do not share a lot of dependencies.. But those are just some ideas and would require cooperation.
If you want to have a look at the new CucTime
implementation, I would appreciate it very much. I created a PR for this here: https://github.com/us-irs/spacepackets-rs/pull/5 . I opted for an approach where the CUC time is not leap second aware (except of course methods like from_now
) , and introducing a helper wrapper object which caches the leap second information and offers the API related to UNIX time handling and date time handling.
Concerning the new API: I updated the UNIX datetime API helpers to include support for the time
library now as well, see here and here .
I just found another good reason to include MonotonicTime
in a separate crate: It is actually a type which could be useful on no_std
systems to track elapsed times where std::time::Instant
is not available. For cases like this, I used UnixTimestamp
, which relies on the non-monotonic UNIX time. That would require layering the code so that alloc
and std
dependent features are behind a feature gate. I actually have not had a closer look at alternative options for generic measurement of elapsed times when a standard runtime is not available, but having a concrete type which can also be used on no_std
systems definitely seems like a viable way.
(sorry for the late PR review of spacepackets
, done!).
I agree that the CUC time should not be leap-second aware, especially that the leap offset could become invalid when the CucTime
is modified by arithmetic operations to a date with a different offset.
Yes, making MonotonicTime
a no_std
crate make a lot of sense. I would prefer to keep it really bare-bone though, I think it is always possible to build related structs like UnixTime
or our seqlocked version of MonotonicTime
in separate crate with very little penalty (I think the only penalty is the assert in MonotonicTime::new()
which could be avoided by having access to the private fields, but we can probably live with that).
The first candidate version of a standalone and more generic version of MonotonicTime
with no-std
, serde
and chrono
support is now available at https://github.com/asynchronics/tai-time/.
I will thus close this issue in favor of https://github.com/asynchronics/tai-time/issues/1.
I would be grateful for any feedback before the official release (please use the above new issue).
What do you think about adding a helper API to retrieve a chrono datetime from a
MonotonicTime
? The chrono::DateTime is an extremely convenient time format to work with, similar to the Python datetime API. If you think this might be a worthwhile addition, I can open a PR. I have already added something similar in the spacepackets crate for a unix timestamp wrapper object.It would probably make sense to hide this behind a
chrono
feature gate similar to how it was done forserde
.