modbus_get/set_float_xxxx() mixing bytes

[bkggkt3d]

After a lot of compile runs for different version I'm now quite sure there is a change in implementation of modbus_get_float_abcd() and modbus_set_float_abcd() funtions, probably all 8 variants are affected.

libmodbus version

branch v3.1.8 sources from github

OS and/or distribution

Linux Kernel 3.10 with Debian 11

Environment

Linux Debian 10 crosscompile for embedded arm with ./configure --host=arm-linux-gnueabihf

Description

• modbus_get_float_abcd() from v3.1.8 acts like modbus_get_float_dcba() from v3.1.7, so all 4 bytes need to have different order to get the right float back
• modbus_set_float_abcd() from v3.1.8 acts like modbus_set_float_cdab() from v3.1.7, so the both 16-bit registers came back swapped from the float. Yes, this is even inconsistent to the problem at get-funtion.

Expected behavior or suggestion

Up to version 3.1.7 all 8 functions are working as expected and how I understood the documentation

[modem-man-gmx]

Could you please give some more easy reproducible example and more details of the counterpart device?
F.i. can this be reproduced with widely available hardware like Raspberry Pi? Which distro is to be installed to reproduce?
Finally, you had in mind, that float implementation between hardware (x86/arm) and platforms and compilers may be incompatible? I ask, because mixing IEEE 754, intel-x86-80bit and some 96 bit on a µC (gcc, IIRC) made me some headache in the past.

Would like to verify your observation on my hardware zoo ...

[morgoth6]

I think same problem hit my project. It's quite easy to reproduce I guess. Not sure I understand this correctly, but run a float by modbus_set_float_badc() && modbus_get_float_badc() should give same float value used at the beginning ?

        float f_in = 123456.0f;
        float f_out;

        uint16_t f16[2];
        modbus_set_float_badc(f_in, f16);
        f_out = modbus_get_float_badc(f16);

        printf("f16[] = 0x%02X%02X%02X%02X\n", ((uint8_t *)&f16)[0], ((uint8_t *)&f16)[1], ((uint8_t *)&f16)[2],
               ((uint8_t *)&f16)[3]);

        printf("in[%f] = 0x%02X%02X%02X%02X\n", f_in, ((uint8_t *)&f_in)[0], ((uint8_t *)&f_in)[1],
               ((uint8_t *)&f_in)[2], ((uint8_t *)&f_in)[3]);
        printf("out[%f] = 0x%02X%02X%02X%02X\n", f_out, ((uint8_t *)&f_out)[0], ((uint8_t *)&f_out)[1],
               ((uint8_t *)&f_out)[2], ((uint8_t *)&f_out)[3]);

Gives in result:

f16[] = 0xF1470020
in[123456.000000] = 0x0020F147
out[-985402689122801928078052294656.000000] = 0x200047F1

Result is the same for x86_64 and aarch64.

[oe5hpm]

the current implementation is obviously wrong.

i did a short bisect on this. following commit breaks things:

49af73d is the first bad commit
commit 49af73d
Author: SZ Lin (林上智) [email protected]
Date: Thu Dec 20 13:35:31 2018 +0800

Fix float endianness issue on big endian architecture.

It converts float values depending on what order they come in.

This patch was modified from rm5248 [1]

[1] https://github.com/synexxus/libmodbus/commit/a511768e7fe7ec52d7bae1d9ae04e33f87a59627

src/modbus-data.c | 110 ++++++++++++++++++++++++++++++++++++++---------
tests/unit-test-client.c | 22 ++++++----
tests/unit-test.h.in | 41 ++++++++++++++++--
3 files changed, 141 insertions(+), 32 deletions(-)

[oe5hpm]

in detail it looks to me, that the modbus_set_float functions are not correct

[ghorwin]

Hi,

I can assert that the current implementation is wrong. I stumbled across this when checking conversion from/to cdab float format.

Simple test code:

