uhd: stream: Bring back zero-latency recv() calls

This introduces a behavioural change in recv() (for RFNoC devices; older
devices are unaffected).

The behavioural changes depend on the way recv() was called:

- nsamps_per_buff > 0, timeout == 0: This is the busy-loop, or polling
  use case. This is typically done in application that require best-case
  latency.
  - OLD CODE: Before this change, the streamer would artificially
    increase the timeout value to 1ms (in order to increase the chance
    of catching an overrun or late error).
  - NEW CODE: After this change, the timeout of 0 is applied everywhere.
    This means that if a timeout occurs while calling recv(), it is not
    sufficient to call recv() again with a zero-timeout, zero-nsamps
    value (see next case) to detect what error occurred.
- nsamps_per_buff == 0, timeout == 0: Within niuhd, this was used to
  determine the error code if a regular call to recv() failed to
  receive all expected samples. The assumption was that calling recv()
  ones with these values (all zero) would suffice to figure out what the
  problem was (i.e., if an overrun or other error had occurred). However,
  this behaviour was never documented or declared to be valid!
  - OLD CODE: In the old code, the timeout was artifically increased to
    1ms, even if we would never process any packets. This was used to
    force a short "sleep" of the recv call, which would increase the
    chance that outstanding errors (overruns or lates) would be made
    available to the recv() thread. However, if a timeout would occur,
    then we would report back ERROR_CODE_NONE to the user, because they
    were requesting zero samples anyway.
  - NEW CODE: After the change, the timeout is not increased to 1ms any
    longer, but the timeout-scrubbing behaviour is retained. Because we
    don't artificially let the thread sleep, there is increased risk
    of missing outstanding errors. If the intention was to always catch
    those, call recv() with a non-zero timeout!
- nsamps_per_buff == 0, timeout > 0: This should be the right call to
  "catch" any outstanding errors (overruns, lates) with an arbitrary
  amount of waiting time.
  - OLD CODE: Ironically, before this change, the code would fix the
    timeout to 1ms, even if a bigger timeout was requested.
  - NEW CODE: The desired timeout is applied, even though no packets
    will be processed. This allows the call site, if they don't care
    about latency, to increase the sleep duration to a safe margin.
- nsamps_per_buff > 0, timeout > 0: This is the normal streaming case,
  and this remains unchanged.

On top of the changes to the RX streaming behaviour for RFNoC devices,
the documentation for recv() was made more detailed to better explain
the contract around recv().

Some background:

- In a2f10eed, a change was made to bump the minimum timeout to 1ms to
  increase the chance that errors are propagated back the call site.
  However, there was never a guarantee in the UHD API that recv() with
  zero samples, zero timeout would return all errors.
- This change had the effect that recv() would return with
  ERROR_CODE_TIMEOUT if nothing was available, which is also undesired.
  So in 0382a1c, a change was made to fix the timeout to 1ms for
  nsamps_per_buff == 0, regardless of the desired timeout.

CHANGELOG

@@ -7,6 +7,13 @@ Change Log for Releases
   - `uhd::math::gcd` and `uhd::math::lcm` are removed from the public
      headers. Use `std::gcd` and `std::lcm` instead if C++17 is available,
      or `boost::math::gcd` and `boost::math::lcm` if not.
+  - When calling `recv()` on an RX streamer connected to an RFNoC device, we
+    no longer artificially inflate the timeout value. The old behaviour was
+    in some cases abused to detect overruns, but this is not a reliable method
+    and is now explicitly declared unsupported.
+* Bug Fixes
+  - Calling `recv()` on an RX streamer connected to an RFNoC device with a
+    zero timeout value now correctly returns immediately.
 
 
 ## 004.008.000.000

host/include/uhd/stream.hpp

@@ -223,7 +223,7 @@ public:
      * code means that the device produced data faster than the application
      * could read, and various buffers filled up leaving no more space for the
      * device to write data to. Note that an overrun on the device will not
-     * immediatiely show up when calling recv(). Depending on the device
+     * immediately show up when calling recv(). Depending on the device
      * implementation, there may be many more valid samples available before the
      * device had to stop writing samples to the FIFO. Only when all valid
      * samples are returned to the call site will the error code be set to
