-
Notifications
You must be signed in to change notification settings - Fork 13
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
add shared modulation types for gfsk, lora #24
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
//! Common GFSK modulation options | ||
|
||
/// Basic GFSK channel configuration | ||
#[derive(Clone, PartialEq, Debug)] | ||
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] | ||
pub struct GfskChannel { | ||
/// Channel frequency in kHz | ||
pub freq_khz: u32, | ||
|
||
/// Channel bandwidth in kHz | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be hertz, at least for the sx1231 most RxBw options are not aligned with kHz. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, is this receiver bandwidth or transmitter frequency deviation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good question, i was aiming for modulation bandwidth (ie. frequency deviation) as ime this is how channels are usually described / receiver bandwidth is typically driven by this |
||
pub bw_khz: u16, | ||
|
||
/// Bitrate in bps | ||
pub bitrate_bps: u32, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
//! Common LoRa modulation options | ||
|
||
/// LoRa mode channel configuration | ||
#[derive(Clone, PartialEq, Debug)] | ||
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] | ||
|
||
pub struct LoRaChannel { | ||
/// LoRa frequency in kHz | ||
pub freq_khz: u32, | ||
/// LoRa channel bandwidth | ||
pub bw_khz: u16, | ||
/// LoRa Spreading Factor | ||
pub sf: SpreadingFactor, | ||
/// LoRa Coding rate | ||
pub cr: CodingRate, | ||
} | ||
|
||
/// Spreading factor for LoRa mode | ||
#[derive(Copy, Clone, PartialEq, Debug)] | ||
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] | ||
#[non_exhaustive] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it means that adding additional spreading factors / coding rates in the future (what if we missed one!) would not be a breaking change and forces consumers to handle the unsupported case, i think it's worth it. |
||
pub enum SpreadingFactor { | ||
/// LoRa Spreading Factor 5, 32 chips / symbol | ||
Sf5, | ||
/// LoRa Spreading Factor 6, 64 chips / symbol | ||
Sf6, | ||
/// LoRa Spreading Factor 7, 128 chips / symbol | ||
Sf7, | ||
/// LoRa Spreading Factor 8, 256 chips / symbol | ||
Sf8, | ||
/// LoRa Spreading Factor 9, 512 chips / symbol | ||
Sf9, | ||
/// LoRa Spreading Factor 10 1024 chips / symbol | ||
Sf10, | ||
/// LoRa Spreading Factor 11 2048 chips / symbol | ||
Sf11, | ||
/// LoRa Spreading Factor 12 4096 chips / symbol | ||
Sf12, | ||
} | ||
|
||
#[derive(Copy, Clone, PartialEq, Debug)] | ||
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] | ||
#[non_exhaustive] | ||
pub enum CodingRate { | ||
/// LoRa Coding rate 4/5 | ||
Cr4_5, | ||
/// LoRa Coding rate 4/6 | ||
Cr4_6, | ||
/// LoRa Coding rate 4/7 | ||
Cr4_7, | ||
/// LoRa Coding rate 4/8 | ||
Cr4_8, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
//! Shared types for radio channel / modulation configuration | ||
|
||
use core::fmt::Debug; | ||
|
||
pub mod gfsk; | ||
|
||
pub mod lora; | ||
|
||
/// Common modulation configuration errors | ||
/// | ||
/// These are provided as a helper for `TryFrom` implementations, | ||
/// and not intended to be prescriptive. | ||
#[derive(Copy, Clone, Debug, PartialEq)] | ||
#[cfg_attr(feature = "defmt", derive(defmt::Format))] | ||
#[non_exhaustive] | ||
pub enum ModError { | ||
UnsupportedBitrate, | ||
UnsupportedFrequency, | ||
UnsupportedBandwidth, | ||
} |
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.
Can't we make this Hz? It would top out at 4.2GHz, but is that going to limit any use case we have in mind?
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.
the decawave gear (DWM1000 etc.) goes up to 6.5GHz so, i think we're better to keep it wider. do you see a need for sub-khz definitions? because if so we could have a more complex
Frequency
typeThere 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.
(and ngl i would love to have a proper parser that understood frequency units)
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 don't have a concrete use case, but this feels like a requirement which is not going to be true for all implementations.
What about using embedded time Rate with concrete type being an associated type which defaults to kHz?
https://docs.rs/embedded-time/0.12.1/embedded_time/rate/trait.Rate.html
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.
at the moment we're avoiding
embedded_time
ine-h
due to the complexity, so, i think i'd prefer a simpler type defined by us for now.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.
Makes sense, we can define our own Frequency trait and implement that for e-t Rate (behind a feature). Then users can decide whether they bring their own type or use e-t.
RTIC uses a similar approach for their Monotonic: https://github.com/rtic-rs/rtic-monotonic/blob/4050b236f36ef99da2d88a9e2d4a211a8d46d262/src/lib.rs#L34