SwitchEndianness causes unexpected side effect.

[iamalexcater]

The generic array argument values have their endianness reversed when calling WriteMultipleRegistersAsync or ReadWriteMultipleRegistersAsync<TRead, TWrite> when SwapBytes is true. This is arguably undesirable with reference to the Principle Of Least Astonishment.

[Apollo3zehn]

Thanks for your input! Endianness handling in this library has caused much confusion and it needs a total redesign. Maybe something like ReadHoldingRegisters(...).Slice(2, 8).Cast<long>().SwitchEndianness();

Related issues:

45

52

86

88

I have very limited time for this project but I see the rising need for a better API and hope to find some time for it soon.

[schotime]

I actually always now connect with the default endianness and then use the api that returns the Memory and perform the following. Definitely agree on a refactor of the endianness.

  public static byte[] ReorderBytes(this byte[] bytes, ModbusEndianness modbusEndianness)
  {
      if (modbusEndianness == ModbusEndianness.Mid)
      {
          for (int i = 0; i < bytes.Length; i += 2)
          {
              (bytes[i], bytes[i + 1]) = (bytes[i + 1], bytes[i]);
          }
      }
      else if (modbusEndianness == ModbusEndianness.Big)
      {
          Array.Reverse(bytes);
      }

      return bytes;
  }

[iamalexcater]

Agreed, I can see that it has become a confusing topic for many people; the API needs some thought in this regard.

The issue I originally attempted to highlight is the fact that if I call WriteMultipleRegistersAsync (and SwapBytes is true) then the T[] 'dataset' argument is modified via the call to ModbusUtils.SwitchEndianness, which might result in the caller observing this mutation. This is undesirable IMO.

I have since discovered that issue #52 describes the very same problem. @Apollo3zehn responded explaining that this was likely related to usage of ArrayPool, but this is not correct; the array that is owned by the client is directly mutated.

The dataset argument should not be modified and IMO ought to be a ReadOnlyMemory<T>

[Apollo3zehn]

Thansks for your input, I will collect everything and come up with a new API design in the coming weeks (and ask for your kind feedback again :-))