float val = 24; 
// on x86 machines in little-endian format (dcba) this is in memory  "00 00 c0 41"

// 4 byte memory array with two registers/words, each uint16_t also stored in little endian format
uint16_t reg[2]; 
modbus_set_float_cdab( val, reg );

// we expect memory of reg to hold "00 00 41 c0" and this is the case, so the implementation of the
// setter function is correct

// backwards
float val2 = modbus_get_float_cdab( reg );

// memory of val2 is now still "00 00 41 c0", in fact ab and cd get got swapped twice

Explanation: consider the function implementations

float modbus_get_float_cdab(const uint16_t *src) {
	// we work directly with uint16_t, which are little-endian format as well
	float f;
	uint32_t i;
	uint8_t a, b, c, d;

	a = (src[0] >> 8) & 0xFF;   // here is the bug, as the higher byte of src[0] is not a, but b!
	b = (src[0] >> 0) & 0xFF;
	c = (src[1] >> 8) & 0xFF;
	d = (src[1] >> 0) & 0xFF;
	...
}

void modbus_set_float_cdab(float f, uint16_t *dest)
{
	uint32_t i;
	uint8_t *out = (uint8_t *) dest; // AAH, we work directly on bytes, much better!
	uint8_t a, b, c, d;
	...
}

The core problem is the usage of uint16_t and the assumption made when using the shift operation.

The fix is simple, and can be combined with a general code cleanup in this file. The old, outdated swap16 and swap32 macros can go, as the useless memcpy calls.

I'll submit a patch/pull request to fix the issue, shortly.

Note, however, that there is a general problem with these encoding functions. For a cross-platform library like libmodbus, we have to address two memory layouts:

• the source memory layout of the float passed to the conversion function (little-endian/big-endian)
• the target memory layout expected in the modbus stream (which is part of the modbus protocol specs of a certain device)

The current function declaration only addresses the target format with the suffix (abcd cdab etc.), yet not the source format. Hence, we need to have separate implementations for systems with big and little endian source floats (switched via compiler defines).

[ghorwin]

Example for a litte-endian/big-endian compatible variant. Consider the following code:

void modbus_set_float_cdab(float f, uint16_t *dest) {

	// interpret memory of float (32 bit) as memory of uint32
	uint32_t * i_ptr = (uint32_t*)(&f);
	uint32_t i = *i_ptr;
	// or short
	//
	// uint32_t i = *(uint32_t*)(&f);
	//
	// explaining the above line would be a nice exam question 
	// for second grade computer science class... :-)

	// extract bytes - code that works for both big and little endian
	// MIND: shift operations consider Endianess!!!
	uint8_t a, b, c, d;
	a = (i >> 24) & 0xFF; // on little endians Byte 4 in memory, on big endians: Byte 1 in memory
	b = (i >> 16) & 0xFF;
	c = (i >> 8) & 0xFF; 
	d = (i >> 0) & 0xFF; 

	// alternatively access memory positions directly - this only works for little endian
	// since it explicitely specifies source memory locations

	uint8_t * in = (uint8_t *)(&f);
	a = in[3];
	b = in[2];
	c = in[1];
	d = in[0];

	// write into byte buffer - here, we MUST directly write into byte array
	// to get the exact pattern as requested
	uint8_t * out = (uint8_t *) dest;
	out[0] = c;
	out[1] = d;
	out[2] = a;
	out[3] = b;
}

Using the first variant (cast to integer and shift) appears to work on both big and little endian, so this might be the way to go.

[ghorwin]

Hi, just noticed, I was working with wrong assumption, so my fix needs to be revised first.

Consider the transfer from modbus buffer to 16-bit registers: (modbus.c:1311)

// uint16_t *dest
for (i = 0; i < rc; i++) {
	/* shift reg hi_byte to temp OR with lo_byte */
	dest[i] = (rsp[offset + 2 + (i << 1)] << 8) | rsp[offset + 3 + (i << 1)];
}

