add gps_time method #967

notoriaga · 2021-04-28T18:03:23Z

Adds

fn gps_time(&self) -> Option<Result<swiftnav_rs::time::GpsTime, crate::GpsTimeError>>;

to the SBPMessage trait behind a swiftnav-rs feature flag. Also reexports GpsTime as sbp::messages::GpsTime

I went with Option<Result<swiftnav_rs::time::GpsTime, crate::GpsTimeError>>, but maybe the Result/Option should be swapped?
Maybe should go with a default impl of gps_time that returns None instead of actually implementing a bunch of functions that return None. would make it easier to search for the impls and validate they are correct (also tests would be nice)
This crate already has a GpsTime struct which might be a little confusing -

libsbp/rust/sbp/src/messages/gnss.rs

Line 92 in f2a1804

pub struct GPSTime {

notoriaga · 2021-04-28T18:33:54Z

generator/sbpg/targets/rust.py

+def gps_time(msg, all_messages):
+  def time_aware_header(type_id):
+    for m in all_messages:
+      if m.identifier == type_id:
+        return any([f.identifier == "t" for f in m.fields])
+    return False
+
+  has_tow = False
+  has_wn = False
+
+  for f in msg.fields:
+    if f.identifier == "header" and time_aware_header(f.type_id):
+      return GPS_TIME_HEADER
+    elif f.identifier == "tow":
+      has_tow = True
+    elif f.identifier == "wn":
+      has_wn = True
+
+  if has_tow and has_wn:
+    return GPS_TIME
+  elif has_tow:
+    return GPS_TIME_ONLY_TOW
+  else:
+    return 'None'


Not sure if this is the best way to determine which messages are time-aware/how that time is stored but it seems to work so far

I think this is a good starting point at the very least. I'd have to peruse all the message definitions to figure out if there's other ways we include a GPS time in messages, but this should cover the vast majority if not all of them.

I think it should be noted that the time values can have different meanings depending on the message type they're in. For instance the time value in MSG_OBS is the time that the observations were made, but in MSG_EPHEMERIS_GPS (or any ephemeris message) it's a center point in the period of time the data is valid so it is typically several minutes to a couple of hours in the future from when the message was actually generated. We can discuss offline if this is a problem for what you're working on.

For this use case the "time of event" or "time of occurrence" is the meaning of .gps_time() -- that is, we want to define a linear time ordering of the messages for use in things like the "sbp chopper" and the interleaver.

I talked with @notoriaga about this briefly and I agree that "t"/"wn"/"tow" covers most of the use case correctly, but we should do an audit of everything that gets grabbed by this and then maybe introduce a "time_ordering" property that we can expose to the yaml spec for this purpose.

silverjam · 2021-04-28T18:45:49Z

I went with Option<Result<swiftnav_rs::time::GpsTime, crate::GpsTimeError>>, but maybe the Result/Option should be swapped?

Should definitely be swapped, without swapping it makes it kind of hard to use with ?, right?

Maybe should go with a default impl of gps_time that returns None instead of actually implementing a bunch of functions that return None. would make it easier to search for the impls and validate they are correct (also tests would be nice)

👍

This crate already has a GpsTime struct which might be a little confusing -

libsbp/rust/sbp/src/messages/gnss.rs

Line 92 in f2a1804

pub struct GPSTime {

Maybe MessageGpsTime or MsgGpsTime could help differentiate?

notoriaga · 2021-04-28T18:57:19Z

Should definitely be swapped, without swapping it makes it kind of hard to use with ?, right?

It depends on if your function returns Option or Result. Maybe a custom enum with three variants is best, with two helpers to convert to option or result

jbangelo

I went with Option<Result<swiftnav_rs::time::GpsTime, crate::GpsTimeError>>, but maybe the Result/Option should be swapped?

I'd vote to keep the Option on the outside, it seems more logical to me. Getting the time is the first operation, and the not having a time isn't an error per se. The second operation is converting the message's time representation into a common format which is a fallible operation. I think the ? operator also works with Option, assuming that the function is returning an Option as well.

rust/sbp/Cargo.toml

john-michaelburke · 2021-04-28T19:22:21Z

LGTM thank you for doing this!!

silverjam · 2021-04-28T19:44:31Z

I went with Option<Result<swiftnav_rs::time::GpsTime, crate::GpsTimeError>>, but maybe the Result/Option should be swapped?

I'd vote to keep the Option on the outside, it seems more logical to me. Getting the time is the first operation, and the not having a time isn't an error per se. The second operation is converting the message's time representation into a common format which is a fallible operation. I think the ? operator also works with Option, assuming that the function is returning an Option as well.

👍

silverjam · 2021-04-28T21:06:15Z

generator/sbpg/targets/rust.py

