rfnoc: node: Fix resolution of properties with circular dependencies

When a node has multiple properties that depend on each other (and
possible have circular dependencies), the previous version of property
propagation would not correctly resolve properties that got flagged
dirty during the execution of other resolvers.

2 files changed, 111 insertions, 41 deletions

diff --git a/host/lib/rfnoc/node.cpp b/host/lib/rfnoc/node.cpp
index cc2d7b7a9..33649b23f 100644
--- a/host/lib/rfnoc/node.cpp
+++ b/host/lib/rfnoc/node.cpp
@@ -72,25 +72,22 @@ void node_t::clear_command_time(const size_t instance)
 void node_t::register_property(property_base_t* prop, resolve_callback_t&& clean_callback)
 {
     std::lock_guard<std::mutex> _l(_prop_mutex);
-
     const auto src_type          = prop->get_src_info().type;
-    auto prop_already_registered = [prop](const property_base_t* existing_prop) {
-        return (prop->get_src_info() == existing_prop->get_src_info()
-                   && prop->get_id() == existing_prop->get_id())
-               || (prop == existing_prop);
-    };
 
     // If the map is empty for this source type, create an empty vector
     if (_props.count(src_type) == 0) {
         _props[src_type] = {};
     }
 
-    // Now go and make sure no one has registered this property before
-    auto& props = _props[src_type];
-    for (const auto& existing_prop : props) {
-        if (prop_already_registered(existing_prop)) {
-            throw uhd::runtime_error("Attempting to double-register prop");
-        }
+    auto prop_already_registered = [prop](const property_base_t* existing_prop) {
+        return (prop == existing_prop) ||
+            (prop->get_src_info() == existing_prop->get_src_info()
+                   && prop->get_id() == existing_prop->get_id());
+    };
+    if (!filter_props(prop_already_registered).empty()) {
+        throw uhd::runtime_error(std::string("Attempting to double-register property: ")
+                                 + prop->get_id() + "[" + prop->get_src_info().to_string()
+                                 + "]");
     }
 
     _props[src_type].push_back(prop);
@@ -347,43 +344,56 @@ void node_t::init_props()
 void node_t::resolve_props()
 {
     prop_accessor_t prop_accessor{};
-    const prop_ptrs_t dirty_props =
+    const prop_ptrs_t initial_dirty_props =
         filter_props([](property_base_t* prop) { return prop->is_dirty(); });
+    std::list<property_base_t*> all_dirty_props(
+        initial_dirty_props.cbegin(), initial_dirty_props.cend());
+    prop_ptrs_t processed_props{};
     prop_ptrs_t written_props{};
-    UHD_LOG_TRACE(get_unique_id(),
-        "Locally resolving " << dirty_props.size() << " dirty properties.");
-
-    // Helper to determine if any element from inputs is in dirty_props
-    auto in_dirty_props = [&dirty_props](const prop_ptrs_t inputs) {
-        return std::any_of(
-            inputs.cbegin(), inputs.cend(), [&dirty_props](property_base_t* prop) {
-                return dirty_props.count(prop) != 1;
-            });
-    };
-
-    for (auto& resolver_tuple : _prop_resolvers) {
-        auto& inputs  = std::get<0>(resolver_tuple);
-        auto& outputs = std::get<1>(resolver_tuple);
-        if (in_dirty_props(inputs)) {
+    RFNOC_LOG_TRACE("Locally resolving " << all_dirty_props.size()
+                                         << " dirty properties plus dependencies.");
+
+    // Loop through all dirty properties. The list can be amended during the
+    // loop execution.
+    for (auto it = all_dirty_props.begin(); it != all_dirty_props.end(); ++it) {
+        auto current_input_prop = *it;
+        if (processed_props.count(current_input_prop)) {
             continue;
         }
+        // Find all resolvers that take this dirty property as an input:
+        for (auto& resolver_tuple : _prop_resolvers) {
+            auto& inputs  = std::get<0>(resolver_tuple);
+            auto& outputs = std::get<1>(resolver_tuple);
+            if (!inputs.count(current_input_prop)) {
+                continue;
+            }
 
-        // Enable outputs
-        std::vector<uhd::utils::scope_exit::uptr> access_holder;
-        access_holder.reserve(outputs.size());
-        for (auto& output : outputs) {
-            access_holder.emplace_back(prop_accessor.get_scoped_prop_access(*output,
-                written_props.count(output) ? property_base_t::access_t::RWLOCKED
-                                            : property_base_t::access_t::RW));
-        }
+            // Enable outputs
+            std::vector<uhd::utils::scope_exit::uptr> access_holder;
+            access_holder.reserve(outputs.size());
+            for (auto& output : outputs) {
+                access_holder.emplace_back(prop_accessor.get_scoped_prop_access(*output,
+                    written_props.count(output) ? property_base_t::access_t::RWLOCKED
+                                                : property_base_t::access_t::RW));
+            }
 
-        // Run resolver
-        std::get<2>(resolver_tuple)();
+            // Run resolver
+            std::get<2>(resolver_tuple)();
 
-        // Take note of outputs
-        written_props.insert(outputs.cbegin(), outputs.cend());
+            // Take note of outputs
+            written_props.insert(outputs.cbegin(), outputs.cend());
 
-        // RW or RWLOCKED gets released here as access_holder goes out of scope.
+            // Add all outputs that are dirty to the list, unless they have
+            // already been processed
+            for (auto& output_prop : outputs) {
+                if (output_prop->is_dirty() && processed_props.count(output_prop) == 0) {
+                    all_dirty_props.push_back(output_prop);
+                }
+            }
+
+            // RW or RWLOCKED gets released here as access_holder goes out of scope.
+        }
+        processed_props.insert(current_input_prop);
     }
 }
 
diff --git a/host/tests/rfnoc_propprop_test.cpp b/host/tests/rfnoc_propprop_test.cpp
index 1b9b94b05..20d7d96f5 100644
--- a/host/tests/rfnoc_propprop_test.cpp
+++ b/host/tests/rfnoc_propprop_test.cpp
@@ -111,6 +111,52 @@ private:
     property_t<double> _out{"out", 2.0, {res_source_info::OUTPUT_EDGE}};
 };
 
+/*! Mock node, circular prop deps
+ */
+class mock_circular_prop_node_t : public node_t
+{
+public:
+    mock_circular_prop_node_t()
+    {
+        register_property(&_x1);
+        register_property(&_x2);
+        register_property(&_x4);
+
+        add_property_resolver({&_x1}, {&_x2}, [this]() {
+            RFNOC_LOG_INFO("Calling resolver for _x1");
+            _x2 = 2.0 * _x1.get();
+        });
+        add_property_resolver({&_x2}, {&_x4}, [this]() {
+            RFNOC_LOG_INFO("Calling resolver for _x2");
+            _x4 = 2.0 * _x2.get();
+        });
+        add_property_resolver({&_x4}, {&_x1}, [this]() {
+            RFNOC_LOG_INFO("Calling resolver for _x4");
+            _x1 = _x4.get() / 4.0;
+        });
+    }
+
+    size_t get_num_input_ports() const
+    {
+        return 1;
+    }
+
+    size_t get_num_output_ports() const
+    {
+        return 1;
+    }
+
+    std::string get_unique_id() const
+    {
+        return "MOCK_CIRCULAR_PROPS";
+    }
+
+    property_t<double> _x1{"x1", 1.0, {res_source_info::USER}};
+    property_t<double> _x2{"x2", 2.0, {res_source_info::USER}};
+    property_t<double> _x4{"x4", 4.0, {res_source_info::USER}};
+};
+
+
 // Do some sanity checks on the mock just so we don't get surprised later
 BOOST_AUTO_TEST_CASE(test_mock)
 {
@@ -364,3 +410,17 @@ BOOST_AUTO_TEST_CASE(test_graph_crisscross_fifo)
     UHD_LOG_INFO("TEST", "Now testing criss-cross prop resolution");
     graph.initialize();
 }
+
+BOOST_AUTO_TEST_CASE(test_circular_deps)
+{
+    node_accessor_t node_accessor{};
+    // Define some mock nodes:
+    // Source radios
+    mock_circular_prop_node_t mock_circular_prop_node{};
+
+    // These init calls would normally be done by the framework
+    node_accessor.init_props(&mock_circular_prop_node);
+
+    mock_circular_prop_node.set_property<double>("x1", 5.0, 0);
+    BOOST_CHECK_EQUAL(mock_circular_prop_node.get_property<double>("x4"), 4 * 5.0);
+}