Expose TX and RX flags

[kevinmehall]

https://github.com/pothosware/SoapySDR/blob/master/include/SoapySDR/Constants.h

For RX streams, perhaps RxStream::read should return Result<RxStatus, Error> rather than Result<usize, Error>, where RxStatus is a struct with fields:

/// Number of samples read from the stream. May be less than the number of samples requested.
samples: usize,

/// Indicates that the received data ends a burst
end_burst: bool,

/// Indicates that stream terminated prematurely. 
end_abrupt: bool

/// Indicates that the packet was fragmented
more_fragments: bool

/// Timestamp in nanoseconds, if provided by the driver
time: Option<i64>,

end_abrupt could use a better name or better description. pothos-soapy uses it as a generalized "we lost samples and must resync timing" indicator. It is unclear how it differs from an error return, besides that you get some samples this way.

RxStream::activate could gain args burst: Option<u64>, which sets the SOAPY_SDR_END_BURST flag and sets numElems.

For TX streams, the simplest would be to add time: Option<i64>, end_burst: bool args to write. This variant could also be a separate method, but I don't have a good name for it.

Propose to leave unsupported for now, as I don't think they are used by any driver:
SOAPY_SDR_ONE_PACKET
SOAPY_SDR_WAIT_TRIGGER

These might want to be configured as a stream setting, as you're not likely to need to switch between manual packet management and regular streaming on a per-call basis.

[razorheadfx]

