Bug #262

Check byte-order writing

Added by Philipp Buluschek about 4 years ago. Updated over 3 years ago.

Status:Feedback Start date:10/09/2014
Priority:Urgent Due date:
Assignee:Stefano Lenzi % Done:

0%

Category:zigbee.common Spent time: -
Target version:org.aaloa.zb4osgi.zigbee.common-0.5.0
Has a patch:No Has license agreement signed:No

Description

I implemented the writing of ZigBee data type IEEE_Address (see ZigBeeType.IEEEAddress) in ByteArrayOutputStreamSerializer. The original Java-data type was String, but I changed it to Long as it is much more adapted (as it's also what is returned from SimpleDriver.getIEEEAddress() ).

Writing the IEEE Address (as a long) in ByteArrayOutputStreamSerializer, I supposed the code should look like:

case IEEEAddress:
final Long l = (Long) data;
append_long(l.longValue());
break;

But noticed that, the IEEE address gets written byte-order reversed over the air. In particular, the ZigBee Spec (1.2.1.3) says that the least significant byte must be sent first. But the implementation of Integers.writeLong() does the reverse (writes msb first to the buffer). For my case, I simply added a implementation of the reversed writing - so not a problem.

But I now wonder - all other data types (larger than 1 byte) are written in the same manner as long in Integers.writeXXX(). Should they all be reversed?


Related issues

related to Bug #292: Missing support for reading Semi-Precision data types New 01/13/2015
related to Improvement #291: Using ByteBuffer instead of Integers In Progress 01/12/2015
blocks Task #117: Create ZB4OSGi suite release 1.0 Feedback 06/24/2011

Associated revisions

Revision 1147
Added by Giancarlo Riolo almost 4 years ago

Zigbee.common deprecated. Partial Deserializer issues resolved (refs #262)

Revision 1151
Added by Giancarlo Riolo almost 4 years ago

Zigbee.common (refs #262) Read and Write 24 bit needs more investigation (negative numbers rapresentation) 32 bits works

Revision 1152
Added by Giancarlo Riolo almost 4 years ago

UnsignedInteger40bit and UnsignedInteger48bit read works. refs #262

Revision 1153
Added by Giancarlo Riolo almost 4 years ago

refs #262 . All three tests integrated, added another plain simple test for serialization and deserialization. Fixed Write and Read for all involved datatypes.

Revision 1154
Added by Giancarlo Riolo almost 4 years ago

refs #262 . Reverted a 4 byte readLong used for debug purpouses in test with appropriate ReadInt

History

#1 Updated by Stefano Lenzi almost 4 years ago

  • Category set to zigbee.zcl.library
  • Status changed from New to In Progress
  • Assignee set to Stefano Lenzi
  • Priority changed from Normal to High
  • Target version set to org.aaloa.zb4osgi.zigbee.zcl.library-1.X

Dear Philipp,

I could not find the code that you wrote on inside ZB4O nevertheless, it seems that all the call to the method ByteArrayOutputStreamSerializer.append_long(long) has never been used as far as I know, while it was used many time other method, so I think that the problem may be limited to the specified method.

I will look further at it, but please can you point me to the file that contains the code that you posted?

#2 Updated by Philipp Buluschek almost 4 years ago

The code I'm talking about is in ByteArrayOutputStreamSerializer.appendZigBeeType(Object, ZigBeeType) which is used for example by WriteAttributeCommand.getPayload() to create the payload to be sent.

In ByteArrayOutputStreamSerializer.appendZigBeeType(Object, ZigBeeType) the types which are greater than 2 bytes are serialized using writeLong. Example

  public void appendZigBeeType(Object data, ZigBeeType type) {
    ...
    case UnsignedInteger40bit:
      append_long((Long) data, 5);
      ...

and append_long calls Integers.writeLong() which writes MSB first to the buffer. I believe this is incorrect.
For example Integers.writeShort() writes the MSB last, and so is correct, in my opinion.

#3 Updated by Stefano Lenzi almost 4 years ago

  • Category changed from zigbee.zcl.library to zigbee.common
  • Priority changed from High to Urgent
  • Target version deleted (org.aaloa.zb4osgi.zigbee.zcl.library-1.X)
The issue was totally clear... I made the following test unit to catch your issue. The idea is to verify that all Integers.writeXXX write data with same ordering, and I had a very bad surprise..
  • the writeLong(byte[] dest, int pos, long data, int size) is bugged when size <> 8
  • the writeShort is the only method that write MSB last, all the others write LSB first

At this point I have a question for you: Have you experienced issues when communicating with other devices on the ZigBee network that were using int24 or int32 data?

I the mean while we will perform some tests in our lab, to check behavior. BTW, I think that writeShort is correct one because it has been validated hundreds of time when the Read Attribute or Write Attribute was invoked, considering that both are using writeShort method when filling the AttributeID into the Command

    public void testShortIntLongToBytes() {
        long l = 0x1234;
        byte[] expShort = new byte[8];
        byte[] expInt24 = new byte[8];
        byte[] expInt = new byte[8];
        byte[] expLong = new byte[8];
        Integers.writeShort(expShort, 6, (short)l);
        Integers.writeInt(expInt, 4, (int)l);
        Integers.writeInt(expInt24, 4, (int)l);
        Integers.writeLong(expLong, 0, l);
        assertArrayEquals(expInt24, expInt);
        assertArrayEquals(expLong, expInt);
        for (int i = 5; i <= 8; i++) {
            expLong[0] = 0;
            expLong[1] = 0;
            Integers.writeLong(expLong, ( 8 - i ), l, i);
            assertArrayEquals(expLong, expInt);
        }
        assertArrayEquals(expShort, expInt);
    }

#4 Updated by Philipp Buluschek almost 4 years ago

I have not had issues with int24/int32 data as I don't use them so far. My original problem was sending a IEEE address (which is 64 bit), and this didn't work with the current code.

Regarding your test, I don't understand it really. My proposal for a test (just for writeLong) would be:

@Test
    public void testWriteLongByteOrder() throws Exception {
        long l = 0x123456789ABCDEF0L;
        byte[] expected = new byte[] { (byte) 0xF0, (byte) 0xDE, (byte) 0xBC,
                (byte) 0x9A, (byte) 0x78, (byte) 0x56, (byte) 0x34, (byte) 0x12 };
        byte[] test64 = new byte[8];
        // checking full 8 bytes write
        Integers.writeLong(test64, 0, l, 8);
        assertArrayEquals(expected, test64);

        // checking partial write
        for (int i = 1; i <= 8; i++) {
            byte[] test = new byte[i];
            Integers.writeLong(test, 0, l, i);
            assertArrayEquals(Arrays.copyOfRange(expected, 0, i), test);
        }
    }

And for negative longs (or unsigned > 0x7FFFFFFFFFFFFFFF )

@Test
    public void testWriteLongByteOrder2() throws Exception {
        BigInteger bi = new BigInteger("FEDCBA9876543210", 16);
        long l = bi.longValue();
        byte[] expected = new byte[] { (byte) 0x10, (byte) 0x32, (byte) 0x54,
                (byte) 0x76, (byte) 0x98, (byte) 0xBA, (byte) 0xDC, (byte) 0xFE };
        byte[] test64 = new byte[8];
        // checking full 8 bytes write
        Integers.writeLong(test64, 0, l, 8);
        assertArrayEquals(expected, test64);

        // checking partial writes
        for (int i = 1; i <= 8; i++) {
            byte[] test = new byte[i];
            Integers.writeLong(test, 0, l, i);
            assertArrayEquals(Arrays.copyOfRange(expected, 0, i), test);
        }
    }

#5 Updated by Stefano Lenzi almost 4 years ago

We are performing some test, but I think that we have big ISSUE here. By the way my test was a complete test for all the data integer data types, and I was checking the behavior among each other so that later changing to the code will be catched.

#6 Updated by Stefano Lenzi almost 4 years ago

  • Tracker changed from Task to Bug
  • Target version set to org.aaloa.zb4osgi.zigbee.common-0.5.0
  • Has a patch set to No
  • Has license agreement signed set to No

It is going to be part of the next release soon, we tested with real device and it is now partially fixed with commits r1122 and r1123 made by Giancarlo.

#7 Updated by Giancarlo Riolo almost 4 years ago

  • Status changed from In Progress to Feedback

Tests added and working. Some issues found in bitwise operations. Added another simple test for serialization and deserialization isolation.

#8 Updated by Philipp Buluschek over 3 years ago

I have just checked the Integers.readLong() method, and I think that building two ByteBuffer Objects, assigning one byte array and iterating over the whole thing twice (once in ByteBuffer's getLong()) is really suboptimal. I agree that using JRE methods is preferable to building your own - but only if they do what you want...

The following implementation only assigns the output long and iterates once.

public static long readLong(byte[] src, int pos, int size) {
  long value = 0;
  for (int i = 0; i < size; i++){
     value += ((long) src[pos + i] & 0xffL) << (8 * i);
  }
  return value;
}

Also available in: Atom PDF