Fixes float encoding/decoding for both big and little endian (fixes #665).

[ghorwin]

Fixes float encoding/decoding for both big and little endian (fixes #665).
Also removes useless memcpy calls and no longer used swap32 and swap16 macros.

This pullrequest modifies a single file. As explained in issue #665, the byte positions in target modbus buffer must be explicitely set according to the modbus protocol specs.

Both for encoding/decoding a cross-platform way working for both big and little endian systems is the use of 32-bit integers. That means, we interpret the memory of the 32-bit float as 32-bit integer, then extract the byte via shift operations. Hereby, the compiler takes care of the big/little endian memory layout of our float/integer.

The new code is way faster than the previous, as the memcpy operations are no longer needed. The type casts may look ugly at first glance, but really describe what we want to achive:

float f;
float * f_ptr = &f;  // pointer to float
uint32_t * i_ptr = (uint32_*)f_ptr; // interpret as pointer to uint32
uint32_t i = *i_ptr; // get the integer value at this memory location

putting this all together:

*(uint32_t*)(&f) = integer number

[cla-bot]

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient...

[ghorwin]

Sorry, need to revise patch first, as src 16-bit registers already match endianess of host architecture

[cla-bot]

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient...

[ghorwin]

Finished revision of encoding/decoding functions. Actually, the encoding functions are now almost identical to the original code (except for the useless memcpy). Also, instead of altering positions of a,b,c,d in assembling the int32, I chose to alter the positions when retrieving a,b,c,d from source int16. The result is the same, yet better readable (I hope).

[cshabee]

Could someone please provide an answer to the question whether this is going to be merge anytime soon?
Thanks in advance!

[ghorwin]

Hi there,
I'm using the patched version of libmodbus for several projects with different devices and 32-bit encodings. I'm also aware of several projects reporting errors or implementing manual workarounds as the buggy libmodbus-version is already present in the apt repository of ubuntu-like distros. Hence, I would be happy if this bugfix would be merged soon.

As for the cla - I have signed it already last year but I can sign it again, to be on the safe side. Just a moment...

[ghorwin]

So, I have completed the contributor license agreement form (again). Should be all clear now.

[ghorwin]

Ok, the CLA bot seems broken... I'm giving up. I've completed the CLA sign procedure now three times, and still get the same error. Bummer...

[stephane]

Thank you @ghorwin I've updated the CLA file (manual operation is required).

[stephane]

You seem to have spend enough time on the subject to propose a good patch. This subject is full of traps and I don't want to spend more time on it these days so I'll trust your code and apply it (almost) blindly!

[stephane]

The CI fails so who is right?

TEST FLOATS
1/4 Set/get float ABCD:
Line 327: assertion error for 'is_memory_equal(tab_rp_registers, UT_IREAL_ABCD_SET, 4)': FAILED Set float ABCD

The gcc warnings are not acceptable:

../../../src/modbus-data.c:105:6: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  105 |     *(uint32_t *)&f = (a << 24) | (b << 16) | (c << 8) | (d << 0);

[stephane]

Don't know how to reopen this PR...

[ghorwin]

Hi Stephane,
thanks for looking into this. I need to check the CI to see if the assumption/definition is wrong or my code.

[stephane]

BTW I'm sure the mask and shift approach w/o memcpy or useless bswap of your PR is the right way.