graph: Restore default resolver callback at node removal

The default resolve callback behavior for a newly-instantiated `node_t`
object resolves all dirty properties associated with the node, then
marks the properties as clean. When the node is added to a graph, its
resolver callback is updated to use the graph property propagation
algorithm in `graph_t::resolve_all_properties()`, which is considerably
more sophisticated and relies on the graph topology to do its work.

When a connection between two nodes is broken via the
`graph::disconnect()` method, nodes which no longer have incoming or
outgoing edges (connections) are removed from the graph. Prior to this
change, the removed node's resolver callback was left pointing at the
graph property propagation algorithm. In certain use cases, this could
result in unexpected client-facing behavior. Consider, for example, this
code (incomplete and for illustrative purposes only) which creates a
streamer on one transmit chain of a multi-channel device, destroys that
streamer, then creates a stream on the other transmit chain. Attempting
to set the TX rate on the first chain after destroying the streamer does
not result in the expected rate change, despite the same code working
correctly before creating the streamer:

    constexpr size_t CH0 = ..., CH1 = ...;

    uhd::usrp::multi_usrp::sptr usrp = uhd::usrp::multi_usrp::make(...);

    // Set a TX rate on both chains; this succeeds
    usrp->set_tx_rate(initial_rate, CH0);
    usrp->set_tx_rate(initial_rate, CH1);
    assert(initial_rate == usrp->get_tx_rate(CH0));
    assert(initial_rate == usrp->get_tx_rate(CH1));

    // Create a TX streamer for channel 0
    std::vector<size_t> chain0_chans{CH0};
    stream_args_t sa;
    sa.channels = chain0_chans;
    sa.otw_format = ...;
    sa.cpu_format = ...;

    uhd::tx_streamer::sptr txs = usrp->get_tx_stream(sa);

    // Destroy the first streamer (disconnecting the graph) and
    // create a streamer for channel 1
    txs.reset();

    std::vector<size_t> chain1_chans{CH1};
    sa.channels = chain1_chans;
    txs = usrp->get_tx_stream(sa);

    // Now try to set a new TX rate on both chains
    usrp->set_tx_rate(updated_rate, CH0);
    usrp->set_tx_rate(updated_rate, CH1);
    assert(updated_rate == usrp->get_tx_rate(CH0)); // <--- FAILS
    assert(updated_rate == usrp->get_tx_rate(CH1));

The reason this fails is because the second call to `set_tx_rate()` on
channel 0 internally sets the 'interp' (interpolation ratio) property on
the DUC node via the call to the DUC block controller's
`set_input_rate()` function. As the DUC node is no longer part of the
graph, having been removed from it when the first streamer instance was
destroyed, the graph property propagation algorithm doesn't 'see' the
node with the dirty property, and the 'interp' property resolver
callback is never invoked. As a result, the DUC's input rate property,
which depends on the interpolation ratio value, is never updated, and
thus calling the `get_tx_rate()` function to query the new rate of the
TX chain results in an unexpected value. In fact, in this particular
case, `set_tx_rate()` actually raises a warning that the TX rate
couldn't be set, and a message is printed to the console.

This commit remedies the situation by restoring the default resolve
callback behavior for a node when it is removed from the graph. This
allows the framework to be able to invoke the property resolver callback
on that node when a property is updated, the expected behavior of a
newly instantiated node.

4 files changed, 35 insertions, 1 deletions

diff --git a/host/include/uhd/rfnoc/node.hpp b/host/include/uhd/rfnoc/node.hpp
index 9d66c516a..0a49d1609 100644
--- a/host/include/uhd/rfnoc/node.hpp
+++ b/host/include/uhd/rfnoc/node.hpp
@@ -570,6 +570,13 @@ private:
         _resolve_all_cb = resolver;
     }
 
