Apollo3zehn / FluentModbus

Lightweight and fast client and server implementation of the Modbus protocol (TCP/RTU).
MIT License
189 stars 71 forks source link

SetBigEndian not working #34

Closed mkroesen closed 3 years ago

mkroesen commented 3 years ago

Hello, either I do something wrong or there is a mistake.

The following behaviour can be observed: grafik grafik

I only want to write 16bit, but it looks like the SetBigEndian method writes more than 16bit and overwrites the other values. If I write only one value it works, with more values this strange behaviour appears.

Without casting to short: grafik That will be because it is int and has 32bit.

I think the same behaviour applies to SetLittleEndian.

Apollo3zehn commented 3 years ago

That looks strange, i will check that on monday.

Apollo3zehn commented 3 years ago

There was a wrong buffer offset calculation (see here). I have released version 2.2.1 which hopefully solves your issue.

mkroesen commented 3 years ago

Works perfectly!

The only thing that would be great would be if you could use int or something like it as address for SetBigEndian/SetLittleEndian, not only ushort, because I calculate the addresses partially and C# makes int by default, which I always have to cast to ushort.

It would be great if SetBigEndian/SetLittleEndian would support the following functions: If I use var data = server.GetInputRegisters(); the size of the value should be written as int 32bit (just like now), as short 16bit, but if I use Span<short> data = server.GetInputRegisters(); the value should be automatically casted to short. Would that be possible?

Because I always have to do somethink like this: data.SetBigEndian(0, (short)1);, if I use data.SetBigEndian(0, 1); the "1" is on address 1 and not longer on address 0.

Registers Address Value Length
var Allow each integral numeric types and cast them to ushort No cast sizeof(value)
Span\ " Cast to short sizeof(short), 16bit
Span\ " Cast to int sizeof(int), 32bit
Span\ " Cast to T sizeof(T), (T)bit
... ... ... ...
Apollo3zehn commented 3 years ago

I have changed all unitIdentifier, startingAddress, registerAddress and count parameters to int. This applies to SetLittleEndian, SetBigEndian and all generic Modbus client methods like ReadHoldingRegisters to avoid casting constantly. I did not do it before because these data types (byte and ushort) enforce always correct input values. Now it is needed to validate them whenever the listed methods above are called, but I think thats fine :-)

Regarding your second request: Unfortunately that is not possible. There is something called implicit operator which could do the cast for you. But to create such an operator for a Span<T>, it would be required to change the source code of that type. And since Span<T> is a ref struct, I cannot inherit from it to implement this myself. With inheritance it would look like:

public static ref struct MySpan<T> : Span<T>
{
  public static implicit operator MySpan(Span<byte> input)
  { 
    return MemoryMarshal.Cast<byte, T>(input);
  }
}

As an alternative to avoid the requirement to cast every value you should provide the T parameter: Instead of calling data.SetBigEndian(0, (short)1); you should call it like data.SetBigEndian<short>(0, 1);. I know there is just a small difference, but the second one is more declarative.

As written in the other issue I will create an update soon (it is already prepared but I need to solve #37 first).