search

opendroneid / opendroneid-core-c

Open Drone ID Core C Library
Apache License 2.0
178 stars 64 forks source link

MAX_TIMESTAMP is incorrect #90

Closed NathanMark03 closed 3 months ago

NathanMark03 commented 3 months ago

Based on what I have read in the final rule on remote id released by the faa, he maximum timestamp should be 36000 not 3600 as is currently the case.

I copied the regulation straight from the ruling: Timestamp Time of applicability expressed in 1/10ths of seconds since the last hour 0–36000: 16 Bit UInt (LE)

Unless I am completely misinterpreting this ruling. I came across this issue when trying to represent larger time values. For example, if the timestamp value taken from a flight controller is 34 minutes, 35 seconds, 6 tenths of a second, it would be written as 20756, this is larger than the max timestamp value and thus can not be transmitted. In my code, I changed the MAX_TIMESTAMP value from (60*60) to 36000 and ran into no issues.

gabrielcox commented 3 months ago

@NathanMark03 On the source of the requirement, to be precise, it's in the ASTM F3411-22a standard, not the FAA rule. But that's ok, I understood what you meant.

This has come up before since MAX_TIMESTAMP (the constant) is expressed in seconds, but within the code, it would convert it to 10ths by multiplying by 10 (providing 36000) (see diff). Given the confusion this may cause, I'm changing it to 10ths to match the actual field value.

As such, MAX_TIMESTAMP, the constant, is now set to 36000.

friissoren commented 3 months ago

@gabrielcox : The code change you did breaks the sanity check in the encodeLocationMessage function.

It takes seconds as the input but that now gets compared against 10'th of seconds.

NathanMark03 commented 3 months ago

@friissoren Thanks for your insight. The reason I thought this was a problem is because I was feeding encodeLocationMessage time data in tenths of seconds instead of seconds. That is my fault and I thank you for pointing that out. I still think it is a little confusing how ASTM F3411-22a states the range is 0-36000 and MAX_TIMESTAMP is 3600, maybe we change it to MAX_TIMESTAMP_S just to clarify that it is seconds?

gabrielcox commented 3 months ago

@gabrielcox : The code change you did breaks the sanity check in the encodeLocationMessage function.

It takes seconds as the input but that now gets compared against 10'th of seconds.

Thanks @friissoren for catching this.

I went back and reviewed all the other constants and see that they are all based on unencoded values rather than encoded values. Therefore, this constant should have been preserved relative to the unencoded which is represented as (floating point) seconds.

So I will change it back and add a comment to the constant clarifying this. This particular one has caused confusion, but it was consistent with the others and should stay that way.

@NathanMark03 I won't add _S to the constant name because it would be inconsistent to add units to the constant name. However, I think the comments will help with the confusion in the future.

gabrielcox commented 3 months ago

Fixed in this diff .

@friissoren , please close if you don't see any other issues.