From f8a7e8c467bf08c58913f1bcdd4c7395a31f4a0a Mon Sep 17 00:00:00 2001 From: Ben Coppock Date: Sat, 26 Jul 2025 14:56:17 -0700 Subject: [PATCH 1/2] Return error if decoding a mapping errors --- lib/modboss.ex | 9 ++++++--- test/modboss_test.exs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/lib/modboss.ex b/lib/modboss.ex index 44db071..b787ac1 100644 --- a/lib/modboss.ex +++ b/lib/modboss.ex @@ -106,7 +106,7 @@ defmodule ModBoss do iex> ModBoss.encode(MyDevice.Schema, foo: "Yay") {:ok, %{{:holding_register, 15} => 22881, {:holding_register, 16} => 30976}} """ - @spec encode(module(), values()) :: {:ok, map()} | {:error, any()} + @spec encode(module(), values()) :: {:ok, map()} | {:error, String.t()} def encode(module, values) when is_atom(module) do with {:ok, mappings} <- get_mappings(:any, module, get_keys(values)), mappings <- put_values(mappings, values), @@ -383,13 +383,16 @@ defmodule ModBoss do end end - @spec decode([Mapping.t()]) :: {:ok, [Mapping.t()]} - defp decode(mappings) do + @spec maybe_decode([Mapping.t()]) :: {:ok, [Mapping.t()]} | {:error, String.t()} Enum.reduce_while(mappings, {:ok, []}, fn mapping, {:ok, acc} -> case decode_value(mapping) do {:ok, decoded_value} -> updated_mapping = %{mapping | value: decoded_value} {:cont, {:ok, [updated_mapping | acc]}} + + {:error, error} -> + message = "Failed to decode #{inspect(mapping.name)}. #{error}" + {:halt, {:error, message}} end end) end diff --git a/test/modboss_test.exs b/test/modboss_test.exs index 52b274c..b91b260 100644 --- a/test/modboss_test.exs +++ b/test/modboss_test.exs @@ -330,6 +330,37 @@ defmodule ModBossTest do ModBoss.read(schema, read_func(device), [:yep, :nope, :text]) end + test "returns an error if decoding fails" do + schema = unique_module() + + Code.compile_string(""" + defmodule #{schema} do + use ModBoss.Schema + + modbus_schema do + # Assumes the function lives in the current module… + holding_register 1, :yep, as: :boolean + holding_register 2, :nope, as: :boolean + end + + def decode_boolean(0), do: {:ok, false} + def decode_boolean(1), do: {:ok, true} + def decode_boolean(value), do: {:error, "Not sure what to do with \#{value}."} + end + """) + + device = start_supervised!({Agent, fn -> @initial_state end}) + + # only 1 and 2 are values expected to be decoded, so this will break… + set_objects(device, %{ + 1 => 1, + 2 => 33 + }) + + message = "Failed to decode :nope. Not sure what to do with 33." + assert {:error, ^message} = ModBoss.read(schema, read_func(device), [:yep, :nope]) + end + test "allows reading of 'raw' values" do schema = unique_module() From a126a98ed4167e9795318dd0bf167cd34a86a864 Mon Sep 17 00:00:00 2001 From: Ben Coppock Date: Sat, 26 Jul 2025 15:08:52 -0700 Subject: [PATCH 2/2] Skip decode logic if `decode: false` is passed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because it was slightly simpler, we were just always decoding before, and then we were returning the raw or decoded value depending on what had been requested. But there may be instances where the decode logic is failing to handle a value, and it may be beneficial to _not_ run the decode logic at all…hence this change. --- lib/modboss.ex | 16 ++++++++++------ test/modboss_test.exs | 28 ++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/lib/modboss.ex b/lib/modboss.ex index b787ac1..8e68617 100644 --- a/lib/modboss.ex +++ b/lib/modboss.ex @@ -67,10 +67,13 @@ defmodule ModBoss do names when is_list(names) -> {names, :plural} end + should_decode = Keyword.get(opts, :decode, true) + field_to_return = if should_decode, do: :value, else: :encoded_value + with {:ok, mappings} <- get_mappings(:readable, module, names), {:ok, mappings} <- read_mappings(module, mappings, read_func), - {:ok, mappings} <- decode(mappings) do - collect_results(mappings, plurality, opts) + {:ok, mappings} <- maybe_decode(mappings, should_decode) do + collect_results(mappings, plurality, field_to_return) end end @@ -80,9 +83,7 @@ defmodule ModBoss do |> Enum.map(fn {name, _mapping} -> name end) end - defp collect_results(mappings, plurality, opts) do - field_to_return = if Keyword.get(opts, :decode, true), do: :value, else: :encoded_value - + defp collect_results(mappings, plurality, field_to_return) do mappings |> Enum.map(&{&1.name, Map.get(&1, field_to_return)}) |> then(fn results -> @@ -383,7 +384,10 @@ defmodule ModBoss do end end - @spec maybe_decode([Mapping.t()]) :: {:ok, [Mapping.t()]} | {:error, String.t()} + @spec maybe_decode([Mapping.t()], boolean()) :: {:ok, [Mapping.t()]} | {:error, String.t()} + defp maybe_decode(mappings, false), do: {:ok, mappings} + + defp maybe_decode(mappings, true) do Enum.reduce_while(mappings, {:ok, []}, fn mapping, {:ok, acc} -> case decode_value(mapping) do {:ok, decoded_value} -> diff --git a/test/modboss_test.exs b/test/modboss_test.exs index b91b260..73c82be 100644 --- a/test/modboss_test.exs +++ b/test/modboss_test.exs @@ -396,6 +396,34 @@ defmodule ModBossTest do ModBoss.read(schema, read_func(device), [:yep, :nope, :text], decode: false) end + test "bypasses (potentially-buggy) decode logic when asked to return raw values" do + schema = unique_module() + + Code.compile_string(""" + defmodule #{schema} do + use ModBoss.Schema + + modbus_schema do + holding_register 1, :yep, as: :boolean + holding_register 2, :nope, as: :boolean + end + + def decode_boolean(0), do: raise "bam!" + def decode_boolean(1), do: raise "kapow!" + end + """) + + device = start_supervised!({Agent, fn -> @initial_state end}) + + set_objects(device, %{ + 1 => 1, + 2 => 0 + }) + + assert {:ok, %{yep: 1, nope: 0}} = + ModBoss.read(schema, read_func(device), [:yep, :nope], decode: false) + end + test "fetches all readable mappings if told to read `:all`" do schema = unique_module()