From cdcdcf5fb2308ba1dd27fbb3c8a28f6e92e13a37 Mon Sep 17 00:00:00 2001 From: Aaron Rossetto Date: Tue, 17 Nov 2020 12:18:49 -0600 Subject: 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 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 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. --- host/lib/include/uhdlib/rfnoc/node_accessor.hpp | 9 +++++++++ host/lib/rfnoc/graph.cpp | 13 +++++++++++++ host/lib/rfnoc/node.cpp | 1 + 3 files changed, 23 insertions(+) (limited to 'host/lib') 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 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); } -- cgit v1.2.3