/system/ntp: type for root-delay, root-dispersion and offset should be double/float
Opened this issue · 8 comments
"type" defined in the RFC for root delay, root dispersion and offset is double/float but in the model "type" for these 3 fields is defined as uint32, uint64 and uint64 respectively. offset can have a negative value and defining type for offset as uint64 gives incorrect values.
root-delay
root-dispersion
offset
Even if we keep 64-bit integers as the type for offset, the inability to represent negative offsets is still a blocker for correctly implementing the NTP model.
@kbchandradeep or @nkitchen would you like to send a PR to change this? At a minimum it should change the type for offset, as @nkitchen pointed out, offset needs to indicate positive and negative offsets.
I see very little precedent for using decimal64 with units of (sub)seconds. In almost every case, the nodes with (sub)second units are of integer type. So I propose to use int64.
I opened #1022 as an alternative issue that doesn't take the position that the type should necessarily be decimal64 and leaves out root-delay and root-dispersion.
Below PR has already been created for this issue.
#995
Units defined in the OC model for offset, root-delay and root-dispersion is milliseconds.
Type defined in the RFC is double/float.
We can change the type for the 3 fields to oc-types:ieeefloat32 as updated in the PR.
If we want to display these fields with microseconds or nanoseconds precision, we can use decimal values.
For example, offset with milliseconds precision can be 2.0, with microseconds precision can be 2.852 and nanoseconds precision can be 2.852346
Hence, would suggest to change the type from unit to oc-types:ieeefloat32 with units as milliseconds for the 3 fields.
Using ieeefloat32 may not be the best choice for these nodes. We have seen confusion from our customers on multiple occasions when they saw the binary encoding of ieeefloat32. I think a decimal64 or integer type is much less likely to result in confusion.
Changing the type for the 3 fields to decimal64 looks fine as fields of type decimal64 can store negative values and positive values with 14 decimal places.
This issue is stale because it has been open 180 days with no activity. If you wish to keep this issue active, please remove the stale label or add a comment, otherwise will be closed in 14 days.