Here, the little/big endianess of the host architecture is already considered (the high byte is first in byte stream, then the low byte. The destination uint16 is composed using shift operations, so that the compiler takes care of the actual number representation). So we have the byte order in memory already matching in each 16-bit register.

Similarly, when encoding the modbus message, the conversion to big-endian modbus stream is handled already (modbus.c:1515)

// const uint16_t *src
for (i = 0; i < nb; i++) {
	req[req_length++] = src[i] >> 8;
	req[req_length++] = src[i] & 0x00FF;
}

I'll revise my patch and submit again.

[ghorwin]

Hi,

here's a variant. What do you think?

/* Get a float from 4 bytes (Modbus) without any conversion (ABCD) */
float modbus_get_float_abcd(const uint16_t *src)
{
	/* Suppose the modbus byte stream contained the 32-bit float 0x10203040  - abcd.
	   On big endian systems, the memory starting at src contains two 16-bit registers 0x1020  and 0x3040
	   On little endian system, the 16-bit registers memory holds 0x2010 and 0x4030
	   (mind, not the _number_ 0x2010, but the actual memory content).

	   To convert the data to float32 on big-endian we only need to cast the pointer and we are done.
	   On little endian systems, we need to swap the bytes in each word again and then assemble
	   an integer via shift operations and finally cast to float32.

	   A portable way is to retrieve low and high bytes of both words using 
	   shift operations, then assemble the 32-bit integer.
	*/

	float f;
	uint8_t a, b, c, d;

	// access buffer memory byte-wise ABCD
	a = src[0] >> 8;      // high byte of first word
	b = src[0] & 0x00ff;  // low byte of first word
	c = src[1] >> 8;      // high byte of second word
	d = src[1] & 0x00ff;  // low byte of second word

	// assemble in memory location of float
	// from right to left: get address of float, interpret as address to uint32, dereference and write uint32
	*(uint32_t *)&f = (a << 24) | (b << 16) | (c << 8) | (d << 0);

	return f;
}

[ghorwin]

Did anyone check my patch, yet? I'd be happy to get some feedback...

[morgoth6]

I will check it in next few days.

[ghorwin]

Hi again, any progress on the issue? I'd appreciate if we introduce a solution to this bug into the current libmodbus, before it gets included in any linux distro packages and breaks existing tools.

Any way I could help?

[modem-man-gmx]

we have to address two memory layouts:

• the source memory layout of the float passed to the conversion function (little-endian/big-endian)
• the target memory layout expected in the modbus stream (which is part of the modbus protocol specs of a certain device)

Hmmm, I can't say, If I got you right. The "Modubus Application Protocol V1.1b" specification says in section 4.2 (cite:)

"MODBUS uses a ‘big-Endian’ representation for addresses and data items. This means that when a numerical quantity larger than a single byte is transmitted, the most significant byte is sent first. So for example

  Register size      value 
  16 - bits          0x1234    the first byte sent is   0x12  then 0x34

."

[ghorwin]

Hi, you are of course right, regarding the modbus protocol specs: here it is clearly stated, that big endian is to be used.

BUT, in practice, we are sometimes dealing with IOT devices, sometimes developed by small companies with their own tool sets and often enough we have float/4 byte integer encodings that do not follow protocol. As long as the byte order in the modbus stream is known, we can just use one of the 4 provided functions and be done. For modbus protocol conform devices, always abcd is the right choice.

So the question is rather: should libmodbus support non-standard modbus implementations, or should we only stick to proper modbus protocol. If we decide for the latter, we can drop all functions except abcd and just use the old, single modbus get/set function.

@stephane: what's your take on the issue?

[stephane]

My take is libmodbus should support non-standard Modbus implementations because there are sadly useful.

[ghorwin]

See further explanation on #757, as @oe5hpm correctly stated, this is indeed a regression bug introduced in commit 49af73d.

Easiest would be to revert this commit and then deal with the encoding issue for abcd, ... for big and little endian in a new commit.