 MESSAGES_TEMPLATE_NAME = "sbp_messages_template.rs"
 MESSAGES_MOD_TEMPLATE_NAME = "sbp_messages_mod.rs"

+GPS_TIME = """\


Mentioned offline, but if the type uses GPSTime, then we potentially have the "nanosecond residual" available... but digging in to this deeper I don't think we need to worry about this (maybe?). The bare GPSTime object is only used in MSG_TRACKING_STATE_DETAILED_DEP_A, MSG_SPECAN, and (the only one that might matter) is MSG_OBS.

It's not really a technical problem since the two structs with the same name are defined in different crates, so they're in different name spaces. It's mostly a potential cause for confusion among developers, right? I'd like to think that's less likely to happen since the libsbp GPSTime struct typically won't be instantiated directly. I imagine it would be accessed as a member of a larger SBP message most of the time. Maybe some clarification in the docs would help?

It's not really a technical problem since the two structs with the same name are defined in different crates, so they're in different name spaces. It's mostly a potential cause for confusion among developers, right? I'd like to think that's less likely to happen since the libsbp GPSTime struct typically won't be instantiated directly. I imagine it would be accessed as a member of a larger SBP message most of the time. Maybe some clarification in the docs would help?

Yeah, that's probably sufficient, I was also thinking a wrapper type might be appropriate in libsbp to disambiguate MessageTime(GpsTime) or something similar.

rust/sbp/src/lib.rs

generator/sbpg/targets/rust.py

jayvdb · 2021-04-29T05:19:23Z

generator/sbpg/targets/resources/sbp_messages_template.rs


 //! (((description | replace("\n", "\n//! "))))

+#[allow(unused_imports)]


we could remove the need for this, and other lint exclusion rules with

diff --git a/generator/sbpg/targets/resources/sbp_messages_template.rs b/generator/sbpg/targets/resources/sbp_messages_template.rs index e2f85da1..ffd7729c 100644 --- a/generator/sbpg/targets/resources/sbp_messages_template.rs +++ b/generator/sbpg/targets/resources/sbp_messages_template.rs @@ -15,16 +15,10 @@ //! (((description | replace("\n", "\n//! ")))) -#[allow(unused_imports)] -use std::convert::TryFrom; - -#[allow(unused_imports)] -use byteorder::{LittleEndian,ReadBytesExt}; - #[allow(unused_imports)] use crate::SbpString; #[allow(unused_imports)] -use crate::serialize::SbpSerialize; +use byteorder::ReadBytesExt; ((*- for i in includes *)) use super::(((i)))::*; diff --git a/generator/sbpg/targets/rust.py b/generator/sbpg/targets/rust.py index cf1e0268..adedd574 100644 --- a/generator/sbpg/targets/rust.py +++ b/generator/sbpg/targets/rust.py @@ -46,6 +46,8 @@ Some(swiftnav_rs::time::GpsTime::new(0, tow_s).map_err(Into::into)) GPS_TIME_IMPL = """\ #[cfg(feature = "swiftnav-rs")] fn gps_time(&self) -> Option<std::result::Result<swiftnav_rs::time::GpsTime, crate::GpsTimeError>> {{ + #[allow(unused_imports)] + use std::convert::TryFrom; {} }} """ @@ -110,7 +112,7 @@ def parse_type(field): return '_buf.read_i8()' elif field.type_id in TYPE_MAP.keys(): # Primitive java types have extractor methods in SBPMessage.Parser - return '_buf.read_%s::<LittleEndian>()' % TYPE_MAP[field.type_id] + return '_buf.read_%s::<byteorder::LittleEndian>()' % TYPE_MAP[field.type_id] if field.type_id == 'array': # Call function to build array t = field.options['fill'].value

clippy doesnt seem to mind; it does however have another 150 warnings :/

notoriaga · 2021-04-30T21:19:03Z

rust/sbp/src/messages/imu.rs

+    fn gps_time(
+        &self,
+    ) -> Option<std::result::Result<crate::time::MessageTime, crate::time::GpsTimeError>> {
+        let tow_s = (self.tow as f64) / 1000.0;


Is is correct to assume that the tow field always is in milliseconds?

@jbangelo @denniszollo are there some messages that report tow in seconds, not ms?

@notoriaga there is a "units" field in the yaml spec that we could potentially inspect here.

I added an assert that the units are ms. Only one wasn't IMU_MSG_RAW. For now just skipping that msg, it seems like it might not actually be representing 'rover' time - but rather 'Time since reference epoch in milliseconds' which seems like not what we want

rust/sbp/src/sbp_tools.rs

john-michaelburke

This looks great!

generator/sbpg/targets/resources/sbp-cargo.toml

generator/sbpg/targets/rust.py

notoriaga · 2021-05-10T19:32:00Z

I pulled together a list of all the rover-time messages

MsgAgeCorrections
MsgAngularRate
MsgBaselineECEF
MsgBaselineECEFDepA
MsgBaselineHeading
MsgBaselineHeadingDepA
MsgBaselineNED
MsgBaselineNEDDepA
MsgDops
MsgDopsDepA
MsgExtEvent
MsgGPSTime
MsgGPSTimeDepA
MsgGPSTimeGnss
MsgInsUpdates
MsgMagRaw
MsgObsDepA
MsgObsDepB
MsgObsDepC
MsgOdometry
MsgOrientEuler
MsgOrientQuat
MsgPosECEF
MsgPosECEFCov
MsgPosECEFCovGnss
MsgPosECEFDepA
MsgPosECEFGnss
MsgPosLLH
MsgPosLLHCov
MsgPosLLHCovGnss
MsgPosLLHDepA
MsgPosLLHGnss
MsgProtectionLevel
MsgProtectionLevelDepA
MsgSbasRaw
MsgSolnMeta
MsgUtcTime
MsgUtcTimeGnss
MsgVelBody
MsgVelECEF
MsgVelECEFCov
MsgVelECEFCovGnss
MsgVelECEFDepA
MsgVelECEFGnss
MsgVelNED
MsgVelNEDCov
MsgVelNEDCovGnss
MsgVelNEDDepA
MsgVelNEDGnss

would be nice to get some eyes on this. One thing I'm wondering is that we have MSG_OBS as base time, but we have MSG_OBS_DEP_{A,B,C} here - is that correct?

silverjam

Adding this to note that we decided to add something custom to handle IMU_RAW and WHEELTICKS messages from thachoppa: https://github.com/swift-nav/thachoppa/blob/master/src/lib.rs#L148

notoriaga added 3 commits April 28, 2021 11:03

add gps_time method

202fdb1

typo

1a526cc

export GpsTime

b84b1bf

notoriaga requested review from jayvdb, jbangelo, john-michaelburke and silverjam April 28, 2021 18:27

notoriaga commented Apr 28, 2021

View reviewed changes

jbangelo reviewed Apr 28, 2021

View reviewed changes

rust/sbp/Cargo.toml Show resolved Hide resolved

notoriaga added 2 commits April 28, 2021 12:50

use default impl

413b506

don't impl msg_time for unknown

4e76b7c

silverjam reviewed Apr 28, 2021

View reviewed changes

Add swiftnav-rs conversions

a077d39

notoriaga force-pushed the steve/time branch from 85a6ddb to a077d39 Compare April 28, 2021 23:56

notoriaga commented Apr 28, 2021

View reviewed changes

rust/sbp/src/lib.rs Show resolved Hide resolved

jayvdb mentioned this pull request Apr 29, 2021

[CPP-80]Add realtime delay when reading an SBPfile and pip cache to GA. swift-nav/swift-toolbox#43

Merged

silverjam reviewed Apr 29, 2021

View reviewed changes

generator/sbpg/targets/rust.py Outdated Show resolved Hide resolved

jayvdb reviewed Apr 29, 2021

View reviewed changes

notoriaga added 5 commits April 29, 2021 10:34

use wip swiftnav-rs branch

8e68c6c

Merge branch 'master' into steve/time

f317235

generate rust

de03bd3

impl from infallible

983e98b

Add MessageTime

6460ba8

notoriaga commented Apr 30, 2021

View reviewed changes

notoriaga marked this pull request as draft April 30, 2021 21:35

feature checks

8c61838

notoriaga added 5 commits May 7, 2021 11:59

Don't ignore gpstime errors

cd6d047

Replace bool with ControlFlow enum

74aad19

Set log level

0ccce8d

Assert tow in ms. Skip MSG_IMU_RAW

b474601

remove pub use

53d35a0

john-michaelburke approved these changes May 7, 2021

View reviewed changes

silverjam approved these changes May 8, 2021

View reviewed changes

jayvdb reviewed May 8, 2021

View reviewed changes

generator/sbpg/targets/resources/sbp-cargo.toml Outdated Show resolved Hide resolved

jayvdb reviewed May 8, 2021

View reviewed changes

generator/sbpg/targets/rust.py Outdated Show resolved Hide resolved

notoriaga added 2 commits May 10, 2021 12:36

Use swiftnav-rs v0.5.0

6a553f5

use .strip()

f190f4d

notoriaga marked this pull request as ready for review May 10, 2021 19:43

notoriaga added 5 commits May 10, 2021 13:01

Typo

2088261

rebuild

b5708a5

Merge branch 'master' into steve/time

c22311f

gen rust

dbd8dc6

.strip() (again)

98058b3

silverjam suggested changes May 11, 2021

View reviewed changes

Support MsgImuRaw/MsgWheeltick

5cf3895

silverjam approved these changes May 11, 2021

View reviewed changes

notoriaga merged commit 40bd485 into master May 11, 2021

notoriaga deleted the steve/time branch May 11, 2021 15:46


		//! (((description \| replace("\n", "\n//! "))))

		#[allow(unused_imports)]

add gps_time method #967

add gps_time method #967

Uh oh!

Conversation

notoriaga commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

silverjam commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notoriaga commented Apr 28, 2021

Uh oh!

jbangelo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

john-michaelburke commented Apr 28, 2021

Uh oh!

silverjam commented Apr 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverjam Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jayvdb Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

john-michaelburke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

notoriaga commented May 10, 2021

Uh oh!

silverjam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

notoriaga commented Apr 28, 2021 •

edited

Loading

silverjam commented Apr 28, 2021 •

edited

Loading

silverjam Apr 28, 2021 •

edited

Loading

jayvdb Apr 29, 2021 •

edited

Loading

silverjam left a comment •

edited

Loading