From 204a76410d88ce9122d78c5ead2bd5fe7eeb8ec5 Mon Sep 17 00:00:00 2001 From: Ben Coppock Date: Sat, 15 Nov 2025 12:01:08 -0800 Subject: [PATCH] Always provide metadata to encoding functions This struct enables encoding when it's necessary to know the number of registers/bits being encoded into. It also allows better error messages since we provide the name of the mapping. --- lib/modboss.ex | 27 ++++--------- lib/modboss/encoding.ex | 65 +++++++++++++++++--------------- lib/modboss/encoding/metadata.ex | 25 ++++++++++++ lib/modboss/mapping.ex | 11 ++++-- test/modboss/encoding_test.exs | 56 ++++++++++++++------------- test/modboss_test.exs | 4 +- 6 files changed, 105 insertions(+), 83 deletions(-) create mode 100644 lib/modboss/encoding/metadata.ex diff --git a/lib/modboss.ex b/lib/modboss.ex index 8e68617..2b275f3 100644 --- a/lib/modboss.ex +++ b/lib/modboss.ex @@ -363,24 +363,12 @@ defmodule ModBoss do defp get_encode_mfa(%Mapping{as: {module, as}} = mapping) do function = String.to_atom("encode_" <> "#{as}") - # In an effort to keep the API simple, when we call a user-defined encode function, - # we only pass the value to be encoded. - # - # However, when calling built-in encoding functions, we pass both the value to be encoded - # _and_ the mapping. We do this because in some cases we need to know how many objects - # we're encoding for in order to provide truly generic encoders. For example, when encoding - # a string to ASCII, we may need to add padding to fill out the mapped objects. - arguments = - case module do - ModBoss.Encoding -> [mapping.value, mapping] - _other -> [mapping.value] - end - - if exists?(module, function, length(arguments)) do - {module, function, arguments} + if exists?(module, function, 2) do + metadata = ModBoss.Encoding.Metadata.from_mapping(mapping) + {module, function, [mapping.value, metadata]} else {:error, - "Modbus mapping #{inspect(mapping.name)} expected #{inspect(module)} to define #{inspect(function)}, but it did not."} + "Expected #{inspect(module)}.#{function}/2 to be defined for ModBoss mapping #{inspect(mapping.name)}."} end end @@ -409,13 +397,12 @@ defmodule ModBoss do defp get_decode_mfa(%Mapping{as: {module, as}} = mapping) do function = String.to_atom("decode_" <> "#{as}") - arguments = [mapping.encoded_value] - if exists?(module, function, length(arguments)) do - {module, function, arguments} + if exists?(module, function, 1) do + {module, function, [mapping.encoded_value]} else {:error, - "Modbus mapping #{inspect(mapping.name)} expected #{inspect(module)} to define #{inspect(function)}, but it did not."} + "Expected #{inspect(module)}.#{function}/1 to be defined for ModBoss mapping #{inspect(mapping.name)}."} end end diff --git a/lib/modboss/encoding.ex b/lib/modboss/encoding.ex index 7ac8ca8..f13a506 100644 --- a/lib/modboss/encoding.ex +++ b/lib/modboss/encoding.ex @@ -17,9 +17,8 @@ defmodule ModBoss.Encoding do when providing your own `encode_*` functions, you'll only be passed the value to be encoded (and not the mapping). """ - alias ModBoss.Mapping - import Bitwise + alias ModBoss.Encoding.Metadata @doc false def encode_raw(value_or_values, _), do: {:ok, value_or_values} @@ -38,9 +37,9 @@ defmodule ModBoss.Encoding do iex> encode_boolean(false, %{}) {:ok, 0} """ - @spec encode_boolean(boolean(), Mapping.t()) :: {:ok, integer()} | {:error, binary()} - def encode_boolean(true, _mapping), do: {:ok, 1} - def encode_boolean(false, _mapping), do: {:ok, 0} + @spec encode_boolean(boolean(), Metadata.t()) :: {:ok, integer()} | {:error, binary()} + def encode_boolean(true, _meta), do: {:ok, 1} + def encode_boolean(false, _meta), do: {:ok, 0} @doc """ Interpret `1` as `true` and `0` as `false` @@ -70,12 +69,12 @@ defmodule ModBoss.Encoding do iex> {:ok, 65_535} = encode_unsigned_int(65_535, %{}) iex> {:error, _too_large} = encode_unsigned_int(65_536, %{}) """ - @spec encode_unsigned_int(integer(), Mapping.t()) :: {:ok, integer()} | {:error, binary()} - def encode_unsigned_int(value, _mapping) when value >= 0 and value <= 65_535 do + @spec encode_unsigned_int(integer(), Metadata.t()) :: {:ok, integer()} | {:error, binary()} + def encode_unsigned_int(value, _meta) when value >= 0 and value <= 65_535 do {:ok, value} end - def encode_unsigned_int(value, _mapping) do + def encode_unsigned_int(value, _meta) do {:error, "Value #{value} is outside the range of a 16-bit unsigned integer (0 to 65,535)"} end @@ -106,13 +105,13 @@ defmodule ModBoss.Encoding do iex> {:ok, -32_768} = encode_signed_int(-32_768, %{}) iex> {:error, _too_large} = encode_signed_int(32_768, %{}) """ - @spec encode_signed_int(integer(), Mapping.t()) :: {:ok, integer()} | {:error, binary()} - def encode_signed_int(value, _mapping) + @spec encode_signed_int(integer(), Metadata.t()) :: {:ok, integer()} | {:error, binary()} + def encode_signed_int(value, _meta) when is_integer(value) and value >= -32768 and value <= 32767 do {:ok, value} end - def encode_signed_int(value, _mapping) do + def encode_signed_int(value, _meta) do {:error, "Value #{value} is outside the range of a 16-bit signed integer (-32768 to 32767)"} end @@ -143,34 +142,32 @@ defmodule ModBoss.Encoding do ## Examples - iex> encode_ascii("Hi!", %ModBoss.Mapping{address_count: 3}) + iex> metadata = %ModBoss.Encoding.Metadata{type: :holding_register, address_count: 3} + iex> encode_ascii("Hi!", metadata) {:ok, [18537, 8448, 0]} - iex> {:error, _too_many_characters} = encode_ascii("Hi!", %ModBoss.Mapping{address_count: 1}) + iex> metadata = %ModBoss.Encoding.Metadata{type: :holding_register, address_count: 1} + iex> {:error, reason} = encode_ascii("Hi!", metadata) + iex> String.match?(reason, ~r/Too many characters/) + true """ - @spec encode_ascii(binary(), Mapping.t()) :: {:ok, list(integer())} | {:error, binary()} - def encode_ascii(text, %Mapping{} = mapping) do - with :ok <- verify_ascii(text), + @spec encode_ascii(binary(), Metadata.t()) :: {:ok, list(integer())} | {:error, binary()} + def encode_ascii(text, %Metadata{type: :holding_register} = meta) when is_binary(text) do + with :ok <- verify_ascii(text, meta), {:ok, chars} <- get_chars(text), - {:ok, padded_chars} <- pad(chars, mapping) do - do_encode_ascii(padded_chars) + {:ok, padded_chars} <- pad(chars, text, meta), + registers <- chars_to_registers(padded_chars) do + {:ok, registers} end end - defp do_encode_ascii(chars) when is_list(chars) do - chars - |> Enum.chunk_every(2, 2) - |> Enum.map(fn [upper_byte, lower_byte] -> (upper_byte <<< 8) + lower_byte end) - |> then(&{:ok, &1}) - end - - defp verify_ascii(text) do + defp verify_ascii(text, %Metadata{name: name}) do text |> String.to_charlist() |> Enum.all?(fn c -> c >= 0 and c < 127 end) |> case do true -> :ok - false -> {:error, "Text contains non-ASCII characters."} + false -> {:error, "Non-ASCII characters in #{text} for ModBoss mapping #{inspect(name)}."} end end @@ -178,14 +175,14 @@ defmodule ModBoss.Encoding do {:ok, :binary.bin_to_list(text)} end - defp pad(chars, mapping) do - max_chars = mapping.address_count * 2 + defp pad(chars, text, %Metadata{name: name, address_count: address_count}) do + max_chars = address_count * 2 pad_count = max_chars - length(chars) if pad_count < 0 do message = """ - Text for #{inspect(mapping.name)} contains too many characters. \ - With #{mapping.address_count} registers, it can hold up to #{max_chars} ASCII characters.\ + Too many characters in #{inspect(text)} for ModBoss mapping #{inspect(name)}. \ + With #{address_count} registers, it can hold up to #{max_chars} ASCII characters.\ """ {:error, message} @@ -194,6 +191,12 @@ defmodule ModBoss.Encoding do end end + defp chars_to_registers(chars) when is_list(chars) and rem(length(chars), 2) == 0 do + chars + |> Enum.chunk_every(2, 2) + |> Enum.map(fn [upper_byte, lower_byte] -> (upper_byte <<< 8) + lower_byte end) + end + @doc """ Decode integer `value`(s) representing ASCII characters to text diff --git a/lib/modboss/encoding/metadata.ex b/lib/modboss/encoding/metadata.ex new file mode 100644 index 0000000..5547597 --- /dev/null +++ b/lib/modboss/encoding/metadata.ex @@ -0,0 +1,25 @@ +defmodule ModBoss.Encoding.Metadata do + @moduledoc """ + Metadata to support encoding + + Sometimes it's essential to know how many registers you're encoding a value + to fit into. For example, when encoding ASCII text, you might choose to pad + unused bits with zeros. See the implementation of + `ModBoss.Encoding.encode_ascii/2` for an example. + """ + + alias ModBoss.Mapping + + @type t() :: %__MODULE__{ + name: Mapping.name(), + type: Mapping.type(), + address_count: pos_integer() + } + + defstruct [:name, :type, :address_count] + + @doc false + def from_mapping(%Mapping{} = mapping) do + struct!(__MODULE__, Map.take(mapping, [:name, :type, :address_count])) + end +end diff --git a/lib/modboss/mapping.ex b/lib/modboss/mapping.ex index f233071..d6b1e0d 100644 --- a/lib/modboss/mapping.ex +++ b/lib/modboss/mapping.ex @@ -3,12 +3,15 @@ defmodule ModBoss.Mapping do Struct representing the Modbus mapping. """ + @type name() :: atom() + @type type() :: :holding_register | :input_register | :coil | :discrete_input + @type t() :: %__MODULE__{ - name: atom(), - type: :holding_register | :input_register | :coil | :discrete_input, + name: name(), + type: type(), addresses: Range.t(), - starting_address: integer(), - address_count: integer(), + starting_address: non_neg_integer(), + address_count: pos_integer(), as: atom() | {module(), atom()}, value: any(), encoded_value: integer() | [integer()], diff --git a/test/modboss/encoding_test.exs b/test/modboss/encoding_test.exs index 745e7e0..3e7a388 100644 --- a/test/modboss/encoding_test.exs +++ b/test/modboss/encoding_test.exs @@ -3,13 +3,13 @@ defmodule ModBoss.EncodingTest do doctest ModBoss.Encoding, import: true alias ModBoss.Encoding - alias ModBoss.Mapping + alias ModBoss.Encoding.Metadata describe "raw" do test "doesn't change the value when encoding" do - assert Encoding.encode_raw(123, %Mapping{}) == {:ok, 123} - assert Encoding.encode_raw(:abc, %Mapping{}) == {:ok, :abc} - assert Encoding.encode_raw([1, 2, 3], %Mapping{}) == {:ok, [1, 2, 3]} + assert Encoding.encode_raw(123, %Metadata{}) == {:ok, 123} + assert Encoding.encode_raw(:abc, %Metadata{}) == {:ok, :abc} + assert Encoding.encode_raw([1, 2, 3], %Metadata{}) == {:ok, [1, 2, 3]} end test "doesn't change the value when decoding" do @@ -21,8 +21,8 @@ defmodule ModBoss.EncodingTest do describe "boolean" do test "encodes true and 1 and false as 0" do - assert Encoding.encode_boolean(true, %Mapping{}) == {:ok, 1} - assert Encoding.encode_boolean(false, %Mapping{}) == {:ok, 0} + assert Encoding.encode_boolean(true, %Metadata{}) == {:ok, 1} + assert Encoding.encode_boolean(false, %Metadata{}) == {:ok, 0} end test "decodes 1 as true and 0 as false" do @@ -33,14 +33,14 @@ defmodule ModBoss.EncodingTest do describe "unsigned_int" do test "returns the value if within the range for 16 bits when encoding" do - assert Encoding.encode_unsigned_int(0, %Mapping{}) == {:ok, 0} - assert Encoding.encode_unsigned_int(65535, %Mapping{}) == {:ok, 65535} + assert Encoding.encode_unsigned_int(0, %Metadata{}) == {:ok, 0} + assert Encoding.encode_unsigned_int(65535, %Metadata{}) == {:ok, 65535} end test "returns an error for out-of-range values when encoding" do - assert {:error, _} = Encoding.encode_unsigned_int(-1, %Mapping{}) - assert {:error, _} = Encoding.encode_unsigned_int(65536, %Mapping{}) - assert {:error, _} = Encoding.encode_unsigned_int(:foo, %Mapping{}) + assert {:error, _} = Encoding.encode_unsigned_int(-1, %Metadata{}) + assert {:error, _} = Encoding.encode_unsigned_int(65536, %Metadata{}) + assert {:error, _} = Encoding.encode_unsigned_int(:foo, %Metadata{}) end test "returns the value when decoding" do @@ -51,7 +51,7 @@ defmodule ModBoss.EncodingTest do test "roundtrips as expected" do [0, 65535] |> Enum.each(fn number -> - {:ok, encoded} = Encoding.encode_unsigned_int(number, %Mapping{}) + {:ok, encoded} = Encoding.encode_unsigned_int(number, %Metadata{}) {:ok, decoded} = Encoding.decode_unsigned_int(encoded) assert number == decoded end) @@ -60,13 +60,13 @@ defmodule ModBoss.EncodingTest do describe "signed_int" do test "returns the value if within the range for 16 bits when encoding" do - assert {:ok, -32768} = Encoding.encode_signed_int(-32768, %Mapping{}) - assert {:ok, 32767} = Encoding.encode_signed_int(32767, %Mapping{}) + assert {:ok, -32768} = Encoding.encode_signed_int(-32768, %Metadata{}) + assert {:ok, 32767} = Encoding.encode_signed_int(32767, %Metadata{}) end test "returns error for out-of-range signed integers when encoding" do - assert {:error, _} = Encoding.encode_signed_int(-32769, %Mapping{}) - assert {:error, _} = Encoding.encode_signed_int(32768, %Mapping{}) + assert {:error, _} = Encoding.encode_signed_int(-32769, %Metadata{}) + assert {:error, _} = Encoding.encode_signed_int(32768, %Metadata{}) end test "interprets the value as a signed int when decoding" do @@ -78,7 +78,7 @@ defmodule ModBoss.EncodingTest do test "roundtrips as expected" do [-32768, 0, 32767] |> Enum.each(fn initial -> - {:ok, encoded} = Encoding.encode_signed_int(initial, %Mapping{}) + {:ok, encoded} = Encoding.encode_signed_int(initial, %Metadata{}) {:ok, decoded} = Encoding.decode_signed_int(encoded) assert initial == decoded end) @@ -87,20 +87,23 @@ defmodule ModBoss.EncodingTest do describe "ascii" do test "encodes ASCII text to the expected number of register values" do - assert {:ok, [0, 0, 0]} == Encoding.encode_ascii("", %Mapping{address_count: 3}) - assert {:ok, [0x4100]} == Encoding.encode_ascii("A", %Mapping{address_count: 1}) - assert {:ok, [0x4142]} == Encoding.encode_ascii("AB", %Mapping{address_count: 1}) - assert {:ok, [0x4142, 0x4300]} == Encoding.encode_ascii("ABC", %Mapping{address_count: 2}) + meta = %Metadata{name: :my_mapping, type: :holding_register} + + assert {:ok, [0, 0, 0]} == Encoding.encode_ascii("", %{meta | address_count: 3}) + assert {:ok, [0x4100]} == Encoding.encode_ascii("A", %{meta | address_count: 1}) + assert {:ok, [0x4142]} == Encoding.encode_ascii("AB", %{meta | address_count: 1}) + assert {:ok, [0x4142, 0x4300]} == Encoding.encode_ascii("ABC", %{meta | address_count: 2}) end test "returns an error when attempting to encode more characters than what the mapped registers can hold" do - mapping = %Mapping{name: :my_mapping, address_count: 2} - assert {:error, message} = Encoding.encode_ascii("ABCDE", mapping) - assert String.match?(message, ~r/too many characters/) + meta = %Metadata{name: :some_mapping_name, type: :holding_register, address_count: 2} + assert {:error, message} = Encoding.encode_ascii("ABCDE", meta) + assert String.match?(message, ~r/Too many characters/) end test "returns an error when attempting to encode non-ASCII text" do - assert {:error, message} = Encoding.encode_ascii("José", %Mapping{address_count: 2}) + meta = %Metadata{name: :my_mapping, type: :holding_register, address_count: 2} + assert {:error, message} = Encoding.encode_ascii("José", meta) assert String.match?(message, ~r/non-ASCII/i) end @@ -117,7 +120,8 @@ defmodule ModBoss.EncodingTest do test "roundtrips as expected" do [{"", 1}, {"A", 1}, {"AB", 2}, {"ABC", 2}, {"ABCD", 2}, {"A", 4}] |> Enum.each(fn {text, count} -> - {:ok, encoded} = Encoding.encode_ascii(text, %Mapping{name: :foo, address_count: count}) + meta = %Metadata{name: :my_mapping, type: :holding_register, address_count: count} + {:ok, encoded} = Encoding.encode_ascii(text, meta) {:ok, decoded} = Encoding.decode_ascii(encoded) assert text == decoded end) diff --git a/test/modboss_test.exs b/test/modboss_test.exs index 73c82be..4275a65 100644 --- a/test/modboss_test.exs +++ b/test/modboss_test.exs @@ -518,8 +518,8 @@ defmodule ModBossTest do holding_register 3..5, :text, as: {Encoding, :ascii}, mode: :w end - def encode_boolean(false), do: {:ok, 0} - def encode_boolean(true), do: {:ok, 1} + def encode_boolean(false, _), do: {:ok, 0} + def encode_boolean(true, _), do: {:ok, 1} end """)