From da5867cdb83a8f2266b29841e518c77754048eb8 Mon Sep 17 00:00:00 2001 From: eduardacoppo Date: Tue, 18 Feb 2025 09:31:13 -0300 Subject: [PATCH 1/9] feat: add fine-grained policy merge Previously, if a parameter was added to the designer-provided policy, no automated policy determination would be performed. That meant that the init flag would not be computed in this case. To fix this, a merge_policy function was implemented to merge the explicit policy (the designer-provided one) and the computed policy (init, for instance) The fix follows these rules: - if a value is in policy, use it; - otherwise use the value from computed_policy Also, if the type policy is set to data, the size policy must be removed, as its only meaningful for the type buffer and it causes the connection to fail. The Ruby method `merge` merges two hashes. According to the documentation: "Returns a new hash containing the contents of other_hash and the contents of hsh. If no block is specified, the value for entries with duplicate keys will be that of other_hash." In this case, to follow the rules of prioritizing the value from explicit_policy in case of key duplication, 'other_hash' is explicit_policy and 'hsh' is computed_policy --- .../network_generation/dataflow_dynamics.rb | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/syskit/network_generation/dataflow_dynamics.rb b/lib/syskit/network_generation/dataflow_dynamics.rb index 6b69b8bc3..03fd89f91 100644 --- a/lib/syskit/network_generation/dataflow_dynamics.rb +++ b/lib/syskit/network_generation/dataflow_dynamics.rb @@ -553,6 +553,16 @@ def compute_connection_policies policy_graph end + def merge_policy(explicit_policy, computed_policy) + merged_policy = computed_policy.merge(explicit_policy) + + if merged_policy[:type] == :data + merged_policy.delete(:size) + end + + merged_policy + end + # @api private # # Compute the policies for all connections starting from a given task @@ -563,13 +573,10 @@ def compute_policies_from(connection_graph, source_task, policy_graph = {}) mappings.each_with_object({}) do |(port_pair, policy), h| policy = policy.dup fallback_policy = policy.delete(:fallback_policy) - if policy.empty? - h[port_pair] = - policy_for(source_task, *port_pair, sink_task, - fallback_policy) - else - h[port_pair] = policy - end + computed_policy = policy_for( + source_task, *port_pair, sink_task, fallback_policy + ) + h[port_pair] = merge_policy(policy, computed_policy) end policy_graph[[source_task, sink_task]] = computed_policies end From ca2d39368284ea9ac965a5bc92eca91d1e4d1bd6 Mon Sep 17 00:00:00 2001 From: eduardacoppo Date: Tue, 18 Feb 2025 13:29:17 -0300 Subject: [PATCH 2/9] test: add tests and update existing --- .../test_dataflow_dynamics.rb | 67 ++++++++++++++++++- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/test/network_generation/test_dataflow_dynamics.rb b/test/network_generation/test_dataflow_dynamics.rb index b638b9856..db65349d6 100644 --- a/test/network_generation/test_dataflow_dynamics.rb +++ b/test/network_generation/test_dataflow_dynamics.rb @@ -298,7 +298,7 @@ module NetworkGeneration policy_graph[[cmp.c_child, task]][%w[out in]]) end - it "uses in-graph policies over the computed ones" do + it "merges in-graph policies with the computed ones" do plan.add(task0 = @task_m.new) plan.add(task1 = @task_m.new) @@ -307,10 +307,13 @@ module NetworkGeneration task0.out_port.connect_to(task1.in_port, type: :buffer, size: 42) - @dynamics.should_receive(:policy_for).never + @dynamics + .should_receive(:policy_for) + .with(task0, "out", "in", task1, nil) + .and_return(type: :buffer, size: 10, init: nil) policy_graph = @dynamics.compute_connection_policies - assert_equal({ type: :buffer, size: 42 }, + assert_equal({ type: :buffer, size: 42, init: nil }, policy_graph[[task0, task1]][%w[out in]]) end @@ -360,6 +363,64 @@ module NetworkGeneration add_agents(tasks[0, 2]) flexmock(@dynamics).should_receive(:propagate).with(tasks[0, 2]) end + + it "handles the case where the explicit policy sets the type to :data" do + plan.add(task0 = @task_m.new) + plan.add(task1 = @task_m.new) + + add_agents(tasks = [task0, task1]) + flexmock(@dynamics).should_receive(:propagate).with(tasks) + + task0.out_port.connect_to task1.in_port, type: :data + + flexmock(@dynamics) + .should_receive(:policy_for) + .with(task0, "out", "in", task1, nil) + .and_return(type: :buffer, size: 10, init: true) + + policy_graph = @dynamics.compute_connection_policies + expected_policy = { type: :data, init: true } + assert_equal(expected_policy, + policy_graph[[task0, task1]][%w[out in]]) + end + end + + describe "merge_policy" do + before do + @dynamics = NetworkGeneration::DataFlowDynamics.new(plan) + end + + it "merges policies by preferring explicit values over " \ + "computed values" do + explicit_policy = { type: :buffer, size: 20, init: true } + computed_policy = { type: :buffer, size: 10, init: true } + + merged_policy = + @dynamics.merge_policy(explicit_policy, computed_policy) + + assert_equal({ type: :buffer, size: 20, init: true }, merged_policy) + end + + it "removes the size value when the type is set to :data" do + explicit_policy = { type: :data, init: true } + computed_policy = { type: :buffer, size: 10, init: true } + + merged_policy = + @dynamics.merge_policy(explicit_policy, computed_policy) + + assert_equal({ type: :data, init: true }, merged_policy) + end + + it "falls back to computed values when explicit values " \ + "are not provided" do + explicit_policy = {} + computed_policy = { type: :buffer, size: 10, init: true } + + merged_policy = + @dynamics.merge_policy(explicit_policy, computed_policy) + + assert_equal({ type: :buffer, size: 10, init: true }, merged_policy) + end end describe "#policy_for" do From 9f5629ea550b9a0a7ec7a16ad8107bee92282a4c Mon Sep 17 00:00:00 2001 From: eduardacoppo Date: Fri, 7 Mar 2025 09:58:01 -0300 Subject: [PATCH 3/9] fix: split policy_for --- .../network_generation/dataflow_dynamics.rb | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/lib/syskit/network_generation/dataflow_dynamics.rb b/lib/syskit/network_generation/dataflow_dynamics.rb index 03fd89f91..e029c7cf8 100644 --- a/lib/syskit/network_generation/dataflow_dynamics.rb +++ b/lib/syskit/network_generation/dataflow_dynamics.rb @@ -583,10 +583,12 @@ def compute_policies_from(connection_graph, source_task, policy_graph = {}) policy_graph end - # Given the current knowledge about the port dynamics, returns the - # policy for the provided connection - def policy_for( - source_task, source_port_name, sink_port_name, sink_task, fallback_policy + def policy_default_init_flag(policy, model) + policy.merge(init: model.init_policy?) + end + + def policy_compute_data_element( + source_task, source_port_name, sink_task, sink_port_name, fallback_policy ) source_port = source_task.find_output_port(source_port_name) sink_port = sink_task.find_input_port(sink_port_name) @@ -625,9 +627,22 @@ def policy_for( "#{sink_port_m.required_connection_type} " \ "on #{sink_port}" end + policy + end + + def policy_for( + source_task, source_port_name, sink_port_name, sink_task, fallback_policy + ) + policy = {} + unless policy[:type] + policy = policy_compute_data_element( + source_task, source_port_name, sink_task, + sink_port_name, fallback_policy + ) + end - source_port_m = source_port.model - policy.merge(init: source_port_m.init_policy?) + model = source_task.find_output_port(source_port_name).model + policy_default_init_flag(policy, model) end def compute_reliable_connection_policy( From 9357a8888f79391e2e3852dda9120fb2106542b4 Mon Sep 17 00:00:00 2001 From: eduardacoppo Date: Fri, 7 Mar 2025 13:37:25 -0300 Subject: [PATCH 4/9] test: add tests for new functions --- .../test_dataflow_dynamics.rb | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/test/network_generation/test_dataflow_dynamics.rb b/test/network_generation/test_dataflow_dynamics.rb index db65349d6..0e4da9c7c 100644 --- a/test/network_generation/test_dataflow_dynamics.rb +++ b/test/network_generation/test_dataflow_dynamics.rb @@ -423,6 +423,93 @@ module NetworkGeneration end end + describe "#policy_compute_data_element" do + before do + @task_m = Syskit::TaskContext.new_submodel do + input_port "in", "/double" + output_port "out", "/double" + end + @source_task_m = @task_m.new_submodel + @sink_task_m = @task_m.new_submodel + plan.add(@source_t = @source_task_m.new) + plan.add(@sink_t = @sink_task_m.new) + end + + it "returns a data connection by default" do + policy = @dynamics.policy_compute_data_element( + @source_t, "out", @sink_t, "in", nil + ) + assert_equal :data, policy[:type] + end + + it "returns a buffer connection of size 1 if the sink's " \ + "required connection type is 'buffer'" do + @sink_task_m.in_port.needs_buffered_connection + policy = @dynamics.policy_compute_data_element( + @source_t, "out", @sink_t, "in", nil + ) + assert_equal :buffer, policy[:type] + assert_equal 1, policy[:size] + end + + it "raises if the required connection type is unknown " \ + "and needs_reliable_connection is not set" do + flexmock(@sink_task_m.in_port) + .should_receive(:required_connection_type).explicitly + .and_return(:something) + assert_raises(UnsupportedConnectionType) do + @dynamics.policy_compute_data_element( + @source_t, "out", @sink_t, "in", nil + ) + end + end + + it "returns the value from compute_reliable_connection_policy " \ + "if the sink port is marked as needs_reliable_connection" do + @sink_task_m.in_port.needs_reliable_connection + fallback_policy = flexmock + expected_policy = flexmock + + expected_policy + .should_receive(:merge) + .and_return(expected_policy) + + flexmock(@dynamics) + .should_receive(:compute_reliable_connection_policy) + .with(@source_t.out_port, @sink_t.in_port, fallback_policy) + .once.and_return(expected_policy) + policy = @dynamics.policy_compute_data_element( + @source_t, "out", @sink_t, "in", fallback_policy + ) + assert_equal expected_policy, policy + end + end + + describe "#policy_default_init_flag" do + before do + @task_m = Syskit::TaskContext.new_submodel do + input_port "in", "/double" + output_port "out", "/double" + end + @source_task_m = @task_m.new_submodel + @sink_task_m = @task_m.new_submodel + plan.add(@source_t = @source_task_m.new) + plan.add(@sink_t = @sink_task_m.new) + end + + it "merges the init flag from the source port's model" do + flexmock(@source_t.out_port.model) + .should_receive(:init_policy?).explicitly + .and_return(true) + + policy = { type: :data } + updated_policy = @dynamics.policy_default_init_flag( + policy, @source_t.out_port.model + ) + assert updated_policy[:init] + end + end + describe "#policy_for" do before do @task_m = Syskit::TaskContext.new_submodel do @@ -526,6 +613,17 @@ module NetworkGeneration assert policy[:init] end + + it "always applies the init flag from the source port's model" do + flexmock(@source_t.out_port.model) + .should_receive(:init_policy?).explicitly + .and_return(true) + + policy = @dynamics.policy_for( + @source_t, "out", "in", @sink_t, nil + ) + assert policy[:init] + end end describe "#compute_reliable_connection_policy" do From 852fce1d11518f56b6bd8ddd3f8059a4e7e1e457 Mon Sep 17 00:00:00 2001 From: eduardacoppo Date: Fri, 14 Mar 2025 17:38:42 -0300 Subject: [PATCH 5/9] fix: merge explicit and computed policies renamed variables and simplified code --- .../network_generation/dataflow_dynamics.rb | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/lib/syskit/network_generation/dataflow_dynamics.rb b/lib/syskit/network_generation/dataflow_dynamics.rb index e029c7cf8..f98aa974b 100644 --- a/lib/syskit/network_generation/dataflow_dynamics.rb +++ b/lib/syskit/network_generation/dataflow_dynamics.rb @@ -571,12 +571,12 @@ def compute_policies_from(connection_graph, source_task, policy_graph = {}) mappings = connection_graph.edge_info(source_task, sink_task) computed_policies = mappings.each_with_object({}) do |(port_pair, policy), h| - policy = policy.dup - fallback_policy = policy.delete(:fallback_policy) - computed_policy = policy_for( - source_task, *port_pair, sink_task, fallback_policy + explicit_policy = policy.dup + fallback_policy = explicit_policy.delete(:fallback_policy) + h[port_pair] = policy_for( + source_task, *port_pair, sink_task, + fallback_policy, explicit_policy: explicit_policy ) - h[port_pair] = merge_policy(policy, computed_policy) end policy_graph[[source_task, sink_task]] = computed_policies end @@ -631,18 +631,23 @@ def policy_compute_data_element( end def policy_for( - source_task, source_port_name, sink_port_name, sink_task, fallback_policy + source_task, source_port_name, sink_port_name, sink_task, + fallback_policy, explicit_policy: {} ) - policy = {} - unless policy[:type] - policy = policy_compute_data_element( - source_task, source_port_name, sink_task, - sink_port_name, fallback_policy - ) - end + computed_policy = + if explicit_policy[:type] + {} + else + policy_compute_data_element( + source_task, source_port_name, sink_task, + sink_port_name, fallback_policy + ) + end model = source_task.find_output_port(source_port_name).model - policy_default_init_flag(policy, model) + computed_policy = policy_default_init_flag(computed_policy, model) + + merge_policy(explicit_policy, computed_policy) end def compute_reliable_connection_policy( From d3e55da6c47728f3abd6567babbfb57983695d9a Mon Sep 17 00:00:00 2001 From: eduardacoppo Date: Fri, 14 Mar 2025 17:39:13 -0300 Subject: [PATCH 6/9] chore: add missing init arg --- test/network_generation/test_engine.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/network_generation/test_engine.rb b/test/network_generation/test_engine.rb index 0270ca9f7..afcbb2f0f 100644 --- a/test/network_generation/test_engine.rb +++ b/test/network_generation/test_engine.rb @@ -838,7 +838,7 @@ def deploy_dev_and_bus syskit_configure(cmp) assert_equal( - { %w[out in] => { type: :buffer, size: 20 } }, + { %w[out in] => { type: :buffer, size: 20, init: nil } }, RequiredDataFlow.edge_info(cmp.source_child, cmp.sink_child) ) end From b648041b01d365b773d850574c39e301bf25cf02 Mon Sep 17 00:00:00 2001 From: eduardacoppo Date: Fri, 14 Mar 2025 17:43:35 -0300 Subject: [PATCH 7/9] fix/test: update and fix the merge tests - Updated test expectations - Updated mock expectations - Added test to make sure the size is not overridden when an explicit policy specifies `type: :buffer` - Fixed the merging logic in the reliable connection policy test, properly verifying that `merge_policy` is called instead of directly modifying the expected policy About this last one. I'm not sure if that's the best way to do it. Tried to think of a way of simplifying it, but the test expects this chain of mocks. --- .../test_dataflow_dynamics.rb | 75 ++++++++++++++----- 1 file changed, 55 insertions(+), 20 deletions(-) diff --git a/test/network_generation/test_dataflow_dynamics.rb b/test/network_generation/test_dataflow_dynamics.rb index 0e4da9c7c..ff6eb1b1e 100644 --- a/test/network_generation/test_dataflow_dynamics.rb +++ b/test/network_generation/test_dataflow_dynamics.rb @@ -232,7 +232,8 @@ module NetworkGeneration task0.out_port.connect_to(task1.in_port) @dynamics.should_receive(:policy_for) - .with(task0, "out", "in", task1, nil) + .with(task0, "out", "in", task1, nil, + explicit_policy: {}) .and_return(type: :buffer, size: 42) policy_graph = @dynamics.compute_connection_policies @@ -252,7 +253,8 @@ module NetworkGeneration task0.out_port.connect_to(task1.in_port) @dynamics.should_receive(:policy_for) - .with(task0, "out", "in", task1, nil) + .with(task0, "out", "in", task1, nil, + explicit_policy: {}) .and_return(type: :buffer, size: 42, init: true) policy_graph = @dynamics.compute_connection_policies @@ -272,7 +274,8 @@ module NetworkGeneration task0.out_port.connect_to(task1.in_port) @dynamics.should_receive(:policy_for) - .with(task0, "out", "in", task1, nil) + .with(task0, "out", "in", task1, nil, + explicit_policy: {}) .and_return(type: :buffer, size: 42, init: false) policy_graph = @dynamics.compute_connection_policies @@ -290,7 +293,8 @@ module NetworkGeneration cmp.c_child.out_port.connect_to(task.in_port) @dynamics.should_receive(:policy_for) - .with(cmp.c_child, "out", "in", task, nil) + .with(cmp.c_child, "out", "in", task, nil, + explicit_policy: {}) .and_return(type: :buffer, size: 42) policy_graph = @dynamics.compute_connection_policies @@ -308,7 +312,7 @@ module NetworkGeneration task0.out_port.connect_to(task1.in_port, type: :buffer, size: 42) @dynamics - .should_receive(:policy_for) + .should_receive(:policy_compute_data_element) .with(task0, "out", "in", task1, nil) .and_return(type: :buffer, size: 10, init: nil) policy_graph = @dynamics.compute_connection_policies @@ -329,12 +333,13 @@ module NetworkGeneration ) @dynamics.should_receive(:policy_for) - .with(task0, "out", "in", task1, { type: :data }) - .and_return(type: :buffer, size: 42) + .with(task0, "out", "in", task1, { type: :data }, + explicit_policy: {}) + .and_return(type: :buffer, size: 42, init: nil) policy_graph = @dynamics.compute_connection_policies - assert_equal({ type: :buffer, size: 42 }, + assert_equal({ type: :buffer, size: 42, init: nil }, policy_graph[[task0, task1]][%w[out in]]) end @@ -351,9 +356,11 @@ module NetworkGeneration fallback_policy: { type: :data } ) - @dynamics.should_receive(:policy_for).never + @dynamics.should_receive(:policy_compute_data_element) + .with(task0, "out", "in", task1, { type: :data }) + .and_return({ type: :buffer, size: 42 }) policy_graph = @dynamics.compute_connection_policies - assert_equal({ type: :buffer, size: 42 }, + assert_equal({ type: :buffer, size: 42, init: nil }, policy_graph[[task0, task1]][%w[out in]]) end @@ -374,12 +381,31 @@ module NetworkGeneration task0.out_port.connect_to task1.in_port, type: :data flexmock(@dynamics) - .should_receive(:policy_for) - .with(task0, "out", "in", task1, nil) - .and_return(type: :buffer, size: 10, init: true) + .should_receive(:policy_compute_data_element) + .never + + policy_graph = @dynamics.compute_connection_policies + expected_policy = { type: :data, init: nil } + assert_equal(expected_policy, + policy_graph[[task0, task1]][%w[out in]]) + end + + it "makes sure the size is not overridden when explicit policy " \ + "sets the type to :buffer" do + plan.add(task0 = @task_m.new) + plan.add(task1 = @task_m.new) + + add_agents(tasks = [task0, task1]) + flexmock(@dynamics).should_receive(:propagate).with(tasks) + + task0.out_port.connect_to task1.in_port, type: :buffer, size: 42 + + flexmock(@dynamics) + .should_receive(:policy_compute_data_element) + .never policy_graph = @dynamics.compute_connection_policies - expected_policy = { type: :data, init: true } + expected_policy = { type: :buffer, size: 42, init: nil } assert_equal(expected_policy, policy_graph[[task0, task1]][%w[out in]]) end @@ -568,20 +594,29 @@ module NetworkGeneration "the sink port is marked as needs_reliable_connection" do @sink_task_m.in_port.needs_reliable_connection fallback_policy = flexmock - expected_policy = flexmock + connection_policy = flexmock + computed_policy = flexmock + merged_policy = flexmock - expected_policy - .should_receive(:merge) - .and_return(expected_policy) + flexmock(@dynamics) + .should_receive(:merge_policy) + .with({}, computed_policy) + .and_return(merged_policy) + + flexmock(@dynamics) + .should_receive(:policy_default_init_flag) + .with(connection_policy, @source_t.out_port.model) + .once.and_return(computed_policy) flexmock(@dynamics) .should_receive(:compute_reliable_connection_policy) .with(@source_t.out_port, @sink_t.in_port, fallback_policy) - .once.and_return(expected_policy) + .once.and_return(connection_policy) + policy = @dynamics.policy_for( @source_t, "out", "in", @sink_t, fallback_policy ) - assert_equal expected_policy, policy + assert_equal merged_policy, policy end it "merges init policy when sink requires reliable connection" do From 3a872dee5f73131c08b1e36ded074ed90e296cd6 Mon Sep 17 00:00:00 2001 From: eduardacoppo Date: Wed, 26 Mar 2025 09:31:29 -0300 Subject: [PATCH 8/9] chore: use positional arg with default value --- .../network_generation/dataflow_dynamics.rb | 4 ++-- test/network_generation/test_dataflow_dynamics.rb | 15 +++++---------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/lib/syskit/network_generation/dataflow_dynamics.rb b/lib/syskit/network_generation/dataflow_dynamics.rb index f98aa974b..0f357a56c 100644 --- a/lib/syskit/network_generation/dataflow_dynamics.rb +++ b/lib/syskit/network_generation/dataflow_dynamics.rb @@ -575,7 +575,7 @@ def compute_policies_from(connection_graph, source_task, policy_graph = {}) fallback_policy = explicit_policy.delete(:fallback_policy) h[port_pair] = policy_for( source_task, *port_pair, sink_task, - fallback_policy, explicit_policy: explicit_policy + fallback_policy, explicit_policy ) end policy_graph[[source_task, sink_task]] = computed_policies @@ -632,7 +632,7 @@ def policy_compute_data_element( def policy_for( source_task, source_port_name, sink_port_name, sink_task, - fallback_policy, explicit_policy: {} + fallback_policy, explicit_policy = {} ) computed_policy = if explicit_policy[:type] diff --git a/test/network_generation/test_dataflow_dynamics.rb b/test/network_generation/test_dataflow_dynamics.rb index ff6eb1b1e..e8d46f7f6 100644 --- a/test/network_generation/test_dataflow_dynamics.rb +++ b/test/network_generation/test_dataflow_dynamics.rb @@ -232,8 +232,7 @@ module NetworkGeneration task0.out_port.connect_to(task1.in_port) @dynamics.should_receive(:policy_for) - .with(task0, "out", "in", task1, nil, - explicit_policy: {}) + .with(task0, "out", "in", task1, nil, {}) .and_return(type: :buffer, size: 42) policy_graph = @dynamics.compute_connection_policies @@ -253,8 +252,7 @@ module NetworkGeneration task0.out_port.connect_to(task1.in_port) @dynamics.should_receive(:policy_for) - .with(task0, "out", "in", task1, nil, - explicit_policy: {}) + .with(task0, "out", "in", task1, nil, {}) .and_return(type: :buffer, size: 42, init: true) policy_graph = @dynamics.compute_connection_policies @@ -274,8 +272,7 @@ module NetworkGeneration task0.out_port.connect_to(task1.in_port) @dynamics.should_receive(:policy_for) - .with(task0, "out", "in", task1, nil, - explicit_policy: {}) + .with(task0, "out", "in", task1, nil, {}) .and_return(type: :buffer, size: 42, init: false) policy_graph = @dynamics.compute_connection_policies @@ -293,8 +290,7 @@ module NetworkGeneration cmp.c_child.out_port.connect_to(task.in_port) @dynamics.should_receive(:policy_for) - .with(cmp.c_child, "out", "in", task, nil, - explicit_policy: {}) + .with(cmp.c_child, "out", "in", task, nil, {}) .and_return(type: :buffer, size: 42) policy_graph = @dynamics.compute_connection_policies @@ -333,8 +329,7 @@ module NetworkGeneration ) @dynamics.should_receive(:policy_for) - .with(task0, "out", "in", task1, { type: :data }, - explicit_policy: {}) + .with(task0, "out", "in", task1, { type: :data }, {}) .and_return(type: :buffer, size: 42, init: nil) policy_graph = @dynamics.compute_connection_policies From cbd87b7ccdd06c6fc136638425855fbce2cb8655 Mon Sep 17 00:00:00 2001 From: Sylvain Date: Fri, 20 Jun 2025 07:00:58 -0300 Subject: [PATCH 9/9] chore: minor cleanup for clarity Removed the one-line, one-use policy_default_init_flag method, and changed the argument order of merge_policy to match the hash merge (i.e. computed, explicit instead of the opposite) --- .../network_generation/dataflow_dynamics.rb | 24 ++--- .../test_dataflow_dynamics.rb | 100 ++++-------------- 2 files changed, 30 insertions(+), 94 deletions(-) diff --git a/lib/syskit/network_generation/dataflow_dynamics.rb b/lib/syskit/network_generation/dataflow_dynamics.rb index 0f357a56c..a6267a593 100644 --- a/lib/syskit/network_generation/dataflow_dynamics.rb +++ b/lib/syskit/network_generation/dataflow_dynamics.rb @@ -553,13 +553,15 @@ def compute_connection_policies policy_graph end - def merge_policy(explicit_policy, computed_policy) - merged_policy = computed_policy.merge(explicit_policy) - - if merged_policy[:type] == :data - merged_policy.delete(:size) - end - + # Merges two connection policies + # + # @param [Hash] default_policy the policy that has the lowest + # priority + # @param [Hash] explicit_policy the policy that has the highest + # priority + def merge_policy(default_policy, explicit_policy) + merged_policy = default_policy.merge(explicit_policy) + merged_policy.delete(:size) if merged_policy[:type] == :data merged_policy end @@ -583,10 +585,6 @@ def compute_policies_from(connection_graph, source_task, policy_graph = {}) policy_graph end - def policy_default_init_flag(policy, model) - policy.merge(init: model.init_policy?) - end - def policy_compute_data_element( source_task, source_port_name, sink_task, sink_port_name, fallback_policy ) @@ -645,9 +643,9 @@ def policy_for( end model = source_task.find_output_port(source_port_name).model - computed_policy = policy_default_init_flag(computed_policy, model) + computed_policy[:init] = model.init_policy - merge_policy(explicit_policy, computed_policy) + merge_policy(computed_policy, explicit_policy) end def compute_reliable_connection_policy( diff --git a/test/network_generation/test_dataflow_dynamics.rb b/test/network_generation/test_dataflow_dynamics.rb index e8d46f7f6..9dded2e99 100644 --- a/test/network_generation/test_dataflow_dynamics.rb +++ b/test/network_generation/test_dataflow_dynamics.rb @@ -414,33 +414,33 @@ module NetworkGeneration it "merges policies by preferring explicit values over " \ "computed values" do explicit_policy = { type: :buffer, size: 20, init: true } - computed_policy = { type: :buffer, size: 10, init: true } + computed_policy = { type: :buffer, size: 10, init: false } merged_policy = - @dynamics.merge_policy(explicit_policy, computed_policy) + @dynamics.merge_policy(computed_policy, explicit_policy) assert_equal({ type: :buffer, size: 20, init: true }, merged_policy) end it "removes the size value when the type is set to :data" do explicit_policy = { type: :data, init: true } - computed_policy = { type: :buffer, size: 10, init: true } + computed_policy = { type: :buffer, size: 10, init: false } merged_policy = - @dynamics.merge_policy(explicit_policy, computed_policy) + @dynamics.merge_policy(computed_policy, explicit_policy) assert_equal({ type: :data, init: true }, merged_policy) end it "falls back to computed values when explicit values " \ "are not provided" do - explicit_policy = {} + explicit_policy = { init: false } computed_policy = { type: :buffer, size: 10, init: true } merged_policy = - @dynamics.merge_policy(explicit_policy, computed_policy) + @dynamics.merge_policy(computed_policy, explicit_policy) - assert_equal({ type: :buffer, size: 10, init: true }, merged_policy) + assert_equal({ type: :buffer, size: 10, init: false }, merged_policy) end end @@ -506,31 +506,6 @@ module NetworkGeneration end end - describe "#policy_default_init_flag" do - before do - @task_m = Syskit::TaskContext.new_submodel do - input_port "in", "/double" - output_port "out", "/double" - end - @source_task_m = @task_m.new_submodel - @sink_task_m = @task_m.new_submodel - plan.add(@source_t = @source_task_m.new) - plan.add(@sink_t = @sink_task_m.new) - end - - it "merges the init flag from the source port's model" do - flexmock(@source_t.out_port.model) - .should_receive(:init_policy?).explicitly - .and_return(true) - - policy = { type: :data } - updated_policy = @dynamics.policy_default_init_flag( - policy, @source_t.out_port.model - ) - assert updated_policy[:init] - end - end - describe "#policy_for" do before do @task_m = Syskit::TaskContext.new_submodel do @@ -588,71 +563,34 @@ module NetworkGeneration it "returns the value from compute_reliable_connection_policy if " \ "the sink port is marked as needs_reliable_connection" do @sink_task_m.in_port.needs_reliable_connection - fallback_policy = flexmock - connection_policy = flexmock - computed_policy = flexmock - merged_policy = flexmock - - flexmock(@dynamics) - .should_receive(:merge_policy) - .with({}, computed_policy) - .and_return(merged_policy) - - flexmock(@dynamics) - .should_receive(:policy_default_init_flag) - .with(connection_policy, @source_t.out_port.model) - .once.and_return(computed_policy) - flexmock(@dynamics) .should_receive(:compute_reliable_connection_policy) - .with(@source_t.out_port, @sink_t.in_port, fallback_policy) - .once.and_return(connection_policy) + .with(@source_t.out_port, @sink_t.in_port, { stage: "fallback" }) + .once.and_return({ stage: "reliable" }) policy = @dynamics.policy_for( - @source_t, "out", "in", @sink_t, fallback_policy + @source_t, "out", "in", @sink_t, { stage: "fallback" } ) - assert_equal merged_policy, policy + assert_equal({ stage: "reliable", init: nil }, policy) end - it "merges init policy when sink requires reliable connection" do - @sink_task_m.in_port.needs_reliable_connection - @source_t.out_port.model.init_policy(true) - fallback_policy = flexmock - - flexmock(@dynamics) - .should_receive(:compute_reliable_connection_policy) - .with(@source_t.out_port, @sink_t.in_port, fallback_policy) - .once.and_return({}) - - policy = @dynamics.policy_for( - @source_t, "out", "in", @sink_t, fallback_policy - ) - - assert policy[:init] - end - - it "merges init policy when sink requires 'buffer' connection type" do - @sink_task_m.in_port.needs_buffered_connection - + it "uses the init flag from the source port's model by default" do flexmock(@source_t.out_port.model) .should_receive(:init_policy?).explicitly .and_return(true) - @source_t.out_port.model.init_policy(true) - policy = @dynamics.policy_for(@source_t, "out", "in", @sink_t, nil) - + policy = @dynamics.policy_for( + @source_t, "out", "in", @sink_t, nil + ) assert policy[:init] end - it "always applies the init flag from the source port's model" do - flexmock(@source_t.out_port.model) - .should_receive(:init_policy?).explicitly - .and_return(true) - + it "uses the explicitly given init flag over the one from the port model" do + @source_t.out_port.model.init_policy(true) policy = @dynamics.policy_for( - @source_t, "out", "in", @sink_t, nil + @source_t, "out", "in", @sink_t, {}, { init: false } ) - assert policy[:init] + refute policy[:init] end end