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

Cannot access GPSTime inside BaseTime #1331

Closed
marcomaida94 opened this issue May 25, 2023 · 4 comments · Fixed by #1332
Closed

Cannot access GPSTime inside BaseTime #1331

marcomaida94 opened this issue May 25, 2023 · 4 comments · Fixed by #1332

Comments

@marcomaida94
Copy link

marcomaida94 commented May 25, 2023

Hello,
I am trying to extract a timestamp from each SBPMessage that contains one (Rust).

I found the function gps_time() by enabling the feature swiftnav:

    // frame: &Sbp
    if let Some(Ok(msg_time)) = sbp::messages::SbpMessage::gps_time(frame) {
        let _tow = match msg_time {
            MessageTime::Rover(RoverTime::Tow(rover_tow)) => rover_tow,
            MessageTime::Rover(RoverTime::GpsTime(gps_time)) => gps_time.tow(),
            MessageTime::Base(BaseTime(gps_time)) => gps_time.tow()
        };
    }

However BaseTime is a struct that contains gps_time as a private field:
https://github.com/swift-nav/libsbp/blob/d122b3b82e048e8a313967332ad97ebf05cc80c3/rust/sbp/src/time.rs#LL92C1-L92C1
So I can't access it.

Is this a bug or am I not supposed to do things this way?

Thanks,
Marco

@notoriaga
Copy link
Contributor

Not sure what exactly your use-case is but there is a function that converts Iterator<Item = Sbp> to Iterator<Item = Sbp, Option<Result<GpsTime, GpsTimeError>>> (also works with Iterator<Item = Result<Sbp>>) -

fn with_rover_time<T>(self) -> swiftnav_impl::RoverTimeIter<Self>

It's not showing up in the docs because it requires the swiftnav feature (which enables some integrations with https://github.com/swift-nav/swiftnav-rs) and I guess we aren't building the docs with all features enabled.

It might not be want you want because it excludes BaseTime messages, but it takes care of tracking the week number for you which is nice. Either way we have a fix for this on the way 😄

@marcomaida94
Copy link
Author

Thank you!

I will patch locally waiting for the fix.

pcrumley added a commit that referenced this issue May 26, 2023
# Description

@swift-nav/devinfra

This PR converts the BaseTime tuple struct into an enum with one variant
and makes it non_exhaustive. This enables users to access the time
inside and makes it similar to how RoverTime can be used.
 
# API compatibility

Does this change introduce a API compatibility risk?

In general you cannot convert an struct to an enum without it being a
breaking change. However, because the enum was a tuple struct with
private fields it's constructor was also private, and you cannot create
it even with the dot syntax.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0369878bb675f5734febf4ad1eebf327

So that means that the chances of this being a breaking change is very
small and I feel comfortable marking this as a minor change.

If the above is "Yes", please detail the compatibility (or migration)
plan:

N/A

# JIRA Reference

would resolve: #1331

---------

Co-authored-by: Jason Mobarak <jason@swift-nav.com>
swiftnav-svc-jenkins added a commit to swift-nav/libsettings that referenced this issue May 26, 2023
# Description

@swift-nav/devinfra

This PR converts the BaseTime tuple struct into an enum with one variant
and makes it non_exhaustive. This enables users to access the time
inside and makes it similar to how RoverTime can be used.
 
# API compatibility

Does this change introduce a API compatibility risk?

In general you cannot convert an struct to an enum without it being a
breaking change. However, because the enum was a tuple struct with
private fields it's constructor was also private, and you cannot create
it even with the dot syntax.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0369878bb675f5734febf4ad1eebf327

So that means that the chances of this being a breaking change is very
small and I feel comfortable marking this as a minor change.

If the above is "Yes", please detail the compatibility (or migration)
plan:

N/A

# JIRA Reference

would resolve: swift-nav/libsbp#1331

---------

Co-authored-by: Jason Mobarak <jason@swift-nav.com>

Triggered-By:	libsbp	25e15c6c7d63c6cdd73cf38f6d6bffd2fe80f84e
Upstream-PR:	http://github.com/swift-nav/libsbp/pull/1332
@silverjam
Copy link
Contributor

Release v4.14.1 with this fix has been cut (and now available on crates.io).

@marcomaida94
Copy link
Author

Thank you so much!

silverjam pushed a commit to swift-nav/libsettings that referenced this issue Jun 7, 2023
# Description

@swift-nav/devinfra

This PR converts the BaseTime tuple struct into an enum with one variant
and makes it non_exhaustive. This enables users to access the time
inside and makes it similar to how RoverTime can be used.
 
# API compatibility

Does this change introduce a API compatibility risk?

In general you cannot convert an struct to an enum without it being a
breaking change. However, because the enum was a tuple struct with
private fields it's constructor was also private, and you cannot create
it even with the dot syntax.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0369878bb675f5734febf4ad1eebf327

So that means that the chances of this being a breaking change is very
small and I feel comfortable marking this as a minor change.

If the above is "Yes", please detail the compatibility (or migration)
plan:

N/A

# JIRA Reference

would resolve: swift-nav/libsbp#1331

---------

Co-authored-by: Jason Mobarak <jason@swift-nav.com>

Triggered-By:	libsbp	25e15c6c7d63c6cdd73cf38f6d6bffd2fe80f84e
Upstream-PR:	http://github.com/swift-nav/libsbp/pull/1332
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 a pull request may close this issue.

3 participants