I threw something together, maybe went a little overboard with the flag "parsing".
read(..)````is now more in line with write(..)Not sure about addinghas_time toRxStreamand using it to switch off the timestamping inread, while it adds the advantage of having a clear API (optional timestamp) it might simply be easier to shift the burden to the user and let her handle "ignoring" the timestamp. Also I read somewhere that, while lime does not have hardware timestamps, it does keep a sample counter which it might (does?) write to the time_ns``` argument.
Any thoughts?

diff --git a/src/device.rs b/src/device.rs
index 6d6030a..c24c6b7 100644
--- a/src/device.rs
+++ b/src/device.rs
@@ -82,6 +82,41 @@ impl ::std::error::Error for Error {
     }
 }
 
+
+/// Indicator for stream status and certain stream error cases
+/// associated with `RxStream::read` and `TxStream::write`.
+/// Usually provided in the context of RxStatus 
+#[repr(u32)]
+#[derive(Clone, Copy, Eq, PartialEq, Debug, Hash)]
+pub enum StreamCode {
+    EndOfBurst = 2,
+    HasTime = 4,
+    EndAbrupt = 8,
+    OnePacket = 16,
+    MoreFragments = 32,
+    WaitTrigger = 64,
+}
+
+impl StreamCode {
+    /// checks this value against an i32 flag
+    pub fn is_set(&self, flag: i32) -> bool {
+        (*self as i32 & flag) == *self as i32
+    }
+
+    /// iterator over all variants of `StreamCode`
+    pub fn variants() -> slice::Iter<'static, StreamCode> {
+        static VARIANTS: [StreamCode; 6] = [
+            StreamCode::EndOfBurst,
+            StreamCode::HasTime,
+            StreamCode::EndAbrupt,
+            StreamCode::OnePacket,
+            StreamCode::MoreFragments,
+            StreamCode::WaitTrigger,
+        ];
+        VARIANTS.into_iter()
+    }
+}
+
 /// Transmit or Receive
 #[repr(u32)]
 #[derive(Copy, Clone, Eq, PartialEq, Debug, Hash)]
@@ -366,8 +401,7 @@ impl Device {
                 device: self,
                 handle: stream,
                 nchannels: channels.len(),
-                flags: 0,
-                time_ns: 0,
+                has_time: self.has_hardware_time(None).unwrap_or(false),
                 active: false,
                 phantom: PhantomData,
             })
@@ -820,8 +854,7 @@ pub struct RxStream<'a, E: StreamSample> {
     device: &'a Device,
     handle: *mut SoapySDRStream,
     nchannels: usize,
-    flags: i32,
-    time_ns: i64,
+    has_time : bool,
     active: bool,
     phantom: PhantomData<fn(&mut[E])>,
 }
@@ -890,7 +923,7 @@ impl<'a, E: StreamSample> RxStream<'a, E> {
     ///
     /// # Panics
     ///  * If `buffers` is not the same length as the `channels` array passed to `Device::rx_stream`.
-    pub fn read(&mut self, buffers: &[&mut[E]], timeout_us: i64) -> Result<usize, Error> {
+    pub fn read(&mut self, buffers: &[&mut[E]], at_ns : Option<i64>, end_burst: bool, timeout_us: i64) -> Result<RxStatus, Error> {
         unsafe {
             assert!(buffers.len() == self.nchannels);
 
@@ -899,23 +932,101 @@ impl<'a, E: StreamSample> RxStream<'a, E> {
             //TODO: avoid this allocation
             let buf_ptrs = buffers.iter().map(|b| b.as_ptr()).collect::<Vec<_>>();
 
-            self.flags = 0;
-            let len = len_result(SoapySDRDevice_readStream(
+            let mut flags = 0i32;
+            if end_burst{
+                flags |= SOAPY_SDR_END_BURST as i32;
+            }
+
+            // the has_time might be removed if we decide we want the user to do these checks herself
+            if at_ns.is_some() && self.has_time{
+                flags |= SOAPY_SDR_HAS_TIME as i32;
+            }
+
+            // decouples the input timestamp from the state changed by the driver
+            let mut time_ns = at_ns.clone().unwrap_or(0);
+
+            let samples = len_result(SoapySDRDevice_readStream(
                 self.device.ptr,
                 self.handle,
                 buf_ptrs.as_ptr() as *const *const _,
                 num_samples,
-                &mut self.flags as *mut _,
-                &mut self.time_ns as *mut _,
+                flags as *mut _,
+                &mut time_ns as *mut _,
                 timeout_us
             ))?;
 
-            Ok(len as usize)
+            // we have 6 cases here: 
+            //  no at_ns provided i.e. input=0:
+            //  1./2.   output == 0 => driver did not change input
+            //                         OR cleared it                                    
+            //  3.      output != 0 => driver changed the input to the actual reception timestamp
+            //                     (what UHD does)
+            //
+            //  at_ns provided i.e. input != 0
+            //  4./5.   output == input => driver did not change the timestamp 
+            //                             OR the driver managed to receive at the exact moment in time
+            //  6.      output != input => driver updated the timestamp to the actual reception timestamp
+
+            match at_ns{
+                None => {
+                    if time_ns != 0{
+                        debug!("Driver updated timestamp from 0 to {}", time_ns)
+                    }else{
+                        debug!("Driver did not change timestamp or set it to 0")
+                    }
+                },
+                Some(input) => {
+                    if input == time_ns{
+                        debug!("Driver did not update timestamp or received at exact moment {}",time_ns)
+                    }
+                    else{
+                        debug!("Driver updated timestamp from {} to {}", input, time_ns)
+                    }
+                }
+            }
+
+
+            let time_ns = if self.has_time{
+                Some(time_ns)
+            }else{
+                None
+            };
+            let samples = samples as usize;
+
+            Ok(RxStatus{
+                samples,
+                time_ns,
+                flags
+            })
         }
     }
 
 }
 
+/// Wraps for `read` related metadata such as indicator flags and timestamps
+pub struct RxStatus {
+    /// Number of samples read from the stream
+    pub samples: usize,
+    /// Timestamp associated with this reception
+    pub time_ns: Option<i64>,
+    /// The last flags associated with the read call
+    pub flags: i32,
+}
+
+impl RxStatus {
+    /// Checks whether a certain StreamCode is set on the flags
+    pub fn has_code(&self, code: StreamCode) -> bool {
+        code.is_set(self.flags)
+    }
+    /// Returns all set StreamCodes
+    pub fn all_codes(&self) -> Vec<StreamCode> {
+        StreamCode::variants()
+            .filter(|c| c.is_set(self.flags))
+            .map(|c| *c)
+            .collect()
+    }
+}
+
 /// A stream open for transmitting.
 ///
 /// To obtain a TxStream, call `Device::tx_stream`. The type parameter `E` represents the type

[kevinmehall]

If one were to design an API covering SoapySDR's functionality from scratch in Rust, I don't think it would look like this. Is there a use case when you would ever want a Vec containing the flags or an enum representing one flag? I think the relevant API is just whether or not the condition conveyed by the flag exists, which could be a series of methods or fields directly on RxStatus.

Do you know if passing HAS_TIME and a timestamp to readStream is meaningful to any of the drivers, or is that purely an out-pointer in the C++ API? My understanding was that to RX at a particular time, you would call activateStream with a timestamp, and then reads just fetch successive samples from there until deactivated.

[razorheadfx]

Having all flag values in an enum makes it more obvious to the user which values flags may take, rather than having to dive into the bindings.

Apart from debugging purposes I agree that getting a Vec of set flags is nonsense.
It is probably easier to pass both flags and the timestamp as manipulated by the driver back in RxStatus and letting the make heads or tails of both according to the actual equipment in use.

Do you know if passing HAS_TIME and a timestamp to readStream is meaningful to any of the drivers, or is that purely an out-pointer in the C++ API?

Looks like the flags are passed straight through C to the C++ API to the appropriate driver adapter.
Looks like they are handled independent of each other too.
From painful experience I can tell that not activating an RxStream prior to calling read (with or without burst and/or time) with USRPs blocks indefinitely xD.
Activating a stream for reading x elements (with implicit deactivation afterwards) would be done by calling SoapySDRDevice_readStream with a number other than 0 for the last argument.
Flipping through the SoapyUHD adapter sources, that [seems to set the correct] values(https://github.com/pothosware/SoapyUHD/blob/master/SoapyUHDDevice.cpp#L240) on the rx_metadata which UHD uses for its signalling.
I don't know how the other drivers handle this.

Come to think of it, having numElems (last argument to SoapySDR_SoapySDRDevice_activateStream(..) in Rx/TxStream.activate(..) exposed (either optional or through another function) would go a long way of being closer to the actual API.

[mokus0]

I'm trying to find a way to access the time_ns field without hacking the library to expose it, and I'm not finding an obvious way but it looks like this proposal would make it available. Is there some other way I'm missing?

[kevinmehall]

time_ns on SoapySDRDevice_readStream is not currently exposed. It does exist as a field of RxStream, the pointer to which is passed to SoapySDR, though, so we could trivially add a method that exposes that value. I'd accept a PR for that.

I suppose the other flags could be exposed with getter methods in the same way. That may even be a better API than the RxStatus struct to keep the common case simple, and it can be added without a breaking change.

[tejeez]

time_ns on SoapySDRDevice_readStream is not currently exposed. It does exist as a field of RxStream, the pointer to which is passed to SoapySDR, though, so we could trivially add a method that exposes that value. I'd accept a PR for that.

I suppose the other flags could be exposed with getter methods in the same way. That may even be a better API than the RxStatus struct to keep the common case simple, and it can be added without a breaking change.

Any update on this? Received timestamps would be important for an application I'm currently writing, so I could try to make a PR. Some discussion on the API before that would be useful.

Indeed, the time_ns field could be made readable with a method, but I think it might confuse programmers since it would be quite inconsistent with SoapySDR bindings in other languages.

For example, readStream in SoapySDR Python bindings returns a StreamResult object containing the return value, flags, timestamp and some other stuff (see https://github.com/pothosware/SoapySDR/blob/bb33b2d27437d962ab4f635cd8a441a55a6d2b0d/swig/python/SoapySDR.in.i#L155 ). Maybe something similar could be done here by adding a StreamResult struct. In my experience the Python binding works well and is easy enough to use, so maybe it would be a good reference for API design here too.

Some applications may also want to pass a timestamp to readStream (to get samples starting from a given timestamp), so a new stream read method with support for all SoapySDR features would be useful anyway. To avoid a breaking change, maybe it could be given a new name, keeping the current read method there for simple use cases where timestamps or flags are not needed.

What do you think?

[tejeez]

Ok, now that thought of it again, using an Option to combine timestamp value and the HAS_TIME flag is probably a good idea, so maybe it should not be look just like, say, the Python API after all.

I think all other stream flags should still be accessible for activate, deactive, read and write calls. It should be also possible to get all the flags returned by read and write calls.

How about a struct StreamFlags which would contain a bool for all the other SoapySDR stream flags (except HAS_TIME)? So it would be something like

pub struct StreamFlags {
    // bools for all SoapySDR stream flags (except HAS_TIME, maybe?)
}

pub struct StreamResult {
    pub samples: usize,
    pub time: Option<i64>,
    pub flags: StreamFlags,
}

pub fn read_with_flags(&mut self, buffers: &mut [&mut [E]], flags: StreamFlags, time_ns: Option<i64>, timeout_us: i32) -> Result<StreamResult, Error>

pub fn write_with_flags(&mut self, buffers: &[&[E]], flags: StreamFlags, time_ns: Option<i64>, timeout_us: i32) -> Result<StreamResult, Error>

Maybe something like this would be both rusty enough and consistent enough with SoapySDR in other languages.

[tejeez]

How about a struct StreamFlags which would contain a bool for all the other SoapySDR stream flags (except HAS_TIME)?

I started working on it and figured out the bitflags crate could be useful here. So, now I'm thinking of:

bitflags! {
    // SoapySDR C API defines flag arguments as signed int
    // but bindgen generates constants as u32.
    // Should it be i32 or u32 here?
    // Well, it does not really matter for bit flags.

    pub struct StreamFlags: i32 {
        const END_BURST      = SOAPY_SDR_END_BURST      as i32;
        // Not sure if it is better to have HAS_TIME here or not
        //const HAS_TIME       = SOAPY_SDR_HAS_TIME       as i32;
        const END_ABRUPT     = SOAPY_SDR_END_ABRUPT     as i32;
        const ONE_PACKET     = SOAPY_SDR_ONE_PACKET     as i32;
        const MORE_FRAGMENTS = SOAPY_SDR_MORE_FRAGMENTS as i32;
        const WAIT_TRIGGER   = SOAPY_SDR_WAIT_TRIGGER   as i32;
    }
}

[tejeez]

I implemented my idea here:
tejeez@ffa046d

I did not make a pull request yet since I would like to hear some feedback or discussion first. Should a similar function exist for TX too? How should they be named?

Btw, another thing I was wondering:

I suppose the other flags could be exposed with getter methods in the same way. That may even be a better API than the RxStatus struct to keep the common case simple, and it can be added without a breaking change.

I understand that a simpler alternative for common, simple usecases is nice, but it just seems weird to me that the "simple" TX function supports timestamps whereas the "simple" RX function does not. How often do you have a usecase where you need timestamps for TX but not for RX?

[kevinmehall]

When I last looked at this, there wasn't much documentation on the flags and I had to go look at the driver code, and it seemed a little inconsistent between them. Even the basics like which flags and time are passed in or out or both via the pointer argument are under-specified.

This approach of exposing all the flags on both call and return I suppose makes for the the maximum flexibility here, but leaves it up to the user to know how they're supposed to use it. What driver are you looking at where passing HAS_TIME into read actually does something? I thought you were supposed to pass the start time to activate, and read only returned the actual time.

For HAS_TIME, yes, an Option is ideal, because it's signaling the validity of the time argument passed separately.

Should a similar function exist for TX too? How should they be named?

For TX, since at_ns and end_burst args were already added, what flags that drivers actually use are not currently available?

[tejeez]

When I last looked at this, there wasn't much documentation on the flags and I had to go look at the driver code, and it seemed a little inconsistent between them. Even the basics like which flags and time are passed in or out or both via the pointer argument are under-specified.

This approach of exposing all the flags on both call and return I suppose makes for the the maximum flexibility here, but leaves it up to the user to know how they're supposed to use it. What driver are you looking at where passing HAS_TIME into read actually does something? I thought you were supposed to pass the start time to activate, and read only returned the actual time.

You might be correct. I looked at code of some drivers again and realized my understanding was probably wrong. I could remove the timestamp parameter from read after all.

For HAS_TIME, yes, an Option is ideal, because it's signaling the validity of the time argument passed separately.

Should a similar function exist for TX too? How should they be named?

For TX, since at_ns and end_burst args were already added, what flags that drivers actually use are not currently available?

I have no idea to be honest. I mainly thought it would be annoying for someone to realize you cannot use a feature of some more obscure SDR just because we have decided to omit support for certain flags. I should probably also take a closer look at what the other flags are supposed to do, to figure out whether they make any sense for a TX write call.

[tejeez]

Yet another thing I was thinking about: seems like in most cases, the returned number of samples would be used to take a [0..length] slice of the RX buffer to be processed. Would it make sense to return a reference to that slice instead of just returning the length as an usize? I mean, something like:

pub struct StreamResult<'a, E: StreamSample> {
    pub samples: &'a [E],
    pub flags: StreamFlags,
    pub time: Option<i64>,
}

I'm not really sure about this. Seems like it would make the API a bit nicer to use, but would it just unnecessarily complicate something?

write_all is also nice to have. How about adding a read_exact function that would similarly repeat reads until the whole buffer has been filled?