self review

FutureSDR · Oct 10, 2024 · 4a299b4 · 4a299b4
1 parent 9cfa98e
commit 4a299b4
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 36 deletions.
diff --git 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
@@ -2,6 +2,7 @@
 name = "seify-hackrfone"
 version = "0.1.0"
 edition = "2021"
+authors = ["Troy Neubauer <troy@foxhunter.ai>"]
 
 [dependencies]
 num-complex = "0.4"

diff --git a/crates/hackrf-one-rs/src/lib.rs b/crates/hackrf-one-rs/src/lib.rs
@@ -174,6 +174,7 @@ pub type Result<T> = std::result::Result<T, Error>;
 /// 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<Self> {
         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<Self> {
         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
@@ -33,10 +33,10 @@ impl HackRfOne {
     /// Create a Hackrf One devices
     pub fn open<A: TryInto<Args>>(args: A) -> Result<Self, Error> {
         let args: Args = args.try_into().or(Err(Error::ValueError))?;
-        log::info!("HackRfOne::open called: {args}");
 
         if let Ok(fd) = args.get::<i32>("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<i64>) -> 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<i64>) -> 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<i64>) -> 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<i64>) -> 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<Vec<String>, 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<Option<f64>, Error> {
-        println!("gain");
         self.gain_element(direction, channel, "IF")
     }
 
     fn gain_range(&self, direction: Direction, channel: usize) -> Result<Range, Error> {
-        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<Range, Error> {
-        // 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<f64, Error> {
         // 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 {