+    /*! Restores the default property resolution behavior of the node.
+     */
+    void clear_resolve_all_callback()
+    {
+        _resolve_all_cb = _default_resolve_all_cb;
+    }
+
     /*! Forward the value of an edge property into this node
      *
      * Note that \p incoming_prop is a reference to the neighbouring node's
@@ -655,7 +662,11 @@ private:
 
     //! A callback that can be called to notify the graph manager that something
     // has changed, and that a property resolution needs to be performed.
-    resolve_callback_t _resolve_all_cb = [this]() {
+    resolve_callback_t _resolve_all_cb;
+
+    //! This is the default implementation of the property resolution
+    // method.
+    const resolve_callback_t _default_resolve_all_cb = [this]() {
         resolve_props();
         clean_props();
     };
diff --git a/host/lib/include/uhdlib/rfnoc/node_accessor.hpp b/host/lib/include/uhdlib/rfnoc/node_accessor.hpp
index 4c63d29e2..5a9fd3a7f 100644
--- a/host/lib/include/uhdlib/rfnoc/node_accessor.hpp
+++ b/host/lib/include/uhdlib/rfnoc/node_accessor.hpp
@@ -69,6 +69,15 @@ public:
         node->set_resolve_all_callback(std::move(resolver));
     }
 
+    /* Restore the resolver callback to its default implementation
+     *
+     * See node_t::clear_resolve_all_callback() for details.
+     */
+    void clear_resolve_all_callback(node_t* node)
+    {
+        node->clear_resolve_all_callback();
+    }
+
     /*! Forward an edge property to \p dst_node
      *
      * See node_t::forward_edge_property() for details.
diff --git a/host/lib/rfnoc/graph.cpp b/host/lib/rfnoc/graph.cpp
index a26ed33fc..1fc60cf6e 100644
--- a/host/lib/rfnoc/graph.cpp
+++ b/host/lib/rfnoc/graph.cpp
@@ -183,11 +183,18 @@ void graph_t::disconnect(node_ref_t src_node, node_ref_t dst_node, graph_edge_t
 {
     std::lock_guard<std::recursive_mutex> l(_graph_mutex);
 
+    node_accessor_t node_accessor{};
+
     // Find vertex descriptor
     if (_node_map.count(src_node) == 0 && _node_map.count(dst_node) == 0) {
         return;
     }
 
+    UHD_LOG_TRACE(LOG_ID,
+        "Disconnecting block " << src_node->get_unique_id() << ":" << edge_info.src_port
+                               << " -> " << dst_node->get_unique_id() << ":"
+                               << edge_info.dst_port);
+
     auto src_vertex_desc = _node_map.at(src_node);
     auto dst_vertex_desc = _node_map.at(dst_node);
 
@@ -202,6 +209,9 @@ void graph_t::disconnect(node_ref_t src_node, node_ref_t dst_node, graph_edge_t
 
     if (boost::degree(src_vertex_desc, _graph) == 0) {
         _remove_node(src_node);
+        UHD_LOG_TRACE(LOG_ID,
+            "Removing block " << src_node->get_unique_id() << ":" << edge_info.src_port);
+        node_accessor.clear_resolve_all_callback(src_node);
     }
 
     // Re-look up the vertex descriptor for dst_node, as the act of removing
@@ -209,6 +219,9 @@ void graph_t::disconnect(node_ref_t src_node, node_ref_t dst_node, graph_edge_t
     dst_vertex_desc = _node_map.at(dst_node);
     if (boost::degree(dst_vertex_desc, _graph) == 0) {
         _remove_node(dst_node);
+        UHD_LOG_TRACE(LOG_ID,
+            "Removing block " << dst_node->get_unique_id() << ":" << edge_info.dst_port);
+        node_accessor.clear_resolve_all_callback(dst_node);
     }
 }
 
diff --git a/host/lib/rfnoc/node.cpp b/host/lib/rfnoc/node.cpp
index 062645b93..e7f166e93 100644
--- a/host/lib/rfnoc/node.cpp
+++ b/host/lib/rfnoc/node.cpp
@@ -19,6 +19,7 @@ dirtifier_t node_t::ALWAYS_DIRTY{};
 
 node_t::node_t()
 {
+    _resolve_all_cb = _default_resolve_all_cb;
     register_property(&ALWAYS_DIRTY);
 }