Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signal PDU data length is in bytes but should be in bits (DIS 7) #116

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

adamwrobinson5
Copy link

Fixes issue #56

@leif81 leif81 self-requested a review September 5, 2023 19:54
@leif81
Copy link
Member

leif81 commented Sep 5, 2023

Thanks @adamwrobinson5 , I'll take a closer look shortly.

Do you happen to have a sample dis7 signal and transmitter pdu to include in a unit test?

@adamwrobinson5
Copy link
Author

Thanks for getting back to me so quickly! Unfortunately, I do not have a sample pdu that I can provide.

@leif81 leif81 linked an issue Sep 8, 2023 that may be closed by this pull request
@leif81 leif81 changed the title DIS7 Signal PDU data length is in bytes but should be in bits Signal PDU data length is in bytes but should be in bits (DIS 7) Sep 8, 2023
public short getDataLength() {
return (short) data.length;
if (dataLength == 0) {
return (short) (data.length * 8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamwrobinson5 Could you explain the importance of checking length for 0 and the line that follows?

We didn't do that on the DIS 6 equivalent Signal PDU and wondering if it's required.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required, it was just a way to abstract a DIS-ism. IAW the standard, the data length should be the length of the data field in bits. Since data is a byte array, the data length field would be the length of the byte array multiplied by 8.

I can take it out though if you'd prefer it not be in there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yes let's take it out and just rely on returning the dataLength.

@leif81
Copy link
Member

leif81 commented Sep 8, 2023

@adamwrobinson5 I think we may also need to modify the unmarshall and marshall methods like we did when making the equivalent fix for the DIS 6 Signal PDU (seen below) .

This change was required to the unmarshall method so that we allocated the right length to the data array.

final int dataLengthBytes = dataLength / Byte.SIZE;
data = new byte[dataLengthBytes];

And in the marshall method we needed to pad the data on 32 bit boundary.

nrOfBytes = dataLength / Byte.SIZE;
buff.put(data);
int paddingBytes = nrOfBytes % 4;//Padding to hit 32 bit boundry
switch (paddingBytes) {
case 0:
break;//No padding needed
case 1:
buff.put((byte) 0);
buff.putShort((short) 0);
break;//adding 3 byte padding
case 2:
buff.putShort((short) 0);
break;//adding 2 byte padding
case 3:
buff.put((byte) 0);
break;//adding 1 byte padding
}

@adamwrobinson5
Copy link
Author

@leif81 Added those changes

@leif81 leif81 merged commit 9f56ced into open-dis:master Oct 17, 2023
@leif81
Copy link
Member

leif81 commented Oct 17, 2023

Thank-you @adamwrobinson5 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signal PDU data length is in bytes but should be in bits (DIS 7)
2 participants