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
