-
Notifications
You must be signed in to change notification settings - Fork 0
Rewrite signal module in Rust #124
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
Conversation
da5632c
to
ceeb98f
Compare
ceeb98f
to
1a3e6a8
Compare
1a3e6a8
to
636a4ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a first initial review, will continue later
pub const NUM_SIGNALS_QZS_L5: u16 = NUM_SATS_QZS; | ||
|
||
/// Total number of signals in the GPS constellation. | ||
pub const NUM_SIGNALS_GPS: u16 = 2 * NUM_SIGNALS_GPS_L1CA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps should we set the 2 and 3 not a magical numbers but with a formal constant name. Could be:
NUM_COMPONENTS_IQ_SIGNAL = 2;
NUM_COMPONENTS_IQX_SIGNAL = 3;
or something like that. It will make it clear the number of components each signal have by setting a name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that'll really make it any more clear, especially when it's not going to be uniform and not all of the signal naming schemes use I/Q/X
/// Frequency range between two adjacent GLO channel in Hz for L1 band | ||
pub const GLO_L1_DELTA_HZ: f64 = 5.625e5; | ||
/// Frequency range between two adjacent GLO channel in Hz for L2 band | ||
pub const GLO_L2_DELTA_HZ: f64 = 4.375e5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've always thought why we don't add somewhere in the documentation of these kind of files from where do we get these constants. It doesn't have to be a really detail reference, but for example at the top of the document put something like:
Information extracted from files:
RTCM version 3.0 ...
BDS reference document...
This way, it's easier for future maintainers to know which files and versions of them have been used to populate all the constants we see here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically all of these come from the constellation ICDs I believe
Co-authored-by: Alejandro Duarte <[email protected]>
…r values against the C implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! I left some comments mostly related to improve the overall documentation of the modules so it's easy for developers to compile the documentation and easily understand how to use these structs / enums.
@@ -0,0 +1,7 @@ | |||
pub(crate) const fn compile_time_max_u16(a: u16, b: u16) -> u16 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may not understand the purpose of this module and function. Could you add a module documentation and function documentation for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a doc comment. Basically we want to find the constellation with the most number of satellites, so we use this here to find the largest value at compile time.
swiftnav/src/signal/mod.rs
Outdated
// THIS CODE AND INFORMATION IS PROVIDED "AS IS" WITHOUT WARRANTY OF ANY KIND, | ||
// EITHER EXPRESSED OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE IMPLIED | ||
// WARRANTIES OF MERCHANTABILITY AND/OR FITNESS FOR A PARTICULAR PURPOSE. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a module documentation with a small inline example? The below one is just an example, perhaps the documentation could contain a description of what the module offers and how to interact with it, using the examples as guidance.
//! Signal module
//!
//! This module provides the `GnssSignal`, `Code` and `Constellation` objects, provided with utility functions for easy handling and manipulation.
//!
//! # Example
//!
//! ```rust
//! // Create new GnssSignal
//! let signal = GnssSignal::new(1, Code::Bds3B5x);
//! println!("{}", signal);
//! ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some explanation and example code.
Hash, | ||
strum::AsRefStr, | ||
strum::Display, | ||
strum::EnumIter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It bothers me that this functionality still has not landed in serde yet as it seems like it would be pretty straightfoward to add and many projects end up using both serde and strum
/// The maximum number of satellites in a single constellation | ||
pub const MAX_NUM_SATS: u16 = compile_time_max_u16( | ||
NUM_SATS_GPS, | ||
compile_time_max_u16( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting way to avoid lazy execution 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like const
traits are still being worked on so there isn't any generic const
comparisons in the standard library. I did find a couple of crates which provide this via proc macros, but this seemed the most straight forward option.
The second chunk of changes from #122.
This re-implements the GNSS signal related types of functionality in native Rust
What's been changed
signal
module has been broken up into multiple files, though these extra files are not visible via the public APIstrum
crate, but the actual string values have remained the sameswiftnav-sys
integer types has been replaced with conversions to/from native Rust integer typesswiftnav-sys
types and instead contain native Rust typesthiserror
crate to reduce boiler plate codeWhat's been removed
Code::sig_count()
Code::chip_count()
Code::chip_rate()
What's been added
signal::consts
moduleConstellation::sat_count()
Constellation::first_prn()
Code::get_carrier_frequency()
Code::get_glo_channel_frequency()
GnssSignal::carrier_frequency()
GnssSignal::get_glo_channel_frequency()