Handle out-of-order receipt of Version Command Class Reports (#1027)

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

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
 

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)

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(%{})

lib/grizzly/command_handlers/wait_report.ex

@@ -7,28 +7,37 @@ 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}
     else
       {: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

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,

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

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

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)
 

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,

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

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