From 4a299b4f99a926369348103cdbb7b0fa2f7dce16 Mon Sep 17 00:00:00 2001 From: Troy Neubauer Date: Sun, 6 Oct 2024 11:25:43 -0700 Subject: [PATCH] self review --- Cargo.toml | 8 +++----- crates/hackrf-one-rs/Cargo.toml | 1 + crates/hackrf-one-rs/src/lib.rs | 34 +++++++++++++++++++-------------- src/impls/hackrfone.rs | 28 +++++++++++---------------- 4 files changed, 35 insertions(+), 36 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 10dc98e..fe93251 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,10 +9,9 @@ license = "Apache-2.0" repository = "https://github.com/FutureSDR/seify" [features] -# default = ["soapy", "hackrfone"] -default = ["hackrfone"] +default = ["soapy"] rtlsdr = ["dep:seify-rtlsdr"] -hackrfone = ["dep:seify-hackrfone"] +hackrfone = ["dep:seify-hackrfone", "dep:nusb"] aaronia = ["dep:aaronia-rtsa"] aaronia_http = ["dep:ureq"] soapy = ["dep:soapysdr"] @@ -26,12 +25,11 @@ futures = "0.3" log = "0.4" nom = "7.1" num-complex = "0.4" -nusb = { git = "https://github.com/kevinmehall/nusb.git" } +nusb = { git = "https://github.com/kevinmehall/nusb.git", optional = true } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" serde_with = "3.6" thiserror = "1.0" -tracing = "0.1.40" [target.'cfg(not(target_arch = "wasm32"))'.dependencies] once_cell = "1.19" diff --git a/crates/hackrf-one-rs/Cargo.toml b/crates/hackrf-one-rs/Cargo.toml index f183bc3..31fe579 100644 --- a/crates/hackrf-one-rs/Cargo.toml +++ b/crates/hackrf-one-rs/Cargo.toml @@ -2,6 +2,7 @@ name = "seify-hackrfone" version = "0.1.0" edition = "2021" +authors = ["Troy Neubauer "] [dependencies] num-complex = "0.4" diff --git a/crates/hackrf-one-rs/src/lib.rs b/crates/hackrf-one-rs/src/lib.rs index f36fea5..e4a80d1 100644 --- a/crates/hackrf-one-rs/src/lib.rs +++ b/crates/hackrf-one-rs/src/lib.rs @@ -174,6 +174,7 @@ pub type Result = std::result::Result; /// descriptors, such as `bcdUSB` and `bcdDevice` in device descriptors. #[derive(Debug, PartialEq, Eq, Clone, Copy, Hash, PartialOrd, Ord)] // Taken from rusb::Version: https://github.com/a1ien/rusb/blob/8f8c3c6bff6a494a140da4d93dd946bf1e564d66/src/fields.rs#L142-L203 +// nusb doesnt currently have this pub struct UsbVersion(pub u8, pub u8, pub u8); impl UsbVersion { @@ -230,6 +231,8 @@ pub struct HackRf { } impl HackRf { + /// Wraps an existing nusb::Device. + /// Useful on platform like Android where you are forced to use [`nusb::Device::from_fd`]. pub fn wrap(device: nusb::Device) -> Result { let interface = device.claim_interface(0)?; @@ -242,6 +245,7 @@ impl HackRf { }); } + /// Opens `info` based on the result of a `nusb` scan. fn open(info: DeviceInfo) -> Result { let device = info.open()?; let interface = device.claim_interface(0)?; @@ -295,6 +299,7 @@ impl HackRf { Err(Error::NotFound) } + /// Resets the HackRf. pub fn reset(self) -> Result<()> { self.check_api_version(UsbVersion::from_bcd(0x0102))?; self.write_control(Request::Reset, 0, 0, &[])?; @@ -337,8 +342,8 @@ impl HackRf { pub fn start_tx(&self, config: &Config) -> Result<()> { tracing::info!("Starting tx: {config:?}"); - // NOTE: perform atomic exchange first so that we only change the transceiver mode once if - // other threads are racing to change the mode + // NOTE: perform atomic exchange first so that we only change the transceiver mode once if + // other threads are racing to change with us if let Err(actual) = self.mode.compare_exchange( Mode::Idle, Mode::Transmit, @@ -372,8 +377,8 @@ impl HackRf { /// This function will return an error if a tx or rx operation is already in progress or if an /// I/O error occurs pub fn start_rx(&self, config: &Config) -> Result<()> { - // NOTE: perform the atomic exchange first so that we only change the hackrf's state once if - // other threads are racing with us + // NOTE: perform atomic exchange first so that we only change the transceiver mode once if + // other threads are racing to change with us if let Err(actual) = self.mode.compare_exchange( Mode::Idle, Mode::Receive, @@ -402,14 +407,16 @@ impl HackRf { // NOTE: perform atomic exchange last so that we prevent other threads from racing to // start tx/rx with the delivery of our TransceiverMode::Off request // - // This means that multiple threads can race on stop_tx/stop_rx, however in the worst case - // the hackrf will receive multiple TransceiverMode::Off requests, but will always end up - // in a valid state with the transceiver disabled. A mutex or other mode variants like - // Mode::IdlePending would solve this, however quickly this begins to look like a manually implemented mutex. + // This means if multiple threads call stop_tx/stop_rx concurrently the hackrf may receive + // multiple TransceiverMode::Off requests, but will always end up in a valid state with the + // transceiver disabled. + // + // Adding something like Mode::IdlePending would solve this, + // however quickly this begins to look like a manually implemented mutex. // // To keep this crate low-level and low-overhead, this solution is fine and we expect // consumers to wrap our type in an Arc and be smart enough to not enable / disable tx / rx - // from multiple threads at the same time. + // from multiple threads at the same time on a single duplex radio. self.write_control( Request::SetTransceiverMode, @@ -425,7 +432,7 @@ impl HackRf { Ordering::Relaxed, ) { return Err(Error::WrongMode { - required: Mode::Idle, + required: Mode::Transmit, actual, }); } @@ -434,8 +441,7 @@ impl HackRf { } pub fn stop_rx(&self) -> Result<()> { - // NOTE: perform atomic exchange last so that we prevent other threads from racing to - // start tx/rx with the delivery of our TransceiverMode::Off request + // NOTE: same as above - perform atomic exchange last self.write_control( Request::SetTransceiverMode, @@ -451,7 +457,7 @@ impl HackRf { Ordering::Relaxed, ) { return Err(Error::WrongMode { - required: Mode::Idle, + required: Mode::Receive, actual, }); } @@ -806,7 +812,7 @@ mod test { assert_eq!(freq_params(u64::MAX), [0xFF; 8]); } - // NOTE: make sure you can transmit on the frequency below in your country! + // NOTE: make sure you can transmit on the frequency below before enabling! // #[test] #[allow(dead_code)] fn device_states() { diff --git a/src/impls/hackrfone.rs b/src/impls/hackrfone.rs index b41bab9..993c278 100644 --- a/src/impls/hackrfone.rs +++ b/src/impls/hackrfone.rs @@ -33,10 +33,10 @@ impl HackRfOne { /// Create a Hackrf One devices pub fn open>(args: A) -> Result { let args: Args = args.try_into().or(Err(Error::ValueError))?; - log::info!("HackRfOne::open called: {args}"); if let Ok(fd) = args.get::("fd") { - log::info!("device open got special fd arg"); + log::info!("Wrapping hackrf fd={fd}"); + // SAFETY: the caller intends to pass ownership to us let fd = unsafe { OwnedFd::from_raw_fd(fd) }; let dev = nusb::Device::from_fd(fd)?; @@ -56,11 +56,11 @@ impl HackRfOne { seify_hackrfone::HackRf::open_bus(bus_number, address)? } (Err(Error::NotFound), Err(Error::NotFound)) => { - println!("Opening first hackrf device"); + log::debug!("Opening first hackrf device"); seify_hackrfone::HackRf::open_first()? } (bus_number, address) => { - println!("HackRfOne::open received invalid args: bus_number: {bus_number:?}, address: {address:?}"); + info::warn!("HackRfOne::open received invalid args: bus_number: {bus_number:?}, address: {address:?}"); return Err(Error::ValueError); } }; @@ -112,7 +112,7 @@ impl crate::RxStreamer for RxStreamer { } fn activate_at(&mut self, _time_ns: Option) -> Result<(), Error> { - // TODO(tjn): sleep precisely for `time_ns` + // TODO: sleep precisely for `time_ns` let config = self.inner.rx_config.lock().unwrap(); self.inner.dev.start_rx(&config)?; @@ -122,9 +122,8 @@ impl crate::RxStreamer for RxStreamer { } fn deactivate_at(&mut self, _time_ns: Option) -> Result<(), Error> { - // TODO(tjn): sleep precisely for `time_ns` + // TODO: sleep precisely for `time_ns` - println!("dropping stream"); let _ = self.stream.take().unwrap(); self.inner.dev.stop_rx()?; Ok(()) @@ -169,7 +168,7 @@ impl crate::TxStreamer for TxStreamer { } fn activate_at(&mut self, _time_ns: Option) -> Result<(), Error> { - // TODO(tjn): sleep precisely for `time_ns` + // TODO: sleep precisely for `time_ns` let config = self.inner.tx_config.lock().unwrap(); self.inner.dev.start_rx(&config)?; @@ -178,7 +177,7 @@ impl crate::TxStreamer for TxStreamer { } fn deactivate_at(&mut self, _time_ns: Option) -> Result<(), Error> { - // TODO(tjn): sleep precisely for `time_ns` + // TODO: sleep precisely for `time_ns` self.inner.dev.stop_tx()?; Ok(()) @@ -298,7 +297,7 @@ impl crate::DeviceTrait for HackRfOne { fn gain_elements(&self, direction: Direction, channel: usize) -> Result, Error> { if channel == 0 { - // TODO(tjn): add support for other gains (RF and baseband) + // TODO: add support for other gains (RF and baseband) // See: https://hackrf.readthedocs.io/en/latest/faq.html#what-gain-controls-are-provided-by-hackrf match direction { Direction::Tx => Ok(vec!["IF".into()]), @@ -335,17 +334,14 @@ impl crate::DeviceTrait for HackRfOne { } fn set_gain(&self, direction: Direction, channel: usize, gain: f64) -> Result<(), Error> { - println!("set_gain"); self.set_gain_element(direction, channel, "IF", gain) } fn gain(&self, direction: Direction, channel: usize) -> Result, Error> { - println!("gain"); self.gain_element(direction, channel, "IF") } fn gain_range(&self, direction: Direction, channel: usize) -> Result { - println!("gain_range"); self.gain_element_range(direction, channel, "IF") } @@ -362,7 +358,6 @@ impl crate::DeviceTrait for HackRfOne { Direction::Tx => todo!(), Direction::Rx => { let mut config = self.inner.rx_config.lock().unwrap(); - println!("setting lna_db to {gain}"); config.lna_db = gain as u16; Ok(()) } @@ -398,7 +393,7 @@ impl crate::DeviceTrait for HackRfOne { channel: usize, name: &str, ) -> Result { - // TODO(tjn): add support for other gains + // TODO: add support for other gains if channel == 0 && name == "IF" { match direction { Direction::Tx => Ok(Range::new(vec![RangeItem::Step(0.0, 47.0, 1.0)])), @@ -480,7 +475,6 @@ impl crate::DeviceTrait for HackRfOne { && name == "TUNER" { self.with_config(direction, |config| { - println!("Set frequency to {frequency}"); config.frequency_hz = frequency as u64; self.inner.dev.set_freq(frequency as u64)?; Ok(()) @@ -492,7 +486,7 @@ impl crate::DeviceTrait for HackRfOne { fn sample_rate(&self, direction: Direction, channel: usize) -> Result { // NOTE: same state for both "directions" lets hope future sdr doesnt assume there are two - // values here, should be fine since we told it were not full duplex + // values here, should be fine since we told it we're not full duplex if channel == 0 { self.with_config(direction, |config| Ok(config.sample_rate_hz as f64)) } else {