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

Implement MISP Time System instant #198

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

bradh
Copy link
Contributor

@bradh bradh commented Mar 5, 2022

This implements the MISP time system as described in the MISB Motion Imagery Handbook (downloadable from https://nsgreg.nga.mil/doc/view?i=5306 - sorry, direct links frequently broken). The implementation follows the TaiInstant implementation, and makes use of the TAI conversion routines where practical.

Resolves #194

The PR includes unit tests for all added code.

There is a change to the UtcRules implementation that adds an abstract method. That won't be source compatible for anyone implementing their own rules (i.e. its arguably an API break). It is not strictly necessary to do so, and although I think the implementation makes more sense in the SystemUtcRules, I could see a case for not doing it that way. I'll update the PR to avoid that if required.

@bradh
Copy link
Contributor Author

bradh commented May 9, 2022

Any thoughts on this PR? Changes or general suggestions?

Copy link
Member

@jodastephen jodastephen left a comment

Choose a reason for hiding this comment

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

Thanks, I think this is a good starting place, but lets avoid creating an n x n combination in UtcRules.

src/main/java/org/threeten/extra/scale/UtcRules.java Outdated Show resolved Hide resolved
src/main/java/org/threeten/extra/scale/MispInstant.java Outdated Show resolved Hide resolved
src/main/java/org/threeten/extra/scale/MispInstant.java Outdated Show resolved Hide resolved
src/main/java/org/threeten/extra/scale/MispInstant.java Outdated Show resolved Hide resolved
@@ -226,6 +226,70 @@ public TaiInstant convertToTai(UtcInstant utcInstant) {
*/
public abstract UtcInstant convertToUtc(TaiInstant taiInstant);

/**
Copy link
Member

Choose a reason for hiding this comment

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

Having thought a bit, my view is that we should make UTC and TAI the primary time scales, and others like MISP secondary. This avoids the risk that UtcRules ends up with conversion methods for all possible time scales (n x n combination problem).

As such, please move these methods to MispInstant (as necessary). Conversion from MISP to UTC will need to go via TaiInstant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still to be addressed, but I think I understand what is needed.

// 59643L is the MJD for 2022-03-05
// 40587L is the MJD for 1970-01-01
assertEquals((59643L - 40587L) * 24 * 60 * 60 + 37, misp.getMispSeconds());
assertEquals(0, misp.getNano());
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing the 8.000082 anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 8.000082 is in the instant that we are converting. I can rework the example (or the comment) if it isn't clear.

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

Successfully merging this pull request may close these issues.

Support for Motion Imagery Standards Profile (MISP) Time System
2 participants