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

Fix timestamp value mask #47

Merged
merged 2 commits into from
Aug 25, 2017

Conversation

bn-dignitas
Copy link
Contributor

getTimestamp_timestampValue was returning the wrong value because of an incorrect mask. Applied the correct mask and shift.

@leif81
Copy link
Member

leif81 commented Aug 23, 2017

Thank you @bn-dignitas

I don't have access to a dis 7 producing sim at the moment...can I ask a favour. Could you capture a PDU (e.g. with Wireshark) that contains this timestamp field and add it to src/test/resources/edu/nps/moves/dis7/ ? Then we can get a unit test added related to this.

@mcgredonps
Copy link
Collaborator

mcgredonps commented Aug 23, 2017

If I remember right there was a bit value at the end of the timestamp field that marked the time value as either relative or absolute, thus the FE vs FF. A question is where the discovery of that should be. If just looking for the time value it might be good to have one method for that, and another method for discovering whether the bit value for the absolute/relative split in the same field is there or not. The other API choice is to just return the whole field and separate them yourself. As I hazily recall that it was all pretty complex. I'm not convinced that all actual DIS developers pay much attention to the standard rules, and instead just throw in millisecond Linux value of some sort. Which of course causes interop problems. That's an argument for hiding it all under an API that does it sorta correctly without much local programmer work.

There was a long section in the 2012 standard on the time field and how to work with it. Took a look and it's Annex G in the 2012 standard. It's interesting reading.

@bn-dignitas
Copy link
Contributor Author

As it stands, the dis7/Timestamp class returns three values related to the timestamp field:

  • the raw 32-bit timestamp value read from the stream
  • the type (absolute/relative) which comes from the least significant bit of the timestamp
  • the "time value", the most significant 31 bits of the decoded timestamp measured in "time units" with 1 time unit being equal to 3600/(2^31) seconds

Just for discussion/background, I could have chosen to use a mask ending with FF versus FE, but in the end the returned value from this function will be the same since the least bit gets truncated anyway during the shift. The mask is to make sure the 31-bit value becomes a positive/unsigned value since the left most bit is the sign bit and upon integer expansion to a long during the unmarshal, the bit will be copied left sometimes resulting in a negative value. That's just the nature of dealing with unsigned types in Java. Normally this value probably would have been read directly as an unsigned type using the DataInputStream methods, but like so many values in DIS it is a combination of values.

Also, if anyone is intereseted, the signed->unsigned conversion is the same technique as Java 8's Integer.toUnsignedLong method.

@bn-dignitas
Copy link
Contributor Author

@mcgredonps I agree it would be nice if the API at least offered convenience methods that accept and return something like epoch times in millis so the devs don't have to do the conversions themselves if they don't want to.

@mcgredonps
Copy link
Collaborator

I'm trying to work on a git-based DIS tutorial, so maybe a bit down the road we can stick a reference to the timestamp issues to that problem in the source code comments. There's just a lot of complex issues in DIS, and I hope to leave the tutorial open for people who know the details to provide examples and explain things they know about.

That can potentially include both some text and a git sub-repo with code. One guy explaining everything seems completely impractical. There's a lot of subjects that wind up with specialized groups.

@mcgredonps mcgredonps merged commit 6438d05 into open-dis:master Aug 25, 2017
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.

3 participants