@@ -232,6 +232,53 @@ public:
      * If the device is streaming continuously, it will reset itself when the
      * FIFO is cleared, and recv() can be called again to retrieve new, valid data.
      *
+     * \section stream_rx_timeouts Timeouts
+     *
+     * The recv() call has a timeout parameter. This parameter is used to limit
+     * the time the call will block. If no data is available within the timeout
+     * period, the call will return with an error code of
+     * error_code_t::ERROR_CODE_TIMEOUT. This is not necessarily an error
+     * condition, but can occur naturally, for example if the upstream source
+     * produces data in a bursty fashion.
+     *
+     * An important fact about the timeout parameter is that it is not a total
+     * timeout value, but is applied to every single call within recv() that
+     * uses a timeout. This means that the total time recv() can block can be
+     * significantly larger than the timeout parameter passed to recv().
+     *
+     * When the timeout is set to zero, recv() will attempt to return as fast
+     * as possible, to minimize latency. This is useful when the application
+     * polls for data in a busy loop. However, note that in this case, it is
+     * possible that the call will return with an error code of timeout, even
+     * though a different error condition occurred, but has not been fully
+     * processed. It is therefore not sufficient to call recv() with a timeout
+     * of zero to check for any outstanding error conditions.
+     *
+     * \section stream_rx_nsamps_zero Calling recv() with nsamps_per_buff = 0
+     *
+     * It is not forbidden to call recv() with nsamps_per_buff = 0, and this
+     * can be useful to retrieve metadata only. In this case, the call will
+     * still apply the timeout value internally (which means the recv() call
+     * will wait at least for the timeout value for any incoming data, even
+     * though it won't be processed). However, the call will never return an
+     * error code of ERROR_CODE_TIMEOUT, as no data is expected to be processed.
+     *
+     * The following code snippet demonstrates how to call recv() with a zero
+     * number of samples to retrieve metadata only:
+     * \code{.cpp}
+     * int num_samps_rcvd = rx_streamer->recv(
+     *     buffs, num_samps_expected, metadata, timeout);
+     * if (num_samps_rcvd < num_samps_expected &&
+     *     metadata.error_code != error_code_t::ERROR_CODE_NONE &&
+     *     metadata.error_code != error_code_t::ERROR_CODE_TIMEOUT) {
+     *     // If we reach this point, we know that an error occurred, but we
+     *     // don't know what it was.
+     *     rx_streamer->recv(buffs, 0, metadata, timeout);
+     *     // Now meta_data.error_code will contain the actual error code,
+     *     // if it was received within timeout.
+     * }
+     * \endcode
+     *
      * \param buffs a vector of writable memory to fill with samples
      * \param nsamps_per_buff the size of each buffer in number of samples
      * \param[out] metadata data to fill describing the buffer

host/lib/include/uhdlib/transport/rx_streamer_impl.hpp

@@ -281,7 +281,7 @@ private:
         const size_t buffer_offset_bytes = 0)
     {
         // A request to read zero samples should effectively be a no-op.
-        // However, in 2af10ee9, a change was made to increase the probability
+        // However, in a2f10ee, a change was made to increase the probability
         // but not guarantee that calling recv() after a radio overflow event
         // would return the overflow condition to the user. That change
         // introduced a side effect that a read of zero samples (assuming there
@@ -289,23 +289,22 @@ private:
         // timeout period and then return ERROR_CODE_TIMEOUT in the RX metadata
         // for the read. (Prior to this change, there was an explicit check for
         // a read of zero samples, which would return to the caller
-        // immediately.) This of course is undesirable--a request to read zero
-        // samples should always be fulfilled immediately, regardless of the
-        // availability of samples. Furthermore, reading zero samples is
-        // conventionally used to surface any stream errors, and it's that
-        // behavior we would like to preserve.
+        // immediately).
         //
-        // This change to call get_recv_buffs() with a zero timeout when
-        // nsamps_per_buff is zero is an attempt to achieve the best of both
-        // worlds. The call to get_recv_buffs() will surface any stream errors,
-        // but using a timeout of 0 means that we'll return as quickly as
-        // possible (with a maximum latency of 1ms; see
-        // rx_streamer_zero_copy.hpp, line 219 or so). If there's any stream
-        // error, it'll be returned in the metadata. However, if the stream
-        // error is ERROR_CODE_TIMEOUT, we'll simply swallow the error, thus
-        // preserving the old behavior.
+        // Now, if nsamps_per_buff is zero, we still honour the timeout, but
+        // will not process any outstanding packets, nor will we return an
+        // ERROR_CODE_TIMEOUT. If we did get a timeout, we make it look like
+        // an ERROR_CODE_NONE.
+        //
+        // The reason we still honour the timeout (even if we don't want to
+        // consume any streaming data) is to allow the user to detect stream
+        // errors. It is also fine to call recv() with a timeout of zero and
+        // nsamps_per_buff also set to zero, but then it's possible that we
+        // return ERROR_CODE_NONE because we just miss the stream error. If the
+        // application is polling recv(), then that is the expected behavior.
         if (nsamps_per_buff == 0) {
-            _zero_copy_streamer.get_recv_buffs(_in_buffs, metadata, eov_positions, 0);
+            _zero_copy_streamer.get_recv_buffs(
+                _in_buffs, metadata, eov_positions, timeout_ms);
             if (metadata.error_code == rx_metadata_t::ERROR_CODE_TIMEOUT) {
                 metadata.error_code = rx_metadata_t::ERROR_CODE_NONE;
             }

host/lib/include/uhdlib/transport/rx_streamer_zero_copy.hpp

@@ -216,7 +216,14 @@ public:
             if (!_stopped_due_to_overrun) {
                 // Packets were not available with zero timeout, wait for them
                 // to arrive using the specified timeout.
-                result = _get_aligned_buffs(std::max(1, timeout_ms));
+                // Note: If the specified timeout is also zero, then it's
+                // possible that an overrun or late occured, but we haven't
+                // been notified yet. That means recv() will return error code
+                // "timeout" instead of the appropriate error code, because it
+                // doesn't know the reason for the timeout.
+                // This is not a problem, because the user will call recv(),
+                // again, and the error code will be set correctly.
+                result = _get_aligned_buffs(timeout_ms);
             }
             if (result == get_aligned_buffs_t::TIMEOUT) {
                 if (_stopped_due_to_overrun) {

host/tests/rx_streamer_test.cpp

@@ -874,9 +874,7 @@ BOOST_AUTO_TEST_CASE(test_recv_two_channel_aggregate_eov)
     }
 }
 
-// A call to `recv()` of zero samples should return immediately, regardless of
-// the timeout parameter, and not return a timeout error despite the potential
-// absence of a packet on the wire.
+// A call to `recv()` of zero samples should not produce a timeout error.
 BOOST_AUTO_TEST_CASE(test_recv_zero_samples)
 {
     const std::string format("fc64");
@@ -889,7 +887,8 @@ BOOST_AUTO_TEST_CASE(test_recv_zero_samples)
 
     const auto start_time = std::chrono::steady_clock::now();
 
-    const size_t num_samps_ret = streamer->recv(buff.data(), 0, metadata, 10.0, false);
+    const size_t num_samps_ret = streamer->recv(buff.data(), 0, metadata, 1.0, false);
+    streamer->recv(buff.data(), 0, metadata, 0.0, false);
 
     const auto end_time = std::chrono::steady_clock::now();
     const std::chrono::duration<double> elapsed_time(end_time - start_time);
@@ -897,7 +896,7 @@ BOOST_AUTO_TEST_CASE(test_recv_zero_samples)
     BOOST_CHECK_EQUAL(num_samps_ret, 0);
     BOOST_CHECK_EQUAL(metadata.error_code, uhd::rx_metadata_t::ERROR_CODE_NONE);
 
-    // Ensure that the `recv()` of zero samples didn't wait the requested
-    // timeout period of 10 seconds.
-    BOOST_CHECK_LE(elapsed_time.count(), 0.5);
+    // Ensure that the `recv()` of zero samples waited for a bit longer than
+    // 1 seconds
+    BOOST_CHECK_LE(elapsed_time.count(), 1.5);
 }