From ea4ac8865ef1731f12c6bf176de41abc84e37cf6 Mon Sep 17 00:00:00 2001 From: Ben Youngblood Date: Thu, 5 Dec 2024 09:50:00 -0700 Subject: [PATCH] Handle out-of-order receipt of Version Command Class Reports (#1027) --- lib/grizzly/command_handler.ex | 4 +- lib/grizzly/command_handlers/ack_response.ex | 2 +- .../command_handlers/aggregate_report.ex | 9 ++--- .../command_handlers/supervision_report.ex | 2 +- lib/grizzly/command_handlers/wait_report.ex | 27 ++++++++----- lib/grizzly/commands/command.ex | 2 +- lib/grizzly/zwave/command.ex | 12 +++++- .../commands/version_command_class_get.ex | 5 +++ .../aggregate_report_test.exs | 8 ++-- .../supervision_report_test.exs | 2 +- .../command_handlers/wait_report_test.exs | 39 +++++++++++++++++++ test/support/server/handler.ex | 37 +++++++++++++++++- 12 files changed, 122 insertions(+), 27 deletions(-) create mode 100644 test/grizzly/command_handlers/wait_report_test.exs diff --git a/lib/grizzly/command_handler.ex b/lib/grizzly/command_handler.ex index d76a7bcd..d21aa3c5 100644 --- a/lib/grizzly/command_handler.ex +++ b/lib/grizzly/command_handler.ex @@ -7,11 +7,11 @@ defmodule Grizzly.CommandHandler do @type handle_response :: {:complete, response :: any()} | {:continue, state :: any()} - @callback init(any()) :: {:ok, state :: any()} + @callback init(original_command :: Command.t(), init_state :: any()) :: {:ok, state :: any()} @callback handle_ack(state :: any()) :: handle_response() - @callback handle_command(Command.t(), state :: any()) :: handle_response() + @callback handle_command(incoming_command :: Command.t(), state :: any()) :: handle_response() @optional_callbacks handle_command: 2 end diff --git a/lib/grizzly/command_handlers/ack_response.ex b/lib/grizzly/command_handlers/ack_response.ex index 008d17b5..8afd6de0 100644 --- a/lib/grizzly/command_handlers/ack_response.ex +++ b/lib/grizzly/command_handlers/ack_response.ex @@ -6,7 +6,7 @@ defmodule Grizzly.CommandHandlers.AckResponse do @behaviour Grizzly.CommandHandler @impl Grizzly.CommandHandler - def init(_) do + def init(_, _) do {:ok, nil} end diff --git a/lib/grizzly/command_handlers/aggregate_report.ex b/lib/grizzly/command_handlers/aggregate_report.ex index 4bb70524..937f3d42 100644 --- a/lib/grizzly/command_handlers/aggregate_report.ex +++ b/lib/grizzly/command_handlers/aggregate_report.ex @@ -14,19 +14,18 @@ defmodule Grizzly.CommandHandlers.AggregateReport do @type opt :: {:complete_report, atom(), aggregate_param: atom()} - @spec init([opt]) :: {:ok, state()} - def init(opts) do + @impl Grizzly.CommandHandler + def init(_, opts) do report_name = Keyword.fetch!(opts, :complete_report) aggregate_param = Keyword.fetch!(opts, :aggregate_param) {:ok, %{complete_report: report_name, aggregate_param: aggregate_param, aggregates: []}} end - @spec handle_ack(state()) :: {:continue, state()} + @impl Grizzly.CommandHandler def handle_ack(state), do: {:continue, state} - @spec handle_command(Command.t(), state()) :: - {:continue, state} | {:complete, Command.t()} + @impl Grizzly.CommandHandler def handle_command(command, state) do if command.name == state.complete_report do do_handle_command(command, state) diff --git a/lib/grizzly/command_handlers/supervision_report.ex b/lib/grizzly/command_handlers/supervision_report.ex index 17f82dc9..d9cf7cb6 100644 --- a/lib/grizzly/command_handlers/supervision_report.ex +++ b/lib/grizzly/command_handlers/supervision_report.ex @@ -8,7 +8,7 @@ defmodule Grizzly.CommandHandlers.SupervisionReport do alias Grizzly.ZWave.Command @impl Grizzly.CommandHandler - def init(opts) do + def init(_, opts) do state = opts |> Enum.into(%{}) diff --git a/lib/grizzly/command_handlers/wait_report.ex b/lib/grizzly/command_handlers/wait_report.ex index 0e955622..00092785 100644 --- a/lib/grizzly/command_handlers/wait_report.ex +++ b/lib/grizzly/command_handlers/wait_report.ex @@ -7,23 +7,24 @@ defmodule Grizzly.CommandHandlers.WaitReport do alias Grizzly.ZWave.Command - @type state :: %{complete_report: atom()} + @type state :: %{complete_report: atom(), get_command: Command.t()} - @type opt :: {:complete_report, atom()} + @type opt :: {:complete_report, atom(), get_command: Command.t()} - @spec init([opt]) :: {:ok, state()} - def init(opts) do + @impl Grizzly.CommandHandler + def init(command, opts) do report_name = Keyword.fetch!(opts, :complete_report) - {:ok, %{complete_report: report_name}} + {:ok, %{complete_report: report_name, get_command: command}} end - @spec handle_ack(state()) :: {:continue, state()} + @impl Grizzly.CommandHandler def handle_ack(state), do: {:continue, state} - @spec handle_command(Command.t(), state()) :: - {:continue, state} | {:complete, Command.t()} + @impl Grizzly.CommandHandler def handle_command(command, state) do - expected? = not is_nil(command) and command.name == state.complete_report + expected? = + not is_nil(command) and command.name == state.complete_report and + report_matches_get?(state.get_command, command) if state.complete_report == :any or expected? do {:complete, command} @@ -31,4 +32,12 @@ defmodule Grizzly.CommandHandlers.WaitReport do {:continue, state} end end + + defp report_matches_get?(get, report) do + if function_exported?(get.impl, :report_matches_get?, 2) do + get.impl.report_matches_get?(get, report) + else + true + end + end end diff --git a/lib/grizzly/commands/command.ex b/lib/grizzly/commands/command.ex index c11b57f3..6e0d71d6 100644 --- a/lib/grizzly/commands/command.ex +++ b/lib/grizzly/commands/command.ex @@ -83,7 +83,7 @@ defmodule Grizzly.Commands.Command do {zwave_command, handler, handler_init_args, false, nil} end - {:ok, handler_state} = handler.init(handler_init_args) + {:ok, handler_state} = handler.init(zwave_command, handler_init_args) %__MODULE__{ handler: handler, diff --git a/lib/grizzly/zwave/command.ex b/lib/grizzly/zwave/command.ex index 8a37819d..b946b0a0 100644 --- a/lib/grizzly/zwave/command.ex +++ b/lib/grizzly/zwave/command.ex @@ -67,7 +67,17 @@ defmodule Grizzly.ZWave.Command do """ @callback decode_params(binary()) :: {:ok, keyword()} | {:error, DecodeError.t()} - @optional_callbacks encode_params: 2 + @doc """ + Returns true if the report is a good match for the get command. This is useful + for commands like Version Command Class Get, which can be sent in rapid succession + during a device interview, which can lead to reports getting matched back to the + wrong get. + + This is an optional callback. If not implemented, command handlers will assume true. + """ + @callback report_matches_get?(get :: t(), report :: t()) :: boolean() + + @optional_callbacks encode_params: 2, report_matches_get?: 2 @doc """ Encode the `Command.t()` into it's binary representation diff --git a/lib/grizzly/zwave/commands/version_command_class_get.ex b/lib/grizzly/zwave/commands/version_command_class_get.ex index b0cf498a..6b0a64dd 100644 --- a/lib/grizzly/zwave/commands/version_command_class_get.ex +++ b/lib/grizzly/zwave/commands/version_command_class_get.ex @@ -47,4 +47,9 @@ defmodule Grizzly.ZWave.Commands.VersionCommandClassGet do %DecodeError{value: cc_byte, param: :command_class, command: :version_command_class_get}} end end + + @impl Grizzly.ZWave.Command + def report_matches_get?(get, report) do + Command.param!(get, :command_class) == Command.param!(report, :command_class) + end end diff --git a/test/grizzly/command_handlers/aggregate_report_test.exs b/test/grizzly/command_handlers/aggregate_report_test.exs index f7cf5cf5..e06973c2 100644 --- a/test/grizzly/command_handlers/aggregate_report_test.exs +++ b/test/grizzly/command_handlers/aggregate_report_test.exs @@ -7,7 +7,7 @@ defmodule Grizzly.CommandHandlers.AggregateReportTest do test "when the waiting report has no reports to follow" do {:ok, state} = - AggregateReport.init(complete_report: :association_report, aggregate_param: :nodes) + AggregateReport.init(nil, complete_report: :association_report, aggregate_param: :nodes) {:ok, association_report} = AssociationReport.new( @@ -25,7 +25,7 @@ defmodule Grizzly.CommandHandlers.AggregateReportTest do test "when the waiting report is aggregated" do {:ok, state} = - AggregateReport.init(complete_report: :association_report, aggregate_param: :nodes) + AggregateReport.init(nil, complete_report: :association_report, aggregate_param: :nodes) {:ok, association_report_one} = AssociationReport.new( @@ -53,7 +53,7 @@ defmodule Grizzly.CommandHandlers.AggregateReportTest do test "when the waiting report has reports to follow" do {:ok, state} = - AggregateReport.init(complete_report: :association_report, aggregate_param: :nodes) + AggregateReport.init(nil, complete_report: :association_report, aggregate_param: :nodes) {:ok, association_report} = AssociationReport.new( @@ -71,7 +71,7 @@ defmodule Grizzly.CommandHandlers.AggregateReportTest do test "when different report is being handled than the one that is being waited on" do {:ok, state} = - AggregateReport.init(complete_report: :association_report, aggregate_param: :nodes) + AggregateReport.init(nil, complete_report: :association_report, aggregate_param: :nodes) {:ok, switch_binary_report} = SwitchBinaryReport.new(target_value: :on) diff --git a/test/grizzly/command_handlers/supervision_report_test.exs b/test/grizzly/command_handlers/supervision_report_test.exs index abb091e9..01dcffdf 100644 --- a/test/grizzly/command_handlers/supervision_report_test.exs +++ b/test/grizzly/command_handlers/supervision_report_test.exs @@ -8,7 +8,7 @@ defmodule Grizzly.CommandHandlers.SupervisionReportTest do ref = make_ref() {:ok, state} = - Handler.init( + Handler.init(nil, session_id: 1, waiter: {self(), nil}, command_ref: ref, diff --git a/test/grizzly/command_handlers/wait_report_test.exs b/test/grizzly/command_handlers/wait_report_test.exs new file mode 100644 index 00000000..36288577 --- /dev/null +++ b/test/grizzly/command_handlers/wait_report_test.exs @@ -0,0 +1,39 @@ +defmodule Grizzly.CommandHandlers.WaitReportTest do + use ExUnit.Case + + setup do + Grizzly.Trace.clear() + on_exit(fn -> Grizzly.Trace.clear() end) + end + + test "orders reports correctly when a get command implements report_matches_get?/2" do + task1 = + Task.async(fn -> + Grizzly.send_command(203, :version_command_class_get, command_class: :alarm) + end) + + task2 = + Task.async(fn -> + # wait 100ms before to ensure the first task completes first + Process.sleep(100) + Grizzly.send_command(203, :version_command_class_get, command_class: :door_lock) + end) + + [{:ok, %{command: report1}}, {:ok, %{command: report2}}] = Task.await_many([task1, task2]) + + assert [command_class: :alarm, version: 1] == report1.params + assert [command_class: :door_lock, version: 2] == report2.params + + literal_packets = + Grizzly.Trace.list() + |> Enum.map(&binary_slice(&1.binary, 7..-1//1)) + |> Enum.reject(&(&1 == "")) + + assert literal_packets == [ + <<134, 19, 113>>, + <<134, 19, 98>>, + <<134, 20, 98, 2>>, + <<134, 20, 113, 1>> + ] + end +end diff --git a/test/support/server/handler.ex b/test/support/server/handler.ex index e480aa27..db3acafd 100644 --- a/test/support/server/handler.ex +++ b/test/support/server/handler.ex @@ -21,14 +21,15 @@ defmodule GrizzlyTest.Server.Handler do FirmwareUpdateMDRequestReport, FirmwareUpdateMDGet, FirmwareUpdateMDStatusReport, - SupervisionReport + SupervisionReport, + VersionCommandClassReport } require Logger @impl ThousandIsland.Handler def handle_connection(_socket, _state) do - {:continue, %{node_id: nil, firmware_update_nack: true}} + {:continue, %{node_id: nil, firmware_update_nack: true, commands_received: []}} end @impl ThousandIsland.Handler @@ -43,6 +44,8 @@ defmodule GrizzlyTest.Server.Handler do def handle_data(data, socket, %{node_id: node_id} = state) do {:ok, zip_packet} = ZWave.from_binary(data) + state = %{state | commands_received: state.commands_received ++ [zip_packet]} + cond do zip_packet.name == :keep_alive -> handle_keep_alive(socket, zip_packet) @@ -86,6 +89,10 @@ defmodule GrizzlyTest.Server.Handler do # TODO only expect an update status report on the last fragment ) + # 203 waits until it receives two version gets, then sends the reports out of order + 203 -> + handle_version_get_command(socket, zip_packet, state) + # Node 301 is a long waiting inclusion meant to exercising stopping inclusion/exclusion 301 -> send_ack_response(socket, zip_packet) @@ -559,4 +566,30 @@ defmodule GrizzlyTest.Server.Handler do # the device asks for image fragments end + + defp handle_version_get_command(socket, zip_packet, state) do + send_ack_response(socket, zip_packet) + + if length(state.commands_received) == 2 do + Process.sleep(100) + + [first, second] = state.commands_received + + cc = second |> Command.param!(:command) |> Command.param!(:command_class) + {:ok, reply} = VersionCommandClassReport.new(command_class: cc, version: 2) + + {:ok, out_packet} = ZIPPacket.with_zwave_command(reply, Grizzly.SeqNumber.get_and_inc()) + :ok = Socket.send(socket, ZWave.to_binary(out_packet)) + + Process.sleep(100) + + cc = first |> Command.param!(:command) |> Command.param!(:command_class) + {:ok, reply} = VersionCommandClassReport.new(command_class: cc, version: 1) + + {:ok, out_packet} = ZIPPacket.with_zwave_command(reply, Grizzly.SeqNumber.get_and_inc()) + :ok = Socket.send(socket, ZWave.to_binary(out_packet)) + end + + state + end end