Re: Adding uhd-style tags, to soapy driver?

[Marcus Müller]

Hi Josh,

@Josh Bailey:
great work! Thanks for the patch :) Sadly, for some reason, `patch` and `git 
apply` only
see the first hunk (i.e. the changes to block_impl.h and the three static const 
pmts), and
I can't directly put your email through `git am` because HTML + MIME-multipart 
seems to be
hard ... ech, tooling.
Anyways, giving us a byte-exact patch not the point of your email, was it. You 
want to
discuss this feature proposal, right?

So, bit of feedback: Yep, this is a super important feature, and even as someone
affiliated with Ettus, I wish more players in the field of low-cost SDRs had it.

Small problem:

// TODO: use timestamp from radio hardware, and < 1s granularity?

exactly that's where things become hard!
The hardware needs to deliver timestamps. That's surprisingly rare.
Also, you need that kind of metadata to be exposed via an API – in this case, 
through
SoapySDR. Since you basically have neither, I'm afraid that while a highly 
desirable
feature, we're kind of missing the central thing here: GNU Radio can't give you 
timestamps
that hardware and driver API don't deliver. Luckily, you can check
SoapySDR::Device::readStream()'s `flags & SOAPY_SDR_HAS_TIME` and if set, call
`readStreamStatus(..., int64_t& timeNs,...)` to get a timestamp.

Now, you only need your hardware to supply timestamps, and your Soapy driver 
developers to
expose them that way.
@Josh of the first Blum kind: Without looking, I'd presume aside from SoapyUHD, 
at most
BladeRF supports that at this point, right?

Couple other pieces of feedback:

> +    std::stringstream str;
> +    str << name() << unique_id();
> +    pmt::pmt_t _id = pmt::string_to_symbol(str.str());

Never do string creation, especially with stringstream, in a general_work – 
they allocate,
deallocate memory, and that leads to unpredictable delays/computational load 
right where
you can use it the least.

> +    std::time_t time_now = std::time(nullptr);
(and then tagging with that)

I'm assuming this is meant more as a placeholder for getting actual SDR time 
from the SDR
device (especially since `time_t` isn't sufficiently fine-grained). It's just 
that I saw
that pattern emerge more than once, so for the future reader:

This is useless to harmful: the time of the host PC is only vaguely related to 
time on the
device: it runs at different speeds, and there's random transport and GNU Radio 
delay. So,
as soon as you tag twice your time stamps will simply be contradicting your 
sample count.
Don't do that; having time tags carrying but PC time is snake oil which, and if 
you
actually use these tags, it will break any assumptions about how time works 
(i.e. it's
possible that an SDR might not sample for a while, which means when you count 
samples from
your last tag, your next tag might be later than expected from
last_tag_time+number_of_samples_since/sampling_rate, but if you see a tag 
that's earlier
than expected, something broke about the universe).

Best regards,
Marcus

On 19.10.21 00:41, Bailey, Josh wrote:
> Hi all,
> 
> The gr-uhd driver, tags samples when center frequency changes and some other 
> events):
> 
> https://github.com/gnuradio/gnuradio/blob/8b23f906844c9784c784934ae06dfdfe10d31a1f/gr-uhd/lib/usrp_source_impl.cc#L619
> <https://github.com/gnuradio/gnuradio/blob/8b23f906844c9784c784934ae06dfdfe10d31a1f/gr-uhd/lib/usrp_source_impl.cc#L619>
> 
> I've been able to make a minimal patch to the soapy driver (see following), 
> that does the
> same thing for all other SDRs supported by soapy (the patch is for proof of
> concept/illustrative purposes only - I appreciate that it isn't compatible 
> with the
> contribution guidelines).
> 
> My question - would a suitable patch that provides these tags, be acceptable 
> to the
> maintainers? I can appreciate that it's possible that there are flow graphs 
> that just
> happen to use these same tags perhaps for other purposes - perhaps I could 
> then add a flag
> to make them optional? In any case, the gr-uhd driver appears to always send 
> them, already.
> 
> Thanks,
> 
> diff --git a/gr-soapy/lib/block_impl.h b/gr-soapy/lib/block_impl.h
> index a1e95fdd0..74b2beffe 100644
> --- a/gr-soapy/lib/block_impl.h
> +++ b/gr-soapy/lib/block_impl.h
> @@ -90,6 +90,7 @@ protected:
>  public:
>      bool start() override;
>      bool stop() override;
> +    bool _tag_now;
>  
>      /*** Begin public API implementation ***/
>  
> diff --git a/gr-soapy/lib/source_impl.cc b/gr-soapy/lib/source_impl.cc
> index f76d4437f..93aa06bb2 100644
> --- a/gr-soapy/lib/source_impl.cc
> +++ b/gr-soapy/lib/source_impl.cc
> @@ -47,6 +47,10 @@ source_impl::source_impl(const std::string& device,
>  {
>  }
>  
> +static const pmt::pmt_t TIME_KEY = pmt::string_to_symbol("rx_time");
> +static const pmt::pmt_t FREQ_KEY = pmt::string_to_symbol("rx_freq");
> +static const pmt::pmt_t RATE_KEY = pmt::string_to_symbol("rx_rate");
> +
>  int source_impl::general_work(int noutput_items,
>                                __GR_ATTR_UNUSED gr_vector_int& ninput_items,
>                                __GR_ATTR_UNUSED gr_vector_const_void_star& 
> input_items,
> @@ -57,6 +61,11 @@ int source_impl::general_work(int noutput_items,
>      const long timeout_us = 500000; // 0.5 sec
>      int nout = 0;
>  
> +    std::stringstream str;
> +    str << name() << unique_id();
> +    pmt::pmt_t _id = pmt::string_to_symbol(str.str());
> +    std::time_t time_now = std::time(nullptr);
> +
>      for (;;) {
>  
>          // No command handlers while reading
> @@ -66,6 +75,25 @@ int source_impl::general_work(int noutput_items,
>              result = d_device->readStream(
>                  d_stream, output_items.data(), noutput_items, flags, 
> time_ns, timeout_us);
>          }
> +        if (_tag_now) {
> +            _tag_now = false;
> +            // create a timestamp pmt for the first sample
> +            // TODO: use timestamp from radio hardware, and < 1s granularity?
> +            const pmt::pmt_t val =
> +                pmt::make_tuple(pmt::from_uint64(time_now),
> +                                pmt::from_double(0));
> +            // create a tag set for each channel
> +            for (size_t i = 0; i < 1; i++) { // TODO: get actual number of 
> channels.
> +                this->add_item_tag(i, nitems_written(0), TIME_KEY, val, _id);
> +                this->add_item_tag(
> +                    i, nitems_written(0), RATE_KEY,
> pmt::from_double(this->get_sample_rate(i)), _id);
> +                this->add_item_tag(i,
> +                                   nitems_written(0),
> +                                   FREQ_KEY,
> +                                   pmt::from_double(this->get_frequency(i)),
> +                                   _id);
> +            }
> +        }
>  
>          if (result >= 0) {
>              nout = result;
> 
>

smime.p7s
Description: S/MIME Cryptographic Signature